From 29d1b2d923292937be4a382384b47bc8f59b7d6c Mon Sep 17 00:00:00 2001 From: ecryptoguru Date: Sat, 19 Sep 2026 20:58:15 +0530 Subject: [PATCH 1/6] fix(interface): remove network side effects from target inference --- UPGRADES.md | 18 + docs/usage/cli.mdx | 10 + lyrashield/interface/main.py | 41 +- lyrashield/interface/utils.py | 209 ++++++++- tests/test_target_inference_no_network.py | 506 ++++++++++++++++++++++ 5 files changed, 758 insertions(+), 26 deletions(-) create mode 100644 tests/test_target_inference_no_network.py diff --git a/UPGRADES.md b/UPGRADES.md index 737459e1..5ef1c136 100644 --- a/UPGRADES.md +++ b/UPGRADES.md @@ -1,5 +1,23 @@ # LyraShield ownership and upstream-import ledger +## Offline target classification and `--target-type` + +Target-kind inference no longer probes `GET /info/refs?service=git-upload-pack` +from the host: that request fired before target authorization and could reach +private or internal addresses during classification alone. Inference is now +offline-only — no DNS resolution, no HTTP — recognizing local paths, `git@` and +`git://` remotes, credential-bearing URLs, and `.git` suffixes as repositories. + +Compatibility change: an HTTP(S) Git remote that does not end in `.git` (for +example `https://github.com/org/repo`) now classifies as `web_application` +instead of being probed. Pass the new `--target-type repository` flag for those +targets. The flag validates the input's shape and errors actionably on a +kind/input mismatch; it never authorizes fetching private or internal addresses, +and repository acquisition still uses the existing guarded clone path. The +upstream-retained `strix.interface` copy is unreachable from the shipped +`lyrashield`/`lyrashield-local` entry points and remains pinned by the +controlled-derivative gate. + ## Security dependency audit and Intel macOS packaging CI audits the frozen Python dependency graph (all extras and groups) and the diff --git a/docs/usage/cli.mdx b/docs/usage/cli.mdx index e16d5da2..d9d36263 100644 --- a/docs/usage/cli.mdx +++ b/docs/usage/cli.mdx @@ -44,6 +44,16 @@ Serves the prebuilt local viewer SPA. Source lives in `lyrashield/interface/view Path to a file containing targets, one per non-empty, non-comment line. Lines starting with `#` are ignored. Can be specified multiple times and combined with `--target`. + + Explicit kind for every `--target`/`--target-list` entry: `repository`, `web_application`, `local_code`, or `ip_address`. The flag validates that each input matches the declared kind and exits with an actionable error when it does not. + + Classification is offline-only: the engine never resolves DNS and never sends HTTP requests to the target while deciding its kind. When omitted, local directories, `git@`/`git://` remotes, credential-bearing URLs, and URLs ending in `.git` classify as repositories; other HTTP(S) URLs and bare domains classify as web applications. + + + Compatibility: an HTTP(S) Git remote that does not end in `.git` (for example `https://github.com/org/repo`) was previously detected by a host-side probe and now classifies as `web_application` unless you pass `--target-type repository`. The flag only classifies input — it never authorizes fetching private or internal addresses, and repository acquisition still uses the existing guarded clone path. + + + Bind-mount a local directory into the sandbox (read-only) instead of copying it in file-by-file. Use this for large repositories that are too big to stream into the container. Can be specified multiple times. diff --git a/lyrashield/interface/main.py b/lyrashield/interface/main.py index 9f55c3df..745b1226 100644 --- a/lyrashield/interface/main.py +++ b/lyrashield/interface/main.py @@ -35,6 +35,7 @@ from lyrashield.interface.cli import run_cli from lyrashield.interface.tui import run_tui from lyrashield.interface.utils import ( + TARGET_TYPE_CHOICES, assign_workspace_subdirs, build_final_stats_text, build_mount_targets_info, @@ -45,11 +46,11 @@ find_oversized_local_targets, generate_run_name, image_exists, - infer_target_type, is_whitebox_scan, process_pull_line, read_target_list_file, resolve_diff_scope_context, + resolve_target_type, rewrite_localhost_targets, validate_config_file, validate_run_name, @@ -621,8 +622,9 @@ def parse_arguments() -> argparse.Namespace: lyrashield --target https://example.com # GitHub repository analysis - lyrashield --target https://github.com/user/repo + lyrashield --target https://github.com/user/repo.git lyrashield --target git@github.com:user/repo.git + lyrashield --target https://git.internal.example/user/repo --target-type repository # Local code analysis lyrashield --target ./my-project @@ -693,6 +695,24 @@ def parse_arguments() -> argparse.Namespace: "Intended for orchestrators that pin a target branch." ), ) + parser.add_argument( + "--target-type", + type=str, + choices=list(TARGET_TYPE_CHOICES), + default=None, + metavar="KIND", + help=( + "Explicit kind for every --target/--target-list entry: " + f"{', '.join(TARGET_TYPE_CHOICES)}. When omitted, the kind is " + "inferred locally — the engine never resolves DNS or sends HTTP " + "requests to the target while deciding. Local directories, git@/" + "git:// remotes, and URLs ending in .git classify as repositories; " + "other HTTP(S) URLs and bare domains classify as web applications. " + "Use '--target-type repository' for an HTTP(S) Git remote that does " + "not end in .git. The flag only classifies input; it is not " + "authorization to fetch private or internal addresses." + ), + ) parser.add_argument( "--mount", type=str, @@ -849,6 +869,11 @@ def parse_arguments() -> argparse.Namespace: if args.resume: if args.run_name: parser.error("Cannot combine --resume with --run-name") + if args.target_type: + parser.error( + "Cannot combine --resume with --target-type. A resumed run reuses the " + "target kinds recorded in its run record." + ) if args.target or args.target_list or args.mount: parser.error( "Cannot combine --resume with --target/--target-list/--mount. " @@ -881,9 +906,15 @@ def parse_arguments() -> argparse.Namespace: except ValueError as e: parser.error(str(e)) + if args.target_type and not targets: + parser.error( + "--target-type applies to --target/--target-list inputs; " + "--mount directories are always classified local_code." + ) + for target in targets: try: - target_type, target_dict = infer_target_type(target) + target_type, target_dict = resolve_target_type(target, args.target_type) if target_type == "local_code": display_target = target_dict.get("target_path", target) @@ -893,8 +924,8 @@ def parse_arguments() -> argparse.Namespace: targets_info.append( {"type": target_type, "details": target_dict, "original": display_target} ) - except ValueError: - parser.error(f"Invalid target '{target}'") + except ValueError as e: + parser.error(str(e)) try: targets_info.extend(build_mount_targets_info(mount_paths)) diff --git a/lyrashield/interface/utils.py b/lyrashield/interface/utils.py index 9a330348..9033d896 100644 --- a/lyrashield/interface/utils.py +++ b/lyrashield/interface/utils.py @@ -17,7 +17,6 @@ from urllib.parse import urlparse import docker -import requests from docker.errors import DockerException, ImageNotFound from rich.console import Console from rich.panel import Panel @@ -1113,17 +1112,6 @@ def resolve_diff_scope_context( ) -def _is_http_git_repo(url: str) -> bool: - check_url = f"{url.rstrip('/')}/info/refs?service=git-upload-pack" - try: - resp = requests.get(check_url, headers={"User-Agent": "git/strix"}, timeout=10) - except (requests.RequestException, ValueError): - return False - if resp.status_code >= 400: - return resp.status_code == 401 - return "x-git-upload-pack-advertisement" in resp.headers.get("Content-Type", "") - - def infer_target_type(target: str) -> tuple[str, dict[str, str]]: if not target: raise ValueError("Target must be a non-empty string") @@ -1138,15 +1126,15 @@ def infer_target_type(target: str) -> tuple[str, dict[str, str]]: parsed = urlparse(target) if parsed.scheme in ("http", "https"): + # Inference is offline-only: the engine never resolves DNS or sends HTTP + # requests to the target while classifying it. Credential-bearing URLs + # and ``.git`` remotes are still recognized as repositories, but an + # ambiguous non-suffixed HTTP(S) URL defaults to a web target — use + # ``--target-type repository`` for a bare HTTP(S) Git remote. if parsed.username or parsed.password: return "repository", {"target_repo": target} if parsed.path.rstrip("/").endswith(".git"): return "repository", {"target_repo": target} - if parsed.query or parsed.fragment: - return "web_application", {"target_url": target} - path_segments = [s for s in parsed.path.split("/") if s] - if len(path_segments) >= 2 and _is_http_git_repo(target): - return "repository", {"target_repo": target} return "web_application", {"target_url": target} try: @@ -1171,10 +1159,9 @@ def infer_target_type(target: str) -> tuple[str, dict[str, str]]: if "/" in target: host_part, _, path_part = target.partition("/") if "." in host_part and not host_part.startswith(".") and path_part: - full_url = f"https://{target}" - if _is_http_git_repo(full_url): - return "repository", {"target_repo": full_url} - return "web_application", {"target_url": full_url} + # Bare host/path (e.g. github.com/org/repo) is ambiguous; offline + # inference treats it as a web target rather than probing it. + return "web_application", {"target_url": f"https://{target}"} if "." in target and "/" not in target and not target.startswith("."): parts = target.split(".") @@ -1192,6 +1179,186 @@ def infer_target_type(target: str) -> tuple[str, dict[str, str]]: ) +TARGET_TYPE_CHOICES: tuple[str, ...] = ( + "repository", + "web_application", + "local_code", + "ip_address", +) + + +def _explicit_repository_ref(target: str) -> str: + """Return the normalized Git remote for a ``--target-type repository`` input.""" + if target.startswith(("git@", "git://")): + return target + parsed = urlparse(target) + if parsed.scheme in ("http", "https"): + if not parsed.netloc: + raise ValueError( + f"--target-type repository requires a usable Git remote; '{target}' has no host." + ) + return target + if parsed.scheme: + raise ValueError( + f"--target-type repository does not accept '{parsed.scheme}:' remotes. " + "Use an https://, git@host:path, or git:// remote." + ) + path = Path(target).expanduser() + try: + exists = path.exists() + except (OSError, RuntimeError) as e: + raise ValueError(f"Invalid target '{target}': {e!s}") from e + if exists: + raise ValueError( + f"--target-type repository requires a remote Git URL; '{target}' is a local " + "path. Use --target-type local_code or omit --target-type." + ) + if "/" in target: + host_part, _, path_part = target.partition("/") + if "." in host_part and not host_part.startswith(".") and path_part: + return f"https://{target}" + raise ValueError( + "--target-type repository requires a Git remote " + "(https://host/org/repo[.git], git@host:org/repo, or git://host/org/repo); " + f"'{target}' is not one. Omit --target-type to classify the input automatically." + ) + + +def _explicit_web_url(target: str) -> str: + """Return the normalized URL for a ``--target-type web_application`` input.""" + if target.startswith(("git@", "git://")): + raise ValueError( + f"--target-type web_application given a Git remote '{target}'. " + "Use --target-type repository or omit --target-type." + ) + parsed = urlparse(target) + if parsed.scheme in ("http", "https"): + if not parsed.netloc: + raise ValueError( + f"--target-type web_application requires a usable URL; '{target}' has no host." + ) + if parsed.username or parsed.password: + raise ValueError( + "--target-type web_application does not accept credentials embedded in " + f"'{target}'. Remove the credentials from the URL, or use " + "--target-type repository for a credential-bearing Git remote." + ) + if parsed.path.rstrip("/").endswith(".git"): + raise ValueError( + f"--target-type web_application given '{target}', which ends with '.git' — " + "a Git remote. Use --target-type repository or omit --target-type." + ) + return target + if parsed.scheme: + raise ValueError( + "--target-type web_application accepts http(s) URLs or domain names only; " + f"'{parsed.scheme}:' is not supported." + ) + try: + ipaddress.ip_address(target) + except ValueError: + pass + else: + raise ValueError( + f"--target-type web_application given IP address '{target}'. " + "Use --target-type ip_address or prefix it with http(s)://." + ) + path = Path(target).expanduser() + try: + exists = path.exists() + except (OSError, RuntimeError) as e: + raise ValueError(f"Invalid target '{target}': {e!s}") from e + if exists: + raise ValueError( + f"--target-type web_application given local path '{target}'. " + "Use --target-type local_code or omit --target-type." + ) + if target.endswith(".git"): + raise ValueError( + f"--target-type web_application given '{target}', which ends with '.git' — " + "a Git remote. Use --target-type repository or omit --target-type." + ) + if "/" in target: + host_part, _, path_part = target.partition("/") + if "." in host_part and not host_part.startswith(".") and path_part: + return f"https://{target}" + if "." in target and not target.startswith("."): + parts = target.split(".") + if len(parts) >= 2 and all(p and p.strip() for p in parts): + return f"https://{target}" + raise ValueError( + f"--target-type web_application requires an http(s) URL or a domain name; " + f"'{target}' is not one." + ) + + +def _explicit_local_path(target: str) -> str: + """Return the resolved directory for a ``--target-type local_code`` input.""" + parsed = urlparse(target) + if parsed.scheme in ("http", "https") or target.startswith(("git@", "git://")): + raise ValueError( + f"--target-type local_code requires a local directory; '{target}' is a remote " + "target. Use --target-type repository or --target-type web_application, or " + "omit --target-type." + ) + path = Path(target).expanduser() + try: + if path.exists(): + if path.is_dir(): + return str(path.resolve()) + raise ValueError(f"Path exists but is not a directory: {target}") + except (OSError, RuntimeError) as e: + raise ValueError(f"Invalid path: {target} - {e!s}") from e + raise ValueError( + f"--target-type local_code requires an existing local directory; '{target}' does " + "not exist. For a remote Git repository use --target-type repository." + ) + + +def _explicit_ip(target: str) -> str: + """Return the normalized address for a ``--target-type ip_address`` input.""" + try: + ip_obj = ipaddress.ip_address(target) + except ValueError: + raise ValueError( + f"--target-type ip_address requires an IPv4 or IPv6 address; '{target}' is " + "not one. For a host URL use --target-type web_application." + ) from None + return str(ip_obj) + + +def resolve_target_type( + target: str, explicit_kind: str | None = None +) -> tuple[str, dict[str, str]]: + """Classify *target*, honoring an operator-supplied *explicit_kind* when given. + + Both modes are offline-only: classification never resolves DNS and never + sends HTTP requests to the target. An explicit kind validates the input's + shape instead of probing it — catching a wrong-kind flag early — but it is + not authorization: URL credential, source-path, and target-authorization + checks still apply downstream. + """ + if explicit_kind is None: + return infer_target_type(target) + if explicit_kind not in TARGET_TYPE_CHOICES: + raise ValueError( + f"Unknown --target-type '{explicit_kind}'. " + f"Valid kinds: {', '.join(TARGET_TYPE_CHOICES)}." + ) + if not target: + raise ValueError("Target must be a non-empty string") + target = target.strip() + if not target: + raise ValueError("Target must be a non-empty string") + if explicit_kind == "repository": + return "repository", {"target_repo": _explicit_repository_ref(target)} + if explicit_kind == "web_application": + return "web_application", {"target_url": _explicit_web_url(target)} + if explicit_kind == "local_code": + return "local_code", {"target_path": _explicit_local_path(target)} + return "ip_address", {"target_ip": _explicit_ip(target)} + + def read_target_list_file(path_str: str) -> list[str]: """Read scan targets from a file, one target per non-empty, non-comment line.""" if not path_str or not path_str.strip(): diff --git a/tests/test_target_inference_no_network.py b/tests/test_target_inference_no_network.py new file mode 100644 index 00000000..1f93b29f --- /dev/null +++ b/tests/test_target_inference_no_network.py @@ -0,0 +1,506 @@ +"""Target classification must never touch the network or DNS from the host. + +Regression coverage for the removal of the ``_is_http_git_repo`` probe: host-side +inference used to issue an unauthenticated ``GET /info/refs?service=git- +upload-pack`` to whatever URL the operator passed — including private/internal +addresses — purely to decide whether the input was a repository. Classification +is now purely offline, and an explicit ``--target-type`` kind is validated +against the input shape without weakening any downstream credential, source-path, +or target-authorization checks. +""" + +from __future__ import annotations + +import socket +import sys +from importlib import import_module +from types import SimpleNamespace +from typing import TYPE_CHECKING, Any +from unittest.mock import Mock + +import pytest +import requests + +from lyrashield.interface import utils as interface_utils +from lyrashield.interface.utils import infer_target_type, resolve_target_type + + +if TYPE_CHECKING: + from pathlib import Path + + +cli_main: Any = import_module("lyrashield.interface.main") + + +@pytest.fixture(autouse=True) +def no_network(monkeypatch: pytest.MonkeyPatch) -> dict[str, Mock]: + """Fail closed on any host-side HTTP or DNS attempt during classification.""" + mocks = { + "get": Mock(name="requests.get"), + "head": Mock(name="requests.head"), + "request": Mock(name="requests.request"), + "dns": Mock(name="socket.getaddrinfo", side_effect=AssertionError("DNS attempted")), + "connect": Mock( + name="socket.create_connection", side_effect=AssertionError("TCP connect attempted") + ), + } + monkeypatch.setattr(requests, "get", mocks["get"]) + monkeypatch.setattr(requests, "head", mocks["head"]) + monkeypatch.setattr(requests, "request", mocks["request"]) + monkeypatch.setattr(socket, "getaddrinfo", mocks["dns"]) + monkeypatch.setattr(socket, "create_connection", mocks["connect"]) + return mocks + + +def _assert_no_http(mocks: dict[str, Mock]) -> None: + mocks["get"].assert_not_called() + mocks["head"].assert_not_called() + mocks["request"].assert_not_called() + + +def _stub_settings(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + cli_main, + "load_settings", + lambda: SimpleNamespace(runtime=SimpleNamespace(max_local_copy_mb=1024)), + ) + + +def _parse(monkeypatch: pytest.MonkeyPatch, argv: list[str]) -> Any: + _stub_settings(monkeypatch) + monkeypatch.setattr(sys, "argv", ["lyrashield", *argv]) + return cli_main.parse_arguments() + + +def test_url_inference_never_probes_from_host(no_network: dict[str, Mock]) -> None: + kind, _ = infer_target_type("http://127.0.0.1/private/repo") + + no_network["get"].assert_not_called() + assert kind == "web_application" + + +# --------------------------------------------------------------------------- +# Offline inference +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "target", + [ + "https://github.com/org/repo", + "https://gitlab.example.com/org/sub/repo", + "http://127.0.0.1/private/repo", + "http://127.0.0.1:8080/org/repo", + "https://10.0.0.5/org/repo", + "https://192.168.1.20/org/repo", + "http://[fd00::1]/org/repo", + "https://example.com/app?next=/login", + "https://example.com/app#frag", + ], +) +def test_ambiguous_http_urls_default_to_web_application_without_probing( + target: str, no_network: dict[str, Mock] +) -> None: + kind, details = infer_target_type(target) + + assert kind == "web_application" + assert details == {"target_url": target} + _assert_no_http(no_network) + + +def test_redirected_probe_is_never_attempted(no_network: dict[str, Mock]) -> None: + # Even a URL whose git-probe would redirect to a link-local/metadata address + # must receive zero requests — classification cannot follow redirects because + # it never opens a connection. + no_network["get"].return_value = Mock( + status_code=302, headers={"Location": "http://169.254.169.254/latest/meta-data"} + ) + + kind, _ = infer_target_type("https://public.example.com/org/repo") + + assert kind == "web_application" + _assert_no_http(no_network) + no_network["dns"].assert_not_called() + no_network["connect"].assert_not_called() + + +@pytest.mark.parametrize( + ("target", "expected_repo"), + [ + ("https://github.com/org/repo.git", "https://github.com/org/repo.git"), + ("http://[fd00::1]/org/repo.git", "http://[fd00::1]/org/repo.git"), + ("https://user:pass@github.com/org/repo", "https://user:pass@github.com/org/repo"), + ("git@github.com:org/repo.git", "git@github.com:org/repo.git"), + ("git://git.example.com/org/repo", "git://git.example.com/org/repo"), + ("uncloned-mirror.git", "uncloned-mirror.git"), + ], +) +def test_offline_recognized_repository_forms( + target: str, expected_repo: str, no_network: dict[str, Mock] +) -> None: + kind, details = infer_target_type(target) + + assert kind == "repository" + assert details == {"target_repo": expected_repo} + _assert_no_http(no_network) + + +def test_credential_bearing_url_still_classifies_as_repository( + no_network: dict[str, Mock], +) -> None: + # The URL credential rule is a classification input, not a probe result — + # it must keep working with zero requests. + kind, _ = infer_target_type("https://oauth2:token@gitlab.example.com/org/repo.git") + assert kind == "repository" + _assert_no_http(no_network) + + +@pytest.mark.parametrize( + ("target", "expected_ip"), + [ + ("192.168.1.10", "192.168.1.10"), + ("10.0.0.5", "10.0.0.5"), + ("::1", "::1"), + ("fd00::42", "fd00::42"), + ], +) +def test_ip_addresses_infer_without_dns( + target: str, expected_ip: str, no_network: dict[str, Mock] +) -> None: + kind, details = infer_target_type(target) + + assert kind == "ip_address" + assert details == {"target_ip": expected_ip} + _assert_no_http(no_network) + no_network["dns"].assert_not_called() + + +def test_local_directory_infers_local_code(tmp_path: Path, no_network: dict[str, Mock]) -> None: + kind, details = infer_target_type(str(tmp_path)) + + assert kind == "local_code" + assert details == {"target_path": str(tmp_path.resolve())} + _assert_no_http(no_network) + + +def test_existing_file_is_not_a_local_code_target( + tmp_path: Path, no_network: dict[str, Mock] +) -> None: + file_path = tmp_path / "a-file.txt" + file_path.write_text("x", encoding="utf-8") + + with pytest.raises(ValueError, match="not a directory"): + infer_target_type(str(file_path)) + _assert_no_http(no_network) + + +@pytest.mark.parametrize( + ("target", "expected_url"), + [ + ("example.com", "https://example.com"), + ("example.com/org/repo", "https://example.com/org/repo"), + ], +) +def test_bare_hosts_default_to_web_application( + target: str, expected_url: str, no_network: dict[str, Mock] +) -> None: + kind, details = infer_target_type(target) + + assert kind == "web_application" + assert details == {"target_url": expected_url} + _assert_no_http(no_network) + + +def test_invalid_target_still_raises(no_network: dict[str, Mock]) -> None: + with pytest.raises(ValueError, match="Invalid target"): + infer_target_type("not-a-target") + _assert_no_http(no_network) + + +# --------------------------------------------------------------------------- +# Explicit --target-type validation (kind must match input shape) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("target", "expected_repo"), + [ + ("https://github.com/org/repo", "https://github.com/org/repo"), + ("http://127.0.0.1:8443/org/repo", "http://127.0.0.1:8443/org/repo"), + ("git@github.com:org/repo.git", "git@github.com:org/repo.git"), + ("git://git.example.com/org/repo", "git://git.example.com/org/repo"), + ("example.com/org/repo", "https://example.com/org/repo"), + ], +) +def test_explicit_repository_accepts_remote_git_shapes( + target: str, expected_repo: str, no_network: dict[str, Mock] +) -> None: + kind, details = resolve_target_type(target, "repository") + + assert kind == "repository" + assert details == {"target_repo": expected_repo} + _assert_no_http(no_network) + + +@pytest.mark.parametrize( + "target", + [ + "example.com", + "192.168.1.10", + "ssh://git@github.com/org/repo.git", + ], +) +def test_explicit_repository_rejects_non_repository_shapes( + target: str, no_network: dict[str, Mock] +) -> None: + with pytest.raises(ValueError, match="--target-type repository"): + resolve_target_type(target, "repository") + _assert_no_http(no_network) + + +def test_explicit_repository_rejects_local_directory( + tmp_path: Path, no_network: dict[str, Mock] +) -> None: + with pytest.raises(ValueError, match="--target-type local_code"): + resolve_target_type(str(tmp_path), "repository") + _assert_no_http(no_network) + + +@pytest.mark.parametrize( + ("target", "expected_url"), + [ + ("https://app.example.com", "https://app.example.com"), + ("http://127.0.0.1:3000/app", "http://127.0.0.1:3000/app"), + ("http://[fd00::1]/app", "http://[fd00::1]/app"), + ("example.com", "https://example.com"), + ("example.com/path", "https://example.com/path"), + ], +) +def test_explicit_web_application_accepts_url_shapes( + target: str, expected_url: str, no_network: dict[str, Mock] +) -> None: + kind, details = resolve_target_type(target, "web_application") + + assert kind == "web_application" + assert details == {"target_url": expected_url} + _assert_no_http(no_network) + + +@pytest.mark.parametrize( + "target", + [ + "https://github.com/org/repo.git", + "git@github.com:org/repo.git", + "git://git.example.com/org/repo", + "192.168.1.10", + "ftp://example.com/pub", + ], +) +def test_explicit_web_application_rejects_non_url_shapes( + target: str, no_network: dict[str, Mock] +) -> None: + with pytest.raises(ValueError, match="--target-type"): + resolve_target_type(target, "web_application") + _assert_no_http(no_network) + + +def test_explicit_web_application_does_not_bypass_url_credential_check( + no_network: dict[str, Mock], +) -> None: + with pytest.raises(ValueError, match="credential"): + resolve_target_type("https://user:pass@example.com/app", "web_application") + _assert_no_http(no_network) + + +def test_explicit_local_code_requires_existing_directory( + tmp_path: Path, no_network: dict[str, Mock] +) -> None: + kind, details = resolve_target_type(str(tmp_path), "local_code") + + assert kind == "local_code" + assert details == {"target_path": str(tmp_path.resolve())} + _assert_no_http(no_network) + + +@pytest.mark.parametrize( + "target", + [ + "https://github.com/org/repo", + "git@github.com:org/repo.git", + "192.168.1.10", + "definitely/missing/path", + ], +) +def test_explicit_local_code_rejects_non_local_shapes( + target: str, no_network: dict[str, Mock] +) -> None: + with pytest.raises(ValueError, match="--target-type local_code"): + resolve_target_type(target, "local_code") + _assert_no_http(no_network) + + +def test_explicit_local_code_rejects_files(tmp_path: Path, no_network: dict[str, Mock]) -> None: + file_path = tmp_path / "a-file.txt" + file_path.write_text("x", encoding="utf-8") + + with pytest.raises(ValueError, match="not a directory"): + resolve_target_type(str(file_path), "local_code") + _assert_no_http(no_network) + + +@pytest.mark.parametrize("target", ["10.0.0.5", "fd00::42"]) +def test_explicit_ip_address_accepts_ip_literals(target: str, no_network: dict[str, Mock]) -> None: + kind, details = resolve_target_type(target, "ip_address") + + assert kind == "ip_address" + assert details == {"target_ip": target} + _assert_no_http(no_network) + + +@pytest.mark.parametrize("target", ["https://10.0.0.5/app", "example.com", "not-an-ip"]) +def test_explicit_ip_address_rejects_non_ip_shapes( + target: str, no_network: dict[str, Mock] +) -> None: + with pytest.raises(ValueError, match="--target-type ip_address"): + resolve_target_type(target, "ip_address") + _assert_no_http(no_network) + + +def test_unknown_explicit_kind_rejected(no_network: dict[str, Mock]) -> None: + with pytest.raises(ValueError, match="Unknown --target-type"): + resolve_target_type("https://example.com", "ssh_config") + _assert_no_http(no_network) + + +def test_omitted_kind_falls_back_to_offline_inference(no_network: dict[str, Mock]) -> None: + kind, details = resolve_target_type("git@github.com:org/repo.git", None) + + assert kind == "repository" + assert details == {"target_repo": "git@github.com:org/repo.git"} + _assert_no_http(no_network) + + +# --------------------------------------------------------------------------- +# CLI wiring +# --------------------------------------------------------------------------- + + +def test_help_lists_target_type_flag( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setattr(sys, "argv", ["lyrashield", "--help"]) + + with pytest.raises(SystemExit) as exc_info: + cli_main.parse_arguments() + + assert exc_info.value.code == 0 + assert "--target-type" in capsys.readouterr().out + + +def test_target_type_repository_marks_ambiguous_url_as_repository( + monkeypatch: pytest.MonkeyPatch, no_network: dict[str, Mock] +) -> None: + args = _parse( + monkeypatch, + ["-t", "https://github.com/org/repo", "--target-type", "repository", "-n"], + ) + + assert args.targets_info[0]["type"] == "repository" + assert args.targets_info[0]["details"]["target_repo"] == "https://github.com/org/repo" + _assert_no_http(no_network) + + +def test_target_type_mismatch_errors_actionably( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + no_network: dict[str, Mock], +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse( + monkeypatch, + ["-t", "https://github.com/org/repo.git", "--target-type", "web_application", "-n"], + ) + + assert exc_info.value.code == 2 + err = capsys.readouterr().err + assert "--target-type repository" in err + _assert_no_http(no_network) + + +def test_target_type_invalid_choice_errors( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse(monkeypatch, ["-t", "https://example.com", "--target-type", "api_spec", "-n"]) + + assert exc_info.value.code == 2 + assert "invalid choice" in capsys.readouterr().err + + +def test_target_type_requires_targets( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + tmp_path: Path, +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse( + monkeypatch, + ["--mount", str(tmp_path), "--target-type", "repository", "-n"], + ) + + assert exc_info.value.code == 2 + assert "--target-type" in capsys.readouterr().err + + +def test_target_type_rejects_resume( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse(monkeypatch, ["--resume", "old-run", "--target-type", "repository"]) + + assert exc_info.value.code == 2 + assert "--target-type" in capsys.readouterr().err + + +def test_target_type_applies_to_target_list_entries( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + no_network: dict[str, Mock], +) -> None: + target_list = tmp_path / "targets.txt" + target_list.write_text( + "https://github.com/org/one\ngit@github.com:org/two.git\n", encoding="utf-8" + ) + + args = _parse( + monkeypatch, + ["--target-list", str(target_list), "--target-type", "repository", "-n"], + ) + + assert [t["type"] for t in args.targets_info] == ["repository", "repository"] + _assert_no_http(no_network) + + +def test_explicit_flag_is_not_authorization_for_private_fetch( + monkeypatch: pytest.MonkeyPatch, no_network: dict[str, Mock] +) -> None: + # Classification records the kind only: a private-address repository target + # produces the same target dict as any other repository — the fetch still + # goes through the existing guarded clone path, never inference-time HTTP. + args = _parse( + monkeypatch, + ["-t", "http://192.168.1.20/internal/repo", "--target-type", "repository", "-n"], + ) + + entry = args.targets_info[0] + assert entry["type"] == "repository" + assert entry["details"]["target_repo"] == "http://192.168.1.20/internal/repo" + assert "cloned_repo_path" not in entry["details"] + _assert_no_http(no_network) + no_network["dns"].assert_not_called() + + +def test_explicit_repository_still_routes_through_guarded_clone() -> None: + # Approved acquisition stays on the existing clone_repository path; the flag + # must not create a second unchecked fetch path. + assert interface_utils.clone_repository is cli_main.clone_repository From 46780b778308fde6fdc47ad16bcc6994b54ffa15 Mon Sep 17 00:00:00 2001 From: ecryptoguru Date: Sat, 19 Sep 2026 21:49:08 +0530 Subject: [PATCH 2/6] feat(interface): add immutable revision and diff-head source acquisition --- docs/usage/cli.mdx | 16 +- lyrashield/artifacts/state.py | 2 + lyrashield/interface/cli.py | 2 + lyrashield/interface/main.py | 160 +++++- lyrashield/interface/tui/app.py | 2 + lyrashield/interface/utils.py | 416 ++++++++++++-- tests/test_local_sources.py | 12 +- tests/test_review_changes.py | 975 ++++++++++++++++++++++++++++++++ 8 files changed, 1537 insertions(+), 48 deletions(-) create mode 100644 tests/test_review_changes.py diff --git a/docs/usage/cli.mdx b/docs/usage/cli.mdx index d9d36263..1eb089ab 100644 --- a/docs/usage/cli.mdx +++ b/docs/usage/cli.mdx @@ -81,11 +81,19 @@ Serves the prebuilt local viewer SPA. Source lives in `lyrashield/interface/view - Code scope mode: `auto` (enable PR diff-scope in CI/headless runs), `diff` (force changed-files scope), or `full` (disable diff-scope). Diff scope narrows repository context for suitable CI/headless work; it is a scope statement, not proof that unchanged code or deployed behavior is safe. + Code scope mode: `auto` (enable PR diff-scope in CI/headless runs), `diff` (force changed-files scope — requires `--diff-base` and `--diff-head`), or `full` (disable diff-scope). Diff scope narrows repository context for suitable CI/headless work; it is a scope statement, not proof that unchanged code or deployed behavior is safe. - Target branch or commit to compare against (e.g., `origin/main`). Defaults to the repository's default branch. + Target branch or commit to compare against (e.g., `origin/main`). Defaults to the repository's default branch. With `--diff-head` this must be a full 40- or 64-character lowercase hex Git object ID. + + + + Asserted comparison head for diff-scope: a full 40- or 64-character lowercase hex Git object ID. Requires `--diff-base`; the checkout's `HEAD` must equal this revision or the run fails closed with a named preflight error. + + + + Exact immutable commit to check out for repository targets (full 40- or 64-character lowercase hex object ID). The clone detaches at this revision and `HEAD` is asserted to match; when combined with `--repository-branch`, the branch is only a fetch hint. @@ -151,8 +159,8 @@ uv run lyrashield --target ./approved-repository --instruction-file ./instructio # Cap cost and per-agent turns uv run lyrashield --target ./approved-repository -n --max-budget 25 --max-turns 300 -# Force diff-scope against a specific base ref -uv run lyrashield -n --target ./ --scan-mode quick --scope-mode diff --diff-base origin/main +# Review Changes: force diff-scope between immutable revisions +uv run lyrashield -n --target ./ --scan-mode quick --scope-mode diff --diff-base --diff-head # Multi-target white-box testing (operator-only; production sends one repository target) uv run lyrashield -t https://github.com/org/app -t https://staging.example.com diff --git a/lyrashield/artifacts/state.py b/lyrashield/artifacts/state.py index ba5045c3..fab07889 100644 --- a/lyrashield/artifacts/state.py +++ b/lyrashield/artifacts/state.py @@ -896,6 +896,8 @@ def set_scan_config(self, config: dict[str, Any]) -> None: "local_sources": sanitize_local_sources(config.get("local_sources", [])), "scope_mode": config.get("scope_mode", "auto"), "diff_base": config.get("diff_base"), + "diff_head": config.get("diff_head"), + "repository_revision": config.get("repository_revision"), } ) self._set_phase("running") diff --git a/lyrashield/interface/cli.py b/lyrashield/interface/cli.py index 98a4aadc..df2785ab 100644 --- a/lyrashield/interface/cli.py +++ b/lyrashield/interface/cli.py @@ -117,6 +117,8 @@ async def run_cli(args: Any) -> None: "local_sources": getattr(args, "local_sources", None) or [], "scope_mode": getattr(args, "scope_mode", "auto"), "diff_base": getattr(args, "diff_base", None), + "diff_head": getattr(args, "diff_head", None), + "repository_revision": getattr(args, "repository_revision", None), "resume_instruction": getattr(args, "user_explicit_instruction", None) or "", } diff --git a/lyrashield/interface/main.py b/lyrashield/interface/main.py index 745b1226..21fea956 100644 --- a/lyrashield/interface/main.py +++ b/lyrashield/interface/main.py @@ -11,6 +11,7 @@ import shutil import sys import tempfile +from datetime import UTC, datetime from pathlib import Path from typing import Any, cast @@ -36,6 +37,8 @@ from lyrashield.interface.tui import run_tui from lyrashield.interface.utils import ( TARGET_TYPE_CHOICES, + _is_full_git_commit_sha, + _is_git_object_id, assign_workspace_subdirs, build_final_stats_text, build_mount_targets_info, @@ -53,6 +56,7 @@ resolve_target_type, rewrite_localhost_targets, validate_config_file, + validate_git_object_id, validate_run_name, ) from lyrashield.lifecycle.inputs import DEFAULT_MAX_TURNS, make_model_settings @@ -692,7 +696,19 @@ def parse_arguments() -> argparse.Namespace: metavar="BRANCH", help=( "Git branch to clone for repository targets. " - "Intended for orchestrators that pin a target branch." + "Intended for orchestrators that pin a target branch. " + "When --repository-revision is set this is only a fetch hint." + ), + ) + parser.add_argument( + "--repository-revision", + type=validate_git_object_id, + metavar="SHA", + help=( + "Exact immutable commit to check out for repository targets " + "(full 40- or 64-character lowercase hex Git object ID). The clone " + "detaches at this revision and HEAD is asserted to match; a missing " + "revision is a named preflight failure, never a silent fallback." ), ) parser.add_argument( @@ -784,7 +800,19 @@ def parse_arguments() -> argparse.Namespace: type=str, help=( "Target branch or commit to compare against (e.g., origin/main). " - "Defaults to the repository's default branch." + "Defaults to the repository's default branch. With --diff-head " + "this must be a full 40- or 64-character lowercase hex object ID." + ), + ) + parser.add_argument( + "--diff-head", + type=validate_git_object_id, + metavar="SHA", + help=( + "Asserted comparison head for diff-scope (full 40- or 64-character " + "lowercase hex Git object ID). Requires --diff-base, and " + "--scope-mode diff requires both. The checkout's HEAD must equal " + "this revision or the run fails closed." ), ) @@ -866,9 +894,54 @@ def parse_arguments() -> argparse.Namespace: args.user_explicit_instruction = args.instruction if args.resume else None + # Immutable-revision flags (Review Changes): --diff-head asserts the + # comparison head, --repository-revision pins the remote checkout. Both + # accept only full object IDs (validated above), and when both are given + # they must name the same commit — the asserted head must equal the + # checked-out revision. + if args.diff_head and not args.diff_base: + parser.error("--diff-head requires --diff-base for the comparison base.") + + if args.diff_head and args.diff_base and not _is_git_object_id(args.diff_base.strip()): + parser.error( + "--diff-base must be a full 40- or 64-character lowercase hex Git " + "object ID when --diff-head is set (a moving branch name cannot be " + "the recorded comparison base)." + ) + + if args.scope_mode == "diff" and not (args.diff_base and args.diff_head): + parser.error( + "--scope-mode diff requires both --diff-base and --diff-head " + "(the immutable comparison revisions)." + ) + + if args.repository_revision and args.diff_head and args.repository_revision != args.diff_head: + parser.error( + "--repository-revision and --diff-head must name the same commit: " + "the asserted comparison head must equal the checked-out revision." + ) + + if args.repository_branch and _is_full_git_commit_sha(args.repository_branch): + branch_sha = args.repository_branch.lower() + if args.repository_revision and args.repository_revision != branch_sha: + parser.error( + f"--repository-branch {args.repository_branch} conflicts with " + f"--repository-revision {args.repository_revision}." + ) + if args.diff_head and args.diff_head != branch_sha: + parser.error( + f"--repository-branch {args.repository_branch} conflicts with " + f"--diff-head {args.diff_head}." + ) + if args.resume: if args.run_name: parser.error("Cannot combine --resume with --run-name") + if args.repository_revision or args.diff_head: + parser.error( + "Cannot combine --resume with --repository-revision/--diff-head. " + "A resumed run reuses the source revisions recorded in its run record." + ) if args.target_type: parser.error( "Cannot combine --resume with --target-type. A resumed run reuses the " @@ -935,6 +1008,15 @@ def parse_arguments() -> argparse.Namespace: targets_info = dedupe_local_targets(targets_info) args.targets_info = targets_info + if args.repository_revision and not any( + t.get("type") == "repository" for t in targets_info + ): + parser.error( + "--repository-revision requires at least one repository target " + "(a remote Git URL). For a checked-out local repository, use " + "--diff-base/--diff-head to assert its revisions instead." + ) + assign_workspace_subdirs(targets_info) rewrite_localhost_targets(targets_info, HOST_GATEWAY_HOSTNAME) @@ -955,7 +1037,9 @@ def parse_arguments() -> argparse.Namespace: return args -def _persist_run_record(args: argparse.Namespace) -> None: +def _persist_run_record( + args: argparse.Namespace, *, terminal: dict[str, Any] | None = None +) -> None: run_dir = run_dir_for(args.run_name) run_dir.mkdir(parents=True, exist_ok=True) # The first observable run.json must already be a complete versioned @@ -973,8 +1057,15 @@ def _persist_run_record(args: argparse.Namespace) -> None: "diff_scope": getattr(args, "diff_scope", {"active": False}), "scope_mode": args.scope_mode, "diff_base": args.diff_base, + "diff_head": getattr(args, "diff_head", None), + "repository_revision": getattr(args, "repository_revision", None), }, ) + if terminal: + # Terminal overrides (e.g. the no-change receipt) are applied after the + # canonical constructor — this is the deliberate end-state written by + # the engine itself, not caller-supplied forgery of required fields. + run_record.update(terminal) # Validate the pre-scan contract before writing, so an incomplete record # never reaches the run directory (I10/C3). validate_run_record(run_record) @@ -1081,6 +1172,11 @@ def _load_resume_state(args: argparse.Namespace, parser: argparse.ArgumentParser source["mount"] = True if state.get("diff_scope"): args.diff_scope = state.get("diff_scope") + # Restore recorded source provenance so a resumed run's run.json keeps the + # revisions it was launched with rather than overwriting them with None. + for key in ("scope_mode", "diff_base", "diff_head", "repository_revision"): + if state.get(key): + setattr(args, key, state.get(key)) persisted_scan_mode = state.get("scan_mode") if persisted_scan_mode and args.scan_mode == "deep": args.scan_mode = persisted_scan_mode @@ -1332,12 +1428,20 @@ def main() -> None: # persist provider credentials under the container home directory. warm_up_usages: list[tuple[str, Any]] = [] args.warm_up_usages = warm_up_usages - if not args.non_interactive: - asyncio.run(warm_up_llm(show_model_warning=False, usages=warm_up_usages)) args.run_name = args.resume or args.run_name or generate_run_name(args.targets_info) if not args.resume: + # --repository-revision pins the remote checkout; --diff-head asserts + # the comparison head. When both are absent a full-SHA + # --repository-branch is the legacy pin form. The validated diff-head + # doubles as the checkout revision when no explicit revision is given — + # Review Changes compares the recorded head, not a moving branch tip. + checkout_revision = args.repository_revision or args.diff_head + required_commits: tuple[str, ...] = () + if args.diff_base and _is_full_git_commit_sha(args.diff_base): + required_commits = (args.diff_base.lower(),) + for target_info in args.targets_info: if target_info["type"] == "repository": repo_url = target_info["details"]["target_repo"] @@ -1347,6 +1451,8 @@ def main() -> None: args.run_name, dest_name, args.repository_branch, + revision=checkout_revision, + required_commits=required_commits, ) target_info["details"]["cloned_repo_path"] = cloned_path @@ -1364,6 +1470,7 @@ def main() -> None: scope_mode=args.scope_mode, diff_base=args.diff_base, non_interactive=args.non_interactive, + diff_head=args.diff_head, ) except ValueError as e: console = Console() @@ -1385,6 +1492,44 @@ def main() -> None: sys.exit(1) args.diff_scope = diff_scope.metadata + + if diff_scope.active and diff_scope.metadata.get("no_change"): + # Empty analyzable diff: record a durable no-change receipt and + # stop. No sandbox, warm-up, or provider call is ever reached — + # the run record below is the entire output of this run. + _persist_run_record( + args, + terminal={ + "status": "completed", + "phase": "completed", + "terminal_reason": "no_change", + "end_time": datetime.now(UTC).isoformat(), + "instruction": None, + "instruction_chars": len(args.instruction or ""), + }, + ) + console = Console() + note_text = Text() + note_text.append("NO ANALYZABLE CHANGES", style="bold #22c55e") + note_text.append("\n\n", style="white") + note_text.append( + "Diff-scope resolved zero analyzable files " + f"({diff_scope.metadata.get('no_change_reason', 'empty_diff')}). " + "The run was recorded as a no-change receipt; no scan was launched.\n", + style="white", + ) + panel = Panel( + note_text, + title="[bold white]LYRASHIELD", + title_align="left", + border_style="#22c55e", + padding=(1, 2), + ) + console.print("\n") + console.print(panel) + console.print() + sys.exit(0) + if diff_scope.instruction_block: if args.instruction: args.instruction = f"{diff_scope.instruction_block}\n\n{args.instruction}" @@ -1393,6 +1538,9 @@ def main() -> None: _persist_run_record(args) + if not args.non_interactive: + asyncio.run(warm_up_llm(show_model_warning=False, usages=warm_up_usages)) + _telemetry_model = load_settings().llm.model _telemetry_scan_mode = args.scan_mode _telemetry_is_whitebox = is_whitebox_scan(args.targets_info) @@ -1455,6 +1603,8 @@ def _non_interactive_exit_code(report_state: Any | None) -> int: if report_state.run_record.get("status") == "completed": return 2 if report_state.vulnerability_reports else 0 match report_state.run_record.get("terminal_reason"): + case "no_change": + return 0 case "budget_exceeded": return 3 case "rate_limited": diff --git a/lyrashield/interface/tui/app.py b/lyrashield/interface/tui/app.py index 8112cfd3..d148bf5e 100644 --- a/lyrashield/interface/tui/app.py +++ b/lyrashield/interface/tui/app.py @@ -849,6 +849,8 @@ def _build_scan_config(self, args: argparse.Namespace) -> dict[str, Any]: "local_sources": getattr(args, "local_sources", None) or [], "scope_mode": getattr(args, "scope_mode", "auto"), "diff_base": getattr(args, "diff_base", None), + "diff_head": getattr(args, "diff_head", None), + "repository_revision": getattr(args, "repository_revision", None), "resume_instruction": getattr(args, "user_explicit_instruction", None) or "", } diff --git a/lyrashield/interface/utils.py b/lyrashield/interface/utils.py index 9033d896..b66c3311 100644 --- a/lyrashield/interface/utils.py +++ b/lyrashield/interface/utils.py @@ -1,6 +1,7 @@ # Modifications © 2026 LyraShield; based on upstream Strix (Apache-2.0) # Controlled subprocess boundary: all subprocess calls below resolve Git and use shell=False. import argparse +import hashlib import ipaddress import json import logging @@ -509,6 +510,52 @@ def generate_run_name(targets_info: list[dict[str, Any]] | None = None) -> str: _SUPPORTED_SCOPE_MODES = {"auto", "diff", "full"} _MAX_FILES_PER_SECTION = 120 +# Bounded source acquisition: clones/fetches must terminate rather than hang a +# worker run on a stalled or hostile remote. +_GIT_CLONE_TIMEOUT_SECONDS = 900 +_GIT_FETCH_TIMEOUT_SECONDS = 300 + + +class SourcePreflightError(ValueError): + """Named preflight failure while pinning or diffing repository source. + + ``reason`` is a stable machine-readable identifier (e.g. + ``"missing_revision"``) so callers and tests can distinguish a fail-closed + preflight rejection from a generic resolution error. A Review Changes run + that hits one of these must stop — it must never fall back to an + unpinned snapshot. + """ + + def __init__(self, reason: str, message: str) -> None: + self.reason = reason + # The stable reason token stays visible in CLI panels and logs. + super().__init__(f"[{reason}] {message}") + + +_GIT_OBJECT_ID_RE = re.compile(r"(?:[0-9a-f]{40}|[0-9a-f]{64})") + + +def _is_git_object_id(value: str) -> bool: + """True iff *value* is a full 40- or 64-character lowercase Git object ID.""" + return bool(_GIT_OBJECT_ID_RE.fullmatch(value)) + + +def validate_git_object_id(value: str) -> str: + """argparse ``type=`` validator for ``--repository-revision``/``--diff-head``. + + Only immutable full-length object IDs are accepted: abbreviated SHAs, + branch/tag names, ``HEAD`` expressions, and ref strings containing + shell-meaningful or option-injection characters are all rejected, so the + validated value is always safe to place in a Git argument array. + """ + candidate = value.strip() + if not _is_git_object_id(candidate): + raise argparse.ArgumentTypeError( + "must be a full 40- or 64-character lowercase hex Git object ID " + "(branches, tags, abbreviated SHAs, and ref expressions are not accepted)" + ) + return candidate + @dataclass class DiffEntry: @@ -530,23 +577,41 @@ class RepoDiffScope: deleted_files: list[str] analyzable_files: list[str] truncated_sections: dict[str, bool] = field(default_factory=dict[str, bool]) + copied_files: list[dict[str, Any]] = field(default_factory=list[dict[str, Any]]) + base_revision: str | None = None + head_revision: str | None = None + requested_base: str | None = None + requested_head: str | None = None + worktree_dirty: bool | None = None + snapshot_digest: str | None = None + context_files: list[str] = field(default_factory=list[str]) def to_metadata(self) -> dict[str, Any]: return { "source_path": self.source_path, "workspace_subdir": self.workspace_subdir, "base_ref": self.base_ref, + "base_revision": self.base_revision, "merge_base": self.merge_base, + "requested_base": self.requested_base, + "requested_head": self.requested_head, + "head_revision": self.head_revision, + "worktree_dirty": self.worktree_dirty, + "snapshot_digest": self.snapshot_digest, "added_files": self.added_files, "modified_files": self.modified_files, "renamed_files": self.renamed_files, + "copied_files": self.copied_files, "deleted_files": self.deleted_files, "analyzable_files": self.analyzable_files, + "context_files": self.context_files, "added_files_count": len(self.added_files), "modified_files_count": len(self.modified_files), "renamed_files_count": len(self.renamed_files), + "copied_files_count": len(self.copied_files), "deleted_files_count": len(self.deleted_files), "analyzable_files_count": len(self.analyzable_files), + "context_files_count": len(self.context_files), "truncated_sections": self.truncated_sections, } @@ -567,7 +632,7 @@ def _git_executable() -> str: def _run_git_command( - repo_path: Path, args: list[str], check: bool = True + repo_path: Path, args: list[str], check: bool = True, timeout: float = 5 ) -> subprocess.CompletedProcess[str]: # Controlled subprocess boundary: Git path is resolved and shell is disabled. return subprocess.run( # noqa: S603 # nosec B603 @@ -575,7 +640,7 @@ def _run_git_command( capture_output=True, text=True, check=check, - timeout=5, + timeout=timeout, ) @@ -802,6 +867,7 @@ def _classify_diff_entries(entries: list[DiffEntry]) -> dict[str, Any]: modified_files: list[str] = [] deleted_files: list[str] = [] renamed_files: list[dict[str, Any]] = [] + copied_files: list[dict[str, Any]] = [] analyzable_files: list[str] = [] analyzable_seen: set[str] = set() modified_seen: set[str] = set() @@ -839,6 +905,13 @@ def _classify_diff_entries(entries: list[DiffEntry]) -> dict[str, Any]: continue if entry.status == "C": + copied_files.append( + { + "old_path": entry.old_path, + "new_path": path, + "similarity": entry.similarity, + } + ) _append_unique(modified_files, modified_seen, path) _append_unique(analyzable_files, analyzable_seen, path) continue @@ -851,6 +924,7 @@ def _classify_diff_entries(entries: list[DiffEntry]) -> dict[str, Any]: "modified_files": modified_files, "deleted_files": deleted_files, "renamed_files": renamed_files, + "copied_files": copied_files, "analyzable_files": analyzable_files, } @@ -880,7 +954,17 @@ def build_diff_scope_instruction(scopes: list[RepoDiffScope]) -> str: lines.append("") lines.append(f"Repository Scope: {repo_name}") lines.append(f"Base reference: {scope.base_ref}") + if scope.base_revision and scope.base_revision != scope.base_ref: + lines.append(f"Base revision: {scope.base_revision}") lines.append(f"Merge base: {scope.merge_base}") + if scope.head_revision: + lines.append(f"Head revision: {scope.head_revision}") + if scope.worktree_dirty: + lines.append( + "Note: the worktree contains uncommitted changes " + f"(snapshot {scope.snapshot_digest}); the analyzed content is the " + "working tree, not exactly the recorded head commit." + ) focus_files, focus_truncated = _truncate_file_list(scope.analyzable_files) scope.truncated_sections["analyzable_files"] = focus_truncated @@ -923,6 +1007,19 @@ def build_diff_scope_instruction(scopes: list[RepoDiffScope]) -> str: lines.append("Renamed files:") lines.extend(rename_lines) + if scope.copied_files: + copy_lines = [] + for copied in scope.copied_files: + old_path = str(copied.get("old_path") or "unknown") + new_path = str(copied.get("new_path") or "unknown") + similarity = copied.get("similarity") + if isinstance(similarity, int): + copy_lines.append(f"- {old_path} -> {new_path} (similarity {similarity}%)") + else: + copy_lines.append(f"- {old_path} -> {new_path}") + lines.append("Copied files:") + lines.extend(copy_lines) + deleted_files, deleted_truncated = _truncate_file_list(scope.deleted_files) scope.truncated_sections["deleted_files"] = deleted_truncated if deleted_files: @@ -934,6 +1031,62 @@ def build_diff_scope_instruction(scopes: list[RepoDiffScope]) -> str: return "\n".join(lines).strip() +def _resolve_commit_sha(repo_path: Path, expr: str, reason: str) -> str: + """Resolve *expr* to a full commit object ID, or raise a named preflight error. + + Ref expressions are resolved once here; downstream ``merge-base``/``diff`` + invocations only ever receive the resolved hex object ID, so an untrusted + ref string can never be reinterpreted as a command-line option. + """ + if not expr or expr.startswith("-") or any(char.isspace() for char in expr): + raise SourcePreflightError(reason, f"Unsafe or empty revision expression: {expr!r}") + try: + result = _run_git_command( + repo_path, ["rev-parse", "--verify", "--quiet", f"{expr}^{{commit}}"], check=False + ) + except (OSError, subprocess.SubprocessError) as e: + raise SourcePreflightError( + reason, f"Could not resolve revision '{expr}' in '{repo_path}': {e}" + ) from e + sha = result.stdout.strip() if result.returncode == 0 else "" + if not _is_git_object_id(sha): + raise SourcePreflightError( + reason, + f"Required commit '{expr}' is not available in '{repo_path}'. " + "Fetch the referenced revision or provide full history; " + "Review Changes never falls back to an unpinned snapshot.", + ) + return sha + + +def _worktree_snapshot_state(repo_path: Path) -> tuple[bool | None, str | None]: + """Return ``(worktree_dirty, snapshot_digest)`` for honest non-commit provenance. + + A dirty worktree means the analyzed content is not exactly the recorded + head commit. The digest covers the porcelain status (which names untracked + paths) plus the full ``HEAD`` diff of tracked content, so two snapshots are + comparable without pretending uncommitted content is the commit. + """ + try: + status = _run_git_command_raw(repo_path, ["status", "--porcelain=v1", "-z"], check=False) + except (OSError, subprocess.SubprocessError): + return None, None + if status.returncode != 0: + return None, None + if not status.stdout.strip(b"\x00"): + return False, None + try: + diff = _run_git_command_raw(repo_path, ["diff", "--binary", "HEAD", "--"], check=False) + except (OSError, subprocess.SubprocessError): + diff = subprocess.CompletedProcess(args=[], returncode=1, stdout=b"") + digest = hashlib.sha256() + digest.update(status.stdout) + digest.update(b"\x00diff\x00") + if diff.returncode == 0: + digest.update(diff.stdout) + return True, f"sha256:{digest.hexdigest()}" + + def _should_activate_auto_scope( local_sources: list[dict[str, str]], non_interactive: bool, env: dict[str, str] ) -> bool: @@ -961,59 +1114,125 @@ def _should_activate_auto_scope( def _resolve_repo_diff_scope( - source: dict[str, str], diff_base: str | None, env: dict[str, str] + source: dict[str, str], + diff_base: str | None, + env: dict[str, str], + diff_head: str | None = None, ) -> RepoDiffScope: source_path = source.get("source_path", "") workspace_subdir = source.get("workspace_subdir") repo_path = Path(source_path) if not _is_git_repo(repo_path): - raise ValueError(f"Source is not a git repository: {source_path}") + raise SourcePreflightError( + "not_a_git_repo", f"Source is not a git repository: {source_path}" + ) if _is_repo_shallow(repo_path): - raise ValueError( + raise SourcePreflightError( + "insufficient_history", "LyraShield requires full git history for diff-scope. Please set fetch-depth: 0 " - "in your CI config." + "in your CI config.", ) + # Resolve HEAD first so the asserted comparison head is checked against the + # actual checkout — Review Changes must refuse to analyze a different + # revision than the one it recorded. + head_revision = _resolve_commit_sha(repo_path, "HEAD", "head_unavailable") + requested_base = diff_base.strip() if diff_base else None + if diff_head: + if head_revision != diff_head: + raise SourcePreflightError( + "head_mismatch", + f"Requested diff head {diff_head} does not match the checkout at " + f"'{source_path}' (HEAD is {head_revision}). The requested immutable " + "head must equal the checkout; refusing to analyze a different revision.", + ) + if not requested_base: + raise SourcePreflightError( + "missing_base", "--diff-head requires --diff-base to compare against." + ) + if not _is_git_object_id(requested_base): + raise SourcePreflightError( + "invalid_base", + f"--diff-base '{requested_base}' must be a full Git object ID when " + "--diff-head is set.", + ) + base_ref = _resolve_base_ref(repo_path, diff_base, env) - merge_base_result = _run_git_command(repo_path, ["merge-base", base_ref, "HEAD"], check=False) + # Resolve the base once; merge-base and the diff range below only consume + # the resolved object ID, never the raw ref expression. + base_revision = _resolve_commit_sha(repo_path, base_ref, "missing_base") + + try: + merge_base_result = _run_git_command( + repo_path, ["merge-base", base_revision, head_revision], check=False + ) + except (OSError, subprocess.SubprocessError) as e: + raise SourcePreflightError( + "insufficient_history", + f"Unable to compute merge-base against '{base_ref}' for '{source_path}': {e}", + ) from e if merge_base_result.returncode != 0: stderr = merge_base_result.stderr.strip() - raise ValueError( + raise SourcePreflightError( + "insufficient_history", f"Unable to compute merge-base against '{base_ref}' for '{source_path}'. " - f"{stderr or 'Ensure the base branch history is fetched and reachable.'}" + f"{stderr or 'Ensure the base branch history is fetched and reachable.'}", ) merge_base = merge_base_result.stdout.strip() if not merge_base: - raise ValueError( + raise SourcePreflightError( + "insufficient_history", f"Unable to compute merge-base against '{base_ref}' for '{source_path}'. " - "Ensure the base branch history is fetched and reachable." + "Ensure the base branch history is fetched and reachable.", ) - diff_result = _run_git_command_raw( - repo_path, - [ - "diff", - "--name-status", - "-z", - "--find-renames", - "--find-copies", - f"{merge_base}...HEAD", - ], - check=False, - ) + try: + diff_result = _run_git_command_raw( + repo_path, + [ + "diff", + "--name-status", + "-z", + "--find-renames", + "--find-copies", + f"{merge_base}...{head_revision}", + ], + check=False, + ) + except (OSError, subprocess.SubprocessError) as e: + raise SourcePreflightError( + "insufficient_history", + f"Unable to resolve changed files for '{source_path}': {e}", + ) from e if diff_result.returncode != 0: stderr = diff_result.stderr.decode("utf-8", errors="replace").strip() - raise ValueError( + raise SourcePreflightError( + "insufficient_history", f"Unable to resolve changed files for '{source_path}'. " - f"{stderr or 'Ensure the repository has enough history for diff-scope.'}" + f"{stderr or 'Ensure the repository has enough history for diff-scope.'}", ) entries = _parse_name_status_z(diff_result.stdout) classified = _classify_diff_entries(entries) + worktree_dirty, snapshot_digest = _worktree_snapshot_state(repo_path) + + context_files: list[str] = [] + context_seen: set[str] = set() + for deleted in classified["deleted_files"]: + _append_unique(context_files, context_seen, deleted) + for rename in classified["renamed_files"]: + old_path = rename.get("old_path") + if isinstance(old_path, str): + _append_unique(context_files, context_seen, old_path) + for copied in classified["copied_files"]: + old_path = copied.get("old_path") + if isinstance(old_path, str): + _append_unique(context_files, context_seen, old_path) + return RepoDiffScope( source_path=source_path, workspace_subdir=workspace_subdir, @@ -1024,6 +1243,14 @@ def _resolve_repo_diff_scope( renamed_files=classified["renamed_files"], deleted_files=classified["deleted_files"], analyzable_files=classified["analyzable_files"], + copied_files=classified["copied_files"], + base_revision=base_revision, + head_revision=head_revision, + requested_base=requested_base, + requested_head=diff_head, + worktree_dirty=worktree_dirty, + snapshot_digest=snapshot_digest, + context_files=context_files, ) @@ -1033,6 +1260,7 @@ def resolve_diff_scope_context( diff_base: str | None, non_interactive: bool, env: dict[str, str] | None = None, + diff_head: str | None = None, ) -> DiffScopeResult: if scope_mode not in _SUPPORTED_SCOPE_MODES: raise ValueError(f"Unsupported scope mode: {scope_mode}") @@ -1069,9 +1297,13 @@ def resolve_diff_scope_context( skipped_non_git.append(source_path) continue try: - repo_scopes.append(_resolve_repo_diff_scope(source, diff_base, env_map)) + repo_scopes.append(_resolve_repo_diff_scope(source, diff_base, env_map, diff_head)) except ValueError as e: - if scope_mode == "auto": + # Auto-mode may degrade to a full snapshot for heuristic inputs — + # but never when an immutable head was asserted. An explicit + # Review Changes comparison fails closed instead of silently + # analyzing the wrong revision. + if scope_mode == "auto" and diff_head is None: skipped_diff_scope.append(f"{source_path} (diff-scope skipped: {e})") continue raise @@ -1091,14 +1323,34 @@ def resolve_diff_scope_context( ) instruction_block = build_diff_scope_instruction(repo_scopes) + total_analyzable = sum(len(scope.analyzable_files) for scope in repo_scopes) + total_deleted = sum(len(scope.deleted_files) for scope in repo_scopes) + total_changed = sum( + len(scope.added_files) + + len(scope.modified_files) + + len(scope.renamed_files) + + len(scope.copied_files) + + len(scope.deleted_files) + for scope in repo_scopes + ) metadata = { "active": True, "mode": scope_mode, + "requested_base": diff_base, + "requested_head": diff_head, "repos": [scope.to_metadata() for scope in repo_scopes], "total_repositories": len(repo_scopes), - "total_analyzable_files": sum(len(scope.analyzable_files) for scope in repo_scopes), - "total_deleted_files": sum(len(scope.deleted_files) for scope in repo_scopes), + "total_analyzable_files": total_analyzable, + "total_deleted_files": total_deleted, + "total_changed_files": total_changed, + "limits": {"max_files_per_section": _MAX_FILES_PER_SECTION}, } + if total_analyzable == 0: + # Explicit applicability accounting: an empty analyzable diff is a + # no-change receipt (no provider calls), and deleted-only diffs record + # the deleted paths as context-only rather than silently scanning. + metadata["no_change"] = True + metadata["no_change_reason"] = "empty_diff" if total_changed == 0 else "no_analyzable_files" if skipped_non_git: metadata["skipped_non_git_sources"] = skipped_non_git if skipped_diff_scope: @@ -1661,7 +1913,79 @@ def _print_clone_error(console: Console, message: str) -> None: def _is_full_git_commit_sha(value: str) -> bool: - return bool(re.fullmatch(r"[0-9a-fA-F]{40}", value)) + return bool(re.fullmatch(r"(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})", value)) + + +def _print_source_preflight_error(console: Console, error: SourcePreflightError) -> None: + error_text = Text() + error_text.append("SOURCE PREFLIGHT FAILED", style="bold red") + error_text.append("\n\n", style="white") + error_text.append(f"{error}\n", style="white") + panel = Panel( + error_text, + title="[bold white]LYRASHIELD", + title_align="left", + border_style="red", + padding=(1, 2), + ) + console.print("\n") + console.print(panel) + console.print() + + +def _commit_available(repo_path: Path, sha: str) -> bool: + return _git_ref_exists(repo_path, f"{sha}^{{commit}}") + + +def _ensure_commit_available(repo_path: Path, sha: str, reason: str) -> None: + """Ensure commit *sha* exists locally; one bounded fetch, then fail closed.""" + if _commit_available(repo_path, sha): + return + try: + fetch = _run_git_command( + repo_path, + ["fetch", "origin", sha], + check=False, + timeout=_GIT_FETCH_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.SubprocessError) as e: + raise SourcePreflightError(reason, f"Fetching required commit {sha} failed: {e}") from e + if fetch.returncode != 0 or not _commit_available(repo_path, sha): + stderr = fetch.stderr.strip() + raise SourcePreflightError( + reason, + f"Required commit {sha} is not available in '{repo_path}'" + + (f": {stderr}" if stderr else "."), + ) + + +def _assert_checkout_revision(repo_path: Path, revision: str) -> None: + """Detach the checkout at *revision* and assert HEAD's object ID matches. + + A branch name is only a fetch hint — the recorded immutable revision is + what must end up checked out. If the commit is not advertised (e.g. a + force-pushed or non-branch head), one bounded direct fetch is attempted + before failing closed. + """ + checkout = _run_git_command(repo_path, ["checkout", "--detach", revision], check=False) + if checkout.returncode != 0: + _ensure_commit_available(repo_path, revision, "missing_revision") + try: + _run_git_command(repo_path, ["checkout", "--detach", revision], check=True) + except subprocess.CalledProcessError as e: + detail = e.stderr.strip() if isinstance(e.stderr, str) else str(e) + raise SourcePreflightError( + "checkout_failed", + f"Unable to detach '{repo_path}' at revision {revision}: {detail}", + ) from e + actual = _run_git_command(repo_path, ["rev-parse", "HEAD"], check=False) + actual_sha = actual.stdout.strip() if actual.returncode == 0 else "" + if actual_sha != revision: + raise SourcePreflightError( + "checkout_mismatch", + f"Checkout assertion failed for '{repo_path}': requested {revision} " + f"but HEAD is {actual_sha or 'unresolved'}.", + ) def clone_repository( @@ -1669,6 +1993,9 @@ def clone_repository( run_name: str, dest_name: str | None = None, branch: str | None = None, + *, + revision: str | None = None, + required_commits: tuple[str, ...] = (), ) -> str: console = Console() @@ -1694,12 +2021,18 @@ def clone_repository( if clone_path.exists(): shutil.rmtree(clone_path) + # An explicit --repository-revision (or a full-SHA --repository-branch, the + # legacy form) pins the checkout to an immutable commit. A branch name is + # only a fetch hint — never the checkout source — so the pinned path clones + # all refs with --no-checkout and detaches at the object ID afterwards. + pinned_revision = revision or (branch if branch and _is_full_git_commit_sha(branch) else None) + try: with console.status(f"[bold cyan]Cloning repository {repo_url}...", spinner="dots"): # Controlled subprocess boundary: Git path is resolved, shell=False, # and -- terminates option parsing before the user-controlled repository URL. clone_args = [git_executable, "clone"] - if branch and _is_full_git_commit_sha(branch): + if pinned_revision: # A full commit SHA is an immutable revision, not a remote branch. Clone the # repository normally so reachable refs are fetched, then detach at that revision. # ``git clone --branch --single-branch`` fails because a SHA is not an @@ -1713,17 +2046,24 @@ def clone_repository( capture_output=True, text=True, check=True, + timeout=_GIT_CLONE_TIMEOUT_SECONDS, ) - if branch and _is_full_git_commit_sha(branch): - subprocess.run( # noqa: S603 # nosec B603 - [git_executable, "-C", str(clone_path), "checkout", "--detach", branch], - capture_output=True, - text=True, - check=True, - ) + if pinned_revision: + _assert_checkout_revision(clone_path, pinned_revision.lower()) + for required in required_commits: + _ensure_commit_available(clone_path, required, "missing_base") return str(clone_path.absolute()) + except SourcePreflightError as e: + _print_source_preflight_error(console, e) + sys.exit(1) + except subprocess.TimeoutExpired as e: + _print_clone_error( + console, + f"Timed out acquiring repository {repo_url}: {e}", + ) + sys.exit(1) except subprocess.CalledProcessError as e: error_text = Text() error_text.append("REPOSITORY CLONE FAILED", style="bold red") diff --git a/tests/test_local_sources.py b/tests/test_local_sources.py index f254e1b8..4de34e12 100644 --- a/tests/test_local_sources.py +++ b/tests/test_local_sources.py @@ -4,6 +4,7 @@ import logging import shutil +import subprocess # nosec B404 import sys from typing import TYPE_CHECKING, Any from unittest.mock import patch @@ -195,10 +196,15 @@ def test_clone_repository_checks_out_the_requested_branch(tmp_path: Path) -> Non def test_clone_repository_checks_out_a_full_commit_sha_detached(tmp_path: Path) -> None: commit_sha = "a" * 40 + + def fake_run(argv: list[str], **_kwargs: Any) -> Any: + stdout = commit_sha if argv[:1] == ["/usr/bin/git"] and "rev-parse" in argv else "" + return subprocess.CompletedProcess(argv, 0, stdout=stdout, stderr="") + with ( patch.object(interface_utils, "_git_executable", return_value="/usr/bin/git"), patch.object(interface_utils.tempfile, "gettempdir", return_value=str(tmp_path)), - patch.object(interface_utils.subprocess, "run") as run, + patch.object(interface_utils.subprocess, "run", side_effect=fake_run) as run, ): clone_repository("https://github.com/org/repo", "sha-run", branch=commit_sha) @@ -213,6 +219,10 @@ def test_clone_repository_checks_out_a_full_commit_sha_detached(tmp_path: Path) "--detach", commit_sha, ] + # The pinned checkout must be verified: a rev-parse HEAD assertion runs + # after the detach so a mismatched checkout can never pass silently. + rev_parse_argv = run.call_args_list[2].args[0] + assert rev_parse_argv[-2:] == ["rev-parse", "HEAD"] def test_build_mount_targets_info_for_valid_dir(tmp_path: Path) -> None: diff --git a/tests/test_review_changes.py b/tests/test_review_changes.py new file mode 100644 index 00000000..4c521295 --- /dev/null +++ b/tests/test_review_changes.py @@ -0,0 +1,975 @@ +"""Review Changes: immutable revision acquisition and diff-scope provenance. + +Coverage for the engine half of the Review Changes workflow: + +- ``--repository-revision``/``--diff-head`` accept only full lowercase hex + object IDs (40 or 64 chars) — never branches, tags, abbreviated SHAs, or + ref strings with option-injection potential. +- Source acquisition pins the checkout to the recorded revision + (``checkout --detach`` + ``rev-parse HEAD`` assertion), treats a branch as + a fetch hint only, performs one bounded fetch for missing objects, and + fails closed with a named preflight error. +- Diff-scope records requested base/head, resolved revisions, merge base, + analyzed files, context-only files, and limits as run provenance; an empty + analyzable diff short-circuits to a no-change receipt with zero provider + calls. + +All Git operations run against ``git init`` fixtures in ``tmp_path`` or are +mocked — no network is touched. +""" + +from __future__ import annotations + +import argparse +import json +import subprocess # nosec B404 +import sys +from importlib import import_module +from types import SimpleNamespace +from typing import TYPE_CHECKING, Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + + +if TYPE_CHECKING: + from pathlib import Path + +import lyrashield.interface.utils as interface_utils +from lyrashield.interface.utils import ( + SourcePreflightError, + clone_repository, + resolve_diff_scope_context, + validate_git_object_id, +) + + +cli_main: Any = import_module("lyrashield.interface.main") + +SHA_A = "a" * 40 +SHA_B = "b" * 40 +SHA_F = "f" * 40 +SHA64 = "ab" * 32 + + +# --------------------------------------------------------------------------- +# Git fixture helpers (all local — file paths and git init only) +# --------------------------------------------------------------------------- + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( # noqa: S603 # nosec B603 + ["git", "-C", str(repo), *args], # noqa: S607 + capture_output=True, + text=True, + check=True, + timeout=30, + ) + + +def _init_repo(path: Path) -> Path: + repo = path / "repo" + repo.mkdir() + subprocess.run( # noqa: S603 # nosec B603 + ["git", "init", "-b", "main", str(repo)], # noqa: S607 + check=True, + capture_output=True, + ) + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "user.name", "Test") + _git(repo, "config", "commit.gpgsign", "false") + return repo + + +def _commit_all(repo: Path, message: str) -> str: + _git(repo, "add", "-A") + _git(repo, "commit", "-m", message, "--allow-empty") + return _git(repo, "rev-parse", "HEAD").stdout.strip() + + +def _sources(repo: Path) -> list[dict[str, Any]]: + return [{"source_path": str(repo), "workspace_subdir": "repo", "mount": False}] + + +@pytest.fixture +def diff_repo(tmp_path: Path) -> dict[str, Any]: + """Two-commit repo: base on main, feature branch with add/modify/delete/ + rename/copy changes.""" + repo = _init_repo(tmp_path) + (repo / "keep.py").write_text("print('v1')\n", encoding="utf-8") + (repo / "deleted.py").write_text("gone\n", encoding="utf-8") + (repo / "old_name.py").write_text("x = 1\n", encoding="utf-8") + (repo / "shared.py").write_text("def f():\n return 1\n", encoding="utf-8") + base = _commit_all(repo, "base") + + _git(repo, "checkout", "-b", "feature") + (repo / "keep.py").write_text("print('v2')\nprint('extra')\n", encoding="utf-8") + (repo / "added.py").write_text("new file\n", encoding="utf-8") + (repo / "deleted.py").unlink() + _git(repo, "mv", "old_name.py", "renamed.py") + (repo / "copied.py").write_text("def f():\n return 1\n", encoding="utf-8") + head = _commit_all(repo, "feature work") + return {"path": repo, "base": base, "head": head} + + +# --------------------------------------------------------------------------- +# Object-ID validation +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("sha", [SHA_A, SHA64]) +def test_validate_git_object_id_accepts_full_hex(sha: str) -> None: + assert validate_git_object_id(sha) == sha + + +@pytest.mark.parametrize( + "value", + [ + "abc123", # abbreviated SHA + "main", # branch name + "origin/main", # ref path + "HEAD", + "HEAD~1", + "main@{u}", + "refs/heads/main", + "A" * 40, # uppercase rejected — stored plans use lowercase + "g" * 40, # non-hex + "a" * 39, + "a" * 41, + "a" * 63, + "-x", # option-shaped + "--upload-pack=evil", + f"{SHA_A};rm -rf /", + "$(id)", + f"{SHA_A} extra", + "", + ], +) +def test_validate_git_object_id_rejects_untrusted_refs(value: str) -> None: + with pytest.raises(argparse.ArgumentTypeError, match="full 40- or 64-character"): + validate_git_object_id(value) + + +# --------------------------------------------------------------------------- +# CLI flag wiring +# --------------------------------------------------------------------------- + + +def _stub_settings(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + cli_main, + "load_settings", + lambda: SimpleNamespace(runtime=SimpleNamespace(max_local_copy_mb=1024)), + ) + + +def _parse(monkeypatch: pytest.MonkeyPatch, argv: list[str]) -> Any: + _stub_settings(monkeypatch) + monkeypatch.setattr(sys, "argv", ["lyrashield", *argv]) + return cli_main.parse_arguments() + + +_REPO = "https://github.com/org/repo.git" + + +def test_help_lists_revision_flags( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setattr(sys, "argv", ["lyrashield", "--help"]) + with pytest.raises(SystemExit) as exc_info: + cli_main.parse_arguments() + assert exc_info.value.code == 0 + out = capsys.readouterr().out + assert "--repository-revision" in out + assert "--diff-head" in out + + +def test_repository_revision_parses(monkeypatch: pytest.MonkeyPatch) -> None: + args = _parse(monkeypatch, ["-t", _REPO, "--repository-revision", SHA_A, "-n"]) + assert args.repository_revision == SHA_A + + +def test_diff_head_requires_diff_base( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse(monkeypatch, ["-t", _REPO, "--diff-head", SHA_B, "-n"]) + assert exc_info.value.code == 2 + assert "--diff-head requires --diff-base" in capsys.readouterr().err + + +@pytest.mark.parametrize( + "argv_tail", + [ + ["--scope-mode", "diff"], + ["--scope-mode", "diff", "--diff-base", SHA_A], + ["--scope-mode", "diff", "--diff-head", SHA_B], + ], +) +def test_scope_mode_diff_requires_both_revisions( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], argv_tail: list[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse(monkeypatch, ["-t", _REPO, "-n", *argv_tail]) + assert exc_info.value.code == 2 + assert "--diff-base" in capsys.readouterr().err + + +def test_scope_mode_diff_with_both_revisions_parses(monkeypatch: pytest.MonkeyPatch) -> None: + args = _parse( + monkeypatch, + [ + "-t", + _REPO, + "--scope-mode", + "diff", + "--diff-base", + SHA_A, + "--diff-head", + SHA_B, + "-n", + ], + ) + assert args.diff_base == SHA_A + assert args.diff_head == SHA_B + assert args.scope_mode == "diff" + + +def test_revision_and_head_must_agree( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse( + monkeypatch, + [ + "-t", + _REPO, + "--scope-mode", + "diff", + "--diff-base", + SHA_A, + "--diff-head", + SHA_B, + "--repository-revision", + SHA_F, + "-n", + ], + ) + assert exc_info.value.code == 2 + assert "must name the same commit" in capsys.readouterr().err + + +def test_sha_branch_conflicts_with_revision( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse( + monkeypatch, + [ + "-t", + _REPO, + "--repository-branch", + SHA_B, + "--repository-revision", + SHA_A, + "-n", + ], + ) + assert exc_info.value.code == 2 + assert "conflicts with --repository-revision" in capsys.readouterr().err + + +def test_repository_revision_requires_repository_target( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], tmp_path: Path +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse( + monkeypatch, + ["-t", str(tmp_path), "--repository-revision", SHA_A, "-n"], + ) + assert exc_info.value.code == 2 + assert "requires at least one repository target" in capsys.readouterr().err + + +@pytest.mark.parametrize("flag", ["--repository-revision", "--diff-head"]) +def test_revision_flags_reject_resume( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], flag: str +) -> None: + argv = ["--resume", "old-run", flag, SHA_A] + if flag == "--diff-head": + argv += ["--diff-base", SHA_B] + with pytest.raises(SystemExit) as exc_info: + _parse(monkeypatch, argv) + assert exc_info.value.code == 2 + assert "--resume" in capsys.readouterr().err + + +def test_diff_base_must_be_object_id_when_head_set( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as exc_info: + _parse( + monkeypatch, + [ + "-t", + _REPO, + "--scope-mode", + "diff", + "--diff-base", + "main", + "--diff-head", + SHA_B, + "-n", + ], + ) + assert exc_info.value.code == 2 + assert "--diff-base" in capsys.readouterr().err + + +def test_scope_mode_full_needs_no_revisions(monkeypatch: pytest.MonkeyPatch) -> None: + args = _parse(monkeypatch, ["-t", _REPO, "--scope-mode", "full", "-n"]) + assert args.scope_mode == "full" + + +# --------------------------------------------------------------------------- +# Source acquisition (mocked git argv — no network) +# --------------------------------------------------------------------------- + + +def _clone_env(tmp_path: Path) -> Any: + return ( + patch.object(interface_utils, "_git_executable", return_value="/usr/bin/git"), + patch.object(interface_utils.tempfile, "gettempdir", return_value=str(tmp_path)), + ) + + +def _ok(argv: list[str], stdout: str = "") -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess(argv, 0, stdout=stdout, stderr="") + + +def test_clone_revision_detaches_and_asserts_head(tmp_path: Path) -> None: + calls: list[list[str]] = [] + + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + calls.append(list(argv)) + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_B}\n") + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with env1, env2, patch.object(interface_utils.subprocess, "run", side_effect=fake_run) as run: + clone_repository("https://github.com/org/repo", "rev-run", revision=SHA_B) + + clone_argv = run.call_args_list[0].args[0] + assert clone_argv[:3] == ["/usr/bin/git", "clone", "--no-checkout"] + # A SHA must never reach --branch/--single-branch. + assert "--branch" not in clone_argv + assert "--single-branch" not in clone_argv + # Bounded acquisition: the clone subprocess carries a timeout. + assert run.call_args_list[0].kwargs.get("timeout", 0) > 0 + checkout_argv = run.call_args_list[1].args[0] + assert checkout_argv[-3:] == ["checkout", "--detach", SHA_B] + rev_parse_argv = run.call_args_list[2].args[0] + assert rev_parse_argv[-2:] == ["rev-parse", "HEAD"] + + +def test_clone_revision_treats_branch_as_fetch_hint_only(tmp_path: Path) -> None: + """A moving branch name never reaches the checkout path when a revision pins it.""" + + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_B}\n") + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with env1, env2, patch.object(interface_utils.subprocess, "run", side_effect=fake_run) as run: + clone_repository( + "https://github.com/org/repo", + "hint-run", + branch="main", + revision=SHA_B, + ) + + clone_argv = run.call_args_list[0].args[0] + assert "main" not in clone_argv + assert "--branch" not in clone_argv + assert "--single-branch" not in clone_argv + + +def test_clone_missing_revision_fetches_then_detaches(tmp_path: Path) -> None: + """An unadvertised head (e.g. force-pushed) triggers one bounded fetch.""" + state = {"fetched": False} + + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + if "fetch" in argv: + state["fetched"] = True + return _ok(argv) + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_B}\n") + if "rev-parse" in argv: + # rev-parse --verify ^{commit} fails until the fetch lands. + return subprocess.CompletedProcess(argv, 0 if state["fetched"] else 1, "", "") + if "checkout" in argv: + return subprocess.CompletedProcess( + argv, 0 if state["fetched"] else 1, "", "unknown revision" + ) + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with env1, env2, patch.object(interface_utils.subprocess, "run", side_effect=fake_run) as run: + clone_repository("https://github.com/org/repo", "fetch-run", revision=SHA_B) + + fetch_calls = [c.args[0] for c in run.call_args_list if "fetch" in c.args[0]] + assert len(fetch_calls) == 1 + assert fetch_calls[0][-3:] == ["fetch", "origin", SHA_B] + checkout_calls = [c.args[0] for c in run.call_args_list if "checkout" in c.args[0]] + assert len(checkout_calls) == 2 + + +def test_clone_missing_revision_is_named_preflight_failure( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_B}\n") + if "rev-parse" in argv: + return subprocess.CompletedProcess(argv, 1, "", "unknown object") + if "checkout" in argv or "fetch" in argv: + return subprocess.CompletedProcess(argv, 128, "", "remote: not found") + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with ( + env1, + env2, + patch.object(interface_utils.subprocess, "run", side_effect=fake_run), + pytest.raises(SystemExit) as exc_info, + ): + clone_repository("https://github.com/org/repo", "gone-run", revision=SHA_B) + + assert exc_info.value.code == 1 + assert "missing_revision" in capsys.readouterr().out + + +def test_clone_checkout_mismatch_fails_closed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """HEAD must equal the requested revision — a mismatched checkout exits.""" + + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_F}\n") # wrong revision checked out + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with ( + env1, + env2, + patch.object(interface_utils.subprocess, "run", side_effect=fake_run), + pytest.raises(SystemExit) as exc_info, + ): + clone_repository("https://github.com/org/repo", "mismatch-run", revision=SHA_B) + + assert exc_info.value.code == 1 + assert "checkout_mismatch" in capsys.readouterr().out + + +def test_clone_required_base_commit_fetched_when_missing(tmp_path: Path) -> None: + """The recorded diff base is fetched if the clone does not already have it.""" + state = {"fetched": False} + + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + if "fetch" in argv: + state["fetched"] = True + return _ok(argv) + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_B}\n") + if "rev-parse" in argv: + return subprocess.CompletedProcess(argv, 0 if state["fetched"] else 1, "", "") + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with env1, env2, patch.object(interface_utils.subprocess, "run", side_effect=fake_run) as run: + clone_repository( + "https://github.com/org/repo", + "base-run", + revision=SHA_B, + required_commits=(SHA_A,), + ) + + fetch_calls = [c.args[0] for c in run.call_args_list if "fetch" in c.args[0]] + assert fetch_calls and fetch_calls[0][-3:] == ["fetch", "origin", SHA_A] + # Bounded: the fetch carries a timeout. + fetch_call = next(c for c in run.call_args_list if "fetch" in c.args[0]) + assert fetch_call.kwargs.get("timeout", 0) > 0 + + +def test_clone_missing_base_is_named_preflight_failure( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A force-pushed base that cannot be fetched fails closed.""" + + def fake_run(argv: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]: + if "rev-parse" in argv and "HEAD" in argv: + return _ok(argv, stdout=f"{SHA_B}\n") + if "rev-parse" in argv: + return subprocess.CompletedProcess(argv, 1, "", "") + if "fetch" in argv: + return subprocess.CompletedProcess(argv, 128, "", "not our ref") + return _ok(argv) + + env1, env2 = _clone_env(tmp_path) + with ( + env1, + env2, + patch.object(interface_utils.subprocess, "run", side_effect=fake_run), + pytest.raises(SystemExit) as exc_info, + ): + clone_repository( + "https://github.com/org/repo", + "basegone-run", + revision=SHA_B, + required_commits=(SHA_A,), + ) + + assert exc_info.value.code == 1 + assert "missing_base" in capsys.readouterr().out + + +# --------------------------------------------------------------------------- +# Diff-scope against real local git fixtures +# --------------------------------------------------------------------------- + + +def test_diff_scope_records_revisions_and_classification(diff_repo: dict[str, Any]) -> None: + result = resolve_diff_scope_context( + _sources(diff_repo["path"]), + "diff", + diff_repo["base"], + non_interactive=True, + env={}, + diff_head=diff_repo["head"], + ) + + assert result.active + meta = result.metadata + assert meta["requested_base"] == diff_repo["base"] + assert meta["requested_head"] == diff_repo["head"] + assert meta["limits"]["max_files_per_section"] > 0 + assert "no_change" not in meta + + scope = meta["repos"][0] + # The effective comparison is merge_base -> head, recorded honestly. + assert scope["merge_base"] == diff_repo["base"] + assert scope["base_revision"] == diff_repo["base"] + assert scope["head_revision"] == diff_repo["head"] + assert scope["worktree_dirty"] is False + assert scope["snapshot_digest"] is None + + assert set(scope["analyzable_files"]) >= { + "added.py", + "keep.py", + "renamed.py", + "copied.py", + } + assert scope["deleted_files"] == ["deleted.py"] + assert scope["renamed_files"][0]["old_path"] == "old_name.py" + assert scope["renamed_files"][0]["new_path"] == "renamed.py" + # Deleted paths and rename/copy sources are context-only provenance. + assert "deleted.py" in scope["context_files"] + assert "old_name.py" in scope["context_files"] + assert "deleted.py" not in scope["analyzable_files"] + + +def test_diff_scope_detects_copied_files(diff_repo: dict[str, Any]) -> None: + result = resolve_diff_scope_context( + _sources(diff_repo["path"]), + "diff", + diff_repo["base"], + non_interactive=True, + env={}, + diff_head=diff_repo["head"], + ) + scope = result.metadata["repos"][0] + # Whether Git labels it A or C, a copied file is always analyzable. + assert "copied.py" in scope["analyzable_files"] + + +def test_copied_status_entries_are_classified_and_recorded() -> None: + # C-status entries (detected with --find-copies when the source is also + # modified) keep their source path as related context. + raw = b"C85\x00old_src.py\x00new_copy.py\x00D\x00gone.py\x00M\x00mod.py\x00" + entries = interface_utils._parse_name_status_z(raw) + classified = interface_utils._classify_diff_entries(entries) + + assert classified["copied_files"] == [ + {"old_path": "old_src.py", "new_path": "new_copy.py", "similarity": 85} + ] + assert "new_copy.py" in classified["analyzable_files"] + assert classified["deleted_files"] == ["gone.py"] + + +def test_identical_revisions_produce_no_change_receipt(diff_repo: dict[str, Any]) -> None: + head = diff_repo["head"] + result = resolve_diff_scope_context( + _sources(diff_repo["path"]), + "diff", + head, + non_interactive=True, + env={}, + diff_head=head, + ) + + meta = result.metadata + assert meta["no_change"] is True + assert meta["no_change_reason"] == "empty_diff" + assert meta["total_analyzable_files"] == 0 + scope = meta["repos"][0] + assert scope["merge_base"] == head # merge-base(head, head) == head + + +def test_deleted_only_diff_gets_applicability_accounting(tmp_path: Path) -> None: + repo = _init_repo(tmp_path) + (repo / "only.py").write_text("x\n", encoding="utf-8") + base = _commit_all(repo, "base") + (repo / "only.py").unlink() + head = _commit_all(repo, "delete it") + + result = resolve_diff_scope_context( + _sources(repo), "diff", base, non_interactive=True, env={}, diff_head=head + ) + + meta = result.metadata + assert meta["no_change"] is True + assert meta["no_change_reason"] == "no_analyzable_files" + assert meta["total_deleted_files"] == 1 + scope = meta["repos"][0] + assert scope["deleted_files"] == ["only.py"] + assert scope["analyzable_files"] == [] + assert scope["context_files"] == ["only.py"] + + +def _different_sha(sha: str) -> str: + return ("0" if sha[0] != "0" else "1") + sha[1:] + + +def test_head_mismatch_rejects_review_changes(diff_repo: dict[str, Any]) -> None: + other = _different_sha(diff_repo["head"]) # valid shape, wrong revision + with pytest.raises(SourcePreflightError) as exc_info: + resolve_diff_scope_context( + _sources(diff_repo["path"]), + "diff", + diff_repo["base"], + non_interactive=True, + env={}, + diff_head=other, + ) + assert exc_info.value.reason == "head_mismatch" + + +def test_missing_base_is_named_preflight_failure(diff_repo: dict[str, Any]) -> None: + with pytest.raises(SourcePreflightError) as exc_info: + resolve_diff_scope_context( + _sources(diff_repo["path"]), + "diff", + SHA_F, # never existed in this repo + non_interactive=True, + env={}, + diff_head=diff_repo["head"], + ) + assert exc_info.value.reason == "missing_base" + + +def test_shallow_repo_is_named_preflight_failure(tmp_path: Path) -> None: + repo = _init_repo(tmp_path) + (repo / "a.py").write_text("x\n", encoding="utf-8") + _commit_all(repo, "one") + shallow = tmp_path / "shallow" + subprocess.run( # noqa: S603 # nosec B603 + ["git", "clone", "--depth", "1", f"file://{repo}", str(shallow)], # noqa: S607 + check=True, + capture_output=True, + ) + + with pytest.raises(SourcePreflightError) as exc_info: + resolve_diff_scope_context( + _sources(shallow), "diff", SHA_A, non_interactive=True, env={}, diff_head=SHA_B + ) + assert exc_info.value.reason == "insufficient_history" + + +def test_unrelated_base_history_is_named_preflight_failure(tmp_path: Path) -> None: + repo = _init_repo(tmp_path) + (repo / "a.py").write_text("x\n", encoding="utf-8") + head = _commit_all(repo, "main work") + _git(repo, "checkout", "--orphan", "unrelated") + _git(repo, "rm", "-rf", ".") + (repo / "other.py").write_text("y\n", encoding="utf-8") + orphan = _commit_all(repo, "orphan") + _git(repo, "checkout", "main") + + with pytest.raises(SourcePreflightError) as exc_info: + resolve_diff_scope_context( + _sources(repo), "diff", orphan, non_interactive=True, env={}, diff_head=head + ) + assert exc_info.value.reason == "insufficient_history" + + +def test_dirty_worktree_gets_snapshot_digest(tmp_path: Path) -> None: + repo = _init_repo(tmp_path) + (repo / "a.py").write_text("x = 1\n", encoding="utf-8") + base = _commit_all(repo, "base") + (repo / "a.py").write_text("x = 2\n", encoding="utf-8") + (repo / "b.py").write_text("new\n", encoding="utf-8") + head = _commit_all(repo, "head") + # Uncommitted content on top of the recorded head. + (repo / "a.py").write_text("x = 3 # dirty\n", encoding="utf-8") + (repo / "untracked.py").write_text("scratch\n", encoding="utf-8") + + result = resolve_diff_scope_context( + _sources(repo), "diff", base, non_interactive=True, env={}, diff_head=head + ) + + scope = result.metadata["repos"][0] + assert scope["head_revision"] == head + assert scope["worktree_dirty"] is True + assert scope["snapshot_digest"].startswith("sha256:") + # Honest provenance: the instruction block tells the agent the analyzed + # content is the working tree, not the recorded commit. + assert "uncommitted changes" in result.instruction_block + + +def test_unsafe_base_ref_cannot_inject_options(diff_repo: dict[str, Any]) -> None: + """``--all`` must never reach ``git merge-base`` as an option.""" + with pytest.raises(SourcePreflightError, match="Unsafe or empty revision"): + interface_utils._resolve_repo_diff_scope(_sources(diff_repo["path"])[0], "--all", {}) + + +def test_unsafe_base_ref_under_diff_head_is_named_failure( + diff_repo: dict[str, Any], +) -> None: + """Under an asserted head, a non-object-ID base fails closed at the + immutable-input check before any Git invocation consumes it.""" + with pytest.raises(SourcePreflightError) as exc_info: + interface_utils._resolve_repo_diff_scope( + _sources(diff_repo["path"])[0], "--all", {}, diff_head=diff_repo["head"] + ) + assert exc_info.value.reason == "invalid_base" + + +def test_diff_scope_with_head_rejects_non_sha_base(diff_repo: dict[str, Any]) -> None: + with pytest.raises(SourcePreflightError) as exc_info: + resolve_diff_scope_context( + _sources(diff_repo["path"]), + "diff", + "main", # moving ref — not an immutable revision + non_interactive=True, + env={}, + diff_head=diff_repo["head"], + ) + assert exc_info.value.reason == "invalid_base" + + +def test_auto_mode_fails_closed_when_head_asserted(diff_repo: dict[str, Any]) -> None: + """Auto mode may skip heuristic inputs, but never an asserted Review + Changes head — that would silently analyze the wrong revision.""" + other = _different_sha(diff_repo["head"]) + env = {"CI": "1", "GITHUB_BASE_REF": "main"} + with pytest.raises(SourcePreflightError) as exc_info: + resolve_diff_scope_context( + _sources(diff_repo["path"]), + "auto", + diff_repo["base"], + non_interactive=True, + env=env, + diff_head=other, + ) + assert exc_info.value.reason == "head_mismatch" + + +def test_auto_mode_without_head_still_skips_unsuitable_repo(tmp_path: Path) -> None: + """Legacy degrade: auto mode with no asserted head keeps skipping a repo + that cannot support diff-scope.""" + repo = _init_repo(tmp_path) + (repo / "a.py").write_text("x\n", encoding="utf-8") + _commit_all(repo, "one") + shallow = tmp_path / "shallow" + subprocess.run( # noqa: S603 # nosec B603 + ["git", "clone", "--depth", "1", f"file://{repo}", str(shallow)], # noqa: S607 + check=True, + capture_output=True, + ) + env = {"CI": "1", "GITHUB_BASE_REF": "main"} + + result = resolve_diff_scope_context( + _sources(shallow), "auto", None, non_interactive=True, env=env + ) + assert result.active is False + assert result.metadata["skipped_diff_scope_sources"] + + +def test_weird_filenames_survive_null_delimited_parsing(tmp_path: Path) -> None: + repo = _init_repo(tmp_path) + (repo / "seed.py").write_text("x\n", encoding="utf-8") + base = _commit_all(repo, "base") + weird = [ + "sp ace.py", + "quote'file.py", + 'double"quote.py', + "uni-üñíçødé.py", + "--flags.py", + "dollar$ign.py", + ] + for name in weird: + (repo / name).write_text("x = 1\n", encoding="utf-8") + head = _commit_all(repo, "weird names") + + result = resolve_diff_scope_context( + _sources(repo), "diff", base, non_interactive=True, env={}, diff_head=head + ) + + scope = result.metadata["repos"][0] + assert set(scope["added_files"]) == set(weird) + assert set(scope["analyzable_files"]) == set(weird) + + +# --------------------------------------------------------------------------- +# main(): no-change receipt with zero provider calls +# --------------------------------------------------------------------------- + + +def _stub_main_env(monkeypatch: pytest.MonkeyPatch, run_cli: Any) -> dict[str, Mock]: + mocks = { + "validate_environment": Mock(), + "check_docker_installed": Mock(), + "pull_docker_image": Mock(), + "warm_up_llm": Mock(), + "run_cli": run_cli, + "posthog": Mock(), + "scarf": Mock(), + } + for name, mock in mocks.items(): + monkeypatch.setattr(cli_main, name, mock) + monkeypatch.setattr( + cli_main, + "load_settings", + lambda: SimpleNamespace( + llm=SimpleNamespace(model="openai/gpt-5.6-terra"), + runtime=SimpleNamespace(max_local_copy_mb=1024, backend="docker", image="img"), + ), + ) + return mocks + + +def test_empty_diff_exits_with_no_change_receipt_and_no_provider_calls( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = _init_repo(tmp_path) + (repo / "a.py").write_text("x\n", encoding="utf-8") + head = _commit_all(repo, "only commit") + + runs_root = tmp_path / "runsroot" + runs_root.mkdir() + monkeypatch.chdir(runs_root) + + run_cli = AsyncMock() + mocks = _stub_main_env(monkeypatch, run_cli) + monkeypatch.setattr( + sys, + "argv", + [ + "lyrashield", + "-t", + str(repo), + "--target-type", + "local_code", + "--scope-mode", + "diff", + "--diff-base", + head, + "--diff-head", + head, + "--run-name", + "nochange1", + "-n", + ], + ) + + with pytest.raises(SystemExit) as exc_info: + cli_main.main() + + assert exc_info.value.code == 0 + # The no-change receipt never reaches the LLM path. + run_cli.assert_not_called() + mocks["warm_up_llm"].assert_not_called() + mocks["posthog"].start.assert_not_called() + + record = json.loads( + (runs_root / "strix_runs" / "nochange1" / "run.json").read_text(encoding="utf-8") + ) + assert record["status"] == "completed" + assert record["terminal_reason"] == "no_change" + assert record["diff_head"] == head + assert record["diff_base"] == head + assert record["scope_mode"] == "diff" + assert record["diff_scope"]["no_change"] is True + assert record["llm_usage"]["requests"] == 0 + + +def test_repository_revision_and_diff_flags_reach_guarded_clone( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, diff_repo: dict[str, Any] +) -> None: + """The worker's Review Changes argv lands on the existing clone path — + revision pinned, base required — never a second unchecked path.""" + base = diff_repo["base"] + head = diff_repo["head"] + clone = Mock(return_value=str(diff_repo["path"])) + mocks = _stub_main_env(monkeypatch, AsyncMock()) + monkeypatch.setattr(cli_main, "clone_repository", clone) + monkeypatch.setattr(cli_main, "_non_interactive_exit_code", lambda _s: 0) + monkeypatch.setattr(cli_main, "get_global_report_state", lambda: None) + monkeypatch.setattr( + sys, + "argv", + [ + "lyrashield", + "-t", + _REPO, + "--repository-revision", + head, + "--scope-mode", + "diff", + "--diff-base", + base, + "--diff-head", + head, + "--run-name", + "review1", + "-n", + ], + ) + runs_root = tmp_path / "runsroot" + runs_root.mkdir() + monkeypatch.chdir(runs_root) + + cli_main.main() + + clone.assert_called_once() + _, kwargs = clone.call_args + assert kwargs["revision"] == head + assert kwargs["required_commits"] == (base,) + mocks["run_cli"].assert_called_once() + + # Run provenance: requested/resolved revisions and the effective merge + # base are all recorded in run.json. + record = json.loads( + (runs_root / "strix_runs" / "review1" / "run.json").read_text(encoding="utf-8") + ) + assert record["repository_revision"] == head + assert record["diff_head"] == head + assert record["diff_base"] == base + repo_meta = record["diff_scope"]["repos"][0] + assert repo_meta["merge_base"] == base + assert repo_meta["head_revision"] == head + assert repo_meta["requested_head"] == head + assert repo_meta["requested_base"] == base From 1d2477a376c9cdeda3ef143cad79105c31dc3366 Mon Sep 17 00:00:00 2001 From: ecryptoguru Date: Sat, 19 Sep 2026 20:55:36 +0530 Subject: [PATCH 3/6] fix(deps): patch AnyIO security advisories --- UPGRADES.md | 23 +++++ tests/test_anyio_security_patch.py | 157 +++++++++++++++++++++++++++++ uv.lock | 6 +- 3 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 tests/test_anyio_security_patch.py diff --git a/UPGRADES.md b/UPGRADES.md index 5ef1c136..2a2ab53e 100644 --- a/UPGRADES.md +++ b/UPGRADES.md @@ -24,6 +24,29 @@ CI audits the frozen Python dependency graph (all extras and groups) and the Desktop Cargo lockfile. Dependabot uses the uv ecosystem without blanket major version ignores. Known vulnerabilities fail the audit rather than being ignored. +### AnyIO 4.14.2 security patch (2026-09-19) + +The frozen audit flagged anyio 4.14.1 for three advisories — CVE-2026-63374 +([GHSA-82r6-8w77-94w6](https://github.com/agronholm/anyio/security/advisories/GHSA-82r6-8w77-94w6), +TLS server-hostname handling), CVE-2026-64847 +([GHSA-5p39-cfhj-2xmp](https://github.com/agronholm/anyio/security/advisories/GHSA-5p39-cfhj-2xmp), +process pool) and CVE-2026-63349 +([GHSA-3w57-8xmc-8v26](https://github.com/agronholm/anyio/security/advisories/GHSA-3w57-8xmc-8v26), +subprocess supplementary groups) — all fixed in 4.14.2. + +`uv lock --upgrade-package anyio==4.14.2` produced a minimal lock diff: the +anyio version plus its sdist/wheel hashes, nothing else. `pyproject.toml` was +not changed; anyio is a transitive dependency and the existing resolver +constraints already express the patch floor. The frozen export re-audits clean. + +Two upstream fixes are covered by `tests/test_anyio_security_patch.py`: +`open_process`/`run_process` now forward `extra_groups` to the backend instead +of silently substituting `group` (Linux-only tests, mocked backend — no real +privilege-changing subprocess), and `TLSStream.wrap` now IDNA-2008-encodes +international `server_hostname` values before `ssl` certificate hostname +checking instead of leaving the obsolete IDNA 2003 mapping to `ssl`. +Certificate validation is not disabled in the tests. + The reviewed lock advances aiohttp to 3.14.3, pypdf to 6.16.1 and cryptography to 50.0.0; cryptography matches the existing sandbox requirements. Version 49 removed Intel macOS wheels, so the existing Intel release target now builds cryptography diff --git a/tests/test_anyio_security_patch.py b/tests/test_anyio_security_patch.py new file mode 100644 index 00000000..f8679991 --- /dev/null +++ b/tests/test_anyio_security_patch.py @@ -0,0 +1,157 @@ +"""Regression tests for the AnyIO 4.14.2 security patch. + +The frozen dependency audit flagged anyio 4.14.1 with CVE-2026-63374, +CVE-2026-64847 and CVE-2026-63349 (fixed in 4.14.2). These tests pin the two +upstream behaviors the engine's frozen environment relies on: + +* ``open_process``/``run_process`` must forward ``extra_groups`` to the async + backend. In 4.14.1 the backend kwarg was populated from ``group`` instead, + silently dropping the requested supplementary groups. The backend is mocked, + so no real privilege-changing subprocess is ever spawned. +* ``TLSStream.wrap`` must encode international hostnames with IDNA 2008 + (UTS #46) before handing ``server_hostname`` to ``ssl``. 4.14.1 passed the + raw string, letting ``ssl`` apply the obsolete IDNA 2003 mapping. Certificate + validation is never disabled here. +""" + +import importlib +import ssl +import sys +from types import SimpleNamespace +from unittest.mock import AsyncMock, Mock + +import pytest + + +pytest.importorskip("anyio", reason="patched dependency under test") + +_sockets = importlib.import_module("anyio._core._sockets") +_subprocesses = importlib.import_module("anyio._core._subprocesses") +_tls = importlib.import_module("anyio.streams.tls") + +linux_only = pytest.mark.skipif( + sys.platform != "linux", + reason="supplementary-group subprocess options are POSIX-only; " + "the Linux sandbox image is the covered runtime", +) + + +@linux_only +async def test_open_process_forwards_extra_groups_to_backend(monkeypatch): + """``extra_groups`` must reach the backend unchanged. + + anyio 4.14.1 populated the backend's ``extra_groups`` kwarg from ``group``, + silently dropping the requested supplementary groups. + """ + backend = SimpleNamespace(open_process=AsyncMock(return_value=Mock(name="process"))) + monkeypatch.setattr(_subprocesses, "get_async_backend", Mock(return_value=backend)) + + await _subprocesses.open_process( + ["fixture-command"], + user=65534, + group=65534, + extra_groups=[100, 200], + umask=0o077, + ) + + backend.open_process.assert_awaited_once() + kwargs = backend.open_process.await_args.kwargs + assert kwargs["extra_groups"] == [100, 200] + assert kwargs["group"] == 65534 + assert kwargs["user"] == 65534 + assert kwargs["umask"] == 0o077 + + +@linux_only +async def test_open_process_omits_extra_groups_when_unset(monkeypatch): + """Unset POSIX identity options must not reach the backend at all.""" + backend = SimpleNamespace(open_process=AsyncMock(return_value=Mock(name="process"))) + monkeypatch.setattr(_subprocesses, "get_async_backend", Mock(return_value=backend)) + + await _subprocesses.open_process(["fixture-command"]) + + kwargs = backend.open_process.await_args.kwargs + assert "extra_groups" not in kwargs + assert "group" not in kwargs + assert "user" not in kwargs + assert "umask" not in kwargs + + +@linux_only +async def test_run_process_forwards_extra_groups(monkeypatch): + """``run_process`` must hand ``extra_groups`` through to ``open_process``.""" + process = SimpleNamespace( + stdin=None, + stdout=None, + stderr=None, + wait=AsyncMock(return_value=0), + returncode=0, + ) + process_cm = AsyncMock(name="process-acm") + process_cm.__aenter__.return_value = process + open_process = AsyncMock(name="open_process", return_value=process_cm) + monkeypatch.setattr(_subprocesses, "open_process", open_process) + + result = await _subprocesses.run_process( + ["fixture-command"], check=False, extra_groups=[100, 200] + ) + + open_process.assert_awaited_once() + assert open_process.await_args.kwargs["extra_groups"] == [100, 200] + process.wait.assert_awaited_once() + assert result.returncode == 0 + + +def _capture_wrap_bio(context: ssl.SSLContext) -> dict[str, object]: + """Record the arguments anyio hands to ``SSLContext.wrap_bio``. + + The real context keeps its certificate validation settings untouched; only + the ``wrap_bio`` call is observed so no handshake (and no network I/O) is + performed. + """ + captured: dict[str, object] = {} + ssl_object = SimpleNamespace(do_handshake=Mock(return_value=None)) + + def capture(bio_in, bio_out, *, server_side, server_hostname): # noqa: ARG001 + captured["server_side"] = server_side + captured["server_hostname"] = server_hostname + return ssl_object + + context.wrap_bio = capture + return captured + + +async def test_tls_wrap_encodes_hostname_with_idna2008(): + """IDN hostnames are encoded with IDNA 2008 before certificate checking. + + ``ssl`` encodes a ``str`` ``server_hostname`` with the obsolete IDNA 2003 + codec (``faß.de`` -> ``fass.de``); the patched stream pre-encodes with the + dependency's supported IDNA 2008 behavior so the certificate hostname check + sees ``xn--fa-hia.de``. + """ + context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) + assert context.check_hostname is True + assert context.verify_mode == ssl.CERT_REQUIRED + captured = _capture_wrap_bio(context) + + transport = SimpleNamespace(send=AsyncMock(), receive=AsyncMock(), aclose=AsyncMock()) + stream = await _tls.TLSStream.wrap(transport, hostname="faß.de", ssl_context=context) + + assert isinstance(stream, _tls.TLSStream) + assert captured["server_side"] is False + server_hostname = captured["server_hostname"] + assert isinstance(server_hostname, bytes) + assert server_hostname == _sockets.idna2008_resolve("faß.de") == b"xn--fa-hia.de" + # The IDNA 2003 mapping (ß -> ss) must not reach hostname verification. + assert server_hostname != "faß.de".encode("idna") + + +async def test_tls_wrap_passes_ascii_hostname_as_bytes(): + """Plain ASCII hostnames still arrive at ``wrap_bio`` as encoded bytes.""" + context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) + captured = _capture_wrap_bio(context) + + transport = SimpleNamespace(send=AsyncMock(), receive=AsyncMock(), aclose=AsyncMock()) + await _tls.TLSStream.wrap(transport, hostname="example.com", ssl_context=context) + + assert captured["server_hostname"] == b"example.com" diff --git a/uv.lock b/uv.lock index 1301fc96..917eb8f9 100644 --- a/uv.lock +++ b/uv.lock @@ -160,15 +160,15 @@ wheels = [ [[package]] name = "anyio" -version = "4.14.1" +version = "4.14.2" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "idna" }, { name = "typing-extensions", marker = "python_full_version < '3.13'" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/3b/72/5562aabb8dd7181e8e860622a38bea08d17842b99ecd4c91f84ac95251b0/anyio-4.14.1.tar.gz", hash = "sha256:8d648a3544c1a700e3ff78615cd679e4c5c3f149904287e73687b2596963629e", size = 254831, upload-time = "2026-06-24T20:56:06.017Z" } +sdist = { url = "https://files.pythonhosted.org/packages/61/cc/a381afa6efea9f496eff839d4a6a1aed3bfafc7b3ab4b0d1b243a12573dd/anyio-4.14.2.tar.gz", hash = "sha256:cfa139f3ed1a23ee8f88a145ddb5ac7605b8bbfd8592baacd7ce3d8bb4313c7f", size = 260176, upload-time = "2026-07-12T20:29:07.082Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/b0/7b/90df4a0a816d98d6ea26f559d87836d494a2cf1fcf063be67df50a7bcc30/anyio-4.14.1-py3-none-any.whl", hash = "sha256:4e5533c5b8ff0a24f5d7a176cbe6877129cd183893f66b537f8f227d10527d72", size = 124875, upload-time = "2026-06-24T20:56:04.413Z" }, + { url = "https://files.pythonhosted.org/packages/da/35/f2287558c17e29fafc8ef3daf819bb9834061cfa43bff8014f7df7f63bdc/anyio-4.14.2-py3-none-any.whl", hash = "sha256:9f505dda5ac9f0c8309b5e8bd445a8c2bf7246f3ce950121e45ea15bc41d1494", size = 125813, upload-time = "2026-07-12T20:29:05.763Z" }, ] [[package]] From 180da0c295ddf5d805654517c7bad3be714a8855 Mon Sep 17 00:00:00 2001 From: ecryptoguru Date: Sun, 20 Sep 2026 00:29:17 +0530 Subject: [PATCH 4/6] fix(containers): refresh kali-last-snapshot InRelease digest --- containers/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/containers/Dockerfile b/containers/Dockerfile index f5b0d8ce..1cbdb129 100644 --- a/containers/Dockerfile +++ b/containers/Dockerfile @@ -9,7 +9,7 @@ FROM kalilinux/kali-rolling@sha256:f49124869e4eee549315879c3bd7ef92f23b8753fec7544537bcec1493277096 AS gobuilder ARG KALI_APT_SUITE=kali-last-snapshot -ARG KALI_APT_INRELEASE_SHA256=6e298f996675e302bd5465a826ebd246f15b6ebcf71c2839287840be1b4277ee +ARG KALI_APT_INRELEASE_SHA256=1aa2f15aa81eac3fda05654c5602f86324c0785d8a698268a4246a2a07cea34b # The pinned InRelease hash and its APT signature authenticate this immutable # archive snapshot; the base image predates the archive's current TLS chain. RUN printf 'deb http://archive.kali.org/kali %s main contrib non-free non-free-firmware\n' "$KALI_APT_SUITE" > /etc/apt/sources.list && \ @@ -46,7 +46,7 @@ FROM kalilinux/kali-rolling@sha256:f49124869e4eee549315879c3bd7ef92f23b8753fec75 LABEL description="AI Agent Penetration Testing Environment with Comprehensive Automated Tools" ARG KALI_APT_SUITE=kali-last-snapshot -ARG KALI_APT_INRELEASE_SHA256=6e298f996675e302bd5465a826ebd246f15b6ebcf71c2839287840be1b4277ee +ARG KALI_APT_INRELEASE_SHA256=1aa2f15aa81eac3fda05654c5602f86324c0785d8a698268a4246a2a07cea34b RUN printf 'deb http://archive.kali.org/kali %s main contrib non-free non-free-firmware\n' "$KALI_APT_SUITE" > /etc/apt/sources.list && \ printf 'Package: *\nPin: release n=%s\nPin-Priority: 1001\n' "$KALI_APT_SUITE" > /etc/apt/preferences.d/kali-snapshot && \ rm -f /etc/apt/sources.list.d/* && \ From 6ce548f3087d1bf5b600d0ad3ff92d4caf18f3db Mon Sep 17 00:00:00 2001 From: ecryptoguru Date: Sun, 20 Sep 2026 00:35:57 +0530 Subject: [PATCH 5/6] fix(interface): fail closed when diff-head is pinned with no repo scopes --- lyrashield/interface/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lyrashield/interface/utils.py b/lyrashield/interface/utils.py index b66c3311..711497fb 100644 --- a/lyrashield/interface/utils.py +++ b/lyrashield/interface/utils.py @@ -1309,7 +1309,7 @@ def resolve_diff_scope_context( raise if not repo_scopes: - if scope_mode == "auto": + if scope_mode == "auto" and diff_head is None: metadata: dict[str, Any] = {"active": False, "mode": scope_mode} if skipped_non_git: metadata["skipped_non_git_sources"] = skipped_non_git From 0266935cc88558a392df6c3afee1ccd02fb01188 Mon Sep 17 00:00:00 2001 From: ecryptoguru Date: Sun, 20 Sep 2026 11:20:51 +0530 Subject: [PATCH 6/6] fix(review-changes): sanitize run-record sources, dedupe changed-path totals --- lyrashield/interface/main.py | 6 ++++-- lyrashield/interface/utils.py | 9 +++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lyrashield/interface/main.py b/lyrashield/interface/main.py index 6899cc9e..9844d776 100644 --- a/lyrashield/interface/main.py +++ b/lyrashield/interface/main.py @@ -25,6 +25,8 @@ get_global_report_state, initial_run_record, sanitize_attachments, + sanitize_local_sources, + sanitize_targets_info, validate_run_record, ) from lyrashield.artifacts.writer import ( @@ -1080,12 +1082,12 @@ def _persist_run_record( run_record = initial_run_record( args.run_name, auth_mode=codex.auth_mode(load_settings().llm.model), - targets_info=args.targets_info, + targets_info=sanitize_targets_info(args.targets_info), extra={ "scan_mode": args.scan_mode, "instruction": args.instruction, "non_interactive": args.non_interactive, - "local_sources": getattr(args, "local_sources", []), + "local_sources": sanitize_local_sources(getattr(args, "local_sources", [])), "attachments": sanitize_attachments(getattr(args, "attachments", [])), "diff_scope": getattr(args, "diff_scope", {"active": False}), "scope_mode": args.scope_mode, diff --git a/lyrashield/interface/utils.py b/lyrashield/interface/utils.py index b9753795..5e3aaeee 100644 --- a/lyrashield/interface/utils.py +++ b/lyrashield/interface/utils.py @@ -1326,13 +1326,10 @@ def resolve_diff_scope_context( instruction_block = build_diff_scope_instruction(repo_scopes) total_analyzable = sum(len(scope.analyzable_files) for scope in repo_scopes) total_deleted = sum(len(scope.deleted_files) for scope in repo_scopes) + # Each changed path counts once: modified already carries copied paths and + # low-similarity renames, so adding renamed+copied again double-counts. total_changed = sum( - len(scope.added_files) - + len(scope.modified_files) - + len(scope.renamed_files) - + len(scope.copied_files) - + len(scope.deleted_files) - for scope in repo_scopes + len(scope.analyzable_files) + len(scope.deleted_files) for scope in repo_scopes ) metadata = { "active": True,