diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index d86497b3f4..f8b904aec5 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -1199,7 +1199,18 @@ jobs: if [ -n "${PR_BASE_SHA:-}" ] && [ -n "${PR_HEAD_SHA:-}" ] \ && trusted_git rev-parse --verify --quiet "$PR_BASE_SHA^{commit}" >/dev/null \ && trusted_git rev-parse --verify --quiet "$PR_HEAD_SHA^{commit}" >/dev/null; then - trusted_git diff --name-only --find-renames "$PR_BASE_SHA" "$PR_HEAD_SHA" + # --name-status (not --name-only) so a detected rename/copy + # surfaces BOTH its old and new path. --name-only collapses a + # rename to a single line naming only the destination, so a + # Rust source file renamed away to a non-Rust extension (or + # moved into a different Cargo package) would otherwise vanish + # from every consumer's changed-file inventory on the origin + # side -- has_changed_rust_files would miss it entirely, and + # rust_coverage_manifests would credit only the destination + # package. R/C status lines carry a third tab-separated field + # (the new path); every other status carries exactly the path. + trusted_git diff --name-status --find-renames "$PR_BASE_SHA" "$PR_HEAD_SHA" | + awk -F'\t' 'NF >= 3 { print $2; print $3; next } { print $2 }' else trusted_git ls-files fi @@ -1218,6 +1229,11 @@ jobs: return "$rc" } + has_changed_rust_files() { + changed_files_for_coverage | + awk '$0 ~ /(^|\/)Cargo\.(toml|lock)$/ || $0 ~ /\.rs$/ { found=1 } END { exit found ? 0 : 1 }' + } + tracked_python_projects_with_tests() { trusted_git ls-files 'pyproject.toml' '*/pyproject.toml' 'requirements.txt' '*/requirements.txt' \ | while IFS= read -r pyproject_file; do @@ -2000,22 +2016,33 @@ jobs: } rust_coverage_manifests() { - if [ -f Cargo.toml ]; then + # A repository-wide coverage run is required when the workspace + # manifest or lockfile changed. Otherwise measure only the + # changed Rust package(s); building every workspace member at once + # can exhaust the review runner disk before tests begin. + if changed_files_for_coverage \ + | awk '$0 == "Cargo.toml" || $0 == "Cargo.lock" { found=1 } END { exit found ? 0 : 1 }'; then printf '%s\n' Cargo.toml return 0 fi - changed_files_for_coverage \ + local manifests + manifests="$(changed_files_for_coverage \ | while IFS= read -r changed_path; do case "$changed_path" in - Cargo.toml|Cargo.lock|*.rs) ;; + Cargo.toml|Cargo.lock|*/Cargo.toml|*/Cargo.lock|*.rs) ;; *) continue ;; esac candidate_dir="$(dirname "$changed_path")" - while [ "$candidate_dir" != "." ] && [ "$candidate_dir" != "/" ]; do + while :; do if [ -f "${candidate_dir}/Cargo.toml" ]; then - printf '%s\n' "${candidate_dir}/Cargo.toml" + if [ "$candidate_dir" = "." ]; then + printf '%s\n' Cargo.toml + else + printf '%s\n' "${candidate_dir}/Cargo.toml" + fi break fi + [ "$candidate_dir" = "." ] && break next_dir="$(dirname "$candidate_dir")" if [ "$next_dir" = "$candidate_dir" ]; then break @@ -2023,7 +2050,15 @@ jobs: candidate_dir="$next_dir" done done \ - | sort -u + | sort -u)" + # A root workspace run already covers every member; do not repeat + # nested package runs when root and member sources changed together. + if printf '%s\n' "$manifests" \ + | awk '$0 == "Cargo.toml" { found=1 } END { exit found ? 0 : 1 }'; then + printf '%s\n' Cargo.toml + elif [ -n "$manifests" ]; then + printf '%s\n' "$manifests" + fi } rust_coverage_fail_under_lines() { @@ -2241,7 +2276,7 @@ jobs: run_r_test_coverage fi - if has_changed_tracked_files 'Cargo.toml' 'Cargo.lock' '*.rs'; then + if has_changed_rust_files; then measured_any=1 run_rust_test_coverage fi diff --git a/CHANGELOG.md b/CHANGELOG.md index bf192f6a9e..dc4a1c647c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1326,6 +1326,10 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Scope OpenCode Rust coverage evidence to changed Cargo packages while + retaining full-workspace coverage for root workspace and lockfile changes, + preventing large repositories from exhausting the review runner before + coverage starts. - Prefer the job-scoped `github.token` when the central OpenCode dispatch publishes a commit status back to the same `.github` repository. The job's declared `statuses: write` permission now reaches the endpoint instead of an diff --git a/scripts/ci/rust_coverage_threshold.py b/scripts/ci/rust_coverage_threshold.py index 910d7f1047..9bb9727521 100644 --- a/scripts/ci/rust_coverage_threshold.py +++ b/scripts/ci/rust_coverage_threshold.py @@ -4,6 +4,7 @@ from __future__ import annotations import argparse +import fnmatch import tomllib from pathlib import Path from typing import Any @@ -36,18 +37,93 @@ def resolve_minimum_lines(document: dict[str, Any]) -> float | None: break if selected_path is None: return None + return _validate_minimum_lines(selected_path, value) + + +def _validate_minimum_lines(path: str, value: Any) -> float: + """Validate one repository-owned coverage baseline and normalize it.""" if isinstance(value, bool) or not isinstance(value, (int, float)): - raise ValueError(f"{selected_path} must be a number from 0 to 100") + raise ValueError(f"{path} must be a number from 0 to 100") threshold = float(value) if not 0 <= threshold <= 100: - raise ValueError(f"{selected_path} must be between 0 and 100") + raise ValueError(f"{path} must be between 0 and 100") return threshold +def _relative_posix_path(path: Path, base: Path) -> str | None: + """Return ``path`` relative to ``base`` as a POSIX string, or None if unrelated.""" + try: + return path.relative_to(base).as_posix() + except ValueError: + return None + + +def _workspace_excludes_package( + workspace_dir: Path, package_dir: Path, workspace_document: dict[str, Any] +) -> bool: + """Return whether a workspace's ``exclude`` patterns cover the package directory. + + Cargo's own automatic workspace-root discovery walks upward from a + package's manifest and skips an ancestor workspace that excludes it, + continuing the search further out rather than treating the excluded + workspace as authoritative. Mirroring that here keeps an excluded (or + otherwise independent) package from inheriting a coverage baseline that + was never configured for it. + """ + workspace_table = workspace_document.get("workspace") + if not isinstance(workspace_table, dict): + return False + excludes = workspace_table.get("exclude") + if not isinstance(excludes, list): + return False + relative = _relative_posix_path(package_dir, workspace_dir) + if relative is None: + return False + for pattern in excludes: + if not isinstance(pattern, str): + continue + normalized_pattern = pattern.rstrip("/") + if relative == normalized_pattern or fnmatch.fnmatch(relative, normalized_pattern): + return True + if relative.startswith(f"{normalized_pattern}/"): + return True + return False + + def read_minimum_lines(manifest: Path) -> float | None: - """Read and resolve one Cargo manifest's line-coverage baseline.""" + """Read a package baseline, falling back to its nearest workspace baseline.""" + manifest = manifest.resolve() document = tomllib.loads(manifest.read_text(encoding="utf-8")) - return resolve_minimum_lines(document) + threshold = resolve_minimum_lines(document) + if threshold is not None: + return threshold + + package_dir = manifest.parent + for parent in manifest.parents: + workspace_manifest = parent / "Cargo.toml" + if not workspace_manifest.is_file(): + continue + workspace_document = tomllib.loads(workspace_manifest.read_text(encoding="utf-8")) + if "workspace" not in workspace_document: + continue + if _workspace_excludes_package(parent, package_dir, workspace_document): + # This ancestor's workspace explicitly excludes the package, so + # it is not this package's workspace root. Keep walking further + # out for an unrelated ancestor workspace that might still + # legitimately claim it, matching Cargo's own root-discovery + # rule for excluded members. + continue + # This is the package's actual (nearest, non-excluding) Cargo + # workspace root -- whether the sole enclosing workspace or a + # nested, independent one. Stop here even when it configures no + # baseline: crossing this boundary to search an even-more-outer, + # unrelated workspace would attribute a threshold that was never + # configured for this package's own workspace. + workspace_value = _nested_value(workspace_document, METADATA_PATHS[1]) + if workspace_value is not None: + return _validate_minimum_lines(METADATA_PATHS[1], workspace_value) + return None + return None def main() -> int: diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 321d25bd57..95a662377e 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1,3 +1,5 @@ +"""Executable contracts for the trusted OpenCode review configuration.""" + import json import os import re @@ -346,6 +348,7 @@ def test_opencode_model_pool_sets_high_effort_for_capable_candidates(): assert f'"{model_name}": {{' in workflow def is_reasoning_capable(model_name: str) -> bool: + """Return whether a model supports the high-effort reasoning contract.""" return ( model_name.startswith("gpt-5") or model_name.startswith("openai/gpt-5") @@ -832,6 +835,13 @@ def test_opencode_target_coverage_materializes_only_after_authorized_dispatch(): assert "rust_coverage_fail_under_lines()" in measure_step assert "package.metadata.opencode.coverage.minimum_lines" in measure_step assert "workspace.metadata.opencode.coverage.minimum_lines" in measure_step + assert "if changed_files_for_coverage" in measure_step + assert '$0 == "Cargo.toml" || $0 == "Cargo.lock"' in measure_step + assert "has_changed_rust_files" in measure_step + assert "Cargo\\.(toml|lock)" in measure_step + assert "changed Rust package(s)" in measure_step + assert 'candidate_dir="$(dirname "$changed_path")"' in measure_step + assert "*/Cargo.toml|*/Cargo.lock|*.rs" in measure_step assert "scripts/ci/rust_coverage_threshold.py" in measure_step assert '--fail-under-lines "$threshold"' in measure_step assert "uv sync --project" not in measure_step @@ -1581,6 +1591,276 @@ def test_opencode_coverage_discovers_changed_nested_javascript_package(tmp_path) assert result.stdout.splitlines() == ["ADFS 연동 라이브러리/Node.JS/Node App"] +def test_opencode_rust_coverage_selects_changed_manifests(tmp_path): + """Select only the Rust manifests whose packages contain changed files.""" + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is required for the extracted workflow function regression test") + + workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text( + encoding="utf-8" + ) + measure_start = workflow.index( + " - name: Measure test and docstring evidence\n" + ) + measure_end = workflow.index("\n - name:", measure_start + 1) + measure_step = workflow[measure_start:measure_end] + changed_start = measure_step.index(" has_changed_tracked_files() {\n") + changed_end = measure_step.index( + "\n\n tracked_python_projects_with_tests()", changed_start + ) + rust_start = measure_step.index(" rust_coverage_manifests() {\n") + rust_end = measure_step.index( + "\n\n rust_coverage_fail_under_lines()", rust_start + ) + shell = "\n".join( + ( + "set -euo pipefail", + "trusted_git() { git \"$@\"; }", + "changed_files_for_coverage() { cat \"$CHANGED_FILE_LIST\"; }", + textwrap.dedent(measure_step[changed_start:changed_end]), + textwrap.dedent(measure_step[rust_start:rust_end]), + "rust_coverage_manifests", + ) + ) + + repo = tmp_path / "repo" + (repo / "crates" / "alpha" / "src").mkdir(parents=True) + (repo / "src").mkdir() + for relative_path, content in { + "Cargo.toml": "[workspace]\nmembers = [\"crates/alpha\"]\n", + "Cargo.lock": "# lock\n", + "crates/alpha/Cargo.toml": "[package]\nname = \"alpha\"\n", + "crates/alpha/Cargo.lock": "# nested lock\n", + "crates/alpha/src/lib.rs": "pub fn alpha() {}\n", + "src/main.rs": "fn main() {}\n", + "README.md": "unrelated\n", + }.items(): + path = repo / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "Coverage Test"], cwd=repo, check=True) + subprocess.run( + ["git", "config", "user.email", "coverage@example.invalid"], + cwd=repo, + check=True, + ) + subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "commit", "-qm", "fixtures"], cwd=repo, check=True) + + def select(changed_paths: str) -> list[str]: + """Run the extracted selector against one changed-file inventory.""" + changed_file_list = repo / "changed-files.txt" + changed_file_list.write_text(changed_paths, encoding="utf-8") + result = subprocess.run( + [bash, "-c", shell], + cwd=repo, + env={**os.environ, "CHANGED_FILE_LIST": str(changed_file_list)}, + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, result.stderr + return result.stdout.splitlines() + + assert select("Cargo.toml\n") == ["Cargo.toml"] + assert select("Cargo.lock\n") == ["Cargo.toml"] + assert select("crates/alpha/Cargo.toml\n") == ["crates/alpha/Cargo.toml"] + assert select("crates/alpha/Cargo.lock\n") == ["crates/alpha/Cargo.toml"] + assert select("crates/alpha/src/lib.rs\n") == ["crates/alpha/Cargo.toml"] + assert select("src/main.rs\n") == ["Cargo.toml"] + assert select("src/main.rs\ncrates/alpha/src/lib.rs\n") == ["Cargo.toml"] + assert select("README.md\n") == [] + + nested_manifest = repo / "crates" / "alpha" / "Cargo.toml" + nested_manifest.unlink() + assert select("crates/alpha/Cargo.toml\n") == ["Cargo.toml"] + + subprocess.run(["git", "rm", "-q", "crates/alpha/Cargo.lock"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-qm", "delete nested rust manifests"], cwd=repo, check=True) + trigger_start = measure_step.index(" has_changed_rust_files() {\n") + trigger_end = measure_step.index( + "\n\n tracked_python_projects_with_tests()", trigger_start + ) + trigger_shell = "\n".join( + ( + "set -euo pipefail", + "trusted_git() { git \"$@\"; }", + "changed_files_for_coverage() { git diff --name-only HEAD~1 HEAD; }", + textwrap.dedent(measure_step[trigger_start:trigger_end]), + "has_changed_rust_files", + ) + ) + trigger = subprocess.run( + [bash, "-c", trigger_shell], cwd=repo, capture_output=True, text=True, timeout=30 + ) + assert trigger.returncode == 0, trigger.stderr + + # Deleting the root lockfile still changes the whole workspace dependency graph. + subprocess.run(["git", "rm", "-q", "Cargo.lock"], cwd=repo, check=True) + subprocess.run( + ["git", "commit", "-qm", "delete root lock"], cwd=repo, check=True + ) + assert select("Cargo.lock\n") == ["Cargo.toml"] + + +def _extract_measure_step(workflow_text: str) -> str: + """Return the source of the Measure test and docstring evidence step.""" + measure_start = workflow_text.index( + " - name: Measure test and docstring evidence\n" + ) + measure_end = workflow_text.index("\n - name:", measure_start + 1) + return workflow_text[measure_start:measure_end] + + +def test_opencode_rust_coverage_inventory_includes_both_rename_endpoints(tmp_path): + """A detected rename must not make Rust coverage lose its origin side. + + ``git diff --name-only`` collapses a rename to a single line naming only + the destination path. If the shared changed-file inventory used that + form, a ``.rs`` file renamed to a non-Rust extension would disappear + from the inventory entirely (bypassing Rust coverage), and a ``.rs`` + file moved between two Cargo packages would only credit the destination + package's manifest, leaving the origin package's coverage unmeasured. + """ + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is required for the extracted workflow function regression test") + try: + subprocess.run( + [bash, "--version"], capture_output=True, text=True, timeout=5, check=True + ) + except (OSError, subprocess.SubprocessError) as exc: + pytest.skip(f"bash is not usable for this regression test: {exc}") + + workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text( + encoding="utf-8" + ) + measure_step = _extract_measure_step(workflow) + + inventory_start = measure_step.index(" trusted_git() {\n") + inventory_end = measure_step.index( + "\n\n has_changed_tracked_files()", inventory_start + ) + trigger_start = measure_step.index(" has_changed_rust_files() {\n") + trigger_end = measure_step.index( + "\n\n tracked_python_projects_with_tests()", trigger_start + ) + rust_start = measure_step.index(" rust_coverage_manifests() {\n") + rust_end = measure_step.index( + "\n\n rust_coverage_fail_under_lines()", rust_start + ) + shell = "\n".join( + ( + "set -euo pipefail", + textwrap.dedent(measure_step[inventory_start:inventory_end]), + textwrap.dedent(measure_step[trigger_start:trigger_end]), + textwrap.dedent(measure_step[rust_start:rust_end]), + 'if [ "$MODE" = rust_files ]; then', + " has_changed_rust_files && echo RUST_FILES_CHANGED || echo RUST_FILES_UNCHANGED", + "else", + " rust_coverage_manifests", + "fi", + ) + ) + + def run_shell(repo: Path, base_sha: str, head_sha: str, mode: str) -> subprocess.CompletedProcess: + """Run the extracted inventory/detector against one commit range.""" + env = { + **os.environ, + "PR_BASE_SHA": base_sha, + "PR_HEAD_SHA": head_sha, + "MODE": mode, + } + return subprocess.run( + [bash, "-c", shell], + cwd=repo, + env=env, + capture_output=True, + text=True, + timeout=30, + ) + + def init_repo(tmp_subdir: str) -> Path: + """Create an isolated git repository for one rename scenario.""" + repo = tmp_path / tmp_subdir + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "Coverage Test"], cwd=repo, check=True) + subprocess.run( + ["git", "config", "user.email", "coverage@example.invalid"], + cwd=repo, + check=True, + ) + return repo + + def commit_all(repo: Path, message: str) -> str: + """Stage every change and commit, returning the new commit SHA.""" + subprocess.run(["git", "add", "-A"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-qm", message], cwd=repo, check=True) + return subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=repo, text=True + ).strip() + + # Scenario 1: a .rs file renamed to a non-Rust extension must still be + # detected as a Rust change (the file's removal from Rust source is + # itself a Rust-relevant change, and --name-only would otherwise report + # only the destination path, which does not match the .rs pattern). + rename_away_repo = init_repo("rename-away") + (rename_away_repo / "src").mkdir() + (rename_away_repo / "Cargo.toml").write_text('[package]\nname = "demo"\n', encoding="utf-8") + (rename_away_repo / "src" / "legacy.rs").write_text( + "pub fn legacy() {}\n" * 5, encoding="utf-8" + ) + base_sha = commit_all(rename_away_repo, "base") + subprocess.run( + ["git", "mv", "src/legacy.rs", "src/legacy.rs.disabled"], + cwd=rename_away_repo, + check=True, + ) + head_sha = commit_all(rename_away_repo, "rename rs away from Rust") + + renamed_away = run_shell(rename_away_repo, base_sha, head_sha, "rust_files") + assert renamed_away.returncode == 0, renamed_away.stderr + assert renamed_away.stdout.strip() == "RUST_FILES_CHANGED", renamed_away.stdout + + # Scenario 2: a .rs file moved between two Cargo packages in the same + # workspace must credit BOTH the origin and destination package + # manifests, not only the destination that --name-only would report. + cross_package_repo = init_repo("cross-package-move") + (cross_package_repo / "crates" / "alpha" / "src").mkdir(parents=True) + (cross_package_repo / "crates" / "beta" / "src").mkdir(parents=True) + (cross_package_repo / "Cargo.toml").write_text( + '[workspace]\nmembers = ["crates/alpha", "crates/beta"]\n', encoding="utf-8" + ) + (cross_package_repo / "crates" / "alpha" / "Cargo.toml").write_text( + '[package]\nname = "alpha"\n', encoding="utf-8" + ) + (cross_package_repo / "crates" / "beta" / "Cargo.toml").write_text( + '[package]\nname = "beta"\n', encoding="utf-8" + ) + (cross_package_repo / "crates" / "alpha" / "src" / "shared.rs").write_text( + "pub fn shared() {}\n" * 5, encoding="utf-8" + ) + (cross_package_repo / "crates" / "beta" / "src" / "lib.rs").write_text( + "pub fn beta_lib() {}\n", encoding="utf-8" + ) + base_sha = commit_all(cross_package_repo, "base") + subprocess.run( + ["git", "mv", "crates/alpha/src/shared.rs", "crates/beta/src/shared.rs"], + cwd=cross_package_repo, + check=True, + ) + head_sha = commit_all(cross_package_repo, "move crate module across packages") + + moved = run_shell(cross_package_repo, base_sha, head_sha, "manifests") + assert moved.returncode == 0, moved.stderr + assert sorted(moved.stdout.splitlines()) == sorted( + ["crates/alpha/Cargo.toml", "crates/beta/Cargo.toml"] + ), moved.stdout + + def test_opencode_runtime_pin_supports_reasoning_options(): """Keep OpenCode runtime new enough to apply model-level reasoning settings.""" review_workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text( diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 2e733ac9e9..93445be5cd 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -1,5 +1,6 @@ """Contract tests for the scheduled OpenCode review-autofix trust boundary.""" +from collections import Counter import hashlib import re import subprocess @@ -17,7 +18,7 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "d86497b3f43bebbabbb4f504eb5132cdf3b7b293" +REVIEW_DISPATCH_BLOB_SHA = "f8b904aec55b9ed8c78e3f74ef7eb020c104d7cc" def _workflow_text(path: Path) -> str: @@ -183,6 +184,59 @@ def test_independent_review_agent_workflow_matches_reviewed_blob() -> None: assert "pr-review-autofix" not in _workflow_text(REVIEW_DISPATCH_WORKFLOW) +def test_independent_review_agent_key_system_is_unchanged() -> None: + """Keep review-write credentials separate while allowing gateway wiring.""" + workflow = _workflow_text(REVIEW_DISPATCH_WORKFLOW) + approved_gh_token_assignments = ( + "${{ steps.metadata_read_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "${{ steps.coverage_read_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "${{ steps.review_read_app_token.outputs.token || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "${{ secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "${{ secrets.OPENCODE_APPROVE_TOKEN || steps.review_read_app_token.outputs.token || github.token }}", + "${{ steps.opencode_app_token.outputs.token }}", + "${{ steps.opencode_app_token.outputs.token }}", + "${{ steps.opencode_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "${{ steps.opencode_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + # Prefer the job-scoped github.token when the central OpenCode dispatch + # publishes a commit status back to the same .github repository (see + # CHANGELOG.md): the job's own statuses: write permission then reaches + # the endpoint instead of an unrelated App installation token. + "${{ needs.validate-pr-metadata.outputs.target_repository == github.repository && github.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN }}", + "${{ needs.validate-pr-metadata.outputs.target_repository == github.repository && github.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.opencode_app_token.outputs.token || github.token }}", + "${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.opencode_app_token.outputs.token || github.token }}", + "${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.opencode_app_token.outputs.token || github.token }}", + ) + assignments = re.findall(r"^ {10}GH_TOKEN:\s*(.+)$", workflow, flags=re.MULTILINE) + assert Counter(assignments) == Counter(approved_gh_token_assignments) + assert not re.search(r"^ {2,9}(?:GH_TOKEN|GITHUB_TOKEN):", workflow, flags=re.MULTILINE) + assert "pr-review-autofix" not in workflow + assert "COPILOT_GITHUB_TOKEN" not in workflow + + # The model pool step itself no longer declares direct provider + # credentials in-line (ContextualWisdomLab/contextual-orchestrator + # gateway migration, docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md): + # it sources scripts/ci/load_contextual_orchestrator_token.sh instead, so + # this test no longer asserts individual provider env var names there. + model_step_start = workflow.index(" - name: Run OpenCode PR Review model pool") + model_step_end = workflow.index("\n - name:", model_step_start + 1) + model_step = workflow[model_step_start:model_step_end] + assert "load_contextual_orchestrator_token.sh" in model_step + for forbidden_credential in ( + "GH_TOKEN:", + "GITHUB_TOKEN:", + "CHECK_LOOKUP_GH_TOKEN:", + "CODE_SCANNING_GH_TOKEN:", + "OPENCODE_APP_TOKEN:", + "PR_REVIEW_MERGE_TOKEN", + "OPENCODE_APPROVE_TOKEN", + "COPILOT_GITHUB_TOKEN", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN", + "ACTIONS_ID_TOKEN_REQUEST_URL", + ): + assert forbidden_credential not in model_step + + def test_ordinary_autofix_uses_the_same_exact_write_scope_as_conflict_repair() -> None: """Snapshot ordinary repairs so ignored and symlink-mediated writes fail closed.""" workflow = _workflow_text(AUTOFIX_WORKFLOW) diff --git a/tests/test_rust_coverage_threshold.py b/tests/test_rust_coverage_threshold.py index c3ed47bdf0..6090bef49a 100644 --- a/tests/test_rust_coverage_threshold.py +++ b/tests/test_rust_coverage_threshold.py @@ -31,6 +31,56 @@ def test_virtual_workspace_metadata_is_supported(tmp_path: Path) -> None: assert threshold.read_minimum_lines(manifest) == 90.0 +def test_nested_package_inherits_nearest_workspace_baseline(tmp_path: Path) -> None: + """A crate without a local override must use the workspace baseline.""" + workspace = tmp_path / "Cargo.toml" + workspace.write_text( + '[workspace]\nmembers = ["crates/core"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 87\n", + encoding="utf-8", + ) + manifest = tmp_path / "crates" / "core" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "core"\nversion = "0.1.0"\n', encoding="utf-8") + + assert threshold.read_minimum_lines(manifest) == 87.0 + + +def test_nested_package_override_beats_workspace_baseline(tmp_path: Path) -> None: + """A crate-specific baseline remains stronger than inherited metadata.""" + workspace = tmp_path / "Cargo.toml" + workspace.write_text( + '[workspace]\nmembers = ["crates/core"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 87\n", + encoding="utf-8", + ) + manifest = tmp_path / "crates" / "core" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text( + '[package]\nname = "core"\nversion = "0.1.0"\n\n' + "[package.metadata.opencode.coverage]\nminimum_lines = 93\n", + encoding="utf-8", + ) + + assert threshold.read_minimum_lines(manifest) == 93.0 + + +def test_nested_package_rejects_invalid_workspace_baseline(tmp_path: Path) -> None: + """An inherited malformed baseline cannot silently restore the central default.""" + workspace = tmp_path / "Cargo.toml" + workspace.write_text( + '[workspace]\nmembers = ["crates/core"]\n\n' + '[workspace.metadata.opencode.coverage]\nminimum_lines = "high"\n', + encoding="utf-8", + ) + manifest = tmp_path / "crates" / "core" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "core"\nversion = "0.1.0"\n', encoding="utf-8") + + with pytest.raises(ValueError, match=r"workspace\.metadata\.opencode\.coverage\.minimum_lines"): + threshold.read_minimum_lines(manifest) + + @pytest.mark.parametrize("value", [True, "90", -1, 101]) def test_invalid_thresholds_fail_closed(value: object) -> None: """Non-numeric and out-of-range baselines cannot weaken the coverage gate.""" @@ -38,7 +88,7 @@ def test_invalid_thresholds_fail_closed(value: object) -> None: "workspace": {"metadata": {"opencode": {"coverage": {"minimum_lines": value}}}} } - with pytest.raises(ValueError, match="workspace.metadata.opencode.coverage.minimum_lines"): + with pytest.raises(ValueError, match=r"workspace\.metadata\.opencode\.coverage\.minimum_lines"): threshold.resolve_minimum_lines(document) @@ -50,6 +100,191 @@ def test_missing_metadata_keeps_central_default() -> None: ) == 0.0 +def test_nested_independent_workspace_does_not_inherit_outer_baseline( + tmp_path: Path, +) -> None: + """A nested [workspace] boundary with no baseline must not leak an outer one. + + Regression for Devin finding "Independent crates inherit unrelated + thresholds": the walk must stop at the nearest ancestor workspace even + when that workspace configures no coverage metadata of its own, rather + than continuing past it to an unrelated, further-out workspace. + """ + outer_workspace = tmp_path / "Cargo.toml" + outer_workspace.write_text( + '[workspace]\nmembers = ["libs/*"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 70\n", + encoding="utf-8", + ) + inner_workspace = tmp_path / "libs" / "independent" / "Cargo.toml" + inner_workspace.parent.mkdir(parents=True) + inner_workspace.write_text('[workspace]\nmembers = ["crate_a"]\n', encoding="utf-8") + manifest = tmp_path / "libs" / "independent" / "crate_a" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "crate_a"\nversion = "0.1.0"\n', encoding="utf-8") + + assert threshold.read_minimum_lines(manifest) is None + + +def test_nested_independent_workspace_own_baseline_wins(tmp_path: Path) -> None: + """A nested workspace's own baseline is used, never the outer workspace's.""" + outer_workspace = tmp_path / "Cargo.toml" + outer_workspace.write_text( + '[workspace]\nmembers = ["libs/*"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 70\n", + encoding="utf-8", + ) + inner_workspace = tmp_path / "libs" / "independent" / "Cargo.toml" + inner_workspace.parent.mkdir(parents=True) + inner_workspace.write_text( + '[workspace]\nmembers = ["crate_a"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 95\n", + encoding="utf-8", + ) + manifest = tmp_path / "libs" / "independent" / "crate_a" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "crate_a"\nversion = "0.1.0"\n', encoding="utf-8") + + assert threshold.read_minimum_lines(manifest) == 95.0 + + +def test_excluded_package_does_not_inherit_outer_workspace_baseline( + tmp_path: Path, +) -> None: + """A package excluded from an outer workspace must not inherit its baseline. + + Regression for Devin finding "Independent crates inherit unrelated + thresholds": ``exclude`` removes the package from that workspace, so its + metadata must not apply -- even though the excluded package still lives + in a directory beneath the workspace root. + """ + workspace = tmp_path / "Cargo.toml" + workspace.write_text( + '[workspace]\nmembers = ["crates/*"]\nexclude = ["crates/excluded"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 70\n", + encoding="utf-8", + ) + manifest = tmp_path / "crates" / "excluded" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "excluded"\nversion = "0.1.0"\n', encoding="utf-8") + + assert threshold.read_minimum_lines(manifest) is None + + +def test_excluded_package_still_inherits_further_ancestor_workspace( + tmp_path: Path, +) -> None: + """A package excluded from one workspace may still inherit a further one. + + Mirrors Cargo's own root-discovery rule: an ancestor workspace that + excludes the package is not its root, so the search must continue + upward rather than stopping at the excluding workspace. + """ + grandparent_workspace = tmp_path / "Cargo.toml" + grandparent_workspace.write_text( + '[workspace]\nmembers = ["nested/crates/*"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 60\n", + encoding="utf-8", + ) + nested_workspace = tmp_path / "nested" / "Cargo.toml" + nested_workspace.parent.mkdir(parents=True) + nested_workspace.write_text( + '[workspace]\nmembers = ["crates/*"]\nexclude = ["crates/excluded"]\n', + encoding="utf-8", + ) + manifest = tmp_path / "nested" / "crates" / "excluded" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "excluded"\nversion = "0.1.0"\n', encoding="utf-8") + + assert threshold.read_minimum_lines(manifest) == 60.0 + + +def test_relative_posix_path_returns_none_for_unrelated_paths(tmp_path: Path) -> None: + """An unrelated path pair cannot be expressed as a relative path.""" + workspace_dir = tmp_path / "workspace" + other_dir = tmp_path / "elsewhere" / "package" + workspace_dir.mkdir() + other_dir.mkdir(parents=True) + + assert threshold._relative_posix_path(other_dir, workspace_dir) is None + + +def test_workspace_excludes_package_ignores_non_dict_workspace_table() -> None: + """A malformed non-table [workspace] value never signals exclusion.""" + assert ( + threshold._workspace_excludes_package( + Path("/repo"), Path("/repo/crates/a"), {"workspace": "not-a-table"} + ) + is False + ) + + +def test_workspace_excludes_package_ignores_non_list_exclude() -> None: + """A malformed non-list exclude value never signals exclusion.""" + assert ( + threshold._workspace_excludes_package( + Path("/repo"), + Path("/repo/crates/a"), + {"workspace": {"exclude": "crates/a"}}, + ) + is False + ) + + +def test_workspace_excludes_package_ignores_unrelated_package_directory( + tmp_path: Path, +) -> None: + """A package outside the workspace directory can never be excluded by it.""" + workspace_dir = tmp_path / "workspace" + other_dir = tmp_path / "elsewhere" / "package" + + assert ( + threshold._workspace_excludes_package( + workspace_dir, other_dir, {"workspace": {"exclude": ["package"]}} + ) + is False + ) + + +def test_workspace_excludes_package_skips_non_string_patterns_and_checks_rest() -> None: + """A non-string exclude entry is skipped rather than raising or matching.""" + assert ( + threshold._workspace_excludes_package( + Path("/repo"), + Path("/repo/crates/a"), + {"workspace": {"exclude": [42, "crates/a"]}}, + ) + is True + ) + assert ( + threshold._workspace_excludes_package( + Path("/repo"), + Path("/repo/crates/other"), + {"workspace": {"exclude": [42, "crates/a"]}}, + ) + is False + ) + + +def test_excluded_package_directory_beneath_excluded_subtree(tmp_path: Path) -> None: + """A package nested beneath an excluded directory inherits its exclusion. + + Covers the exclude-prefix branch: the package path is not an exact or + glob match for the exclude entry, only a path beneath it. + """ + workspace = tmp_path / "Cargo.toml" + workspace.write_text( + '[workspace]\nmembers = ["tools/legacy/*"]\nexclude = ["tools/legacy"]\n\n' + "[workspace.metadata.opencode.coverage]\nminimum_lines = 70\n", + encoding="utf-8", + ) + manifest = tmp_path / "tools" / "legacy" / "sub" / "Cargo.toml" + manifest.parent.mkdir(parents=True) + manifest.write_text('[package]\nname = "sub"\nversion = "0.1.0"\n', encoding="utf-8") + + assert threshold.read_minimum_lines(manifest) is None + + def test_cli_prints_normalized_workspace_threshold( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: