From d262e446bef2c13359c057515f1cf05a49bd4905 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 23:05:19 +0000 Subject: [PATCH] fix(review): discover base coverage locks by content, not exact filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OpenCode coverage-evidence sandbox installs the Python locks that `materialize_base_python_requirements.py` extracts from the PR base commit, then runs each repo's `pytest tests` offline. Discovery was gated on an exact-basename whitelist `{requirements-hashes.txt, requirements.lock}`, so repositories whose hash-pinned locks live under a service subdirectory (keyverse: `services/account_unification/requirements-dev.txt`) or use conventional dev/test names at the root (semantic-data-portal: `requirements.txt` / `requirements-dev.txt` / `requirements-test.txt`) materialized NOTHING. Their offline test suites then failed to import (`pydantic`/`fastapi`/…), coverage evidence FAILED, and OpenCode could never approve — the ruleset then blocked merge on the resulting REQUEST_CHANGES. This is the root cause of the org-wide PR backlog. Select locks by content instead of name: any tracked `requirements*.txt` or `requirements.lock` blob at the validated base SHA whose content is fully hash-pinned is materialized. This preserves the trust invariant — an unpinned or PR-mutable requirements file is still excluded, now by content rather than by name, and blobs are still read only from the exact base SHA. `_requirement_lines` joins pip-compile/uv-export backslash continuations so a spec and its `--hash=` lines are evaluated together (the previous per-physical-line view treated the bare `pkg==x \` spec line as unpinned and wrongly rejected fully-pinned locks). Verified: 13 tests pass with 100% line + docstring coverage on the changed module; running the patched materializer against the real base trees now discovers `services/account_unification/requirements-dev.txt` for keyverse and all four hash-pinned root locks for semantic-data-portal (previously empty), while an unpinned `requirements.txt` and a non-requirements `uv.lock` stay excluded. semantic-data-portal additionally needs a src-layout PYTHONPATH fix (separate follow-up) to import its `src/`-based package. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH --- .../materialize_base_python_requirements.py | 52 +++++++++++++- ...st_materialize_base_python_requirements.py | 69 +++++++++++++++++++ 2 files changed, 118 insertions(+), 3 deletions(-) diff --git a/scripts/ci/materialize_base_python_requirements.py b/scripts/ci/materialize_base_python_requirements.py index f4f5a0fe3..5b1ed0a23 100644 --- a/scripts/ci/materialize_base_python_requirements.py +++ b/scripts/ci/materialize_base_python_requirements.py @@ -4,6 +4,7 @@ from __future__ import annotations import argparse +import fnmatch import json import pathlib import re @@ -12,7 +13,49 @@ SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") -HASH_LOCK_NAMES = frozenset({"requirements-hashes.txt", "requirements.lock"}) + + +def _is_candidate_lock_name(name: str) -> bool: + """Return whether a file name is a possible pip requirements lock.""" + return name == "requirements.lock" or fnmatch.fnmatch(name, "requirements*.txt") + + +def _requirement_lines(content: bytes) -> list[str]: + """Return logical requirement lines, joining backslash line-continuations. + + ``pip-compile``/``uv export`` write each requirement as a spec line ending in + a backslash followed by indented ``--hash=`` continuation lines. Joining the + continuations first keeps a spec and its hashes on one logical line so the + hash-pin check sees them together. + """ + text = content.decode("utf-8", errors="ignore").replace("\r\n", "\n") + joined = text.replace("\\\n", " ") + lines: list[str] = [] + for raw_line in joined.splitlines(): + line = raw_line.strip() + if not line or line.startswith("#"): + continue + lines.append(line) + return lines + + +def _is_hash_pinned(content: bytes) -> bool: + """Return whether lock content is fully hash-pinned and safe to materialize. + + Discovery is content-based rather than name-based so hash-pinned locks in any + location (a service subdirectory, ``requirements-dev.txt``, + ``requirements-test.txt``) are installed for offline coverage, while an + unpinned or PR-mutable requirements file is still excluded from the networked + build context. An empty file carries no installable dependency and is not + materialized. + """ + lines = _requirement_lines(content) + if not lines: + return False + return any(line == "--require-hashes" for line in lines) or all( + "--hash=" in line or line.startswith(("-r ", "--requirement ")) + for line in lines + ) def _git(repo_root: pathlib.Path, *args: str) -> bytes: @@ -55,10 +98,13 @@ def base_hash_locks(repo_root: pathlib.Path, base_sha: str) -> list[tuple[str, b or not mode.startswith("100") or candidate.is_absolute() or ".." in candidate.parts - or candidate.name not in HASH_LOCK_NAMES + or not _is_candidate_lock_name(candidate.name) ): continue - locks.append((path, _git(repo_root, "show", f"{base_sha}:{path}"))) + content = _git(repo_root, "show", f"{base_sha}:{path}") + if not _is_hash_pinned(content): + continue + locks.append((path, content)) return sorted(locks, key=lambda item: item[0]) diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index df60d5f3a..b79ced1f5 100644 --- a/tests/test_materialize_base_python_requirements.py +++ b/tests/test_materialize_base_python_requirements.py @@ -82,6 +82,75 @@ def test_materializes_only_regular_hash_locks_from_exact_base(tmp_path: Path) -> ) +def test_materializes_hash_pinned_locks_named_beyond_the_legacy_whitelist( + tmp_path: Path, +) -> None: + """Hash-pinned locks in service subdirs and dev/test files are materialized. + + Discovery is content-based: a hash-pinned ``requirements-dev.txt`` under a + service directory and a hash-pinned ``requirements-test.txt`` are installed + for offline coverage, while a non-requirements ``uv.lock`` (excluded by name) + and an unpinned ``requirements-extra.txt`` (excluded by content) are not. + """ + repo = tmp_path / "repo" + repo.mkdir() + git(repo, "init") + git(repo, "config", "user.name", "Test") + git(repo, "config", "user.email", "test@example.invalid") + + service = repo / "services" / "account_unification" + service.mkdir(parents=True) + (service / "requirements-dev.txt").write_text( + "fastapi==1 --hash=sha256:" + ("a" * 64) + "\n", + encoding="utf-8", + ) + (repo / "requirements-test.txt").write_text( + "hypothesis==6 --hash=sha256:" + ("b" * 64) + "\n", + encoding="utf-8", + ) + (repo / "uv.lock").write_text( + "version = 1\n[[package]]\nname = 'x'\n", encoding="utf-8" + ) + (repo / "requirements-extra.txt").write_text("unpinned==1\n", encoding="utf-8") + git(repo, "add", ".") + git(repo, "commit", "-m", "base") + base_sha = git(repo, "rev-parse", "HEAD") + + output = tmp_path / "output" + manifest = materializer.materialize(repo, base_sha, output) + + assert [entry["source"] for entry in manifest] == [ + "requirements-test.txt", + "services/account_unification/requirements-dev.txt", + ] + + +def test_lock_name_candidates_are_pip_requirements_files() -> None: + """Requirements files and requirements.lock are candidates; other names are not.""" + assert materializer._is_candidate_lock_name("requirements.lock") + assert materializer._is_candidate_lock_name("requirements-dev.txt") + assert materializer._is_candidate_lock_name("requirements.txt") + assert not materializer._is_candidate_lock_name("uv.lock") + assert not materializer._is_candidate_lock_name("pyproject.toml") + + +def test_hash_pin_detection_includes_pinned_and_excludes_unpinned_or_empty() -> None: + """Only fully hash-pinned, non-empty lock content is materialized.""" + assert not materializer._is_hash_pinned(b"# comment only\n\n") + assert materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") + assert materializer._is_hash_pinned(b"demo==1 --hash=sha256:" + b"a" * 64 + b"\n") + assert materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert not materializer._is_hash_pinned(b"untrusted==1\n") + # uv export / pip-compile multi-line continuation format (spec, then --hash= lines). + assert materializer._is_hash_pinned( + b"foo==1 \\\n --hash=sha256:" + + b"a" * 64 + + b" \\\n --hash=sha256:" + + b"b" * 64 + + b"\n" + ) + + def test_rejects_invalid_base_sha(tmp_path: Path) -> None: """Git options and symbolic refs cannot cross the exact-SHA boundary.""" with pytest.raises(ValueError, match="40 hexadecimal"):