From 61f4389f043921c566ed5394c844b9689a4537d1 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 07:55:18 +0900 Subject: [PATCH 1/9] fix(review): isolate untrusted PR source blobs --- .github/workflows/opencode-review.yml | 60 ++-- scripts/ci/materialize_pr_review_source.py | 314 +++++++++++++++++++++ scripts/ci/test_strix_quick_gate.sh | 24 +- tests/test_materialize_pr_review_source.py | 150 ++++++++++ tests/test_opencode_agent_contract.py | 12 + 5 files changed, 533 insertions(+), 27 deletions(-) create mode 100644 scripts/ci/materialize_pr_review_source.py create mode 100644 tests/test_materialize_pr_review_source.py diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index c2f88f6d..8c4839cf 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -1874,7 +1874,7 @@ jobs: echo "token=$app_token" } >>"$GITHUB_OUTPUT" - - name: Materialize pull request head for OpenCode review data + - name: Materialize inert pull request blobs for OpenCode review data env: GH_TOKEN: ${{ steps.review_read_app_token.outputs.token || secrets.OPENCODE_APPROVE_TOKEN || github.token }} GH_REPOSITORY: ${{ needs.validate-pr-metadata.outputs.target_repository }} @@ -1882,24 +1882,35 @@ jobs: PR_BASE_REF: ${{ needs.validate-pr-metadata.outputs.base_ref }} PR_BASE_SHA: ${{ needs.validate-pr-metadata.outputs.base_sha }} PR_HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} + OPENCODE_SOURCE_GIT_DIR: ${{ runner.temp }}/opencode-pr-objects.git OPENCODE_SOURCE_WORKDIR: ${{ runner.temp }}/opencode-pr-head + OPENCODE_SOURCE_MANIFEST: ${{ runner.temp }}/opencode-pr-source-manifest.json run: | set -euo pipefail gh auth setup-git - git remote remove pr-source 2>/dev/null || true - git remote add pr-source "$GITHUB_SERVER_URL/$GH_REPOSITORY.git" - git fetch --no-tags pr-source \ + rm -rf "$OPENCODE_SOURCE_GIT_DIR" "$OPENCODE_SOURCE_WORKDIR" + rm -f "$OPENCODE_SOURCE_MANIFEST" + git init --bare "$OPENCODE_SOURCE_GIT_DIR" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" remote add pr-source \ + "$GITHUB_SERVER_URL/$GH_REPOSITORY.git" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch --no-tags --no-recurse-submodules pr-source \ "+refs/heads/${PR_BASE_REF}:refs/remotes/pr-source/${PR_BASE_REF}" - if ! git cat-file -e "${PR_BASE_SHA}^{commit}" >/dev/null 2>&1; then - git fetch --no-tags pr-source "$PR_BASE_SHA" + if ! git --git-dir="$OPENCODE_SOURCE_GIT_DIR" cat-file -e "${PR_BASE_SHA}^{commit}" >/dev/null 2>&1; then + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch --no-tags --no-recurse-submodules \ + pr-source "$PR_BASE_SHA" fi - if ! git cat-file -e "${PR_HEAD_SHA}^{commit}" >/dev/null 2>&1; then - git fetch --no-tags pr-source "$PR_HEAD_SHA" || true + if ! git --git-dir="$OPENCODE_SOURCE_GIT_DIR" cat-file -e "${PR_HEAD_SHA}^{commit}" >/dev/null 2>&1; then + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch --no-tags --no-recurse-submodules \ + pr-source "$PR_HEAD_SHA" || true fi - if ! git cat-file -e "${PR_HEAD_SHA}^{commit}" >/dev/null 2>&1; then + if ! git --git-dir="$OPENCODE_SOURCE_GIT_DIR" cat-file -e "${PR_HEAD_SHA}^{commit}" >/dev/null 2>&1; then for pr_head_fetch_attempt in 1 2 3 4 5 6; do - git fetch --no-tags --prune pr-source "+refs/pull/${PR_NUMBER}/head:refs/remotes/pr-source/pull/${PR_NUMBER}/head" - fetched_head_sha="$(git rev-parse "refs/remotes/pr-source/pull/${PR_NUMBER}/head")" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch --no-tags --prune --no-recurse-submodules \ + pr-source "+refs/pull/${PR_NUMBER}/head:refs/remotes/pr-source/pull/${PR_NUMBER}/head" + fetched_head_sha="$( + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" rev-parse \ + "refs/remotes/pr-source/pull/${PR_NUMBER}/head" + )" if [ "$fetched_head_sha" = "$PR_HEAD_SHA" ]; then break fi @@ -1909,11 +1920,26 @@ jobs: fi done fi - git cat-file -e "${PR_BASE_SHA}^{commit}" - git cat-file -e "${PR_HEAD_SHA}^{commit}" - rm -rf "$OPENCODE_SOURCE_WORKDIR" - git worktree add --detach "$OPENCODE_SOURCE_WORKDIR" "$PR_HEAD_SHA" - git -C "$OPENCODE_SOURCE_WORKDIR" status --short + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" cat-file -e "${PR_BASE_SHA}^{commit}" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" cat-file -e "${PR_HEAD_SHA}^{commit}" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" update-ref refs/heads/opencode-review "$PR_HEAD_SHA" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" symbolic-ref HEAD refs/heads/opencode-review + python3 scripts/ci/materialize_pr_review_source.py \ + --git-dir "$OPENCODE_SOURCE_GIT_DIR" \ + --head-sha "$PR_HEAD_SHA" \ + --output-dir "$OPENCODE_SOURCE_WORKDIR" \ + --manifest "$OPENCODE_SOURCE_MANIFEST" + [ "$(git -C "$OPENCODE_SOURCE_WORKDIR" rev-parse HEAD)" = "$PR_HEAD_SHA" ] + if find "$OPENCODE_SOURCE_WORKDIR" -type l -print -quit | grep -q .; then + echo "::error::Inert OpenCode source unexpectedly contains a symbolic link." + exit 1 + fi + if find "$OPENCODE_SOURCE_WORKDIR" -type f -perm /111 -print -quit | grep -q .; then + echo "::error::Inert OpenCode source unexpectedly contains an executable file." + exit 1 + fi + jq '{head_sha, tree_entries, written_files, tree_bytes, skipped, special_representations}' \ + "$OPENCODE_SOURCE_MANIFEST" - name: Configure git identity for OpenCode action run: | @@ -2800,7 +2826,7 @@ jobs: printf '\n```\n' printf '\n## Review inspection contract\n\n' - printf 'Use the local checkout for exact source and diff inspection.\n' + printf 'Use the inert local source tree and isolated read-only Git objects for exact source and diff inspection.\n' printf 'Do not run a broad full-diff read into the model context; inspect changed files and focused hunks only.\n' printf 'If direct file reads fail but focused changed hunks are present above, review those hunks; do not return file-inaccessible findings for paths shown in this evidence.\n' } >"$OPENCODE_EVIDENCE_FILE" diff --git a/scripts/ci/materialize_pr_review_source.py b/scripts/ci/materialize_pr_review_source.py new file mode 100644 index 00000000..1cdb4beb --- /dev/null +++ b/scripts/ci/materialize_pr_review_source.py @@ -0,0 +1,314 @@ +#!/usr/bin/env python3 +"""Materialize an inert PR source tree from an isolated bare Git repository. + +The privileged OpenCode review job must inspect pull-request content without +checking an untrusted commit out into the trusted workflow repository. This +helper copies only validated Git blobs into a fresh directory, strips every +executable bit, represents symbolic links as inert regular files, and connects +read-only Git queries to the separate bare object store through a trusted +``.git`` pointer. +""" + +from __future__ import annotations + +import argparse +import json +import os +from pathlib import Path, PurePosixPath +import re +import shutil + +# Git is invoked through an absolute executable path, a fixed argv, and no shell. +import subprocess # nosec B404 +import sys +from typing import BinaryIO + + +SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") +REGULAR_MODES = {"100644", "100755"} +SYMLINK_MODE = "120000" +GITLINK_MODE = "160000" +RESERVED_ROOTS = {".codegraph", ".git"} +DEFAULT_MAX_FILES = 100_000 +DEFAULT_MAX_BYTES = 1_073_741_824 +GIT_EXECUTABLE = shutil.which("git") +if not GIT_EXECUTABLE or not Path(GIT_EXECUTABLE).is_absolute(): + raise RuntimeError("an absolute Git executable path is required") + + +def positive_int(value: str) -> int: + """Parse a positive integer command-line limit.""" + parsed = int(value) + if parsed < 1: + raise argparse.ArgumentTypeError("value must be positive") + return parsed + + +def parse_args(argv: list[str]) -> argparse.Namespace: + """Parse command-line arguments.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--git-dir", type=Path, required=True) + parser.add_argument("--head-sha", required=True) + parser.add_argument("--output-dir", type=Path, required=True) + parser.add_argument("--manifest", type=Path, required=True) + parser.add_argument("--max-files", type=positive_int, default=DEFAULT_MAX_FILES) + parser.add_argument("--max-bytes", type=positive_int, default=DEFAULT_MAX_BYTES) + args = parser.parse_args(argv) + if not SHA_RE.fullmatch(args.head_sha): + parser.error("--head-sha must be a 40-character hexadecimal commit SHA") + return args + + +def git_bytes(git_dir: Path, *args: str) -> bytes: + """Run a read-only Git command against the isolated object store.""" + # argv only; the Git directory and commit inputs are validated before use. + completed = subprocess.run( # nosec B603 + [GIT_EXECUTABLE, f"--git-dir={git_dir}", *args], + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=False, + ) + if completed.returncode != 0: + detail = completed.stderr.decode("utf-8", errors="replace").strip() + raise RuntimeError(detail or f"git {' '.join(args)} failed") + return completed.stdout + + +def validate_git_dir(git_dir: Path, head_sha: str) -> Path: + """Return a resolved, bare Git directory containing the requested commit.""" + resolved = git_dir.resolve(strict=True) + if not resolved.is_dir(): + raise ValueError("--git-dir must resolve to a directory") + bare = git_bytes(resolved, "rev-parse", "--is-bare-repository").decode().strip() + if bare != "true": + raise ValueError("--git-dir must be an isolated bare repository") + commit = git_bytes(resolved, "rev-parse", f"{head_sha}^{{commit}}").decode().strip() + if commit.lower() != head_sha.lower(): + raise ValueError("--head-sha did not resolve to the exact requested commit") + return resolved + + +def validate_output_path(output_dir: Path, git_dir: Path) -> Path: + """Require a fresh output path that cannot overlap the Git object store.""" + output = output_dir.absolute() + if output.exists() or output.is_symlink(): + raise ValueError("--output-dir must not already exist") + output_parent = output.parent.resolve(strict=True) + output = output_parent / output.name + if output == git_dir or output in git_dir.parents or git_dir in output.parents: + raise ValueError("--output-dir and --git-dir must not overlap") + return output + + +def safe_relative_path(raw_path: bytes) -> PurePosixPath: + """Decode and validate a Git tree path without accepting traversal.""" + try: + decoded = raw_path.decode("utf-8", errors="strict") + except UnicodeDecodeError as exc: + raise ValueError("Git tree contains a non-UTF-8 path") from exc + path = PurePosixPath(decoded) + if not decoded or decoded.startswith("/") or path.is_absolute(): + raise ValueError(f"unsafe absolute or empty Git tree path: {decoded!r}") + if any(part in {"", ".", ".."} for part in path.parts): + raise ValueError(f"unsafe Git tree path component: {decoded!r}") + return path + + +def parse_tree( + git_dir: Path, head_sha: str +) -> list[tuple[str, str, str, int, PurePosixPath]]: + """Return validated recursive Git tree entries.""" + raw = git_bytes(git_dir, "ls-tree", "-r", "-z", "-l", "--full-tree", head_sha) + entries: list[tuple[str, str, str, int, PurePosixPath]] = [] + for record in raw.split(b"\0"): + if not record: + continue + try: + metadata, raw_path = record.split(b"\t", 1) + mode_raw, kind_raw, oid_raw, size_raw = metadata.split(maxsplit=3) + mode = mode_raw.decode("ascii") + kind = kind_raw.decode("ascii") + oid = oid_raw.decode("ascii") + size_text = size_raw.decode("ascii") + except (UnicodeDecodeError, ValueError) as exc: + raise ValueError("could not parse Git tree entry") from exc + if not SHA_RE.fullmatch(oid): + raise ValueError("Git tree entry has an invalid object id") + if mode == GITLINK_MODE and kind == "commit" and size_text == "-": + size = len(f"Submodule commit {oid}\n".encode()) + elif kind == "blob" and size_text.isdigit(): + size = int(size_text) + else: + raise ValueError(f"unsupported Git tree entry type/mode: {kind}/{mode}") + entries.append((mode, kind, oid, size, safe_relative_path(raw_path))) + return entries + + +def open_batch_reader(git_dir: Path) -> subprocess.Popen[bytes]: + """Start one Git batch process for bounded blob reads.""" + # Fixed Git subcommand and no shell evaluation. + return subprocess.Popen( # nosec B603 + [GIT_EXECUTABLE, f"--git-dir={git_dir}", "cat-file", "--batch"], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + +def read_blob(process: subprocess.Popen[bytes], oid: str, expected_size: int) -> bytes: + """Read one exact blob through the long-lived Git batch process.""" + stdin: BinaryIO | None = process.stdin + stdout: BinaryIO | None = process.stdout + if stdin is None or stdout is None: + raise RuntimeError("Git cat-file batch pipes are unavailable") + stdin.write(f"{oid}\n".encode("ascii")) + stdin.flush() + header = stdout.readline().rstrip(b"\n") + fields = header.split() + if len(fields) != 3 or fields[0].decode("ascii", errors="replace") != oid: + raise RuntimeError("Git cat-file returned an unexpected object header") + if fields[1] != b"blob" or not fields[2].isdigit(): + raise RuntimeError("Git cat-file object is not a blob") + actual_size = int(fields[2]) + if actual_size != expected_size: + raise RuntimeError("Git blob size changed after tree validation") + data = stdout.read(actual_size) + delimiter = stdout.read(1) + if len(data) != actual_size or delimiter != b"\n": + raise RuntimeError("Git cat-file returned a truncated blob") + return data + + +def write_inert_file(path: Path, data: bytes) -> None: + """Create one non-executable regular file without following links.""" + path.parent.mkdir(parents=True, exist_ok=True, mode=0o755) + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL + if hasattr(os, "O_NOFOLLOW"): + flags |= os.O_NOFOLLOW + descriptor = os.open(path, flags, 0o600) + try: + with os.fdopen(descriptor, "wb") as handle: + handle.write(data) + except BaseException: + try: + os.close(descriptor) + except OSError: + pass + raise + path.chmod(0o444) + + +def materialize(args: argparse.Namespace) -> dict[str, object]: + """Materialize validated inert files and return provenance metadata.""" + git_dir = validate_git_dir(args.git_dir, args.head_sha) + output_dir = validate_output_path(args.output_dir, git_dir) + manifest_path = args.manifest.absolute() + if manifest_path.exists() or manifest_path.is_symlink(): + raise ValueError("--manifest must not already exist") + if manifest_path == output_dir or output_dir in manifest_path.parents: + raise ValueError("--manifest must be outside --output-dir") + + entries = parse_tree(git_dir, args.head_sha) + if len(entries) > args.max_files: + raise ValueError( + f"Git tree exceeds --max-files ({len(entries)} > {args.max_files})" + ) + total_bytes = sum(entry[3] for entry in entries) + if total_bytes > args.max_bytes: + raise ValueError( + f"Git tree exceeds --max-bytes ({total_bytes} > {args.max_bytes})" + ) + + output_dir.mkdir(mode=0o755) + skipped: list[dict[str, str]] = [] + special: list[dict[str, str]] = [] + written = 0 + process = open_batch_reader(git_dir) + try: + for mode, kind, oid, size, relative in entries: + rendered = relative.as_posix() + if relative.parts[0] in RESERVED_ROOTS: + skipped.append({"path": rendered, "reason": "reserved-review-metadata"}) + continue + destination = output_dir.joinpath(*relative.parts) + if mode == GITLINK_MODE and kind == "commit": + data = f"Submodule commit {oid}\n".encode() + special.append( + { + "path": rendered, + "original_mode": mode, + "representation": "gitlink-marker", + } + ) + elif kind == "blob" and mode in REGULAR_MODES | {SYMLINK_MODE}: + data = read_blob(process, oid, size) + if mode == SYMLINK_MODE: + special.append( + { + "path": rendered, + "original_mode": mode, + "representation": "inert-regular-file", + } + ) + elif mode == "100755": + special.append( + { + "path": rendered, + "original_mode": mode, + "representation": "non-executable-regular-file", + } + ) + else: + raise ValueError(f"unsupported Git entry {kind}/{mode} at {rendered}") + write_inert_file(destination, data) + written += 1 + finally: + if process.stdin is not None: + process.stdin.close() + stderr = ( + process.stderr.read().decode("utf-8", errors="replace") + if process.stderr + else "" + ) + return_code = process.wait() + if return_code != 0 and sys.exc_info()[0] is None: + raise RuntimeError(stderr.strip() or "Git cat-file batch failed") + + git_pointer = output_dir / ".git" + write_inert_file(git_pointer, f"gitdir: {git_dir}\n".encode()) + metadata: dict[str, object] = { + "schema": 1, + "head_sha": args.head_sha.lower(), + "git_dir": str(git_dir), + "source_dir": str(output_dir), + "tree_entries": len(entries), + "written_files": written, + "tree_bytes": total_bytes, + "skipped": skipped, + "special_representations": special, + } + manifest_path.parent.mkdir(parents=True, exist_ok=True) + write_inert_file( + manifest_path, (json.dumps(metadata, sort_keys=True, indent=2) + "\n").encode() + ) + return metadata + + +def main(argv: list[str] | None = None) -> int: + """Run the inert PR source materializer.""" + args = parse_args(sys.argv[1:] if argv is None else argv) + try: + metadata = materialize(args) + except (OSError, RuntimeError, ValueError) as exc: + print(f"materialize_pr_review_source: {exc}", file=sys.stderr) + return 1 + print( + "Materialized inert PR source blobs: " + f"head={metadata['head_sha']} files={metadata['written_files']} bytes={metadata['tree_bytes']}" + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 64f79a5a..f11403ef 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -561,13 +561,17 @@ assert_opencode_review_uses_codegraph_and_gpt5_fallback() { assert_file_contains "$workflow_file" 'status publication failed because pr_head_sha was empty' "opencode repository_dispatch status fails closed when current-head identity is unavailable" assert_file_not_contains "$workflow_file" "actions/cache@" "opencode coverage does not restore PR-writable static R caches" assert_file_not_contains "$workflow_file" 'ref: ${{ github.event.client_payload.pr_head_sha }}' "opencode review must not checkout PR head into the trusted workflow workspace" - assert_file_contains "$workflow_file" "Materialize pull request head for OpenCode review data" "opencode review materializes PR-head source as read-only review data" - assert_file_contains "$workflow_file" 'git remote add pr-source "$GITHUB_SERVER_URL/$GH_REPOSITORY.git"' "opencode review fetches target PR commits through a separate PR-source remote" + assert_file_contains "$workflow_file" "Materialize inert pull request blobs for OpenCode review data" "opencode review materializes PR-head blobs as inert read-only review data" + assert_file_contains "$workflow_file" 'remote add pr-source' "opencode review fetches target PR commits through a separate PR-source remote" assert_file_contains "$workflow_file" 'refs/pull/${PR_NUMBER}/head' "opencode review can fetch fork PR heads without local workflow copies" - assert_file_contains "$workflow_file" 'git worktree add --detach "$OPENCODE_SOURCE_WORKDIR" "$PR_HEAD_SHA"' "opencode review materializes the PR head without actions/checkout credentials" - assert_file_contains "$workflow_file" 'cd "$OPENCODE_SOURCE_WORKDIR"' "opencode CodeGraph indexing runs against the PR-head source worktree" - assert_file_contains "$workflow_file" 'PR_MERGE_BASE="$(git -C "$OPENCODE_SOURCE_WORKDIR" merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA")"' "opencode review evidence diffs use the PR-head worktree merge base" - assert_file_contains "$workflow_file" 'git -C "$OPENCODE_SOURCE_WORKDIR" diff' "opencode review builds changed-file evidence from the PR-head worktree" + assert_file_contains "$workflow_file" 'git init --bare "$OPENCODE_SOURCE_GIT_DIR"' "opencode review isolates PR objects from the trusted workflow checkout" + assert_file_contains "$workflow_file" 'python3 scripts/ci/materialize_pr_review_source.py' "opencode review materializes only validated inert PR blobs" + assert_file_not_contains "$workflow_file" 'git worktree add --detach "$OPENCODE_SOURCE_WORKDIR"' "opencode review never creates an untrusted PR worktree" + assert_file_contains "$workflow_file" 'find "$OPENCODE_SOURCE_WORKDIR" -type l' "opencode review rejects materialized symbolic links" + assert_file_contains "$workflow_file" 'find "$OPENCODE_SOURCE_WORKDIR" -type f -perm /111' "opencode review rejects materialized executable files" + assert_file_contains "$workflow_file" 'cd "$OPENCODE_SOURCE_WORKDIR"' "opencode CodeGraph indexing runs against inert PR-head blobs" + assert_file_contains "$workflow_file" 'PR_MERGE_BASE="$(git -C "$OPENCODE_SOURCE_WORKDIR" merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA")"' "opencode review evidence diffs use the isolated bare object store merge base" + assert_file_contains "$workflow_file" 'git -C "$OPENCODE_SOURCE_WORKDIR" diff' "opencode review builds changed-file evidence from the isolated bare object store" assert_file_not_contains "$workflow_file" 'ref: ${{ github.event.pull_request.base.sha' "opencode trusted checkout avoids dynamic pull_request refs that Scorecard flags" assert_file_not_contains "$workflow_file" 'ref: ${{ github.event.pull_request.head.sha || github.event.client_payload.pr_head_sha || github.sha }}' "opencode review must not checkout PR head into the trusted workflow workspace" assert_file_not_contains "$workflow_file" 'secrets.GITHUB_TOKEN' "opencode review uses github.token instead of a nonexistent GITHUB_TOKEN secret" @@ -691,7 +695,7 @@ assert_opencode_review_uses_codegraph_and_gpt5_fallback() { assert_file_contains "$workflow_file" "Run OpenCode PR Review model pool" "opencode review includes a broad catalog fallback pool" assert_file_not_contains "$workflow_file" "steps.opencode_review_model_pool.outcome == 'success'" "opencode approval gate still runs after model pool failure to publish a reason" assert_file_contains "$workflow_file" "github-models/deepseek/deepseek-v3-0324 openai/gpt-5.6-luna github-models/openai/gpt-4.1 github-models/openai/gpt-5" "opencode review starts with DeepSeek V3 before full-size GPT fallbacks" - assert_file_contains "$workflow_file" "The publish gate re-runs source-backed validation against PR-head data" "opencode review publish gate validates model output against the PR-head worktree" + assert_file_contains "$workflow_file" "The publish gate re-runs source-backed validation against PR-head data" "opencode review publish gate validates model output against inert PR-head blobs" assert_file_contains "$workflow_file" '"openai/o3"' "opencode config declares OpenAI o3 fallback" assert_file_contains "$workflow_file" '"openai/o4-mini"' "opencode config declares OpenAI o4-mini fallback" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OpenCode %s attempt %s/%s failed with exit %s.' "opencode review logs per-model retry attempts" @@ -1317,8 +1321,8 @@ assert_opencode_review_uses_codegraph_and_gpt5_fallback() { assert_file_contains "$REPO_ROOT/scripts/ci/emit_opencode_failed_check_fallback_findings.sh" "get_validated_pr_diff_range" "failed-check fallback validates PR diff range before comparing trusted Strix inputs" assert_file_contains "$workflow_file" ".github/workflows/strix.yml" "opencode inline fallback watches Strix workflow changes" assert_file_contains "$workflow_file" "self_modifying_strix_base_failure" "opencode approval detects trusted-base Strix failures for self-modifying workflow PRs" - assert_file_contains "$workflow_file" 'local source_root="${OPENCODE_SOURCE_WORKDIR:-${GITHUB_WORKSPACE:-$PWD}}"' "opencode trusted-base Strix lag detection inspects the PR-head worktree" - assert_file_contains "$workflow_file" 'git -C "$source_root" diff --quiet' "opencode trusted-base Strix lag detection compares trusted-input changes in the PR-head worktree" + assert_file_contains "$workflow_file" 'local source_root="${OPENCODE_SOURCE_WORKDIR:-${GITHUB_WORKSPACE:-$PWD}}"' "opencode trusted-base Strix lag detection inspects inert PR-head blobs" + assert_file_contains "$workflow_file" 'git -C "$source_root" diff --quiet' "opencode trusted-base Strix lag detection compares trusted-input changes in the isolated bare object store" assert_file_contains "$workflow_file" "opencode.jsonc: No such file or directory" "opencode approval recognizes base-workflow Strix self-test evidence that cannot see PR-head OpenCode config" assert_file_contains "$workflow_file" "latest_current_head_manual_strix_run" "opencode approval inspects same-head manual Strix repository_dispatch runs before suppressing trusted-base Strix failures" assert_file_contains "$workflow_file" 'wait_for_peer_github_checks "$pending_checks_file"' "opencode approval waits for pending same-head manual Strix evidence before failing self-modifying workflow PRs" @@ -1850,7 +1854,7 @@ EOF assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" "never say no source files changed, no test files changed, or no executable changes when exact changed-file evidence lists workflow, script, source, or test files" "opencode prompt rejects contradictory changed-file kind claims" assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" "Never approve material workflow, script, source, config, package, or test changes with a reason or summary that says simple typo fix" "opencode prompt rejects trivial approval claims for material changes" assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" "OPENCODE_CHANGED_FILES_FILE" "opencode workflow exports exact current-head changed files" - assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" 'git -C "$OPENCODE_SOURCE_WORKDIR" diff --name-only --find-renames "$PR_MERGE_BASE" "$PR_HEAD_SHA" |' "opencode workflow derives exact changed files from the PR-head worktree" + assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" 'git -C "$OPENCODE_SOURCE_WORKDIR" diff --name-only --find-renames "$PR_MERGE_BASE" "$PR_HEAD_SHA" |' "opencode workflow derives exact changed files from the isolated bare object store" assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" 'awk '\''NF > 0 && $0 !~ /^\// && $0 !~ /(^|\/)\.\.($|\/)/ { print }'\'' >"$OPENCODE_CHANGED_FILES_FILE"' "opencode workflow writes path-safe exact changed files for the normalizer" assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" "changed-files.txt" "opencode workflow copies exact changed-file evidence into the isolated review workspace" assert_file_contains "$REPO_ROOT/.github/workflows/opencode-review.yml" 'A["text"]' "opencode prompt requires quoted Mermaid labels" diff --git a/tests/test_materialize_pr_review_source.py b/tests/test_materialize_pr_review_source.py new file mode 100644 index 00000000..4c6240bf --- /dev/null +++ b/tests/test_materialize_pr_review_source.py @@ -0,0 +1,150 @@ +"""Tests for inert pull-request source materialization.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +import stat +import subprocess + +from scripts.ci import materialize_pr_review_source as materializer + + +REPO_ROOT = Path(__file__).resolve().parents[1] +SCRIPT = REPO_ROOT / "scripts" / "ci" / "materialize_pr_review_source.py" + + +def git(repo: Path, *args: str) -> str: + """Run Git in a test repository and return stdout.""" + completed = subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + return completed.stdout.strip() + + +def build_repository(tmp_path: Path) -> tuple[Path, Path, str, str]: + """Create a work repository and an isolated bare clone.""" + work = tmp_path / "work" + work.mkdir() + git(work, "init", "--quiet") + git(work, "config", "user.name", "test") + git(work, "config", "user.email", "test@example.com") + (work / "README.md").write_text("base\n", encoding="utf-8") + git(work, "add", "README.md") + git(work, "commit", "--quiet", "-m", "base fixture") + base_sha = git(work, "rev-parse", "HEAD") + (work / "src").mkdir() + (work / "src" / "app.py").write_text("print('review data')\n", encoding="utf-8") + executable = work / "run.sh" + executable.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + executable.chmod(0o755) + os.symlink("src/app.py", work / "app-link") + (work / ".codegraph").mkdir() + (work / ".codegraph" / "config.json").write_text( + '{"untrusted": true}\n', encoding="utf-8" + ) + git(work, "add", ".") + git(work, "commit", "--quiet", "-m", "fixture") + head_sha = git(work, "rev-parse", "HEAD") + bare = tmp_path / "objects.git" + subprocess.run( + ["git", "clone", "--quiet", "--bare", str(work), str(bare)], + check=True, + ) + return work, bare, base_sha, head_sha + + +def test_materializes_only_inert_validated_blobs(tmp_path: Path) -> None: + """Executable, symlink, and CodeGraph-controlled paths stay inert.""" + _work, bare, base_sha, head_sha = build_repository(tmp_path) + source = tmp_path / "source" + manifest = tmp_path / "manifest.json" + + completed = subprocess.run( + [ + "python3", + str(SCRIPT), + "--git-dir", + str(bare), + "--head-sha", + head_sha, + "--output-dir", + str(source), + "--manifest", + str(manifest), + ], + check=False, + capture_output=True, + text=True, + ) + + assert completed.returncode == 0, completed.stderr + assert (source / "src" / "app.py").read_text( + encoding="utf-8" + ) == "print('review data')\n" + assert stat.S_IMODE((source / "run.sh").stat().st_mode) == 0o444 + assert not (source / "app-link").is_symlink() + assert (source / "app-link").read_text(encoding="utf-8") == "src/app.py" + assert not (source / ".codegraph").exists() + assert (source / ".git").is_file() + assert git(source, "rev-parse", "HEAD") == head_sha + assert git(source, "merge-base", base_sha, head_sha) == base_sha + assert set(git(source, "diff", "--name-only", base_sha, head_sha).splitlines()) == { + ".codegraph/config.json", + "app-link", + "run.sh", + "src/app.py", + } + + evidence = json.loads(manifest.read_text(encoding="utf-8")) + assert evidence["head_sha"] == head_sha + assert evidence["written_files"] == 4 + assert {entry["path"] for entry in evidence["skipped"]} == { + ".codegraph/config.json" + } + representations = { + entry["path"]: entry["representation"] + for entry in evidence["special_representations"] + } + assert representations == { + "app-link": "inert-regular-file", + "run.sh": "non-executable-regular-file", + } + + +def test_rejects_non_bare_git_directory(tmp_path: Path) -> None: + """A normal work repository cannot be confused with the object store.""" + work, _bare, _base_sha, head_sha = build_repository(tmp_path) + args = materializer.parse_args( + [ + "--git-dir", + str(work / ".git"), + "--head-sha", + head_sha, + "--output-dir", + str(tmp_path / "source"), + "--manifest", + str(tmp_path / "manifest.json"), + ] + ) + + try: + materializer.materialize(args) + except ValueError as exc: + assert "isolated bare repository" in str(exc) + else: + raise AssertionError("non-bare repository was accepted") + + +def test_rejects_unsafe_tree_paths() -> None: + """Traversal and absolute paths fail closed before filesystem writes.""" + for unsafe in (b"../escape", b"/absolute", b"a/../escape"): + try: + materializer.safe_relative_path(unsafe) + except ValueError: + continue + raise AssertionError(f"unsafe path was accepted: {unsafe!r}") diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 01bb0858..9d7aba2b 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1656,6 +1656,18 @@ def test_opencode_privileged_review_security_boundaries_are_fail_closed(): ) < target_job.index( "Exchange OpenCode app token for target repository review reads" ) + materialize_step = target_job.split( + " - name: Materialize inert pull request blobs for OpenCode review data", 1 + )[1].split("\n - name:", 1)[0] + assert 'git init --bare "$OPENCODE_SOURCE_GIT_DIR"' in materialize_step + assert 'git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch' in materialize_step + assert "python3 scripts/ci/materialize_pr_review_source.py" in materialize_step + assert 'git worktree add --detach "$OPENCODE_SOURCE_WORKDIR"' not in target_job + assert 'find "$OPENCODE_SOURCE_WORKDIR" -type l' in materialize_step + assert 'find "$OPENCODE_SOURCE_WORKDIR" -type f -perm /111' in materialize_step + assert materialize_step.index("materialize_pr_review_source.py") < materialize_step.index( + 'git -C "$OPENCODE_SOURCE_WORKDIR" rev-parse HEAD' + ) codegraph_step = target_job.split( " - name: Initialize CodeGraph index for OpenCode", 1 )[1].split("\n - name:", 1)[0] From 9548f668e725630a018d8369863fec791a20e848 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 09:27:20 +0900 Subject: [PATCH 2/9] fix(review): bind approvals and isolate Git metadata --- .github/workflows/noema-review.yml | 2 +- .github/workflows/opencode-review.yml | 111 +++++++-- scripts/ci/materialize_pr_review_source.py | 195 +++++++++++++--- scripts/ci/noema_review_gate.py | 125 ++++++++-- scripts/ci/opencode_dispatch_status.py | 44 +++- scripts/ci/opencode_existing_approval_gate.py | 29 ++- scripts/ci/pr_review_fix_scheduler.py | 10 +- scripts/ci/pr_review_merge_scheduler.py | 55 ++++- scripts/ci/test_strix_quick_gate.sh | 2 +- tests/test_materialize_pr_review_source.py | 70 ++++++ tests/test_noema_review_gate.py | 220 +++++++++++++++--- tests/test_opencode_agent_contract.py | 17 +- tests/test_opencode_existing_approval_gate.py | 66 +++++- tests/test_opencode_security_boundaries.py | 83 ++++++- tests/test_pr_review_fix_scheduler.py | 19 +- tests/test_pr_review_merge_scheduler.py | 93 ++++++-- 16 files changed, 970 insertions(+), 171 deletions(-) diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index e52371d4..43c2d41b 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -9,7 +9,7 @@ run-name: >- on: pull_request_target: - types: [opened, synchronize, reopened, ready_for_review, closed] + types: [opened, synchronize, reopened, ready_for_review, edited, closed] workflow_run: workflows: ["Required OpenCode Review", "Strix Security Scan"] types: [completed] diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 8c4839cf..433dee3f 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -10,7 +10,7 @@ on: # pull_request workflow is evaluated from the PR merge ref and therefore lets # an in-repository branch rewrite steps before repository secrets are bound. pull_request_target: - types: [opened, synchronize, reopened, ready_for_review, closed] + types: [opened, synchronize, reopened, ready_for_review, edited, closed] # repository_dispatch is evaluated only from the default branch. This keeps # privileged retries from loading workflow code from a caller-selected ref. repository_dispatch: @@ -630,7 +630,16 @@ jobs: # racing trusted result publication. export OPENCODE_SANDBOX_UID=65532 export OPENCODE_SANDBOX_GID=65532 - chown -R --no-dereference "$OPENCODE_SANDBOX_UID:$OPENCODE_SANDBOX_GID" /work + # Keep repository-local Git metadata outside the untrusted test + # identity's write boundary. The source tree itself stays writable so + # package managers and tests can create ordinary build artifacts. + chown "$OPENCODE_SANDBOX_UID:$OPENCODE_SANDBOX_GID" /work + find /work -mindepth 1 -maxdepth 1 ! -name .git \ + -exec chown -R --no-dereference "$OPENCODE_SANDBOX_UID:$OPENCODE_SANDBOX_GID" {} + + if [ -e /work/.git ] || [ -L /work/.git ]; then + chown -R root:root /work/.git + chmod -R go-w /work/.git + fi mkdir -p "$RUNNER_TEMP" /work/.opencode-sandbox-home /work/.opencode-sandbox-cache chown "$OPENCODE_SANDBOX_UID:$OPENCODE_SANDBOX_GID" /work/.opencode-sandbox-home /work/.opencode-sandbox-cache chmod 0700 "$RUNNER_TEMP" @@ -761,17 +770,30 @@ jobs: rm -f "$log_file" } + trusted_git() { + env -i \ + PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ + HOME=/tmp \ + GIT_CONFIG_NOSYSTEM=1 \ + GIT_CONFIG_GLOBAL=/dev/null \ + git \ + -c core.fsmonitor=false \ + -c core.hooksPath=/dev/null \ + -c core.quotePath=false \ + "$@" + } + has_tracked_files() { - git -c core.quotePath=false ls-files "$@" | awk 'NF { found=1 } END { exit found ? 0 : 1 }' + trusted_git ls-files "$@" | awk 'NF { found=1 } END { exit found ? 0 : 1 }' } changed_files_for_coverage() { if [ -n "${PR_BASE_SHA:-}" ] && [ -n "${PR_HEAD_SHA:-}" ] \ - && git rev-parse --verify --quiet "$PR_BASE_SHA^{commit}" >/dev/null \ - && git rev-parse --verify --quiet "$PR_HEAD_SHA^{commit}" >/dev/null; then - git -c core.quotePath=false diff --name-only --find-renames "$PR_BASE_SHA" "$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" else - git -c core.quotePath=false ls-files + trusted_git ls-files fi } @@ -780,7 +802,7 @@ jobs: changed_list="$(mktemp)" tracked_list="$(mktemp)" changed_files_for_coverage >"$changed_list" - git -c core.quotePath=false ls-files "$@" >"$tracked_list" + trusted_git ls-files "$@" >"$tracked_list" awk 'NR==FNR { changed[$0]=1; next } ($0 in changed) { found=1 } END { exit found ? 0 : 1 }' \ "$changed_list" "$tracked_list" local rc=$? @@ -789,7 +811,7 @@ jobs: } tracked_python_projects_with_tests() { - git -c core.quotePath=false ls-files 'pyproject.toml' '*/pyproject.toml' 'requirements.txt' '*/requirements.txt' \ + trusted_git ls-files 'pyproject.toml' '*/pyproject.toml' 'requirements.txt' '*/requirements.txt' \ | while IFS= read -r pyproject_file; do project_dir="$(dirname "$pyproject_file")" if [ "$project_dir" = "." ]; then @@ -982,7 +1004,7 @@ jobs: manifest="${candidate_dir}/package.json" fi if [ -f "$manifest" ] \ - && git ls-files --error-unmatch -- "$manifest" >/dev/null 2>&1; then + && trusted_git ls-files --error-unmatch -- "$manifest" >/dev/null 2>&1; then printf '%s\n' "$candidate_dir" break fi @@ -3765,6 +3787,7 @@ jobs: GH_TOKEN: ${{ steps.opencode_app_token.outputs.token }} GH_REPOSITORY: ${{ needs.validate-pr-metadata.outputs.target_repository }} PR_NUMBER: ${{ needs.validate-pr-metadata.outputs.pr_number }} + PR_BASE_REF: ${{ needs.validate-pr-metadata.outputs.base_ref }} HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} RUN_ID: ${{ github.run_id }} RUN_ATTEMPT: ${{ github.run_attempt }} @@ -4310,6 +4333,8 @@ jobs: "- Reason: ${model_reason}" \ "- Scope: \`${CENTRAL_REVIEW_PROCESS_FALLBACK_SCOPE_LABEL:-unknown}\`" \ "- Changed files: \`${CENTRAL_REVIEW_PROCESS_FALLBACK_CHANGED_COUNT:-unknown}\`" \ + "- Base ref: \`${PR_BASE_REF}\`" \ + "- Base SHA: \`${PR_BASE_SHA}\`" \ "- Head SHA: \`${HEAD_SHA}\`" \ "- Workflow run: ${RUN_ID}" \ "- Workflow attempt: ${RUN_ATTEMPT}" \ @@ -4326,9 +4351,11 @@ jobs: echo "::warning::CENTRAL_FAST_APPROVAL_LIVE_HEAD_UNAVAILABLE: could not re-check the live pull request head immediately before publishing an approval for ${HEAD_SHA}; skipping this GitHub side effect." exit 0 fi + live_base_ref="$(jq -r '.base.ref // empty' "$live_head_file")" + live_base_sha="$(jq -r '.base.sha // empty' "$live_head_file")" live_head="$(jq -r '.head.sha // empty' "$live_head_file")" - if [ "$live_head" != "$HEAD_SHA" ]; then - echo "::notice::CENTRAL_FAST_APPROVAL_STALE_HEAD: expected ${HEAD_SHA}, observed ${live_head:-missing}; skipping review publication." + if [ "$live_base_ref" != "$PR_BASE_REF" ] || [ "$live_base_sha" != "$PR_BASE_SHA" ] || [ "$live_head" != "$HEAD_SHA" ]; then + echo "::notice::CENTRAL_FAST_APPROVAL_STALE_IDENTITY: expected ${PR_BASE_REF}@${PR_BASE_SHA}..${HEAD_SHA}, observed ${live_base_ref:-missing}@${live_base_sha:-missing}..${live_head:-missing}; skipping review publication." exit 0 fi if ! curl_api_write -X POST --data-binary "@${payload_file}" \ @@ -4403,6 +4430,7 @@ jobs: NPM_CONFIG_IGNORE_SCRIPTS: "true" NO_COLOR: "1" PR_NUMBER: ${{ needs.validate-pr-metadata.outputs.pr_number }} + PR_BASE_REF: ${{ needs.validate-pr-metadata.outputs.base_ref }} HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} RUN_ID: ${{ github.run_id }} RUN_ATTEMPT: ${{ github.run_attempt }} @@ -4512,10 +4540,11 @@ jobs: --input "$review_payload_file" >"$response_file" 2>"$error_file" } - review_live_head_sha() { + review_live_pr_identity() { timeout "${REVIEW_PUBLISH_GH_API_TIMEOUT_SECONDS:-120}s" \ env GH_TOKEN="$review_head_guard_token" \ - gh api -X GET "repos/${GH_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha // empty' + gh api -X GET "repos/${GH_REPOSITORY}/pulls/${PR_NUMBER}" \ + --jq '[.base.ref // "", .base.sha // "", .head.sha // ""] | @tsv' } dismiss_stale_published_review() { @@ -4545,19 +4574,20 @@ jobs: validate_published_review_head() { local token_value="$1" response_file="$2" error_file="$3" - local live_head + local live_identity live_base_ref live_base_sha live_head - if ! live_head="$(review_live_head_sha 2>>"$error_file")"; then + if ! live_identity="$(review_live_pr_identity 2>>"$error_file")"; then REVIEW_PUBLICATION_STALE_HEAD=1 - printf 'OPENCODE_REVIEW_STALE_HEAD: live PR head could not be verified after publication for expected head %s.\n' "$HEAD_SHA" >>"$error_file" + printf 'OPENCODE_REVIEW_STALE_IDENTITY: live PR identity could not be verified after publication for expected base %s@%s and head %s.\n' "$PR_BASE_REF" "$PR_BASE_SHA" "$HEAD_SHA" >>"$error_file" return 1 fi - if [ "$live_head" = "$HEAD_SHA" ]; then + IFS=$'\t' read -r live_base_ref live_base_sha live_head <<<"$live_identity" + if [ "$live_base_ref" = "$PR_BASE_REF" ] && [ "$live_base_sha" = "$PR_BASE_SHA" ] && [ "$live_head" = "$HEAD_SHA" ]; then return 0 fi REVIEW_PUBLICATION_STALE_HEAD=1 - printf 'OPENCODE_REVIEW_STALE_HEAD: publication raced with a head update; expected %s, observed %s.\n' "$HEAD_SHA" "${live_head:-missing}" >>"$error_file" + printf 'OPENCODE_REVIEW_STALE_IDENTITY: publication raced with a PR identity update; expected %s@%s..%s, observed %s@%s..%s.\n' "$PR_BASE_REF" "$PR_BASE_SHA" "$HEAD_SHA" "${live_base_ref:-missing}" "${live_base_sha:-missing}" "${live_head:-missing}" >>"$error_file" dismiss_stale_published_review "$token_value" "$response_file" "$live_head" "$error_file" || true return 1 } @@ -4593,7 +4623,7 @@ jobs: post_pull_review_with_retry() { local token_label="$1" token_value="$2" review_payload_file="$3" error_file="$4" response_file="$5" - local attempts default_sleep attempt sleep_seconds api_timeout publish_status live_head + local attempts default_sleep attempt sleep_seconds api_timeout publish_status live_identity live_base_ref live_base_sha live_head attempts="${REVIEW_PUBLISH_RETRY_ATTEMPTS:-3}" default_sleep="${REVIEW_PUBLISH_RETRY_SLEEP_SECONDS:-30}" @@ -4602,9 +4632,15 @@ jobs: while :; do : >"$error_file" : >"$response_file" - if ! live_head="$(review_live_head_sha 2>>"$error_file")" || [ "$live_head" != "$HEAD_SHA" ]; then + if ! live_identity="$(review_live_pr_identity 2>>"$error_file")"; then + REVIEW_PUBLICATION_STALE_HEAD=1 + printf 'OPENCODE_REVIEW_STALE_IDENTITY: refusing publication because live PR identity could not be read for expected %s@%s..%s.\n' "$PR_BASE_REF" "$PR_BASE_SHA" "$HEAD_SHA" >>"$error_file" + return 1 + fi + IFS=$'\t' read -r live_base_ref live_base_sha live_head <<<"$live_identity" + if [ "$live_base_ref" != "$PR_BASE_REF" ] || [ "$live_base_sha" != "$PR_BASE_SHA" ] || [ "$live_head" != "$HEAD_SHA" ]; then REVIEW_PUBLICATION_STALE_HEAD=1 - printf 'OPENCODE_REVIEW_STALE_HEAD: refusing publication because expected head %s no longer matches live head %s.\n' "$HEAD_SHA" "${live_head:-missing}" >>"$error_file" + printf 'OPENCODE_REVIEW_STALE_IDENTITY: refusing publication because expected %s@%s..%s no longer matches live %s@%s..%s.\n' "$PR_BASE_REF" "$PR_BASE_SHA" "$HEAD_SHA" "${live_base_ref:-missing}" "${live_base_sha:-missing}" "${live_head:-missing}" >>"$error_file" return 1 fi printf 'OpenCode publishing pull review with %s token (attempt %s/%s, timeout %ss).\n' "$token_label" "$attempt" "$attempts" "$api_timeout" >&2 @@ -4696,6 +4732,8 @@ jobs: printf 'OpenCode current-head approval bridge\n\n' printf 'OpenCode approved current head `%s` with the primary review token, but legacy OpenCode `REQUEST_CHANGES` reviews published by `github-actions[bot]` can still determine GitHub `reviewDecision`. This same-head bridge approval supersedes only those stale OpenCode workflow reviews.\n\n' "$HEAD_SHA" printf -- '- Result: `APPROVE`\n' + printf -- '- Base ref: `%s`\n' "$PR_BASE_REF" + printf -- '- Base SHA: `%s`\n' "$PR_BASE_SHA" printf -- '- Head SHA: `%s`\n' "$HEAD_SHA" printf -- '- Workflow run: %s\n' "$RUN_ID" printf -- '- Workflow attempt: %s\n' "$RUN_ATTEMPT" @@ -6985,6 +7023,8 @@ jobs: printf '%s\n' "$reviews_json" | python3 scripts/ci/opencode_existing_approval_gate.py \ --head "$HEAD_SHA" \ + --base-ref "$PR_BASE_REF" \ + --base-sha "$PR_BASE_SHA" \ --require-opencode-app } @@ -7330,6 +7370,8 @@ jobs: "" \ "- Result: APPROVE" \ "- Reason: ${reason}" \ + "- Base ref: \`${PR_BASE_REF}\`" \ + "- Base SHA: \`${PR_BASE_SHA}\`" \ "- Head SHA: \`${HEAD_SHA}\`" \ "- Workflow run: ${RUN_ID}" \ "- Workflow attempt: ${RUN_ATTEMPT}" @@ -7448,6 +7490,8 @@ jobs: GH_TOKEN: ${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }} GH_REPOSITORY: ${{ needs.validate-pr-metadata.outputs.target_repository }} PR_NUMBER: ${{ needs.validate-pr-metadata.outputs.pr_number }} + PR_BASE_REF: ${{ needs.validate-pr-metadata.outputs.base_ref }} + PR_BASE_SHA: ${{ needs.validate-pr-metadata.outputs.base_sha }} PR_HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} OPENCODE_MODEL_POOL_OUTCOME: ${{ steps.opencode_review_model_pool.outputs.review_status }} @@ -7482,6 +7526,8 @@ jobs: --model-outcome "${OPENCODE_MODEL_POOL_OUTCOME:-missing}" \ --coverage-result "${COVERAGE_EVIDENCE_RESULT:-missing}" \ --expected-head "$PR_HEAD_SHA" \ + --expected-base-ref "$PR_BASE_REF" \ + --expected-base-sha "$PR_BASE_SHA" \ --pull-request-file "$pull_request_file" \ --reviews-file "$reviews_file" )" @@ -7507,6 +7553,7 @@ jobs: SCHEDULER_MUTATION_TOKEN_SOURCE: ${{ secrets.PR_REVIEW_MERGE_TOKEN != '' && 'PR_REVIEW_MERGE_TOKEN' || secrets.OPENCODE_APPROVE_TOKEN != '' && 'OPENCODE_APPROVE_TOKEN' || steps.opencode_app_token.outputs.available == 'true' && 'opencode-app' || 'github-token' }} GH_REPOSITORY: ${{ needs.validate-pr-metadata.outputs.target_repository }} PR_BASE_REF: ${{ needs.validate-pr-metadata.outputs.base_ref }} + PR_BASE_SHA: ${{ needs.validate-pr-metadata.outputs.base_sha }} PR_NUMBER: ${{ needs.validate-pr-metadata.outputs.pr_number }} PR_HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} run: | @@ -7536,6 +7583,8 @@ jobs: if printf '%s\n' "$reviews_json" | python3 scripts/ci/opencode_existing_approval_gate.py \ --head "$PR_HEAD_SHA" \ + --base-ref "$PR_BASE_REF" \ + --base-sha "$PR_BASE_SHA" \ --require-opencode-app \ 2>"$gate_error_file"; then approval_visible=1 @@ -7563,6 +7612,24 @@ jobs: exit 0 fi + live_pr_file="$(mktemp)" + live_pr_error_file="$(mktemp)" + if ! GH_TOKEN="$approval_read_token" timeout 30s \ + gh api "repos/${GH_REPOSITORY}/pulls/${PR_NUMBER}" >"$live_pr_file" 2>"$live_pr_error_file"; then + live_reason="$(tail -n 1 "$live_pr_error_file" 2>/dev/null || true)" + rm -f "$live_pr_file" "$live_pr_error_file" + printf '::warning::Merge scheduler follow-up skipped because live PR identity could not be revalidated after approval: %s.\n' "${live_reason:-GitHub API lookup failed}" + exit 0 + fi + live_base_ref="$(jq -r '.base.ref // empty' "$live_pr_file")" + live_base_sha="$(jq -r '.base.sha // empty' "$live_pr_file")" + live_head_sha="$(jq -r '.head.sha // empty' "$live_pr_file")" + rm -f "$live_pr_file" "$live_pr_error_file" + if [ "$live_base_ref" != "$PR_BASE_REF" ] || [ "$live_base_sha" != "$PR_BASE_SHA" ] || [ "$live_head_sha" != "$PR_HEAD_SHA" ]; then + printf '::warning::Merge scheduler follow-up skipped because PR identity changed after approval validation. Expected=%s@%s..%s observed=%s@%s..%s.\n' "$PR_BASE_REF" "$PR_BASE_SHA" "$PR_HEAD_SHA" "${live_base_ref:-missing}" "${live_base_sha:-missing}" "${live_head_sha:-missing}" + exit 0 + fi + default_branch="$( gh api "repos/${GH_REPOSITORY}" --jq '.default_branch // empty' 2>/dev/null || true )" diff --git a/scripts/ci/materialize_pr_review_source.py b/scripts/ci/materialize_pr_review_source.py index 1cdb4beb..a21d6ee0 100644 --- a/scripts/ci/materialize_pr_review_source.py +++ b/scripts/ci/materialize_pr_review_source.py @@ -16,11 +16,13 @@ import os from pathlib import Path, PurePosixPath import re +import selectors import shutil # Git is invoked through an absolute executable path, a fixed argv, and no shell. import subprocess # nosec B404 import sys +import time from typing import BinaryIO @@ -31,6 +33,9 @@ RESERVED_ROOTS = {".codegraph", ".git"} DEFAULT_MAX_FILES = 100_000 DEFAULT_MAX_BYTES = 1_073_741_824 +DEFAULT_TREE_TIMEOUT_SECONDS = 60 +TREE_READ_CHUNK_BYTES = 65_536 +MAX_TREE_RECORD_BYTES = 1_048_576 GIT_EXECUTABLE = shutil.which("git") if not GIT_EXECUTABLE or not Path(GIT_EXECUTABLE).is_absolute(): raise RuntimeError("an absolute Git executable path is required") @@ -53,6 +58,11 @@ def parse_args(argv: list[str]) -> argparse.Namespace: parser.add_argument("--manifest", type=Path, required=True) parser.add_argument("--max-files", type=positive_int, default=DEFAULT_MAX_FILES) parser.add_argument("--max-bytes", type=positive_int, default=DEFAULT_MAX_BYTES) + parser.add_argument( + "--tree-timeout-seconds", + type=positive_int, + default=DEFAULT_TREE_TIMEOUT_SECONDS, + ) args = parser.parse_args(argv) if not SHA_RE.fullmatch(args.head_sha): parser.error("--head-sha must be a 40-character hexadecimal commit SHA") @@ -115,34 +125,150 @@ def safe_relative_path(raw_path: bytes) -> PurePosixPath: return path +def parse_tree_entry( + record: bytes, +) -> tuple[str, str, str, int, PurePosixPath]: + """Parse one bounded NUL-delimited ``git ls-tree`` record.""" + try: + metadata, raw_path = record.split(b"\t", 1) + mode_raw, kind_raw, oid_raw, size_raw = metadata.split(maxsplit=3) + mode = mode_raw.decode("ascii") + kind = kind_raw.decode("ascii") + oid = oid_raw.decode("ascii") + size_text = size_raw.decode("ascii") + except (UnicodeDecodeError, ValueError) as exc: + raise ValueError("could not parse Git tree entry") from exc + if not SHA_RE.fullmatch(oid): + raise ValueError("Git tree entry has an invalid object id") + if mode == GITLINK_MODE and kind == "commit" and size_text == "-": + size = len(f"Submodule commit {oid}\n".encode()) + elif kind == "blob" and size_text.isdigit(): + size = int(size_text) + else: + raise ValueError(f"unsupported Git tree entry type/mode: {kind}/{mode}") + return mode, kind, oid, size, safe_relative_path(raw_path) + + +def open_tree_reader(git_dir: Path, head_sha: str) -> subprocess.Popen[bytes]: + """Start bounded streaming enumeration of one exact commit tree.""" + return subprocess.Popen( # nosec B603 + [ + GIT_EXECUTABLE, + f"--git-dir={git_dir}", + "ls-tree", + "-r", + "-z", + "-l", + "--full-tree", + head_sha, + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=False, + ) + + +def terminate_process(process: subprocess.Popen[bytes]) -> None: + """Stop a tree producer promptly after a limit or parser failure.""" + if process.poll() is not None: + return + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + process.wait(timeout=5) + + def parse_tree( - git_dir: Path, head_sha: str -) -> list[tuple[str, str, str, int, PurePosixPath]]: - """Return validated recursive Git tree entries.""" - raw = git_bytes(git_dir, "ls-tree", "-r", "-z", "-l", "--full-tree", head_sha) + git_dir: Path, + head_sha: str, + *, + max_files: int, + max_bytes: int, + timeout_seconds: int, +) -> tuple[list[tuple[str, str, str, int, PurePosixPath]], int]: + """Stream and bound validated recursive Git tree entries.""" + process = open_tree_reader(git_dir, head_sha) + stdout = process.stdout + if stdout is None: + terminate_process(process) + raise RuntimeError("Git ls-tree stdout pipe is unavailable") + + selector = selectors.DefaultSelector() + selector.register(stdout, selectors.EVENT_READ) + deadline = time.monotonic() + timeout_seconds + pending = bytearray() entries: list[tuple[str, str, str, int, PurePosixPath]] = [] - for record in raw.split(b"\0"): - if not record: - continue - try: - metadata, raw_path = record.split(b"\t", 1) - mode_raw, kind_raw, oid_raw, size_raw = metadata.split(maxsplit=3) - mode = mode_raw.decode("ascii") - kind = kind_raw.decode("ascii") - oid = oid_raw.decode("ascii") - size_text = size_raw.decode("ascii") - except (UnicodeDecodeError, ValueError) as exc: - raise ValueError("could not parse Git tree entry") from exc - if not SHA_RE.fullmatch(oid): - raise ValueError("Git tree entry has an invalid object id") - if mode == GITLINK_MODE and kind == "commit" and size_text == "-": - size = len(f"Submodule commit {oid}\n".encode()) - elif kind == "blob" and size_text.isdigit(): - size = int(size_text) - else: - raise ValueError(f"unsupported Git tree entry type/mode: {kind}/{mode}") - entries.append((mode, kind, oid, size, safe_relative_path(raw_path))) - return entries + total_bytes = 0 + try: + reached_eof = False + while not reached_eof: + remaining = deadline - time.monotonic() + if remaining <= 0: + raise ValueError( + f"Git tree enumeration exceeded --tree-timeout-seconds ({timeout_seconds})" + ) + events = selector.select(timeout=min(1.0, remaining)) + if not events: + if process.poll() is not None: + reached_eof = True + continue + chunk = os.read(stdout.fileno(), TREE_READ_CHUNK_BYTES) + if not chunk: + reached_eof = True + continue + pending.extend(chunk) + while True: + separator = pending.find(0) + if separator < 0: + if len(pending) > MAX_TREE_RECORD_BYTES: + raise ValueError( + "Git tree entry exceeds the bounded record-size limit" + ) + break + if separator > MAX_TREE_RECORD_BYTES: + raise ValueError( + "Git tree entry exceeds the bounded record-size limit" + ) + record = bytes(pending[:separator]) + del pending[: separator + 1] + if not record: + continue + entry = parse_tree_entry(record) + next_file_count = len(entries) + 1 + if next_file_count > max_files: + raise ValueError( + f"Git tree exceeds --max-files ({next_file_count} > {max_files})" + ) + next_total_bytes = total_bytes + entry[3] + if next_total_bytes > max_bytes: + raise ValueError( + f"Git tree exceeds --max-bytes ({next_total_bytes} > {max_bytes})" + ) + entries.append(entry) + total_bytes = next_total_bytes + if pending: + raise ValueError("Git tree output ended with an unterminated record") + except BaseException: + terminate_process(process) + raise + finally: + selector.close() + + try: + return_code = process.wait(timeout=5) + except subprocess.TimeoutExpired as exc: + terminate_process(process) + raise RuntimeError("Git ls-tree did not exit after output completed") from exc + stderr = ( + process.stderr.read().decode("utf-8", errors="replace").strip() + if process.stderr + else "" + ) + if return_code != 0: + raise RuntimeError(stderr or "Git ls-tree failed") + return entries, total_bytes def open_batch_reader(git_dir: Path) -> subprocess.Popen[bytes]: @@ -209,16 +335,13 @@ def materialize(args: argparse.Namespace) -> dict[str, object]: if manifest_path == output_dir or output_dir in manifest_path.parents: raise ValueError("--manifest must be outside --output-dir") - entries = parse_tree(git_dir, args.head_sha) - if len(entries) > args.max_files: - raise ValueError( - f"Git tree exceeds --max-files ({len(entries)} > {args.max_files})" - ) - total_bytes = sum(entry[3] for entry in entries) - if total_bytes > args.max_bytes: - raise ValueError( - f"Git tree exceeds --max-bytes ({total_bytes} > {args.max_bytes})" - ) + entries, total_bytes = parse_tree( + git_dir, + args.head_sha, + max_files=args.max_files, + max_bytes=args.max_bytes, + timeout_seconds=args.tree_timeout_seconds, + ) output_dir.mkdir(mode=0o755) skipped: list[dict[str, str]] = [] diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5b024c84..b45602d2 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -29,6 +29,8 @@ "opencode-review-control-v1", ) REVIEW_BODY_HEAD_SHA_RE = re.compile(r"Head SHA:\s*`([0-9a-fA-F]{40})`") +REVIEW_BODY_BASE_REF_RE = re.compile(r"Base ref:\s*`([A-Za-z0-9._/-]+)`") +REVIEW_BODY_BASE_SHA_RE = re.compile(r"Base SHA:\s*`([0-9a-fA-F]{40})`") IGNORED_RUNNING_CHECKS = { "approve-after-primary-review", "noema-review", @@ -109,6 +111,8 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: title body isDraft + baseRefName + baseRefOid headRefOid reviewDecision reviewThreads(first: 100) { @@ -186,20 +190,42 @@ def review_body_head_sha(review: dict[str, Any]) -> str | None: return matches[-1] if matches else None -def review_matches_current_head(review: dict[str, Any], head_sha: str) -> bool: - """Return whether commit and explicit review-body evidence match the live head.""" +def review_matches_current_head( + review: dict[str, Any], + pr: dict[str, Any], + *, + require_base_identity: bool = False, +) -> bool: + """Return whether review evidence matches the live pull-request identity.""" + head_sha = str(pr.get("headRefOid") or "") if not head_sha or review_commit(review) != head_sha: return False body_head = review_body_head_sha(review) - return body_head is None or body_head.lower() == head_sha.lower() + if body_head is not None and body_head.lower() != head_sha.lower(): + return False + if not require_base_identity: + return True + body = str(review.get("body") or "") + base_refs = REVIEW_BODY_BASE_REF_RE.findall(body) + base_shas = REVIEW_BODY_BASE_SHA_RE.findall(body) + base_ref = str(pr.get("baseRefName") or "") + base_sha = str(pr.get("baseRefOid") or "") + return bool( + body_head + and base_ref + and base_sha + and base_refs + and base_refs[-1] == base_ref + and base_shas + and base_shas[-1].lower() == base_sha.lower() + ) def current_primary_approval(pr: dict[str, Any]) -> dict[str, Any] | None: """Return the current-head OpenCode approval when it matches the contract.""" - head_sha = str(pr.get("headRefOid") or "") - reviews = (((pr.get("reviews") or {}).get("nodes")) or []) + reviews = ((pr.get("reviews") or {}).get("nodes")) or [] for review in reversed(reviews): - if not review_matches_current_head(review, head_sha): + if not review_matches_current_head(review, pr, require_base_identity=True): continue if str(review.get("state") or "").upper() != "APPROVED": continue @@ -212,36 +238,48 @@ def current_primary_approval(pr: dict[str, Any]) -> dict[str, Any] | None: def has_current_changes_requested(pr: dict[str, Any]) -> bool: """Return whether the current head has any changes-requested review.""" - head_sha = str(pr.get("headRefOid") or "") - reviews = (((pr.get("reviews") or {}).get("nodes")) or []) + reviews = ((pr.get("reviews") or {}).get("nodes")) or [] for review in reversed(reviews): - if review_matches_current_head(review, head_sha) and str(review.get("state") or "").upper() == "CHANGES_REQUESTED": + if ( + review_matches_current_head(review, pr) + and str(review.get("state") or "").upper() == "CHANGES_REQUESTED" + ): return True return False def has_unresolved_threads(pr: dict[str, Any]) -> bool: """Return whether any non-outdated review thread is unresolved.""" - threads = (((pr.get("reviewThreads") or {}).get("nodes")) or []) - return any(not thread.get("isResolved") and not thread.get("isOutdated") for thread in threads) + threads = ((pr.get("reviewThreads") or {}).get("nodes")) or [] + return any( + not thread.get("isResolved") and not thread.get("isOutdated") + for thread in threads + ) def check_label(node: dict[str, Any]) -> str: """Return a human-readable label for a status context or check run.""" if node.get("__typename") == "StatusContext": return str(node.get("context") or "") - workflow = ((((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name") or "") + workflow = ( + ((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {} + ).get("name") or "" name = str(node.get("name") or "") return f"{workflow} / {name}" if workflow else name def blocking_checks(pr: dict[str, Any]) -> list[str]: """Return check contexts that should block Noema review.""" - contexts = ((((pr.get("statusCheckRollup") or {}).get("contexts") or {}).get("nodes")) or []) + contexts = ( + ((pr.get("statusCheckRollup") or {}).get("contexts") or {}).get("nodes") + ) or [] blockers: list[str] = [] for node in contexts: label = check_label(node) - if label in IGNORED_RUNNING_CHECKS or str(node.get("name") or "") in IGNORED_RUNNING_CHECKS: + if ( + label in IGNORED_RUNNING_CHECKS + or str(node.get("name") or "") in IGNORED_RUNNING_CHECKS + ): continue if node.get("__typename") == "StatusContext": state = str(node.get("state") or "").upper() @@ -259,12 +297,15 @@ def blocking_checks(pr: dict[str, Any]) -> list[str]: def existing_noema_review(pr: dict[str, Any], actor: str) -> bool: """Return whether Noema already reviewed the current head.""" - head_sha = str(pr.get("headRefOid") or "") marker = "", + f"", ] ) payload = { @@ -594,7 +667,9 @@ def inspect_and_review(repo: str, number: int) -> int: print("Current head already has a Noema review; nothing to do.") return 0 if not current_primary_approval(pr): - print("Current head does not have a primary OpenCode approval; Noema review skipped.") + print( + "Current head does not have a primary OpenCode approval; Noema review skipped." + ) return 0 if has_current_changes_requested(pr): print("Current head has requested changes; Noema review skipped.") diff --git a/scripts/ci/opencode_dispatch_status.py b/scripts/ci/opencode_dispatch_status.py index e413fb00..ba185be7 100644 --- a/scripts/ci/opencode_dispatch_status.py +++ b/scripts/ci/opencode_dispatch_status.py @@ -11,10 +11,17 @@ APPROVAL_AUTHORS = frozenset({"opencode-agent", "opencode-agent[bot]"}) HEAD_SHA_RE = re.compile(r"Head SHA:\s*`?([0-9a-fA-F]{40})`?", re.IGNORECASE) +BASE_SHA_RE = re.compile(r"Base SHA:\s*`?([0-9a-fA-F]{40})`?", re.IGNORECASE) +BASE_REF_RE = re.compile(r"Base ref:\s*`?([A-Za-z0-9._/-]+)`?", re.IGNORECASE) -def _has_current_approval(reviews: Sequence[dict[str, Any]], head_sha: str) -> bool: - """Return whether the latest OpenCode decision explicitly approves the exact head.""" +def _has_current_approval( + reviews: Sequence[dict[str, Any]], + head_sha: str, + base_ref: str, + base_sha: str, +) -> bool: + """Return whether the latest OpenCode decision approves the exact PR identity.""" for review in reversed(reviews): author = str((review.get("user") or {}).get("login") or "").casefold() if author not in APPROVAL_AUTHORS: @@ -24,6 +31,16 @@ def _has_current_approval(reviews: Sequence[dict[str, Any]], head_sha: str) -> b body_heads = HEAD_SHA_RE.findall(str(review.get("body") or "")) if not body_heads or body_heads[-1].lower() != head_sha.lower(): continue + body = str(review.get("body") or "") + body_base_refs = BASE_REF_RE.findall(body) + body_base_shas = BASE_SHA_RE.findall(body) + if ( + not body_base_refs + or body_base_refs[-1] != base_ref + or not body_base_shas + or body_base_shas[-1].lower() != base_sha.lower() + ): + return False return str(review.get("state") or "").upper() == "APPROVED" return False @@ -33,23 +50,36 @@ def decide_status( model_outcome: str, coverage_result: str, expected_head: str, + expected_base_ref: str, + expected_base_sha: str, pull_request: dict[str, Any], reviews: Sequence[dict[str, Any]], ) -> dict[str, str]: """Return a fail-closed GitHub commit-status decision.""" live_head = str((pull_request.get("head") or {}).get("sha") or "") + live_base_ref = str((pull_request.get("base") or {}).get("ref") or "") + live_base_sha = str((pull_request.get("base") or {}).get("sha") or "") if model_outcome != "success": reason = "OpenCode model review did not produce approval evidence." elif coverage_result != "success": reason = "OpenCode coverage evidence did not pass for the current head." elif not expected_head or live_head.lower() != expected_head.lower(): reason = "OpenCode status target is stale or the live PR head is unavailable." - elif not _has_current_approval(reviews, expected_head): - reason = "No validated exact-current-head OpenCode approval was published." + elif ( + not expected_base_ref + or live_base_ref != expected_base_ref + or not expected_base_sha + or live_base_sha.lower() != expected_base_sha.lower() + ): + reason = "OpenCode status target base changed or is unavailable." + elif not _has_current_approval( + reviews, expected_head, expected_base_ref, expected_base_sha + ): + reason = "No validated exact-current-PR OpenCode approval was published." else: return { "state": "success", - "description": "Validated current-head OpenCode approval and coverage passed.", + "description": "Validated base-bound current-head OpenCode approval and coverage.", } return {"state": "failure", "description": reason} @@ -60,6 +90,8 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: parser.add_argument("--model-outcome", required=True) parser.add_argument("--coverage-result", required=True) parser.add_argument("--expected-head", required=True) + parser.add_argument("--expected-base-ref", required=True) + parser.add_argument("--expected-base-sha", required=True) parser.add_argument("--pull-request-file", required=True, type=Path) parser.add_argument("--reviews-file", required=True, type=Path) return parser.parse_args(argv) @@ -78,6 +110,8 @@ def main(argv: Sequence[str] | None = None) -> int: model_outcome=args.model_outcome, coverage_result=args.coverage_result, expected_head=args.expected_head, + expected_base_ref=args.expected_base_ref, + expected_base_sha=args.expected_base_sha, pull_request=pull_request, reviews=reviews, ), diff --git a/scripts/ci/opencode_existing_approval_gate.py b/scripts/ci/opencode_existing_approval_gate.py index 6712c022..19ab4e73 100644 --- a/scripts/ci/opencode_existing_approval_gate.py +++ b/scripts/ci/opencode_existing_approval_gate.py @@ -35,6 +35,7 @@ re.IGNORECASE | re.DOTALL, ) SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") +BASE_REF_RE = re.compile(r"^(?!-)[A-Za-z0-9._/-]+$") WORKFLOW_RUN_RE = re.compile(r"(?m)^- Workflow run: [1-9][0-9]*\s*$") WORKFLOW_ATTEMPT_RE = re.compile(r"(?m)^- Workflow attempt: [1-9][0-9]*\s*$") REQUIRED_PROBE_FIELDS = ( @@ -119,10 +120,12 @@ def adversarial_rejection_reason(body: str) -> str | None: def review_rejection_reason( review: dict[str, Any], head_sha: str, + base_ref: str, + base_sha: str, *, approval_authors: frozenset[str] = APPROVAL_AUTHORS, ) -> str | None: - """Explain why a review cannot prove a real current-head model approval.""" + """Explain why a review cannot prove a real current-PR model approval.""" if str(review.get("state") or "").upper() != "APPROVED": return "review state is not APPROVED" if str(review.get("commit_id") or "").lower() != head_sha.lower(): @@ -142,6 +145,10 @@ def review_rejection_reason( return "review body lacks an APPROVE result" if f"- Head SHA: `{head_sha}`" not in body: return "review body lacks the exact current-head SHA" + if f"- Base ref: `{base_ref}`" not in body: + return "review body lacks the exact current base ref" + if f"- Base SHA: `{base_sha}`" not in body: + return "review body lacks the exact current base SHA" if not WORKFLOW_RUN_RE.search(body): return "review body lacks a workflow run id" if not WORKFLOW_ATTEMPT_RE.search(body): @@ -152,6 +159,8 @@ def review_rejection_reason( def has_reusable_real_model_approval( reviews: list[dict[str, Any]], head_sha: str, + base_ref: str, + base_sha: str, *, log: TextIO, approval_authors: frozenset[str] = APPROVAL_AUTHORS, @@ -170,13 +179,15 @@ def has_reusable_real_model_approval( reason = review_rejection_reason( review, head_sha, + base_ref, + base_sha, approval_authors=approval_authors, ) review_id = review.get("id", "unknown") if reason is None: print( "existing-approval gate accepted real-model review " - f"id={review_id} author={login} head={head_sha}", + f"id={review_id} author={login} base={base_ref}@{base_sha} head={head_sha}", file=log, ) return True @@ -188,7 +199,7 @@ def has_reusable_real_model_approval( print( "existing-approval gate found no reusable real-model approval " - f"for head={head_sha}; same-head candidates={candidate_count}", + f"for base={base_ref}@{base_sha} head={head_sha}; same-head candidates={candidate_count}", file=log, ) return False @@ -198,6 +209,8 @@ def parse_args(argv: list[str]) -> argparse.Namespace: """Parse existing-approval gate command-line arguments.""" parser = argparse.ArgumentParser() parser.add_argument("--head", required=True) + parser.add_argument("--base-ref", required=True) + parser.add_argument("--base-sha", required=True) parser.add_argument( "--require-opencode-app", action="store_true", @@ -214,6 +227,14 @@ def main(argv: list[str]) -> int: "existing-approval gate requires a 40-character head SHA", file=sys.stderr ) return 2 + if not BASE_REF_RE.fullmatch(args.base_ref): + print("existing-approval gate requires a valid base ref", file=sys.stderr) + return 2 + if not SHA_RE.fullmatch(args.base_sha): + print( + "existing-approval gate requires a 40-character base SHA", file=sys.stderr + ) + return 2 try: reviews = flatten_reviews(json.load(sys.stdin)) except (json.JSONDecodeError, ValueError) as exc: @@ -227,6 +248,8 @@ def main(argv: list[str]) -> int: if has_reusable_real_model_approval( reviews, args.head, + args.base_ref, + args.base_sha, log=sys.stderr, approval_authors=approval_authors, ) diff --git a/scripts/ci/pr_review_fix_scheduler.py b/scripts/ci/pr_review_fix_scheduler.py index 5ffc1368..44c2959d 100755 --- a/scripts/ci/pr_review_fix_scheduler.py +++ b/scripts/ci/pr_review_fix_scheduler.py @@ -343,6 +343,7 @@ def fetch_comments(pr_number: int) -> tuple[int, list[dict[str, Any]]]: def self_test() -> int: """Run cheap contract checks.""" head = "a" * 40 + base = "b" * 40 comments = [{"body": f"{FIX_MARKER} head_sha={head} epoch={int(time.time())} -->"}] assert recent_fix_marker_exists(comments, head, 24 * 3600) assert not recent_fix_marker_exists(comments, "b" * 40, 24 * 3600) @@ -371,11 +372,18 @@ def self_test() -> int: "state": "APPROVED", "author": {"login": "opencode-agent"}, "commit": {"oid": head}, - "body": "Approved.", + "body": ( + "Approved.\n" + "- Base ref: `main`\n" + f"- Base SHA: `{base}`\n" + f"- Head SHA: `{head}`" + ), } ] }, "reviewThreads": {"nodes": []}, + "baseRefName": "main", + "baseRefOid": base, "headRefOid": head, "mergeStateStatus": "DIRTY", } diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index c13adfe9..df3decd9 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -125,7 +125,9 @@ GIT_REF_RE = re.compile(r"^(?!-)[A-Za-z0-9._/-]+$") GIT_SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") GITHUB_REPOSITORY_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") -REVIEW_BODY_HEAD_SHA_RE = re.compile(r"Head SHA:\s*`([0-9a-fA-F]{40})`") +REVIEW_BODY_HEAD_SHA_RE = re.compile(r"Head SHA:\s*`([^`\s]+)`") +REVIEW_BODY_BASE_REF_RE = re.compile(r"Base ref:\s*`([A-Za-z0-9._/-]+)`") +REVIEW_BODY_BASE_SHA_RE = re.compile(r"Base SHA:\s*`([^`\s]+)`") ACTIONS_JOB_DETAILS_URL_RE = re.compile(r"/actions/runs/\d+/job/(\d+)(?:[/?#]|$)") DIRECT_MERGE_AUTO_FALLBACK_MARKERS = ( "base branch policy prohibits the merge", @@ -987,17 +989,38 @@ def parse_github_datetime(value: str | None) -> datetime | None: def review_matches_current_head(review: dict[str, Any], pr: dict[str, Any]) -> bool: - """Return whether a review is valid evidence for the current head commit.""" + """Return whether a review is valid evidence for the current PR identity.""" head = pr.get("headRefOid") commit = (review.get("commit") or {}).get("oid") if not head: return False body_head = review_body_head_sha(review) + head_matches = False if commit == head: - return body_head is None or body_head.lower() == head.lower() - if not commit and body_head is not None: - return body_head.lower() == head.lower() - return False + head_matches = body_head is None or body_head.lower() == head.lower() + elif not commit and body_head is not None: + head_matches = body_head.lower() == head.lower() + if not head_matches: + return False + if str(review.get("state") or "").upper() != "APPROVED": + return True + + body = str(review.get("body") or "") + if any(marker in body.lower() for marker in DETERMINISTIC_APPROVAL_MARKERS): + return True + base_refs = REVIEW_BODY_BASE_REF_RE.findall(body) + base_shas = REVIEW_BODY_BASE_SHA_RE.findall(body) + base_ref = str(pr.get("baseRefName") or "") + base_sha = str(pr.get("baseRefOid") or "") + return bool( + body_head + and base_ref + and base_sha + and base_refs + and base_refs[-1] == base_ref + and base_shas + and base_shas[-1].lower() == base_sha.lower() + ) def review_body_head_sha(review: dict[str, Any]) -> str | None: @@ -3180,7 +3203,12 @@ def self_test() -> None: { "state": "APPROVED", "author": {"login": "opencode-agent"}, - "body": "OpenCode Agent approved this head.", + "body": ( + "OpenCode Agent approved this head.\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + "- Head SHA: `abc`" + ), "submittedAt": "2026-06-25T15:42:19Z", "commit": {"oid": "abc"}, } @@ -3371,6 +3399,12 @@ def self_test() -> None: assert decision.action == "update_branch" assert "branch is outdated before review dispatch" in decision.reason sample["reviews"]["nodes"][0]["commit"]["oid"] = "abc" + sample["reviews"]["nodes"][0]["body"] = ( + "OpenCode approved the current PR identity.\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + "- Head SHA: `abc`" + ) decision = inspect_pr( "owner/repo", sample, @@ -3552,7 +3586,12 @@ def self_test() -> None: { "state": "APPROVED", "author": {"login": "opencode-agent"}, - "body": "OpenCode Agent approved this head.", + "body": ( + "OpenCode Agent approved this head.\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + "- Head SHA: `abc`" + ), "submittedAt": "2026-06-25T15:42:19Z", "commit": {"oid": "abc"}, } diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index f11403ef..cf22d408 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -461,7 +461,7 @@ assert_opencode_review_uses_codegraph_and_gpt5_fallback() { local opencode_config="$REPO_ROOT/opencode.jsonc" assert_file_contains "$workflow_file" "pull_request_target:" "opencode review workflow loads privileged review logic from the protected base ref" - assert_file_contains "$workflow_file" "types: [opened, synchronize, reopened, ready_for_review, closed]" "opencode required workflow reacts to current PR head changes and closed-PR cleanup" + assert_file_contains "$workflow_file" "types: [opened, synchronize, reopened, ready_for_review, edited, closed]" "opencode required workflow reacts to current PR/base changes and closed-PR cleanup" assert_file_contains "$workflow_file" "repository_dispatch:" "opencode review supports default-branch scheduler current-head dispatch" assert_file_contains "$workflow_file" "types: [opencode-review]" "opencode repository dispatch accepts only its dedicated event type" assert_file_not_contains "$workflow_file" "workflow_dispatch:" "privileged opencode retries cannot load a caller-selected workflow ref" diff --git a/tests/test_materialize_pr_review_source.py b/tests/test_materialize_pr_review_source.py index 4c6240bf..512b047c 100644 --- a/tests/test_materialize_pr_review_source.py +++ b/tests/test_materialize_pr_review_source.py @@ -7,6 +7,8 @@ from pathlib import Path import stat import subprocess +import sys +import time from scripts.ci import materialize_pr_review_source as materializer @@ -148,3 +150,71 @@ def test_rejects_unsafe_tree_paths() -> None: except ValueError: continue raise AssertionError(f"unsafe path was accepted: {unsafe!r}") + + +def test_tree_file_limit_stops_streaming_producer_early( + monkeypatch, tmp_path: Path +) -> None: + """The parser terminates Git as soon as the next entry exceeds the limit.""" + oid = "a" * 40 + + def record(name: str) -> bytes: + return f"100644 blob {oid} 1\t{name}\0".encode() + + producer = ( + "import os,time; " + f"os.write(1, {record('one')!r}); " + f"os.write(1, {record('two')!r}); " + "time.sleep(30)" + ) + + def open_producer(_git_dir: Path, _head_sha: str): + return subprocess.Popen( + [sys.executable, "-c", producer], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + monkeypatch.setattr(materializer, "open_tree_reader", open_producer) + started = time.monotonic() + try: + materializer.parse_tree( + tmp_path, + oid, + max_files=1, + max_bytes=10, + timeout_seconds=10, + ) + except ValueError as exc: + assert "--max-files (2 > 1)" in str(exc) + else: + raise AssertionError("oversized streaming tree was accepted") + assert time.monotonic() - started < 5 + + +def test_tree_byte_limit_fails_before_materializing_output(tmp_path: Path) -> None: + """Accumulated blob sizes are rejected before the output directory exists.""" + _work, bare, _base_sha, head_sha = build_repository(tmp_path) + output = tmp_path / "bounded-source" + args = materializer.parse_args( + [ + "--git-dir", + str(bare), + "--head-sha", + head_sha, + "--output-dir", + str(output), + "--manifest", + str(tmp_path / "bounded-manifest.json"), + "--max-bytes", + "1", + ] + ) + + try: + materializer.materialize(args) + except ValueError as exc: + assert "--max-bytes" in str(exc) + else: + raise AssertionError("oversized tree bytes were accepted") + assert not output.exists() diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 75d40dbe..56b15f28 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -7,6 +7,23 @@ from scripts.ci import noema_review_gate as noema +HEAD = "a" * 40 +BASE_REF = "main" +BASE_SHA = "b" * 40 + + +def identity_body(marker: str = "Result: APPROVE", *, head: str = HEAD) -> str: + """Build review text bound to the default test PR identity.""" + return "\n".join( + ( + marker, + f"- Base ref: `{BASE_REF}`", + f"- Base SHA: `{BASE_SHA}`", + f"- Head SHA: `{head}`", + ) + ) + + def fake_secret(*parts: str) -> str: return "".join(parts) @@ -18,7 +35,9 @@ def make_pr(**overrides): "title": "Noema", "body": "", "isDraft": False, - "headRefOid": "head", + "baseRefName": BASE_REF, + "baseRefOid": BASE_SHA, + "headRefOid": HEAD, "reviews": {"nodes": []}, "reviewThreads": {"nodes": []}, "statusCheckRollup": {"contexts": {"nodes": []}}, @@ -27,11 +46,11 @@ def make_pr(**overrides): return value -def review(state="APPROVED", commit="head", login="opencode-agent", body="Result: APPROVE"): +def review(state="APPROVED", commit=HEAD, login="opencode-agent", body=None): """Build a minimal review node for Noema tests.""" return { "state": state, - "body": body, + "body": identity_body() if body is None else body, "author": {"login": login}, "commit": {"oid": commit}, } @@ -92,30 +111,65 @@ def fake_run(args, stdin=None): def test_review_state_helpers_cover_current_head_logic(): - marker_body = "OpenCode reviewed the current-head bounded evidence and found no blocking issues." + marker_body = identity_body( + "OpenCode reviewed the current-head bounded evidence and found no blocking issues." + ) current = review(body=marker_body) old = review(commit="old", body=marker_body) pr = make_pr(reviews={"nodes": [old, current]}) assert noema.review_author(current) == "opencode-agent" assert noema.review_author({}) == "" - assert noema.review_commit(current) == "head" + assert noema.review_commit(current) == HEAD assert noema.review_commit({}) == "" assert noema.current_primary_approval(pr) == current + assert ( + noema.current_primary_approval( + make_pr(baseRefName="release", reviews={"nodes": [current]}) + ) + is None + ) + assert ( + noema.current_primary_approval( + make_pr(baseRefOid="c" * 40, reviews={"nodes": [current]}) + ) + is None + ) assert noema.current_primary_approval(make_pr(reviews={"nodes": [old]})) is None - assert noema.current_primary_approval(make_pr(reviews={"nodes": [review("COMMENTED", body=marker_body)]})) is None - assert noema.current_primary_approval(make_pr(reviews={"nodes": [review(login="human", body=marker_body)]})) is None - assert noema.current_primary_approval( - make_pr( - reviews={ - "nodes": [review(login="github-actions[bot]", body=marker_body)] - } + assert ( + noema.current_primary_approval( + make_pr(reviews={"nodes": [review("COMMENTED", body=marker_body)]}) ) - ) is None - assert noema.has_current_changes_requested(make_pr(reviews={"nodes": [review("CHANGES_REQUESTED")]})) - assert not noema.has_current_changes_requested(make_pr(reviews={"nodes": [review("CHANGES_REQUESTED", commit="old")]})) - assert noema.has_unresolved_threads(make_pr(reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]})) - assert not noema.has_unresolved_threads(make_pr(reviewThreads={"nodes": [{"isResolved": False, "isOutdated": True}]})) + is None + ) + assert ( + noema.current_primary_approval( + make_pr(reviews={"nodes": [review(login="human", body=marker_body)]}) + ) + is None + ) + assert ( + noema.current_primary_approval( + make_pr( + reviews={ + "nodes": [review(login="github-actions[bot]", body=marker_body)] + } + ) + ) + is None + ) + assert noema.has_current_changes_requested( + make_pr(reviews={"nodes": [review("CHANGES_REQUESTED")]}) + ) + assert not noema.has_current_changes_requested( + make_pr(reviews={"nodes": [review("CHANGES_REQUESTED", commit="old")]}) + ) + assert noema.has_unresolved_threads( + make_pr(reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}) + ) + assert not noema.has_unresolved_threads( + make_pr(reviewThreads={"nodes": [{"isResolved": False, "isOutdated": True}]}) + ) def test_review_state_helpers_reject_explicit_previous_head_evidence(): @@ -128,7 +182,7 @@ def test_review_state_helpers_reject_explicit_previous_head_evidence(): ) exact_approval = review( commit=current_head, - body=f"{approval_marker}\n\n- Head SHA: `{current_head}`", + body=identity_body(approval_marker, head=current_head), ) stale_change_request = review( "CHANGES_REQUESTED", @@ -192,31 +246,59 @@ def test_check_helpers_and_existing_noema_review(): assert "CI / lint: FAILURE" in blockers assert "CI / slow: IN_PROGRESS" in blockers assert noema.existing_noema_review( - make_pr(reviews={"nodes": [review(login="noema", body="")]}), + make_pr( + reviews={ + "nodes": [ + review( + login="noema", + body=identity_body( + "" + ), + ) + ] + } + ), "noema", ) - assert not noema.existing_noema_review(make_pr(reviews={"nodes": [review("DISMISSED", login="noema")]}), "noema") - assert not noema.existing_noema_review(make_pr(reviews={"nodes": [review(commit="old", login="noema")]}), "noema") + assert not noema.existing_noema_review( + make_pr(reviews={"nodes": [review("DISMISSED", login="noema")]}), "noema" + ) + assert not noema.existing_noema_review( + make_pr(reviews={"nodes": [review(commit="old", login="noema")]}), "noema" + ) def test_current_actor_fetch_diff_and_json_extraction(monkeypatch): monkeypatch.setattr(noema, "run", lambda *args, **kwargs: "noema\n") assert noema.current_actor() == "noema" - monkeypatch.setattr(noema, "run", lambda *args, **kwargs: (_ for _ in ()).throw(RuntimeError("no gh"))) + monkeypatch.setattr( + noema, + "run", + lambda *args, **kwargs: (_ for _ in ()).throw(RuntimeError("no gh")), + ) assert noema.current_actor() == "" - monkeypatch.setattr(noema, "run", lambda *args, **kwargs: "x" * (noema.MAX_DIFF_CHARS + 5)) + monkeypatch.setattr( + noema, "run", lambda *args, **kwargs: "x" * (noema.MAX_DIFF_CHARS + 5) + ) diff, truncated = noema.fetch_diff("owner/repo", 1) assert truncated assert len(diff) == noema.MAX_DIFF_CHARS - assert noema.extract_json_object('{"decision":"approve"}') == {"decision": "approve"} - assert noema.extract_json_object('prefix {"decision":"comment"} suffix') == {"decision": "comment"} + assert noema.extract_json_object('{"decision":"approve"}') == { + "decision": "approve" + } + assert noema.extract_json_object('prefix {"decision":"comment"} suffix') == { + "decision": "comment" + } with pytest.raises(RuntimeError, match="did not contain"): noema.extract_json_object("not-json") -def test_review_context_builders_include_codegraph_threads_and_files(monkeypatch, tmp_path): +def test_review_context_builders_include_codegraph_threads_and_files( + monkeypatch, tmp_path +): assert noema.truncate_text("abc", 10) == "abc" assert "truncated 2 characters" in noema.truncate_text("abcdef", 4) assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "") @@ -479,18 +561,29 @@ def test_format_findings_and_submit_review(monkeypatch): calls = [] monkeypatch.setenv("NOEMA_REVIEW_TOKEN_SOURCE", "oidc") - monkeypatch.setattr(noema, "run", lambda args, stdin=None: calls.append((args, json.loads(stdin))) or "") + monkeypatch.setattr( + noema, + "run", + lambda args, stdin=None: calls.append((args, json.loads(stdin))) or "", + ) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) noema.submit_review( "owner/repo", 7, make_pr(), "noema", - {"decision": "request_changes", "summary": "fix it", "findings": [{"file": "a.py", "line": 1, "message": "bad"}]}, + { + "decision": "request_changes", + "summary": "fix it", + "findings": [{"file": "a.py", "line": 1, "message": "bad"}], + }, ) payload = calls[0][1] assert payload["event"] == "REQUEST_CHANGES" - assert payload["commit_id"] == "head" + assert payload["commit_id"] == HEAD assert "Noema LLM review" in payload["body"] + assert f"- Base ref: `{BASE_REF}`" in payload["body"] + assert f"- Base SHA: `{BASE_SHA}`" in payload["body"] assert "oidc" in payload["body"] calls.clear() @@ -498,9 +591,21 @@ def test_format_findings_and_submit_review(monkeypatch): assert calls[0][1]["event"] == "COMMENT" assert "No blocking findings" in calls[0][1]["body"] + monkeypatch.setattr( + noema, + "fetch_pr", + lambda repo, number: make_pr(baseRefName="release"), + ) + with pytest.raises(RuntimeError, match="live base/head identity changed"): + noema.submit_review( + "owner/repo", 7, make_pr(), "noema", {"decision": "approve"} + ) + def test_inspect_and_review_skip_paths(monkeypatch): - marker_body = "OpenCode reviewed the current-head bounded evidence and found no blocking issues." + marker_body = identity_body( + "OpenCode reviewed the current-head bounded evidence and found no blocking issues." + ) clean_pr = make_pr(reviews={"nodes": [review(body=marker_body)]}) calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) @@ -516,10 +621,51 @@ def test_inspect_and_review_skip_paths(monkeypatch): cases = [ (make_pr(), "noema"), (make_pr(isDraft=True), "noema"), - (make_pr(reviews={"nodes": [review(login="noema", body="")]}), "noema"), - (make_pr(reviews={"nodes": [review("CHANGES_REQUESTED"), review(body=marker_body)]}), "noema"), - (make_pr(reviews={"nodes": [review(body=marker_body)]}, reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}), "noema"), - (make_pr(reviews={"nodes": [review(body=marker_body)]}, statusCheckRollup={"contexts": {"nodes": [{"__typename": "StatusContext", "context": "ci", "state": "FAILURE"}]}}), "noema"), + ( + make_pr( + reviews={ + "nodes": [ + review( + login="noema", + body=identity_body(""), + ) + ] + } + ), + "noema", + ), + ( + make_pr( + reviews={ + "nodes": [review("CHANGES_REQUESTED"), review(body=marker_body)] + } + ), + "noema", + ), + ( + make_pr( + reviews={"nodes": [review(body=marker_body)]}, + reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}, + ), + "noema", + ), + ( + make_pr( + reviews={"nodes": [review(body=marker_body)]}, + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "StatusContext", + "context": "ci", + "state": "FAILURE", + } + ] + } + }, + ), + "noema", + ), (clean_pr, "opencode-agent"), ] for pr, actor in cases: @@ -536,7 +682,11 @@ def test_parse_args_and_main(monkeypatch): assert parsed.pr_number == 9 seen = [] - monkeypatch.setattr(noema, "inspect_and_review", lambda repo, number: seen.append((repo, number)) or 0) + monkeypatch.setattr( + noema, + "inspect_and_review", + lambda repo, number: seen.append((repo, number)) or 0, + ) assert noema.main(["--repo", "owner/repo", "--pr-number", "9"]) == 0 assert seen == [("owner/repo", 9)] diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 9d7aba2b..d9d135e7 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -346,7 +346,16 @@ def test_opencode_target_coverage_materializes_only_after_authorized_dispatch(): assert "target=/work" in measure_step assert "/var/run/docker.sock" not in measure_step assert "OPENCODE_SANDBOX_UID=65532" in measure_step - assert "chown -R --no-dereference" in measure_step + assert 'chown "$OPENCODE_SANDBOX_UID:$OPENCODE_SANDBOX_GID" /work' in measure_step + assert "find /work -mindepth 1 -maxdepth 1 ! -name .git" in measure_step + assert "chown -R root:root /work/.git" in measure_step + assert "chmod -R go-w /work/.git" in measure_step + assert "trusted_git()" in measure_step + assert "GIT_CONFIG_NOSYSTEM=1" in measure_step + assert "GIT_CONFIG_GLOBAL=/dev/null" in measure_step + assert "-c core.fsmonitor=false" in measure_step + assert "-c core.hooksPath=/dev/null" in measure_step + assert "git -c core.quotePath=false ls-files" not in measure_step assert 'setpriv \\\n --reuid "$OPENCODE_SANDBOX_UID"' in measure_step assert 'pkill -KILL -u "$OPENCODE_SANDBOX_UID"' in measure_step assert 'python3 -I - "$1"' in measure_step @@ -639,7 +648,7 @@ def test_opencode_coverage_discovers_changed_nested_javascript_package(tmp_path) measure_end = workflow.index("\n - name:", measure_start + 1) measure_step = workflow[measure_start:measure_end] - changed_start = measure_step.index(" changed_files_for_coverage() {\n") + changed_start = measure_step.index(" trusted_git() {\n") changed_end = measure_step.index( "\n\n has_changed_tracked_files()", changed_start ) @@ -1803,9 +1812,9 @@ def test_opencode_review_publication_prefers_app_token_for_review_writes(): assert 'review_write_token="${OPENCODE_APP_TOKEN:-}"' in workflow assert 'post_pull_review_with_retry "fallback review"' not in workflow assert "OPENCODE_REVIEW_IDENTITY_UNAVAILABLE" in workflow - assert "OPENCODE_REVIEW_STALE_HEAD" in workflow + assert "OPENCODE_REVIEW_STALE_IDENTITY" in workflow assert "OPENCODE_OVERVIEW_STALE_HEAD" in workflow - assert "review_live_head_sha()" in workflow + assert "review_live_pr_identity()" in workflow assert "validate_published_review_head()" in workflow assert "dismiss_stale_published_review()" in workflow assert 'review_head_guard_token="${GH_TOKEN:-$review_write_token}"' in workflow diff --git a/tests/test_opencode_existing_approval_gate.py b/tests/test_opencode_existing_approval_gate.py index 7602b2a1..e9437767 100644 --- a/tests/test_opencode_existing_approval_gate.py +++ b/tests/test_opencode_existing_approval_gate.py @@ -12,6 +12,8 @@ HEAD = "a" * 40 +BASE_REF = "main" +BASE_SHA = "b" * 40 SOURCE_LINES = ( b"name: Required OpenCode Review", b"on:", @@ -105,6 +107,8 @@ def valid_body(head: str = HEAD) -> str: "```", "", "- Result: APPROVE", + f"- Base ref: `{BASE_REF}`", + f"- Base SHA: `{BASE_SHA}`", f"- Head SHA: `{head}`", "- Workflow run: 123", "- Workflow attempt: 2", @@ -129,7 +133,9 @@ def review(**overrides): def test_flatten_reviews_and_accept_real_model_approval(payload): reviews = gate.flatten_reviews(payload) log = io.StringIO() - assert gate.has_reusable_real_model_approval(reviews, HEAD, log=log) + assert gate.has_reusable_real_model_approval( + reviews, HEAD, BASE_REF, BASE_SHA, log=log + ) assert "accepted real-model review" in log.getvalue() @@ -175,6 +181,34 @@ def test_extract_adversarial_evidence_uses_last_parseable_block(): ), "current-head", ), + ( + lambda value: value.update( + body=value["body"].replace(f"- Base ref: `{BASE_REF}`", "") + ), + "base ref", + ), + ( + lambda value: value.update( + body=value["body"].replace(f"- Base SHA: `{BASE_SHA}`", "") + ), + "base SHA", + ), + ( + lambda value: value.update( + body=value["body"].replace( + f"- Base ref: `{BASE_REF}`", "- Base ref: `release`" + ) + ), + "base ref", + ), + ( + lambda value: value.update( + body=value["body"].replace( + f"- Base SHA: `{BASE_SHA}`", f"- Base SHA: `{'c' * 40}`" + ) + ), + "base SHA", + ), ( lambda value: value.update( body=value["body"].replace("- Workflow run: 123", "") @@ -198,7 +232,7 @@ def test_extract_adversarial_evidence_uses_last_parseable_block(): def test_review_rejection_reason_rejects_non_model_evidence(mutate, reason): value = review() mutate(value) - assert reason in gate.review_rejection_reason(value, HEAD) + assert reason in gate.review_rejection_reason(value, HEAD, BASE_REF, BASE_SHA) @pytest.mark.parametrize( @@ -298,6 +332,8 @@ def test_has_reusable_real_model_approval_logs_rejected_candidates(): fallback, ], HEAD, + BASE_REF, + BASE_SHA, log=log, ) assert "rejected same-head review" in log.getvalue() @@ -310,11 +346,13 @@ def test_opencode_app_only_mode_rejects_github_actions_approval(): strict_log = io.StringIO() assert not gate.has_reusable_real_model_approval( - [actions_review], HEAD, log=default_log + [actions_review], HEAD, BASE_REF, BASE_SHA, log=default_log ) assert not gate.has_reusable_real_model_approval( [actions_review], HEAD, + BASE_REF, + BASE_SHA, log=strict_log, approval_authors=gate.OPENCODE_APP_APPROVAL_AUTHORS, ) @@ -328,6 +366,8 @@ def test_opencode_app_only_mode_accepts_app_approval(): assert gate.has_reusable_real_model_approval( [review()], HEAD, + BASE_REF, + BASE_SHA, log=log, approval_authors=gate.OPENCODE_APP_APPROVAL_AUTHORS, ) @@ -405,30 +445,36 @@ def test_adversarial_validation_rejects_forged_traversal_receipt(): def test_parse_args_and_main(monkeypatch, capsys): - args = gate.parse_args(["--head", HEAD]) + gate_args = ["--head", HEAD, "--base-ref", BASE_REF, "--base-sha", BASE_SHA] + args = gate.parse_args(gate_args) assert args.head == HEAD + assert args.base_ref == BASE_REF + assert args.base_sha == BASE_SHA assert not args.require_opencode_app - strict_args = gate.parse_args(["--head", HEAD, "--require-opencode-app"]) + strict_args = gate.parse_args([*gate_args, "--require-opencode-app"]) assert strict_args.require_opencode_app monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps([[review()]]))) - assert gate.main(["--head", HEAD]) == 0 + assert gate.main(gate_args) == 0 monkeypatch.setattr(sys, "stdin", io.StringIO("not-json")) - assert gate.main(["--head", HEAD]) == 2 + assert gate.main(gate_args) == 2 assert "could not parse reviews" in capsys.readouterr().err monkeypatch.setattr(sys, "stdin", io.StringIO("[]")) - assert gate.main(["--head", "short"]) == 2 + assert ( + gate.main(["--head", "short", "--base-ref", BASE_REF, "--base-sha", BASE_SHA]) + == 2 + ) assert "40-character" in capsys.readouterr().err monkeypatch.setattr(sys, "stdin", io.StringIO("[]")) - assert gate.main(["--head", HEAD]) == 1 + assert gate.main(gate_args) == 1 monkeypatch.setattr( sys, "stdin", io.StringIO(json.dumps([[review(user={"login": "github-actions[bot]"})]])), ) - assert gate.main(["--head", HEAD, "--require-opencode-app"]) == 1 + assert gate.main([*gate_args, "--require-opencode-app"]) == 1 diff --git a/tests/test_opencode_security_boundaries.py b/tests/test_opencode_security_boundaries.py index b3fde646..047e81e0 100644 --- a/tests/test_opencode_security_boundaries.py +++ b/tests/test_opencode_security_boundaries.py @@ -245,13 +245,20 @@ def test_safe_pytest_cli_paths_and_invalid_execution( assert exc.value.code == 0 +BASE_REF = "main" +BASE_SHA = "b" * 40 + + def approval_review(head_sha: str, **overrides: object) -> dict[str, object]: """Build one exact-current-head OpenCode approval review.""" review: dict[str, object] = { "state": "APPROVED", "commit_id": head_sha, "user": {"login": "opencode-agent[bot]"}, - "body": f"- Result: APPROVE\n- Head SHA: `{head_sha}`", + "body": ( + f"- Result: APPROVE\n- Base ref: `{BASE_REF}`\n" + f"- Base SHA: `{BASE_SHA}`\n- Head SHA: `{head_sha}`" + ), } review.update(overrides) return review @@ -264,7 +271,12 @@ def test_dispatch_status_requires_live_current_head_approval_and_coverage() -> N model_outcome="success", coverage_result="success", expected_head=head, - pull_request={"head": {"sha": head}}, + expected_base_ref=BASE_REF, + expected_base_sha=BASE_SHA, + pull_request={ + "head": {"sha": head}, + "base": {"ref": BASE_REF, "sha": BASE_SHA}, + }, reviews=[approval_review(head)], ) @@ -284,13 +296,57 @@ def test_dispatch_status_latest_current_head_decision_is_authoritative() -> None model_outcome="success", coverage_result="success", expected_head=head, - pull_request={"head": {"sha": head}}, + expected_base_ref=BASE_REF, + expected_base_sha=BASE_SHA, + pull_request={ + "head": {"sha": head}, + "base": {"ref": BASE_REF, "sha": BASE_SHA}, + }, reviews=reviews, ) assert decision["state"] == "failure" +@pytest.mark.parametrize( + ("live_base_ref", "live_base_sha", "review_body"), + [ + ("release", BASE_SHA, None), + (BASE_REF, "c" * 40, None), + ( + BASE_REF, + BASE_SHA, + f"- Result: APPROVE\n- Base ref: `release`\n" + f"- Base SHA: `{BASE_SHA}`\n- Head SHA: `{'a' * 40}`", + ), + ], +) +def test_dispatch_status_rejects_unchanged_head_base_retarget( + live_base_ref: str, + live_base_sha: str, + review_body: str | None, +) -> None: + """A same-head approval cannot be replayed after the base identity changes.""" + head = "a" * 40 + review = approval_review(head) + if review_body is not None: + review["body"] = review_body + decision = dispatch_status.decide_status( + model_outcome="success", + coverage_result="success", + expected_head=head, + expected_base_ref=BASE_REF, + expected_base_sha=BASE_SHA, + pull_request={ + "head": {"sha": head}, + "base": {"ref": live_base_ref, "sha": live_base_sha}, + }, + reviews=[review], + ) + + assert decision["state"] == "failure" + + @pytest.mark.parametrize( ("model_outcome", "coverage_result", "live_head", "review_overrides"), [ @@ -316,7 +372,12 @@ def test_dispatch_status_fails_closed_without_validated_approval( model_outcome=model_outcome, coverage_result=coverage_result, expected_head=head, - pull_request={"head": {"sha": observed_head}}, + expected_base_ref=BASE_REF, + expected_base_sha=BASE_SHA, + pull_request={ + "head": {"sha": observed_head}, + "base": {"ref": BASE_REF, "sha": BASE_SHA}, + }, reviews=[approval_review(head, **review_overrides)], ) @@ -333,7 +394,15 @@ def test_dispatch_status_cli_and_evidence_shape_validation( head = "a" * 40 pr_file = tmp_path / "pr.json" reviews_file = tmp_path / "reviews.json" - pr_file.write_text(json.dumps({"head": {"sha": head}}), encoding="utf-8") + pr_file.write_text( + json.dumps( + { + "head": {"sha": head}, + "base": {"ref": BASE_REF, "sha": BASE_SHA}, + } + ), + encoding="utf-8", + ) reviews_file.write_text(json.dumps([approval_review(head)]), encoding="utf-8") args = [ "--model-outcome", @@ -342,6 +411,10 @@ def test_dispatch_status_cli_and_evidence_shape_validation( "success", "--expected-head", head, + "--expected-base-ref", + BASE_REF, + "--expected-base-sha", + BASE_SHA, "--pull-request-file", str(pr_file), "--reviews-file", diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index a0ea3fe6..ef978cdb 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -402,6 +402,8 @@ def fake_run(argv, *, stdin=None): def _approved_dirty_pr(**overrides): """Return an approved PR that GitHub reports as conflicting.""" + head = "a" * 40 + base = "b" * 40 fields = { "mergeStateStatus": "DIRTY", "reviews": { @@ -409,8 +411,13 @@ def _approved_dirty_pr(**overrides): { "state": "APPROVED", "author": {"login": "opencode-agent"}, - "commit": {"oid": "a" * 40}, - "body": "Approved.", + "commit": {"oid": head}, + "body": ( + "Approved.\n" + "- Base ref: `main`\n" + f"- Base SHA: `{base}`\n" + f"- Head SHA: `{head}`" + ), } ] }, @@ -431,6 +438,14 @@ def test_needs_conflict_resolution_requires_approved_and_conflicting(): assert fix.needs_conflict_resolution( _approved_dirty_pr(mergeStateStatus="CLEAN") ) == (False, ()) + # A base retarget or base advance invalidates the prior approval even when + # the head commit itself did not change. + assert fix.needs_conflict_resolution( + _approved_dirty_pr(baseRefName="release") + ) == (False, ()) + assert fix.needs_conflict_resolution( + _approved_dirty_pr(baseRefOid="c" * 40) + ) == (False, ()) def test_dispatch_autofix_passes_resolve_conflict_flag(capsys): diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 1fe10da7..bcc26ea1 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -78,6 +78,12 @@ def opencode_review( "author": {"login": login}, "submittedAt": submitted_at, "commit": {"oid": commit}, + "body": ( + "OpenCode review evidence.\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + f"- Head SHA: `{commit}`" + ), } @@ -1013,6 +1019,19 @@ def test_review_state_and_failed_checks(): assert not sched.has_current_head_approval( make_pr(headRefOid="", reviews={"nodes": [opencode_review("APPROVED", "head")]}) ) + current_approval = opencode_review("APPROVED", "head") + assert not sched.has_current_head_approval( + make_pr( + baseRefName="release", + reviews={"nodes": [current_approval]}, + ) + ) + assert not sched.has_current_head_approval( + make_pr( + baseRefOid="advanced-base", + reviews={"nodes": [current_approval]}, + ) + ) exact_head = "a" * 40 stale_body_head = "b" * 40 body_sha_mismatch = make_pr( @@ -1034,7 +1053,12 @@ def test_review_state_and_failed_checks(): "nodes": [ { **opencode_review("APPROVED", exact_head), - "body": f"## Gate evidence\n\n- Head SHA: `{exact_head.upper()}`", + "body": ( + "## Gate evidence\n\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + f"- Head SHA: `{exact_head.upper()}`" + ), } ] }, @@ -1046,7 +1070,12 @@ def test_review_state_and_failed_checks(): "nodes": [ { **opencode_review("APPROVED", ""), - "body": f"## Gate evidence\n\n- Head SHA: `{exact_head}`", + "body": ( + "## Gate evidence\n\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + f"- Head SHA: `{exact_head}`" + ), } ] }, @@ -1145,7 +1174,7 @@ def test_review_state_and_failed_checks(): ] } ) - assert sched.has_current_head_approval(missing_review_time) + assert not sched.has_current_head_approval(missing_review_time) human_review_only = make_pr( reviews={"nodes": [opencode_review("APPROVED", "head", login="human")]} ) @@ -1196,7 +1225,12 @@ def test_review_state_and_failed_checks(): exact_head_approval = { **opencode_review("APPROVED", exact_head), "databaseId": 302, - "body": f"## Gate evidence\n\n- Head SHA: `{exact_head}`", + "body": ( + "## Gate evidence\n\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + f"- Head SHA: `{exact_head}`" + ), } stale_approval_history = make_pr( headRefOid=exact_head, @@ -1345,7 +1379,12 @@ def test_body_head_sha_approval_prevents_same_run_opencode_rerun(monkeypatch): "nodes": [ { **opencode_review("APPROVED", ""), - "body": f"## Gate evidence\n\n- Head SHA: `{head}`", + "body": ( + "## Gate evidence\n\n" + "- Base ref: `main`\n" + "- Base SHA: `base`\n" + f"- Head SHA: `{head}`" + ), } ] }, @@ -4482,27 +4521,55 @@ def test_action_error_guidance_distinguishes_update_branch_from_merge(): assert "PR head likely changed after inspection" in stale_head_error assert "reads the new head before mutating" in stale_head_error + def test_parse_conflict_reason_success(): """Test parse_conflict_reason with valid complete conflict strings.""" - assert sched.parse_conflict_reason("merge conflict: DIRTY; base=main,head=feature-branch") == ("DIRTY", "main", "feature-branch") - assert sched.parse_conflict_reason("Some prior text. merge conflict: BEHIND; base=develop,head=feat/123") == ("BEHIND", "develop", "feat/123") - assert sched.parse_conflict_reason("merge conflict: DIRTY; foo=bar; base=master,head=bugfix; other=stuff") == ("DIRTY", "master", "bugfix") + assert sched.parse_conflict_reason( + "merge conflict: DIRTY; base=main,head=feature-branch" + ) == ("DIRTY", "main", "feature-branch") + assert sched.parse_conflict_reason( + "Some prior text. merge conflict: BEHIND; base=develop,head=feat/123" + ) == ("BEHIND", "develop", "feat/123") + assert sched.parse_conflict_reason( + "merge conflict: DIRTY; foo=bar; base=master,head=bugfix; other=stuff" + ) == ("DIRTY", "master", "bugfix") + def test_parse_conflict_reason_no_prefix(): """Test parse_conflict_reason returns None when prefix is missing.""" assert sched.parse_conflict_reason("no conflict here") is None assert sched.parse_conflict_reason("merge conflict: space issue") is None + def test_parse_conflict_reason_empty_state(): """Test parse_conflict_reason defaults state to UNKNOWN if missing or empty.""" - assert sched.parse_conflict_reason("merge conflict: ; base=main,head=feature") == ("UNKNOWN", "main", "feature") - assert sched.parse_conflict_reason("merge conflict: ") == ("UNKNOWN", "base", "head") + assert sched.parse_conflict_reason("merge conflict: ; base=main,head=feature") == ( + "UNKNOWN", + "main", + "feature", + ) + assert sched.parse_conflict_reason("merge conflict: ") == ( + "UNKNOWN", + "base", + "head", + ) + def test_parse_conflict_reason_missing_branches(): """Test parse_conflict_reason uses defaults when branch info is missing or malformed.""" - assert sched.parse_conflict_reason("merge conflict: DIRTY; some other segment") == ("DIRTY", "base", "head") - assert sched.parse_conflict_reason("merge conflict: DIRTY; base=,head=something") == ("DIRTY", "base", "something") - assert sched.parse_conflict_reason("merge conflict: DIRTY; base=main,head=") == ("DIRTY", "main", "head") + assert sched.parse_conflict_reason("merge conflict: DIRTY; some other segment") == ( + "DIRTY", + "base", + "head", + ) + assert sched.parse_conflict_reason( + "merge conflict: DIRTY; base=,head=something" + ) == ("DIRTY", "base", "something") + assert sched.parse_conflict_reason("merge conflict: DIRTY; base=main,head=") == ( + "DIRTY", + "main", + "head", + ) def test_run_masks_secrets(): From 469c9ebc488bc62a5f994b9ab63033b41838f9b0 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 09:51:14 +0900 Subject: [PATCH 3/9] fix(review): fail closed before Noema credentials --- .github/workflows/noema-review.yml | 96 ++++++++++++++++++- scripts/ci/noema_review_gate.py | 44 ++++----- tests/test_noema_review_gate.py | 92 ++++++++++++------ .../test_required_workflow_queue_contract.py | 27 +++++- 4 files changed, 205 insertions(+), 54 deletions(-) diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 43c2d41b..ce9dc4fd 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -66,6 +66,96 @@ jobs: run: | echo "::notice::Noema review skipped: no pull request number is associated with this event." + - name: Bind Noema inputs to the live organization pull request + if: env.PR_NUMBER != '' + id: live_pr + env: + GH_TOKEN: ${{ github.token }} + EVENT_NAME: ${{ github.event_name }} + DISPATCH_ACTOR: ${{ github.triggering_actor }} + DISPATCH_SENDER: ${{ github.event.sender.login || '' }} + ALLOWED_DISPATCH_ACTOR: ${{ vars.NOEMA_REPOSITORY_DISPATCH_ACTOR || '' }} + ALLOWED_DISPATCH_TARGETS: ${{ vars.NOEMA_REPOSITORY_DISPATCH_TARGETS || '' }} + SUPPLIED_BASE_REF: ${{ github.event.client_payload.pr_base_ref || '' }} + SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha || '' }} + SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref || '' }} + SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || '' }} + run: | + set -euo pipefail + + fail_validation() { + echo "::error::$1" + exit 1 + } + + if [[ ! "$TARGET_REPOSITORY" =~ ^ContextualWisdomLab/[A-Za-z0-9_.-]+$ ]]; then + fail_validation "Noema target repository must be an exact ContextualWisdomLab repository name." + fi + if [[ ! "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]]; then + fail_validation "Noema pull request number must be a positive integer." + fi + + if [ "$EVENT_NAME" = "repository_dispatch" ]; then + if [ -z "$ALLOWED_DISPATCH_ACTOR" ]; then + fail_validation "NOEMA_REPOSITORY_DISPATCH_ACTOR must be configured before repository_dispatch can mint reviewer credentials." + fi + if [ "$DISPATCH_ACTOR" != "$ALLOWED_DISPATCH_ACTOR" ] || [ "$DISPATCH_SENDER" != "$ALLOWED_DISPATCH_ACTOR" ]; then + fail_validation "Noema repository_dispatch actor and sender must exactly match the configured identity." + fi + target_allowed=false + IFS=',' read -ra allowed_targets <<<"$ALLOWED_DISPATCH_TARGETS" + for allowed_target in "${allowed_targets[@]}"; do + if [ "$TARGET_REPOSITORY" = "${allowed_target//[[:space:]]/}" ]; then + target_allowed=true + break + fi + done + if [ "$target_allowed" != true ]; then + fail_validation "Noema repository_dispatch target is not present in NOEMA_REPOSITORY_DISPATCH_TARGETS." + fi + fi + + if ! live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")"; then + fail_validation "Noema could not load the requested live pull request." + fi + state="$(jq -r '.state // empty' <<<"$live_pr")" + base_repository="$(jq -r '.base.repo.full_name // empty' <<<"$live_pr")" + head_repository="$(jq -r '.head.repo.full_name // empty' <<<"$live_pr")" + base_ref="$(jq -r '.base.ref // empty' <<<"$live_pr")" + base_sha="$(jq -r '.base.sha // empty' <<<"$live_pr")" + head_ref="$(jq -r '.head.ref // empty' <<<"$live_pr")" + head_sha="$(jq -r '.head.sha // empty' <<<"$live_pr")" + + if [ "$state" != "open" ]; then + fail_validation "Noema only reviews open pull requests." + fi + if [ "$base_repository" != "$TARGET_REPOSITORY" ] || [ "$head_repository" != "$TARGET_REPOSITORY" ]; then + fail_validation "Noema reviewer credentials are restricted to same-repository pull requests." + fi + if [[ ! "$base_ref" =~ ^[A-Za-z0-9._/-]+$ ]] || [[ ! "$head_ref" =~ ^[A-Za-z0-9._/-]+$ ]]; then + fail_validation "Noema live pull request refs are invalid." + fi + if [[ ! "$base_sha" =~ ^[0-9a-fA-F]{40}$ ]] || [[ ! "$head_sha" =~ ^[0-9a-fA-F]{40}$ ]]; then + fail_validation "Noema live pull request SHAs are invalid." + fi + + if [ "$EVENT_NAME" = "repository_dispatch" ]; then + if [ "$SUPPLIED_BASE_REF" != "$base_ref" ] || [ "$SUPPLIED_BASE_SHA" != "$base_sha" ] || \ + [ "$SUPPLIED_HEAD_REF" != "$head_ref" ] || [ "$SUPPLIED_HEAD_SHA" != "$head_sha" ]; then + fail_validation "Noema repository_dispatch payload does not match the live base/head identity." + fi + fi + + { + echo "target_repository=$TARGET_REPOSITORY" + echo "repository_name=${TARGET_REPOSITORY#*/}" + echo "pr_number=$PR_NUMBER" + echo "base_ref=$base_ref" + echo "base_sha=$base_sha" + echo "head_ref=$head_ref" + echo "head_sha=$head_sha" + } >>"$GITHUB_OUTPUT" + - name: Resolve trusted Noema review source ref if: env.PR_NUMBER != '' id: trusted_source @@ -139,6 +229,7 @@ jobs: if: env.PR_NUMBER != '' id: noema_credential env: + TARGET_REPOSITORY: ${{ steps.live_pr.outputs.target_repository }} NOEMA_GITHUB_APP_CLIENT_ID: ${{ vars.NOEMA_GITHUB_APP_CLIENT_ID || '' }} NOEMA_GITHUB_APP_PRIVATE_KEY: ${{ secrets.NOEMA_GITHUB_APP_PRIVATE_KEY || '' }} TOKEN_EXCHANGE_URL: ${{ vars.NOEMA_TOKEN_EXCHANGE_URL || vars.NOEMA_EXCHANGE_URL || '' }} @@ -182,7 +273,7 @@ jobs: client-id: ${{ vars.NOEMA_GITHUB_APP_CLIENT_ID }} private-key: ${{ secrets.NOEMA_GITHUB_APP_PRIVATE_KEY }} owner: ContextualWisdomLab - repositories: ${{ steps.noema_credential.outputs.repository }} + repositories: ${{ steps.live_pr.outputs.repository_name }} permission-actions: read permission-checks: read permission-contents: read @@ -196,6 +287,7 @@ jobs: if: env.PR_NUMBER != '' && steps.noema_credential.outputs.source == 'oidc' id: noema_oidc_token env: + TARGET_REPOSITORY: ${{ steps.live_pr.outputs.target_repository }} OIDC_AUDIENCE: ${{ vars.NOEMA_OIDC_AUDIENCE || 'cwl-noema-review' }} TOKEN_EXCHANGE_URL: ${{ vars.NOEMA_TOKEN_EXCHANGE_URL || vars.NOEMA_EXCHANGE_URL || '' }} run: | @@ -253,6 +345,8 @@ jobs: - name: Run Noema LLM review and submit verdict if: env.PR_NUMBER != '' env: + TARGET_REPOSITORY: ${{ steps.live_pr.outputs.target_repository }} + PR_NUMBER: ${{ steps.live_pr.outputs.pr_number }} GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} NOEMA_REVIEW_TOKEN_SOURCE: ${{ steps.noema_credential.outputs.source == 'pat' && 'noema-review-pat' || steps.noema_credential.outputs.source == 'github-app' && 'noema-review-github-app' || 'noema-review-app-oidc' }} NOEMA_LLM_API_URL: ${{ vars.NOEMA_LLM_API_URL || '' }} diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index b45602d2..bd9341f5 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -298,6 +298,8 @@ def blocking_checks(pr: dict[str, Any]) -> list[str]: def existing_noema_review(pr: dict[str, Any], actor: str) -> bool: """Return whether Noema already reviewed the current head.""" marker = ""), + ) + ] + } + ), + "noema", + ) + assert not noema.existing_noema_review( + make_pr(reviews={"nodes": [review(login="noema", body=identity_body())]}), + "noema", + ) assert not noema.existing_noema_review( make_pr(reviews={"nodes": [review("DISMISSED", login="noema")]}), "noema" ) @@ -407,7 +424,11 @@ def test_call_llm_handles_configuration_and_verdicts(monkeypatch): monkeypatch.setenv("NOEMA_LLM_API_URL", "file:///etc/passwd") monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") - with pytest.raises(ValueError, match="URL scheme must be http or https"): + with pytest.raises(ValueError, match="must use https://"): + noema.call_llm("owner/repo", 1, pr, "diff", False) + + monkeypatch.setenv("NOEMA_LLM_API_URL", "http://llm.example.test/chat") + with pytest.raises(ValueError, match="must use https://"): noema.call_llm("owner/repo", 1, pr, "diff", False) monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") @@ -452,21 +473,21 @@ def fake_urlopen_defer(request, timeout=None): # Test invalid scheme (and no original URL in error) monkeypatch.setenv("NOEMA_LLM_API_URL", "file:///etc/passwd") - with pytest.raises(ValueError, match="URL scheme must be http or https"): + with pytest.raises(ValueError, match="must use https://"): noema.call_llm("owner/repo", 1, pr, "diff", False) # Test localhost rejection - monkeypatch.setenv("NOEMA_LLM_API_URL", "http://localhost/chat") + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://localhost/chat") with pytest.raises(ValueError, match="URL cannot target localhost"): noema.call_llm("owner/repo", 1, pr, "diff", False) # Test missing hostname - monkeypatch.setenv("NOEMA_LLM_API_URL", "http:///chat") + monkeypatch.setenv("NOEMA_LLM_API_URL", "https:///chat") with pytest.raises(ValueError, match="URL must have a valid hostname"): noema.call_llm("owner/repo", 1, pr, "diff", False) # Test internal IP rejection - monkeypatch.setenv("NOEMA_LLM_API_URL", "http://169.254.169.254/chat") + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://169.254.169.254/chat") with pytest.raises(ValueError, match="URL cannot target internal IP addresses"): noema.call_llm("owner/repo", 1, pr, "diff", False) @@ -474,7 +495,7 @@ def fake_urlopen_defer(request, timeout=None): original_getaddrinfo = socket.getaddrinfo # Test DNS resolution bypass - monkeypatch.setenv("NOEMA_LLM_API_URL", "http://resolved-to-local.example.com/chat") + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://resolved-to-local.example.com/chat") def fake_getaddrinfo(host, port, *args, **kwargs): if host == "resolved-to-local.example.com": return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("127.0.0.1", 0))] @@ -484,7 +505,7 @@ def fake_getaddrinfo(host, port, *args, **kwargs): noema.call_llm("owner/repo", 1, pr, "diff", False) # Test unresolved hostname does not break - monkeypatch.setenv("NOEMA_LLM_API_URL", "http://unresolved.example.com/chat") + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://unresolved.example.com/chat") def fake_getaddrinfo_error(host, port, *args, **kwargs): raise socket.gaierror("Name or service not known") monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo_error) @@ -492,7 +513,7 @@ def fake_getaddrinfo_error(host, port, *args, **kwargs): assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" # Test invalid IP string from getaddrinfo (unlikely but theoretically possible) - monkeypatch.setenv("NOEMA_LLM_API_URL", "http://weird-dns.example.com/chat") + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://weird-dns.example.com/chat") def fake_getaddrinfo_invalid_ip(host, port, *args, **kwargs): if host == "weird-dns.example.com": return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("not_an_ip", 0))] @@ -532,7 +553,7 @@ def raise_gaierror(host, port, *args, **kwargs): raise socket.gaierror("Name or service not known") monkeypatch.setattr(socket, "getaddrinfo", raise_gaierror) - with pytest.raises(ValueError, match="must start with http:// or https://"): + with pytest.raises(ValueError, match="must use https://"): noema.call_llm("owner/repo", 1, pr, "diff", False) @@ -544,7 +565,7 @@ def test_call_llm_rejects_non_http_parsed_scheme(monkeypatch): parsed = noema.urllib.parse.ParseResult("file", "llm.example.test", "/chat", "", "", "") monkeypatch.setattr(noema.urllib.parse, "urlparse", lambda _: parsed) - with pytest.raises(ValueError, match="URL scheme must be http or https"): + with pytest.raises(ValueError, match="must use https://"): noema.call_llm("owner/repo", 1, pr, "diff", False) @@ -618,21 +639,35 @@ def test_inspect_and_review_skip_paths(monkeypatch): assert noema.inspect_and_review("owner/repo", 7) == 0 assert calls + existing_review = ( + make_pr( + reviews={ + "nodes": [ + review( + login="noema", + body=identity_body(""), + ) + ] + } + ), + "noema", + ) + calls.clear() + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: existing_review[0]) + monkeypatch.setattr(noema, "current_actor", lambda: existing_review[1]) + assert noema.inspect_and_review("owner/repo", 7) == 0 + assert calls == [] + cases = [ - (make_pr(), "noema"), - (make_pr(isDraft=True), "noema"), + (make_pr(), "noema", "primary OpenCode approval"), + (make_pr(isDraft=True), "noema", "draft pull request"), ( make_pr( - reviews={ - "nodes": [ - review( - login="noema", - body=identity_body(""), - ) - ] - } + reviews={"nodes": [review(body=marker_body)]}, + reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}, ), "noema", + "unresolved review threads", ), ( make_pr( @@ -641,13 +676,7 @@ def test_inspect_and_review_skip_paths(monkeypatch): } ), "noema", - ), - ( - make_pr( - reviews={"nodes": [review(body=marker_body)]}, - reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}, - ), - "noema", + "request-changes review", ), ( make_pr( @@ -665,14 +694,17 @@ def test_inspect_and_review_skip_paths(monkeypatch): }, ), "noema", + "Blocking checks remain", ), - (clean_pr, "opencode-agent"), + (clean_pr, "opencode-agent", "independent Noema reviewer"), + (clean_pr, "", "credential identity"), ] - for pr, actor in cases: + for pr, actor, message in cases: calls.clear() monkeypatch.setattr(noema, "fetch_pr", lambda repo, number, pr=pr: pr) monkeypatch.setattr(noema, "current_actor", lambda actor=actor: actor) - assert noema.inspect_and_review("owner/repo", 7) == 0 + with pytest.raises(RuntimeError, match=message): + noema.inspect_and_review("owner/repo", 7) assert calls == [] diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 8e66277f..3d973085 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -334,6 +334,31 @@ def test_noema_review_credentials_and_llm_configuration_fail_closed() -> None: assert "Noema app token is unavailable; review skipped." not in workflow +def test_noema_dispatch_is_authorized_and_bound_to_live_pr_before_token_mint() -> None: + """Untrusted dispatch payloads must never select a token repository or PR.""" + workflow = workflow_text("noema-review.yml") + validation = workflow.index( + "- name: Bind Noema inputs to the live organization pull request" + ) + credential = workflow.index("- name: Select fail-closed Noema reviewer credential") + mint = workflow.index("- name: Mint repository-scoped Noema GitHub App token") + + assert validation < credential < mint + assert "NOEMA_REPOSITORY_DISPATCH_ACTOR" in workflow + assert "NOEMA_REPOSITORY_DISPATCH_TARGETS" in workflow + assert '"$DISPATCH_ACTOR" != "$ALLOWED_DISPATCH_ACTOR"' in workflow + assert '"$DISPATCH_SENDER" != "$ALLOWED_DISPATCH_ACTOR"' in workflow + assert 'gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}"' in workflow + assert '"$base_repository" != "$TARGET_REPOSITORY"' in workflow + assert '"$head_repository" != "$TARGET_REPOSITORY"' in workflow + assert "payload does not match the live base/head identity" in workflow + assert "repositories: ${{ steps.live_pr.outputs.repository_name }}" in workflow + assert workflow.count( + "TARGET_REPOSITORY: ${{ steps.live_pr.outputs.target_repository }}" + ) >= 3 + assert "PR_NUMBER: ${{ steps.live_pr.outputs.pr_number }}" in workflow + + def test_noema_workflow_run_without_pull_request_skips_before_token_exchange() -> None: workflow = workflow_text("noema-review.yml") @@ -381,7 +406,7 @@ def test_noema_review_mints_a_least_privilege_github_app_token() -> None: assert "client-id: ${{ vars.NOEMA_GITHUB_APP_CLIENT_ID }}" in workflow assert "private-key: ${{ secrets.NOEMA_GITHUB_APP_PRIVATE_KEY }}" in workflow assert "owner: ContextualWisdomLab" in workflow - assert "repositories: ${{ steps.noema_credential.outputs.repository }}" in workflow + assert "repositories: ${{ steps.live_pr.outputs.repository_name }}" in workflow for permission in ( "permission-actions: read", "permission-checks: read", From 9e3508f3f0e64fdc9e72e83d22e9830c79a7bbd4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 09:59:16 +0900 Subject: [PATCH 4/9] fix(review): trust isolated coverage worktree --- .github/workflows/opencode-review.yml | 1 + tests/test_opencode_agent_contract.py | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 433dee3f..42802a59 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -777,6 +777,7 @@ jobs: GIT_CONFIG_NOSYSTEM=1 \ GIT_CONFIG_GLOBAL=/dev/null \ git \ + -c safe.directory=/work \ -c core.fsmonitor=false \ -c core.hooksPath=/dev/null \ -c core.quotePath=false \ diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index d9d135e7..4a382edf 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -353,6 +353,7 @@ def test_opencode_target_coverage_materializes_only_after_authorized_dispatch(): assert "trusted_git()" in measure_step assert "GIT_CONFIG_NOSYSTEM=1" in measure_step assert "GIT_CONFIG_GLOBAL=/dev/null" in measure_step + assert "-c safe.directory=/work" in measure_step assert "-c core.fsmonitor=false" in measure_step assert "-c core.hooksPath=/dev/null" in measure_step assert "git -c core.quotePath=false ls-files" not in measure_step From 461ddf25d49c680b99a72ffc4b8aaedc90057bf3 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 10:04:28 +0900 Subject: [PATCH 5/9] feat(review): scope organization queue sweeps --- .../workflows/pr-review-merge-scheduler.yml | 22 +++++++++++++++++++ .../test_required_workflow_queue_contract.py | 18 +++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 4ab0c1b8..7affb313 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -498,6 +498,9 @@ jobs: GH_TOKEN: ${{ github.token }} DRY_RUN: ${{ github.event.client_payload.dry_run == true || inputs.dry_run == true }} ORG_SWEEP_OWNER: ContextualWisdomLab + # A repository_dispatch caller can bound a repair sweep to one exact + # organization repository instead of waking every open PR in the org. + ORG_SWEEP_TARGET_REPOSITORY: ${{ github.event.client_payload.target_repository || '' }} # Inspect the complete practical queue for every repository. The previous # default of 30 silently omitted older PRs whenever a repository had a # larger queue (BandScope had 34 during the incident that established @@ -705,6 +708,25 @@ jobs: | "\(.full_name)\t\(.default_branch)" ' <<<"$repositories_json" ) + if [ -n "$ORG_SWEEP_TARGET_REPOSITORY" ]; then + if [[ ! "$ORG_SWEEP_TARGET_REPOSITORY" =~ ^ContextualWisdomLab/[A-Za-z0-9_.-]+$ ]]; then + echo "::error::Scoped organization sweep target must be an exact ContextualWisdomLab repository name." + exit 1 + fi + mapfile -t sweep_targets < <( + jq -r --arg target "$ORG_SWEEP_TARGET_REPOSITORY" ' + .[] + | select(.archived == false and .disabled == false) + | select(.full_name == $target) + | select(.full_name != "ContextualWisdomLab/.github") + | "\(.full_name)\t\(.default_branch)" + ' <<<"$repositories_json" + ) + if [ "${#sweep_targets[@]}" -ne 1 ]; then + echo "::error::Scoped organization sweep target was not found as one active non-central repository." + exit 1 + fi + fi echo "Sweeping ${#sweep_targets[@]} repositories." failures=0 diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 3d973085..ecc0ed70 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -584,6 +584,24 @@ def test_org_queue_sweep_manual_cadence_inputs_reach_the_sweep_job() -> None: assert 'if [ "$ORG_SWEEP_UPDATE_BRANCHES" = "true" ]; then' in workflow +def test_org_queue_sweep_can_be_scoped_to_one_exact_repository() -> None: + """A targeted repair must not wake unrelated organization PR queues.""" + workflow = workflow_text("pr-review-merge-scheduler.yml") + + assert ( + "ORG_SWEEP_TARGET_REPOSITORY: " + "${{ github.event.client_payload.target_repository || '' }}" + ) in workflow + assert ( + '"$ORG_SWEEP_TARGET_REPOSITORY" =~ ' + "^ContextualWisdomLab/[A-Za-z0-9_.-]+$" + ) in workflow + assert 'jq -r --arg target "$ORG_SWEEP_TARGET_REPOSITORY"' in workflow + assert "select(.full_name == $target)" in workflow + assert 'if [ "${#sweep_targets[@]}" -ne 1 ]; then' in workflow + assert "Scoped organization sweep target was not found" in workflow + + def test_org_queue_sweep_active_run_aggregation_tolerates_error_payloads() -> None: """An inaccessible Actions page must not add a secondary jq null error.""" jq = shutil.which("jq") From 171ba464251765473deec5ac07a06dcd31ce093a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 10:53:18 +0900 Subject: [PATCH 6/9] fix(review): close Strix trust-boundary findings --- .github/workflows/noema-review.yml | 86 ++++++++++++ .github/workflows/opencode-review.yml | 6 +- scripts/ci/materialize_pr_review_source.py | 44 +++++- scripts/ci/noema_review_gate.py | 29 +++- scripts/ci/opencode_existing_approval_gate.py | 45 ++++++- scripts/ci/pr_review_fix_scheduler.py | 45 ++++++- scripts/ci/pr_review_merge_scheduler.py | 9 -- tests/test_materialize_pr_review_source.py | 53 ++++++++ tests/test_noema_review_gate.py | 126 ++++++++++++++++-- tests/test_opencode_agent_contract.py | 7 +- tests/test_opencode_existing_approval_gate.py | 43 ++++++ tests/test_pr_review_fix_scheduler.py | 51 ++++++- tests/test_pr_review_merge_scheduler.py | 10 +- .../test_required_workflow_queue_contract.py | 8 +- 14 files changed, 511 insertions(+), 51 deletions(-) diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index ce9dc4fd..85d1ad8d 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -342,6 +342,88 @@ jobs: echo "::add-mask::$app_token" echo "token=$app_token" >>"$GITHUB_OUTPUT" + - name: Materialize exact OpenCode approval evidence + if: env.PR_NUMBER != '' + id: primary_approval_evidence + env: + GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} + GH_REPOSITORY: ${{ steps.live_pr.outputs.target_repository }} + PR_BASE_REF: ${{ steps.live_pr.outputs.base_ref }} + PR_BASE_SHA: ${{ steps.live_pr.outputs.base_sha }} + PR_HEAD_REF: ${{ steps.live_pr.outputs.head_ref }} + PR_HEAD_SHA: ${{ steps.live_pr.outputs.head_sha }} + OPENCODE_SOURCE_GIT_DIR: ${{ runner.temp }}/noema-pr-objects.git + OPENCODE_SOURCE_WORKDIR: ${{ runner.temp }}/noema-pr-head + OPENCODE_SOURCE_MANIFEST: ${{ runner.temp }}/noema-pr-source-manifest.json + OPENCODE_CHANGED_FILES_FILE: ${{ runner.temp }}/opencode-changed-files.txt + OPENCODE_ARTIFACT_MANIFEST_FILE: ${{ runner.temp }}/opencode-artifact-manifest.json + run: | + set -euo pipefail + if [ -z "${GH_TOKEN:-}" ]; then + echo "::error::Noema reviewer token is unavailable for exact approval evidence." + exit 1 + fi + git check-ref-format "refs/heads/$PR_BASE_REF" + git check-ref-format "refs/heads/$PR_HEAD_REF" + gh auth setup-git + rm -rf "$OPENCODE_SOURCE_GIT_DIR" "$OPENCODE_SOURCE_WORKDIR" + rm -f "$OPENCODE_SOURCE_MANIFEST" "$OPENCODE_CHANGED_FILES_FILE" \ + "$OPENCODE_ARTIFACT_MANIFEST_FILE" + git init --bare "$OPENCODE_SOURCE_GIT_DIR" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" remote add pr-source \ + "$GITHUB_SERVER_URL/$GH_REPOSITORY.git" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch --no-tags --no-recurse-submodules \ + pr-source "+refs/heads/${PR_BASE_REF}:refs/remotes/pr-source/base" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" fetch --no-tags --no-recurse-submodules \ + pr-source "+refs/heads/${PR_HEAD_REF}:refs/remotes/pr-source/head" + [ "$(git --git-dir="$OPENCODE_SOURCE_GIT_DIR" rev-parse refs/remotes/pr-source/base)" = "$PR_BASE_SHA" ] + [ "$(git --git-dir="$OPENCODE_SOURCE_GIT_DIR" rev-parse refs/remotes/pr-source/head)" = "$PR_HEAD_SHA" ] + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" update-ref refs/heads/noema-review "$PR_HEAD_SHA" + git --git-dir="$OPENCODE_SOURCE_GIT_DIR" symbolic-ref HEAD refs/heads/noema-review + python3 scripts/ci/materialize_pr_review_source.py \ + --git-dir "$OPENCODE_SOURCE_GIT_DIR" \ + --head-sha "$PR_HEAD_SHA" \ + --output-dir "$OPENCODE_SOURCE_WORKDIR" \ + --manifest "$OPENCODE_SOURCE_MANIFEST" + merge_base="$(git -C "$OPENCODE_SOURCE_WORKDIR" merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA")" + git -C "$OPENCODE_SOURCE_WORKDIR" diff --name-only --find-renames \ + "$merge_base" "$PR_HEAD_SHA" >"$OPENCODE_CHANGED_FILES_FILE" + if [ ! -s "$OPENCODE_CHANGED_FILES_FILE" ]; then + echo "::error::Noema cannot validate an approval without a non-empty exact-head changed-file manifest." + exit 1 + fi + python3 <<'PY' + import hashlib + import json + import os + from pathlib import Path + + runner_temp = Path(os.environ["RUNNER_TEMP"]).resolve(strict=True) + changed_files = Path(os.environ["OPENCODE_CHANGED_FILES_FILE"]).resolve(strict=True) + if changed_files != runner_temp / "opencode-changed-files.txt": + raise SystemExit("changed-file evidence escaped runner temp") + changed_files.chmod(0o600) + manifest = Path(os.environ["OPENCODE_ARTIFACT_MANIFEST_FILE"]) + manifest.write_text( + json.dumps( + { + "schema": 1, + "artifacts": { + changed_files.name: hashlib.sha256( + changed_files.read_bytes() + ).hexdigest() + }, + }, + sort_keys=True, + ), + encoding="utf-8", + ) + manifest.chmod(0o600) + digest = hashlib.sha256(manifest.read_bytes()).hexdigest() + with Path(os.environ["GITHUB_OUTPUT"]).open("a", encoding="utf-8") as output: + output.write(f"manifest_sha256={digest}\n") + PY + - name: Run Noema LLM review and submit verdict if: env.PR_NUMBER != '' env: @@ -352,6 +434,10 @@ jobs: NOEMA_LLM_API_URL: ${{ vars.NOEMA_LLM_API_URL || '' }} NOEMA_LLM_MODEL: ${{ vars.NOEMA_LLM_MODEL || '' }} NOEMA_LLM_API_KEY: ${{ secrets.NOEMA_LLM_API_KEY || secrets.OPENAI_API_KEY || '' }} + OPENCODE_REQUIRE_ADVERSARIAL_VALIDATION: "true" + OPENCODE_SOURCE_WORKDIR: ${{ runner.temp }}/noema-pr-head + OPENCODE_CHANGED_FILES_FILE: ${{ runner.temp }}/opencode-changed-files.txt + OPENCODE_ARTIFACT_MANIFEST_SHA256: ${{ steps.primary_approval_evidence.outputs.manifest_sha256 }} run: | set -euo pipefail if [ -z "${PR_NUMBER:-}" ]; then diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 42802a59..a17b075c 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -542,7 +542,7 @@ jobs: chmod 0700 "$sandbox_result_dir" sandbox_status=0 - docker run --rm --init \ + docker run --rm --init --network=none \ --name "opencode-coverage-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}" \ --pids-limit 2048 \ --memory 14g \ @@ -708,7 +708,7 @@ jobs: HOME=/work/.opencode-sandbox-home \ XDG_CACHE_HOME=/work/.opencode-sandbox-cache \ CARGO_HOME=/work/.opencode-sandbox-home/.cargo \ - PATH="/work/.opencode-sandbox-home/.cargo/bin:${PATH}" \ + PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ "$@" >"$log_file" 2>&1 local rc=$? set -e @@ -754,7 +754,7 @@ jobs: HOME=/work/.opencode-sandbox-home \ XDG_CACHE_HOME=/work/.opencode-sandbox-cache \ CARGO_HOME=/work/.opencode-sandbox-home/.cargo \ - PATH="/work/.opencode-sandbox-home/.cargo/bin:${PATH}" \ + PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ "$@" >"$log_file" 2>&1 local rc=$? set -e diff --git a/scripts/ci/materialize_pr_review_source.py b/scripts/ci/materialize_pr_review_source.py index a21d6ee0..2c79bdb5 100644 --- a/scripts/ci/materialize_pr_review_source.py +++ b/scripts/ci/materialize_pr_review_source.py @@ -18,6 +18,7 @@ import re import selectors import shutil +import stat # Git is invoked through an absolute executable path, a fixed argv, and no shell. import subprocess # nosec B404 @@ -33,9 +34,11 @@ RESERVED_ROOTS = {".codegraph", ".git"} DEFAULT_MAX_FILES = 100_000 DEFAULT_MAX_BYTES = 1_073_741_824 +DEFAULT_MAX_TREE_METADATA_BYTES = 67_108_864 DEFAULT_TREE_TIMEOUT_SECONDS = 60 TREE_READ_CHUNK_BYTES = 65_536 MAX_TREE_RECORD_BYTES = 1_048_576 +TREE_ENTRY_METADATA_OVERHEAD_BYTES = 128 GIT_EXECUTABLE = shutil.which("git") if not GIT_EXECUTABLE or not Path(GIT_EXECUTABLE).is_absolute(): raise RuntimeError("an absolute Git executable path is required") @@ -58,6 +61,11 @@ def parse_args(argv: list[str]) -> argparse.Namespace: parser.add_argument("--manifest", type=Path, required=True) parser.add_argument("--max-files", type=positive_int, default=DEFAULT_MAX_FILES) parser.add_argument("--max-bytes", type=positive_int, default=DEFAULT_MAX_BYTES) + parser.add_argument( + "--max-tree-metadata-bytes", + type=positive_int, + default=DEFAULT_MAX_TREE_METADATA_BYTES, + ) parser.add_argument( "--tree-timeout-seconds", type=positive_int, @@ -99,9 +107,24 @@ def validate_git_dir(git_dir: Path, head_sha: str) -> Path: return resolved +def reject_symlink_components(path: Path, option: str) -> Path: + """Return an absolute path only when no existing component is a symlink.""" + absolute = path.absolute() + current = Path(absolute.anchor) + for part in absolute.parts[1:]: + current /= part + try: + mode = current.lstat().st_mode + except FileNotFoundError: + continue + if stat.S_ISLNK(mode): + raise ValueError(f"{option} contains a symbolic-link path component") + return absolute + + def validate_output_path(output_dir: Path, git_dir: Path) -> Path: """Require a fresh output path that cannot overlap the Git object store.""" - output = output_dir.absolute() + output = reject_symlink_components(output_dir, "--output-dir") if output.exists() or output.is_symlink(): raise ValueError("--output-dir must not already exist") output_parent = output.parent.resolve(strict=True) @@ -187,6 +210,7 @@ def parse_tree( max_files: int, max_bytes: int, timeout_seconds: int, + max_tree_metadata_bytes: int = DEFAULT_MAX_TREE_METADATA_BYTES, ) -> tuple[list[tuple[str, str, str, int, PurePosixPath]], int]: """Stream and bound validated recursive Git tree entries.""" process = open_tree_reader(git_dir, head_sha) @@ -201,6 +225,7 @@ def parse_tree( pending = bytearray() entries: list[tuple[str, str, str, int, PurePosixPath]] = [] total_bytes = 0 + total_metadata_bytes = 0 try: reached_eof = False while not reached_eof: @@ -235,6 +260,16 @@ def parse_tree( del pending[: separator + 1] if not record: continue + next_metadata_bytes = ( + total_metadata_bytes + + len(record) + + TREE_ENTRY_METADATA_OVERHEAD_BYTES + ) + if next_metadata_bytes > max_tree_metadata_bytes: + raise ValueError( + "Git tree exceeds --max-tree-metadata-bytes " + f"({next_metadata_bytes} > {max_tree_metadata_bytes})" + ) entry = parse_tree_entry(record) next_file_count = len(entries) + 1 if next_file_count > max_files: @@ -248,6 +283,7 @@ def parse_tree( ) entries.append(entry) total_bytes = next_total_bytes + total_metadata_bytes = next_metadata_bytes if pending: raise ValueError("Git tree output ended with an unterminated record") except BaseException: @@ -329,7 +365,7 @@ def materialize(args: argparse.Namespace) -> dict[str, object]: """Materialize validated inert files and return provenance metadata.""" git_dir = validate_git_dir(args.git_dir, args.head_sha) output_dir = validate_output_path(args.output_dir, git_dir) - manifest_path = args.manifest.absolute() + manifest_path = reject_symlink_components(args.manifest, "--manifest") if manifest_path.exists() or manifest_path.is_symlink(): raise ValueError("--manifest must not already exist") if manifest_path == output_dir or output_dir in manifest_path.parents: @@ -341,6 +377,7 @@ def materialize(args: argparse.Namespace) -> dict[str, object]: max_files=args.max_files, max_bytes=args.max_bytes, timeout_seconds=args.tree_timeout_seconds, + max_tree_metadata_bytes=args.max_tree_metadata_bytes, ) output_dir.mkdir(mode=0o755) @@ -411,6 +448,9 @@ def materialize(args: argparse.Namespace) -> dict[str, object]: "skipped": skipped, "special_representations": special, } + # Recheck after materialization so an ancestor swapped to a link cannot + # redirect the final provenance write. + reject_symlink_components(manifest_path, "--manifest") manifest_path.parent.mkdir(parents=True, exist_ok=True) write_inert_file( manifest_path, (json.dumps(metadata, sort_keys=True, indent=2) + "\n").encode() diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index bd9341f5..4edf7157 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -18,16 +18,16 @@ from collections.abc import Sequence from typing import Any +try: + from opencode_existing_approval_gate import review_rejection_reason +except ModuleNotFoundError: + from scripts.ci.opencode_existing_approval_gate import review_rejection_reason + PRIMARY_REVIEW_AUTHORS = { "opencode-agent[bot]", "opencode-agent", } -PRIMARY_REVIEW_MARKERS = ( - "OpenCode reviewed the current-head bounded evidence and found no blocking issues.", - "Result: APPROVE", - "opencode-review-control-v1", -) REVIEW_BODY_HEAD_SHA_RE = re.compile(r"Head SHA:\s*`([0-9a-fA-F]{40})`") REVIEW_BODY_BASE_REF_RE = re.compile(r"Base ref:\s*`([A-Za-z0-9._/-]+)`") REVIEW_BODY_BASE_SHA_RE = re.compile(r"Base SHA:\s*`([0-9a-fA-F]{40})`") @@ -229,9 +229,24 @@ def current_primary_approval(pr: dict[str, Any]) -> dict[str, Any] | None: continue if str(review.get("state") or "").upper() != "APPROVED": continue - body = str(review.get("body") or "") author = review_author(review) - if author in PRIMARY_REVIEW_AUTHORS and any(marker in body for marker in PRIMARY_REVIEW_MARKERS): + if author not in PRIMARY_REVIEW_AUTHORS: + continue + rest_review = { + "state": review.get("state"), + "commit_id": review_commit(review), + "user": {"login": author}, + "body": str(review.get("body") or ""), + } + if ( + review_rejection_reason( + rest_review, + str(pr.get("headRefOid") or ""), + str(pr.get("baseRefName") or ""), + str(pr.get("baseRefOid") or ""), + ) + is None + ): return review return None diff --git a/scripts/ci/opencode_existing_approval_gate.py b/scripts/ci/opencode_existing_approval_gate.py index 19ab4e73..5d938f46 100644 --- a/scripts/ci/opencode_existing_approval_gate.py +++ b/scripts/ci/opencode_existing_approval_gate.py @@ -36,8 +36,13 @@ ) SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") BASE_REF_RE = re.compile(r"^(?!-)[A-Za-z0-9._/-]+$") -WORKFLOW_RUN_RE = re.compile(r"(?m)^- Workflow run: [1-9][0-9]*\s*$") -WORKFLOW_ATTEMPT_RE = re.compile(r"(?m)^- Workflow attempt: [1-9][0-9]*\s*$") +WORKFLOW_RUN_RE = re.compile(r"(?m)^- Workflow run: ([1-9][0-9]*)\s*$") +WORKFLOW_ATTEMPT_RE = re.compile(r"(?m)^- Workflow attempt: ([1-9][0-9]*)\s*$") +RESULT_LINE_RE = re.compile(r"(?m)^- Result: ([A-Z_]+)\s*$") +CONTROL_BLOCK_RE = re.compile( + r"", + re.DOTALL, +) REQUIRED_PROBE_FIELDS = ( "path", "hypothesis", @@ -75,6 +80,20 @@ def extract_adversarial_evidence(body: str) -> dict[str, Any] | None: return evidence +def extract_control_payload(body: str) -> tuple[dict[str, Any] | None, str | None]: + """Return one unambiguous structured OpenCode control payload.""" + matches = list(CONTROL_BLOCK_RE.finditer(body)) + if len(matches) != 1: + return None, "review body must contain exactly one OpenCode control block" + try: + payload = json.loads(matches[0].group("payload")) + except json.JSONDecodeError: + return None, "OpenCode control block is not parseable JSON" + if not isinstance(payload, dict): + return None, "OpenCode control block must be a JSON object" + return payload, None + + def adversarial_rejection_reason(body: str) -> str | None: """Explain why structured adversarial evidence is not reusable.""" evidence = extract_adversarial_evidence(body) @@ -141,18 +160,32 @@ def review_rejection_reason( return "review body is deterministic or model-unavailable fallback evidence" if PRIMARY_APPROVAL_MARKER not in body: return "review body lacks the real-model approval marker" - if "- Result: APPROVE" not in body: - return "review body lacks an APPROVE result" + if RESULT_LINE_RE.findall(body) != ["APPROVE"]: + return "review body must contain exactly one unambiguous APPROVE result" if f"- Head SHA: `{head_sha}`" not in body: return "review body lacks the exact current-head SHA" if f"- Base ref: `{base_ref}`" not in body: return "review body lacks the exact current base ref" if f"- Base SHA: `{base_sha}`" not in body: return "review body lacks the exact current base SHA" - if not WORKFLOW_RUN_RE.search(body): + workflow_run = WORKFLOW_RUN_RE.search(body) + if not workflow_run: return "review body lacks a workflow run id" - if not WORKFLOW_ATTEMPT_RE.search(body): + workflow_attempt = WORKFLOW_ATTEMPT_RE.search(body) + if not workflow_attempt: return "review body lacks a workflow attempt" + control, control_error = extract_control_payload(body) + if control_error: + return control_error + assert control is not None + if str(control.get("result") or "").upper() != "APPROVE": + return "OpenCode control result is not APPROVE" + if str(control.get("head_sha") or "").lower() != head_sha.lower(): + return "OpenCode control head does not match current head" + if str(control.get("run_id") or "") != workflow_run.group(1): + return "OpenCode control workflow run does not match review metadata" + if str(control.get("run_attempt") or "") != workflow_attempt.group(1): + return "OpenCode control workflow attempt does not match review metadata" return adversarial_rejection_reason(body) diff --git a/scripts/ci/pr_review_fix_scheduler.py b/scripts/ci/pr_review_fix_scheduler.py index 44c2959d..35d83ecb 100755 --- a/scripts/ci/pr_review_fix_scheduler.py +++ b/scripts/ci/pr_review_fix_scheduler.py @@ -72,18 +72,37 @@ def issue_comments(repo: str, number: int) -> list[dict[str, Any]]: return [comment for page in pages for comment in page] +def current_token_actor() -> str: + """Return the exact login represented by the active mutation credential.""" + try: + actor = run_json(["api", "user"]) + except (RuntimeError, json.JSONDecodeError): + return "" + if not isinstance(actor, dict): + return "" + return str(actor.get("login") or "").strip() + + def recent_fix_marker_exists( comments: list[dict[str, Any]], head_sha: str, min_interval_seconds: int, + trusted_author: str, ) -> bool: - """Return whether this head was already dispatched recently.""" + """Return whether this credential already dispatched the head recently.""" + trusted_author = trusted_author.strip().casefold() + if not trusted_author: + return False now = int(time.time()) for comment in reversed(comments): + author = str(((comment.get("user") or {}).get("login")) or "").casefold() + if author != trusted_author: + continue match = FIX_MARKER_RE.search(str(comment.get("body") or "")) if not match or match.group(1).lower() != head_sha.lower(): continue - return now - int(match.group(2)) < min_interval_seconds + age_seconds = now - int(match.group(2)) + return 0 <= age_seconds < min_interval_seconds return False @@ -258,7 +277,12 @@ def inspect_pr( if comments is None: comments = issue_comments(repo, number) - if recent_fix_marker_exists(comments, str(pr["headRefOid"]), args.retry_hours * 3600): + if recent_fix_marker_exists( + comments, + str(pr["headRefOid"]), + args.retry_hours * 3600, + current_token_actor(), + ): return "wait", ("recent autofix marker exists for this head",) dispatch_autofix( @@ -344,9 +368,18 @@ def self_test() -> int: """Run cheap contract checks.""" head = "a" * 40 base = "b" * 40 - comments = [{"body": f"{FIX_MARKER} head_sha={head} epoch={int(time.time())} -->"}] - assert recent_fix_marker_exists(comments, head, 24 * 3600) - assert not recent_fix_marker_exists(comments, "b" * 40, 24 * 3600) + comments = [ + { + "body": f"{FIX_MARKER} head_sha={head} epoch={int(time.time())} -->", + "user": {"login": "github-actions[bot]"}, + } + ] + assert recent_fix_marker_exists( + comments, head, 24 * 3600, "github-actions[bot]" + ) + assert not recent_fix_marker_exists( + comments, "b" * 40, 24 * 3600, "github-actions[bot]" + ) pr = { "reviews": { "nodes": [ diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index df3decd9..2a3d13c3 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1419,18 +1419,9 @@ def failed_status_checks(pr: dict[str, Any]) -> list[str]: ): latest_check_runs[key] = (started_at, index, node) - successful_status_contexts = { - node.get("context") - for node in status_contexts - if (node.get("state") or "").upper() == "SUCCESS" - } for _, _, node in sorted(latest_check_runs.values(), key=lambda item: item[1]): conclusion = (node.get("conclusion") or "").upper() if conclusion in FAILED_CHECK_CONCLUSIONS: - if is_strix_context(node) and "strix" in successful_status_contexts: - continue - if is_opencode_context(node) and "opencode-review" in successful_status_contexts: - continue failed.append(node.get("name") or "check-run") for node in status_contexts: state = (node.get("state") or "").upper() diff --git a/tests/test_materialize_pr_review_source.py b/tests/test_materialize_pr_review_source.py index 512b047c..82754a31 100644 --- a/tests/test_materialize_pr_review_source.py +++ b/tests/test_materialize_pr_review_source.py @@ -152,6 +152,28 @@ def test_rejects_unsafe_tree_paths() -> None: raise AssertionError(f"unsafe path was accepted: {unsafe!r}") +def test_rejects_symlink_ancestors_for_manifest_and_output(tmp_path: Path) -> None: + """Existing parent links cannot redirect source or provenance writes.""" + outside = tmp_path / "outside" + intended = tmp_path / "intended" + outside.mkdir() + intended.mkdir() + os.symlink(outside, intended / "linked-parent") + + for option_path, option in ( + (intended / "linked-parent" / "manifest.json", "--manifest"), + (intended / "linked-parent" / "source", "--output-dir"), + ): + try: + materializer.reject_symlink_components(option_path, option) + except ValueError as exc: + assert option in str(exc) + assert "symbolic-link path component" in str(exc) + else: + raise AssertionError(f"{option} accepted a symlink ancestor") + assert not (outside / "manifest.json").exists() + + def test_tree_file_limit_stops_streaming_producer_early( monkeypatch, tmp_path: Path ) -> None: @@ -218,3 +240,34 @@ def test_tree_byte_limit_fails_before_materializing_output(tmp_path: Path) -> No else: raise AssertionError("oversized tree bytes were accepted") assert not output.exists() + + +def test_tree_metadata_budget_stops_large_paths_before_append( + monkeypatch, tmp_path: Path +) -> None: + """Aggregate path metadata is bounded independently of blob payload bytes.""" + oid = "a" * 40 + record = f"100644 blob {oid} 1\t{'x' * 256}\0".encode() + producer = f"import os; os.write(1, {record!r})" + + def open_producer(_git_dir: Path, _head_sha: str): + return subprocess.Popen( + [sys.executable, "-c", producer], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + monkeypatch.setattr(materializer, "open_tree_reader", open_producer) + try: + materializer.parse_tree( + tmp_path, + oid, + max_files=10, + max_bytes=10, + max_tree_metadata_bytes=128, + timeout_seconds=10, + ) + except ValueError as exc: + assert "--max-tree-metadata-bytes" in str(exc) + else: + raise AssertionError("oversized tree path metadata was accepted") diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 4092404f..3afe0773 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1,15 +1,67 @@ import base64 +import hashlib import json import sys import pytest from scripts.ci import noema_review_gate as noema +from scripts.ci import opencode_existing_approval_gate as approval_gate +from scripts.ci import opencode_review_normalize_output as normalizer HEAD = "a" * 40 BASE_REF = "main" BASE_SHA = "b" * 40 +SOURCE_PATH = "src/app.py" +SOURCE_LINES = (b"def one():", b" return 1") + + +@pytest.fixture(autouse=True) +def trusted_primary_approval_artifacts(tmp_path, monkeypatch): + """Provide exact-head source and changed-file evidence to the shared gate.""" + runner_temp = tmp_path / "runner-temp" + source_root = tmp_path / "source" + source_path = source_root / SOURCE_PATH + runner_temp.mkdir() + source_path.parent.mkdir(parents=True) + source_path.write_bytes(b"\n".join(SOURCE_LINES) + b"\n") + changed_files = runner_temp / "opencode-changed-files.txt" + changed_files.write_text(f"{SOURCE_PATH}\n", encoding="utf-8") + manifest = runner_temp / "opencode-artifact-manifest.json" + manifest.write_text( + json.dumps( + { + "schema": 1, + "artifacts": { + changed_files.name: hashlib.sha256( + changed_files.read_bytes() + ).hexdigest() + }, + } + ), + encoding="utf-8", + ) + monkeypatch.setenv("RUNNER_TEMP", str(runner_temp)) + monkeypatch.setenv("OPENCODE_SOURCE_WORKDIR", str(source_root)) + monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + monkeypatch.setenv("OPENCODE_REQUIRE_ADVERSARIAL_VALIDATION", "true") + monkeypatch.setenv( + "OPENCODE_ARTIFACT_MANIFEST_SHA256", + hashlib.sha256(manifest.read_bytes()).hexdigest(), + ) + cached_helpers = ( + normalizer.trusted_artifact_manifest, + normalizer.current_changed_files, + normalizer.trusted_execution_receipts, + ) + for helper in cached_helpers: + if hasattr(helper, "cache_clear"): + helper.cache_clear() + yield + for helper in cached_helpers: + if hasattr(helper, "cache_clear"): + helper.cache_clear() def identity_body(marker: str = "Result: APPROVE", *, head: str = HEAD) -> str: @@ -24,6 +76,59 @@ def identity_body(marker: str = "Result: APPROVE", *, head: str = HEAD) -> str: ) +def valid_primary_body(*, head: str = HEAD) -> str: + """Build a primary review accepted by the shared strict approval gate.""" + evidence = { + "status": "passed", + "probes": [ + { + "path": SOURCE_PATH, + "line": line, + "hypothesis": f"Approval bypass hypothesis {line}.", + "attack_or_counterexample": f"Forged review evidence variant {line}.", + "evidence": ( + f"Source trace at {SOURCE_PATH}:{line} rejected the variant. " + "source-line-sha256=" + hashlib.sha256(source_line).hexdigest() + ), + "outcome": "falsified", + } + for line, source_line in enumerate(SOURCE_LINES, start=1) + ], + "residual_risk": "Repository branch policy remains externally enforced.", + } + control = { + "head_sha": head, + "run_id": "123", + "run_attempt": "2", + "result": "APPROVE", + "reason": "No blocking findings.", + "summary": "Exact-head evidence passed adversarial validation.", + "adversarial_validation": evidence, + "findings": [], + } + return "\n".join( + ( + approval_gate.PRIMARY_APPROVAL_MARKER, + "", + "", + "", + "## Adversarial validation", + "```json", + json.dumps(evidence), + "```", + "", + "- Result: APPROVE", + f"- Base ref: `{BASE_REF}`", + f"- Base SHA: `{BASE_SHA}`", + f"- Head SHA: `{head}`", + "- Workflow run: 123", + "- Workflow attempt: 2", + ) + ) + + def fake_secret(*parts: str) -> str: return "".join(parts) @@ -50,7 +155,7 @@ def review(state="APPROVED", commit=HEAD, login="opencode-agent", body=None): """Build a minimal review node for Noema tests.""" return { "state": state, - "body": identity_body() if body is None else body, + "body": valid_primary_body(head=commit) if body is None else body, "author": {"login": login}, "commit": {"oid": commit}, } @@ -111,9 +216,7 @@ def fake_run(args, stdin=None): def test_review_state_helpers_cover_current_head_logic(): - marker_body = identity_body( - "OpenCode reviewed the current-head bounded evidence and found no blocking issues." - ) + marker_body = valid_primary_body() current = review(body=marker_body) old = review(commit="old", body=marker_body) pr = make_pr(reviews={"nodes": [old, current]}) @@ -123,6 +226,15 @@ def test_review_state_helpers_cover_current_head_logic(): assert noema.review_commit(current) == HEAD assert noema.review_commit({}) == "" assert noema.current_primary_approval(pr) == current + marker_only = review( + body=identity_body(approval_gate.PRIMARY_APPROVAL_MARKER) + ) + assert ( + noema.current_primary_approval( + make_pr(reviews={"nodes": [marker_only]}) + ) + is None + ) assert ( noema.current_primary_approval( make_pr(baseRefName="release", reviews={"nodes": [current]}) @@ -182,7 +294,7 @@ def test_review_state_helpers_reject_explicit_previous_head_evidence(): ) exact_approval = review( commit=current_head, - body=identity_body(approval_marker, head=current_head), + body=valid_primary_body(head=current_head), ) stale_change_request = review( "CHANGES_REQUESTED", @@ -624,9 +736,7 @@ def test_format_findings_and_submit_review(monkeypatch): def test_inspect_and_review_skip_paths(monkeypatch): - marker_body = identity_body( - "OpenCode reviewed the current-head bounded evidence and found no blocking issues." - ) + marker_body = valid_primary_body() clean_pr = make_pr(reviews={"nodes": [review(body=marker_body)]}) calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 4a382edf..055ba5f9 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -366,7 +366,12 @@ def test_opencode_target_coverage_materializes_only_after_authorized_dispatch(): in measure_step ) assert "CARGO_HOME=/work/.opencode-sandbox-home/.cargo" in measure_step - assert 'PATH="/work/.opencode-sandbox-home/.cargo/bin:${PATH}"' in measure_step + assert "docker run --rm --init --network=none" in measure_step + assert ( + 'PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"' + in measure_step + ) + assert 'PATH="/work/.opencode-sandbox-home/.cargo/bin:${PATH}"' not in measure_step assert "cargo llvm-cov --version" not in measure_step assert "emit_captured_log()" in measure_step assert 'append_command "$@"' in measure_step diff --git a/tests/test_opencode_existing_approval_gate.py b/tests/test_opencode_existing_approval_gate.py index e9437767..0fd7f6f0 100644 --- a/tests/test_opencode_existing_approval_gate.py +++ b/tests/test_opencode_existing_approval_gate.py @@ -94,12 +94,26 @@ def valid_body(head: str = HEAD) -> str: ], "residual_risk": "Hosted token permissions remain externally enforced.", } + control = { + "head_sha": head, + "run_id": "123", + "run_attempt": "2", + "result": "APPROVE", + "reason": "No blocking findings after adversarial validation.", + "summary": "The exact current-head evidence passed the strict review gate.", + "adversarial_validation": evidence, + "findings": [], + } return "\n".join( ( "## Pull request overview", "", gate.PRIMARY_APPROVAL_MARKER, "", + "", + "", "## Adversarial validation", "", "```json", @@ -175,6 +189,35 @@ def test_extract_adversarial_evidence_uses_last_parseable_block(): ), "APPROVE result", ), + ( + lambda value: value.update( + body=value["body"] + "\n- Result: REQUEST_CHANGES" + ), + "unambiguous APPROVE result", + ), + ( + lambda value: value.update( + body=value["body"].replace( + '"result": "APPROVE"', + '"result": "REQUEST_CHANGES"', + ) + ), + "control result", + ), + ( + lambda value: value.update( + body=value["body"].replace('"run_id": "123"', '"run_id": "999"') + ), + "control workflow run", + ), + ( + lambda value: value.update( + body=value["body"].replace( + '"run_attempt": "2"', '"run_attempt": "9"' + ) + ), + "control workflow attempt", + ), ( lambda value: value.update( body=value["body"].replace(f"- Head SHA: `{HEAD}`", "") diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index ef978cdb..c81773ff 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -28,13 +28,42 @@ def make_pr(**overrides): def test_recent_fix_marker_is_head_scoped(): - """Fix markers are scoped to the exact PR head.""" + """Only exact-head markers from the token actor and past time are trusted.""" head = "a" * 40 - comments = [{"body": f"{fix.FIX_MARKER} head_sha={head} epoch={int(time.time())} -->"}] + actor = "github-actions[bot]" + now = int(time.time()) + comments = [ + { + "body": f"{fix.FIX_MARKER} head_sha={head} epoch={now} -->", + "user": {"login": actor}, + } + ] + + assert fix.recent_fix_marker_exists(comments, head, 24 * 3600, actor) + assert not fix.recent_fix_marker_exists(comments, "b" * 40, 24 * 3600, actor) + assert not fix.recent_fix_marker_exists( + [{"body": f"{fix.FIX_MARKER} head_sha={head} epoch=oops -->"}], + head, + 24 * 3600, + actor, + ) + forged = [{**comments[0], "user": {"login": "contributor"}}] + assert not fix.recent_fix_marker_exists(forged, head, 24 * 3600, actor) + future = [ + { + **comments[0], + "body": f"{fix.FIX_MARKER} head_sha={head} epoch={now + 86_400} -->", + } + ] + assert not fix.recent_fix_marker_exists(future, head, 24 * 3600, actor) + assert not fix.recent_fix_marker_exists(comments, head, 24 * 3600, "") - assert fix.recent_fix_marker_exists(comments, head, 24 * 3600) - assert not fix.recent_fix_marker_exists(comments, "b" * 40, 24 * 3600) - assert not fix.recent_fix_marker_exists([{"body": f"{fix.FIX_MARKER} head_sha={head} epoch=oops -->"}], head, 24 * 3600) + +def test_current_token_actor_fails_closed(monkeypatch): + monkeypatch.setattr(fix, "run_json", lambda _args: {"login": "scheduler[bot]"}) + assert fix.current_token_actor() == "scheduler[bot]" + monkeypatch.setattr(fix, "run_json", lambda _args: []) + assert fix.current_token_actor() == "" def test_needs_autofix_uses_current_head_evidence(): @@ -542,7 +571,17 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch): ) monkeypatch.setattr(fix, "needs_autofix", lambda pr: (True, ("reason",))) - monkeypatch.setattr(fix, "issue_comments", lambda repo, number: [{"body": f"{fix.FIX_MARKER} head_sha={'a' * 40} epoch={int(time.time())} -->"}]) + monkeypatch.setattr(fix, "current_token_actor", lambda: "scheduler[bot]") + monkeypatch.setattr( + fix, + "issue_comments", + lambda repo, number: [ + { + "body": f"{fix.FIX_MARKER} head_sha={'a' * 40} epoch={int(time.time())} -->", + "user": {"login": "scheduler[bot]"}, + } + ], + ) assert fix.inspect_pr("owner/repo", make_pr(), args) == ("wait", ("recent autofix marker exists for this head",)) pr1 = make_pr(number=1) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index bcc26ea1..3d18a408 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1302,7 +1302,10 @@ def test_review_state_and_failed_checks(): } } ) - assert sched.failed_status_checks(manual_strix_supersedes_pr_target_failure) == ["lint"] + assert sched.failed_status_checks(manual_strix_supersedes_pr_target_failure) == [ + "strix", + "lint", + ] opencode_pr_target_failure_without_status = make_pr( statusCheckRollup={ "contexts": { @@ -1324,7 +1327,10 @@ def test_review_state_and_failed_checks(): } } ) - assert sched.failed_status_checks(manual_opencode_supersedes_pr_target_failure) == ["lint"] + assert sched.failed_status_checks(manual_opencode_supersedes_pr_target_failure) == [ + "opencode-review", + "lint", + ] def test_workflow_run_followup_defers_deterministic_fallback_retry(monkeypatch): diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index ecc0ed70..c6191a97 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -342,8 +342,10 @@ def test_noema_dispatch_is_authorized_and_bound_to_live_pr_before_token_mint() - ) credential = workflow.index("- name: Select fail-closed Noema reviewer credential") mint = workflow.index("- name: Mint repository-scoped Noema GitHub App token") + evidence = workflow.index("- name: Materialize exact OpenCode approval evidence") + review = workflow.index("- name: Run Noema LLM review and submit verdict") - assert validation < credential < mint + assert validation < credential < mint < evidence < review assert "NOEMA_REPOSITORY_DISPATCH_ACTOR" in workflow assert "NOEMA_REPOSITORY_DISPATCH_TARGETS" in workflow assert '"$DISPATCH_ACTOR" != "$ALLOWED_DISPATCH_ACTOR"' in workflow @@ -357,6 +359,10 @@ def test_noema_dispatch_is_authorized_and_bound_to_live_pr_before_token_mint() - "TARGET_REPOSITORY: ${{ steps.live_pr.outputs.target_repository }}" ) >= 3 assert "PR_NUMBER: ${{ steps.live_pr.outputs.pr_number }}" in workflow + assert "git check-ref-format" in workflow + assert "materialize_pr_review_source.py" in workflow + assert "OPENCODE_REQUIRE_ADVERSARIAL_VALIDATION: \"true\"" in workflow + assert "OPENCODE_ARTIFACT_MANIFEST_SHA256" in workflow def test_noema_workflow_run_without_pull_request_skips_before_token_exchange() -> None: From 448b42d0acfec98fc32095d85adff2f7d3332149 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 11:24:07 +0900 Subject: [PATCH 7/9] fix(security): fail closed on incomplete Strix scans --- .github/workflows/strix.yml | 42 ++----------------- scripts/ci/strix_quick_gate.sh | 6 ++- scripts/ci/test_strix_quick_gate.sh | 9 ++-- .../test_required_workflow_queue_contract.py | 26 ++++++------ 4 files changed, 28 insertions(+), 55 deletions(-) diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index efbfb361..5b80ad7f 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -752,50 +752,16 @@ jobs: export "STRIX_PROCESS_${budget_suffix}_SECONDS=$process_budget_seconds" export "STRIX_TOTAL_${budget_suffix}_SECONDS=5700" - # Capture the gate exit code plus its console output. The gate returns - # exit 1 both for genuine blocking vulnerabilities AND for - # LLM-backend-unavailable outcomes (GitHub Models "Too many requests" - # rate limits, OpenAI quota starvation, 413 tokens_limit_reached - # token-cap, connection/warm-up failures) that could not complete a scan. A backend outage is CI - # infrastructure noise, not a security finding, so it must not fail - # the required check and block merges. + # Capture the gate exit code plus its console output, then preserve it + # exactly. A provider outage is not a vulnerability, but it also is not + # current-head security evidence: a required security check must fail + # closed whenever Strix cannot produce and validate a report. strix_run_log="$RUNNER_TEMP/strix_gate_console.log" strix_rc=0 set +e bash "$TRUSTED_STRIX_GATE" 2>&1 | tee "$strix_run_log" strix_rc="${PIPESTATUS[0]}" set -e - - if [ "$strix_rc" -eq 0 ]; then - exit 0 - fi - - # Preserve configuration failures (exit 2) and any unexpected exit - # code as hard failures — only the scan-failure code (1) can be an - # infrastructure/backend-unavailability outcome. - if [ "$strix_rc" -ne 1 ]; then - exit "$strix_rc" - fi - - # Recognized signals that the LLM backend was unavailable / starved. - backend_unavailable_signal='RateLimitError|Too many requests\. For more on scraping GitHub|exceeded your current quota|insufficient_quota|billing details|"status"[[:space:]]*:[[:space:]]*"RESOURCE_EXHAUSTED"|tokens_limit_reached|Request body too large|Max size:[[:space:]]*[0-9]+[[:space:]]+tokens|Error code:[[:space:]]*413|LLM CONNECTION FAILED|Could not establish connection to the language model|LLM warm-up failed|Configured model and fallback models were unavailable|Configured Vertex model and fallback models were unavailable|emitted provider infrastructure or failure-signal output|before provider infrastructure failure' - # Any evidence that a vulnerability was actually reported. Its presence - # forces a hard failure so real findings are NEVER downgraded. Keep the - # severity branch anchored away from identifiers so environment lines - # such as STRIX_FAIL_ON_MIN_SEVERITY do not look like findings. - reported_vulnerability_signal='Vulnerabilities[[:space:]]+[1-9]|(^|[^A-Za-z0-9_])severity[[:space:]]*:' - - # Neutral skip only when ALL hold: a backend-unavailability signal is - # present and no vulnerability was reported anywhere. This preserves - # real security gating while keeping uncontrollable provider outages - # from blocking current-head merge progress. - if grep -Eiq "$backend_unavailable_signal" "$strix_run_log" \ - && ! grep -Eiq "$reported_vulnerability_signal" "$strix_run_log"; then - echo "::warning title=Strix backend unavailable::Strix could not complete because its LLM backend was unavailable (rate limit / token cap / connection or warm-up failure) before producing a vulnerability report. Treating as a neutral skip so an infrastructure outage does not block merges; genuine findings still fail the check. See the strix-reports artifact and the run log." - exit 0 - fi - - echo "Strix reported security findings or failed for a non-backend reason; failing the required check (gate exit ${strix_rc})." >&2 exit "$strix_rc" - name: Collect Strix reports for artifact upload diff --git a/scripts/ci/strix_quick_gate.sh b/scripts/ci/strix_quick_gate.sh index 8a8f8b31..4faddd4b 100755 --- a/scripts/ci/strix_quick_gate.sh +++ b/scripts/ci/strix_quick_gate.sh @@ -2274,7 +2274,11 @@ child_model_for_api_base() { if [ -n "$llm_api_base_value" ] && is_github_models_api_base "$llm_api_base_value"; then case "$model" in github_models/openai/*) - printf '%s\n' "${model#github_models/}" + # LiteLLM consumes the first openai/ segment as its provider + # selector. Preserve the second one so the GitHub Models request + # body receives the catalog ID openai/ instead of the + # invalid publisher-less value. + printf 'openai/%s\n' "${model#github_models/}" return 0 ;; github_models/*) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index cf22d408..8338cc96 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -676,7 +676,8 @@ assert_opencode_review_uses_codegraph_and_gpt5_fallback() { assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "not a generic model-exhaustion message" "opencode review tells models to return concrete missing-evidence findings instead of progress-only output" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "tokens_limit_reached" "opencode review detects provider context-window overflow" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "skipping remaining attempts for this model" "opencode review skips same-model retries after context-window overflow" - assert_file_contains "$REPO_ROOT/.github/workflows/strix.yml" "exceeded your current quota" "strix wrapper neutralizes quota-only provider failures without vulnerability reports" + assert_file_contains "$REPO_ROOT/.github/workflows/strix.yml" "a required security check must fail" "strix wrapper fails closed when provider outages prevent current-head security evidence" + assert_file_not_contains "$REPO_ROOT/.github/workflows/strix.yml" "Treating as a neutral skip" "strix wrapper must not convert provider failure into a successful required check" assert_file_contains "$REPO_ROOT/scripts/ci/strix_quick_gate.sh" "billing details" "strix quick gate classifies provider quota starvation as infrastructure" assert_file_contains "$workflow_file" 'timeout-minutes: 300' "opencode review target contains evidence, the bounded long-review pool, publication, and cleanup overhead" assert_file_contains "$workflow_file" 'timeout-minutes: 12' "opencode evidence preparation fails closed before it ties up the review queue" @@ -3264,7 +3265,7 @@ REPORT echo "openai.RateLimitError: Error code: 429" exit 1 ;; - openai/o3) + openai/openai/o3) if [ "${LLM_API_KEY:-}" != "github-models-fallback-token" ]; then echo "unexpected GitHub Models key for fallback (${LLM_API_KEY:-})" >&2 exit 16 @@ -5767,7 +5768,7 @@ run_filtered_gate_case_if_requested() { "0" \ "REGEX:Strix quick scan succeeded with fallback model 'github_models/openai/o3' in [0-9]+s\\." \ "2" \ - "openai/gpt-5.6-luna|openai/o3" \ + "openai/gpt-5.6-luna|openai/openai/o3" \ "|https://models.github.ai/inference" \ "vertex_ai" \ "" \ @@ -11645,7 +11646,7 @@ run_gate_case "openai-direct-quota-github-models-fallback-success" \ "0" \ "REGEX:Strix quick scan succeeded with fallback model 'github_models/openai/o3' in [0-9]+s\\." \ "2" \ - "openai/gpt-5.6-luna|openai/o3" \ + "openai/gpt-5.6-luna|openai/openai/o3" \ "|https://models.github.ai/inference" \ "vertex_ai" \ "" \ diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index c6191a97..72e57e00 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -934,21 +934,23 @@ def test_optional_strix_workflow_absence_is_logged_without_failing_lookup() -> N assert 'if target_workflow_available "strix.yml"; then' in failed_check_evidence -def test_strix_provider_outage_without_findings_is_neutralized() -> None: +def test_strix_provider_outage_without_report_fails_closed() -> None: workflow = workflow_text("strix.yml") - assert "RateLimitError|Too many requests" in workflow - assert "exceeded your current quota" in workflow - assert "billing details" in workflow - assert "LLM warm-up failed" in workflow - assert "zero_vulnerabilities_signal" not in workflow - assert "(^|[^A-Za-z0-9_])severity[[:space:]]*:" in workflow assert "STRIX_FAIL_ON_MIN_SEVERITY: MEDIUM" in workflow - assert "before producing a vulnerability report" in workflow - assert "genuine findings still fail the check" in workflow - assert ( - '&& ! grep -Eiq "$reported_vulnerability_signal" "$strix_run_log"' in workflow - ) + assert "a required security check must fail" in workflow + assert 'strix_rc="${PIPESTATUS[0]}"' in workflow + assert 'exit "$strix_rc"' in workflow + assert "Treating as a neutral skip" not in workflow + assert "backend_unavailable_signal=" not in workflow + + +def test_strix_github_models_openai_id_preserves_publisher() -> None: + gate = (REPO_ROOT / "scripts/ci/strix_quick_gate.sh").read_text(encoding="utf-8") + + assert "github_models/openai/*)" in gate + assert "catalog ID openai/" in gate + assert "printf 'openai/%s\\n' \"${model#github_models/}\"" in gate def test_strix_cross_repo_dispatch_uses_target_token_for_pr_scoping() -> None: From e7e4ffdaae469e29dd82ad3774ca8e6afe76d3b4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 13:05:19 +0900 Subject: [PATCH 8/9] test(review): prove offline coverage paths --- tests/test_materialize_pr_review_source.py | 466 ++++++++++++++++++ tests/test_noema_review_gate.py | 1 + tests/test_opencode_existing_approval_gate.py | 47 ++ tests/test_pr_review_fix_scheduler.py | 6 + 4 files changed, 520 insertions(+) diff --git a/tests/test_materialize_pr_review_source.py b/tests/test_materialize_pr_review_source.py index 82754a31..64b63e8d 100644 --- a/tests/test_materialize_pr_review_source.py +++ b/tests/test_materialize_pr_review_source.py @@ -2,14 +2,20 @@ from __future__ import annotations +import argparse +import io import json import os from pathlib import Path +from pathlib import PurePosixPath +import runpy import stat import subprocess import sys import time +import pytest + from scripts.ci import materialize_pr_review_source as materializer @@ -271,3 +277,463 @@ def open_producer(_git_dir: Path, _head_sha: str): assert "--max-tree-metadata-bytes" in str(exc) else: raise AssertionError("oversized tree path metadata was accepted") + + +def test_argument_git_and_path_validation_edges(monkeypatch, tmp_path: Path) -> None: + """Malformed limits, Git identities, and output paths fail before writes.""" + with pytest.raises(argparse.ArgumentTypeError, match="positive"): + materializer.positive_int("0") + with pytest.raises(SystemExit): + materializer.parse_args( + [ + "--git-dir", + str(tmp_path), + "--head-sha", + "short", + "--output-dir", + str(tmp_path / "out"), + "--manifest", + str(tmp_path / "manifest"), + ] + ) + + monkeypatch.setattr( + materializer.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + args[0], 1, stdout=b"", stderr=b"git detail" + ), + ) + with pytest.raises(RuntimeError, match="git detail"): + materializer.git_bytes(tmp_path, "rev-parse", "HEAD") + + not_directory = tmp_path / "not-directory" + not_directory.write_text("x", encoding="utf-8") + with pytest.raises(ValueError, match="resolve to a directory"): + materializer.validate_git_dir(not_directory, "a" * 40) + + responses = iter((b"true\n", b"b" * 40 + b"\n")) + monkeypatch.setattr(materializer, "git_bytes", lambda *args: next(responses)) + with pytest.raises(ValueError, match="exact requested commit"): + materializer.validate_git_dir(tmp_path, "a" * 40) + + existing = tmp_path / "existing" + existing.mkdir() + with pytest.raises(ValueError, match="must not already exist"): + materializer.validate_output_path(existing, tmp_path) + with pytest.raises(ValueError, match="must not overlap"): + materializer.validate_output_path(tmp_path / "nested", tmp_path) + + with pytest.raises(ValueError, match="non-UTF-8"): + materializer.safe_relative_path(b"bad-\xff") + + +def test_tree_entry_parser_covers_gitlink_and_malformed_records() -> None: + """Tree metadata accepts only exact blob and gitlink shapes.""" + oid = "a" * 40 + mode, kind, parsed_oid, size, path = materializer.parse_tree_entry( + f"160000 commit {oid} -\tvendor/submodule".encode() + ) + assert (mode, kind, parsed_oid, path.as_posix()) == ( + "160000", + "commit", + oid, + "vendor/submodule", + ) + assert size == len(f"Submodule commit {oid}\n".encode()) + + for record, reason in ( + (b"not-a-tree-entry", "could not parse"), + (b"100644 blob short 1\tfile", "invalid object id"), + (f"100644 tree {oid} 1\tfile".encode(), "unsupported"), + ): + with pytest.raises(ValueError, match=reason): + materializer.parse_tree_entry(record) + + +def test_process_termination_handles_finished_and_stubborn_producers() -> None: + """Producer cleanup returns early or escalates from terminate to kill.""" + + class Finished: + def poll(self): + return 0 + + materializer.terminate_process(Finished()) + + class Stubborn: + terminated = False + killed = False + waits = 0 + + def poll(self): + return None + + def terminate(self): + self.terminated = True + + def kill(self): + self.killed = True + + def wait(self, timeout=None): + self.waits += 1 + if self.waits == 1: + raise subprocess.TimeoutExpired("git", timeout) + return 0 + + process = Stubborn() + materializer.terminate_process(process) + assert process.terminated and process.killed and process.waits == 2 + + +def test_tree_reader_missing_stdout_and_timeout(monkeypatch, tmp_path: Path) -> None: + """Missing pipes and stalled enumeration terminate without materialization.""" + terminated = [] + class Missing: + stdout = None + + missing = Missing() + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: missing) + monkeypatch.setattr(materializer, "terminate_process", terminated.append) + with pytest.raises(RuntimeError, match="stdout pipe"): + materializer.parse_tree( + tmp_path, + "a" * 40, + max_files=1, + max_bytes=1, + timeout_seconds=1, + ) + assert terminated == [missing] + + process = subprocess.Popen( + [sys.executable, "-c", "import time; time.sleep(30)"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: process) + monkeypatch.setattr( + materializer, + "terminate_process", + lambda candidate: (candidate.kill(), candidate.wait()), + ) + with pytest.raises(ValueError, match="tree-timeout-seconds"): + materializer.parse_tree( + tmp_path, + "a" * 40, + max_files=1, + max_bytes=1, + timeout_seconds=0, + ) + + +def test_tree_stream_record_and_exit_edge_cases(monkeypatch, tmp_path: Path) -> None: + """EOF, bounded records, trailing bytes, wait timeout, and Git errors fail closed.""" + oid = "a" * 40 + + def producer(payload: bytes, *, linger: bool = False): + program = f"import os; os.write(1, {payload!r})" + if linger: + program += "; import time; time.sleep(30)" + return subprocess.Popen( + [sys.executable, "-c", program], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + monkeypatch.setattr(materializer, "MAX_TREE_RECORD_BYTES", 3) + for payload in (b"abcd", b"abcd\0"): + process = producer(payload, linger=True) + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args, p=process: p) + with pytest.raises(ValueError, match="record-size"): + materializer.parse_tree( + tmp_path, + oid, + max_files=10, + max_bytes=10, + timeout_seconds=10, + ) + + monkeypatch.setattr(materializer, "MAX_TREE_RECORD_BYTES", 1024) + process = producer(b"partial") + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: process) + with pytest.raises(ValueError, match="unterminated"): + materializer.parse_tree( + tmp_path, + oid, + max_files=10, + max_bytes=10, + timeout_seconds=10, + ) + + valid = f"100644 blob {oid} 1\tone".encode() + process = producer(b"\0" + valid + b"\0") + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: process) + entries, total = materializer.parse_tree( + tmp_path, + oid, + max_files=10, + max_bytes=10, + timeout_seconds=10, + ) + assert len(entries) == 1 and total == 1 + + class Selector: + def register(self, *args): + return None + + def select(self, timeout=None): + return [] + + def close(self): + return None + + class Pipe(io.BytesIO): + def fileno(self): + return 1 + + class Exited: + stdout = Pipe() + stderr = None + + def poll(self): + return 0 + + def wait(self, timeout=None): + return 0 + + monkeypatch.setattr(materializer.selectors, "DefaultSelector", Selector) + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: Exited()) + assert materializer.parse_tree( + tmp_path, + oid, + max_files=1, + max_bytes=1, + timeout_seconds=1, + ) == ([], 0) + + class ReadySelector(Selector): + def select(self, timeout=None): + return [object()] + + class WaitFailure(Exited): + stderr = io.BytesIO(b"tree failed") + + def __init__(self, result): + self.result = result + + def wait(self, timeout=None): + if self.result == "timeout": + raise subprocess.TimeoutExpired("git", timeout) + return self.result + + monkeypatch.setattr(materializer.selectors, "DefaultSelector", ReadySelector) + monkeypatch.setattr(materializer.os, "read", lambda *args: b"") + stopped = [] + monkeypatch.setattr(materializer, "terminate_process", stopped.append) + timeout_process = WaitFailure("timeout") + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: timeout_process) + with pytest.raises(RuntimeError, match="did not exit"): + materializer.parse_tree( + tmp_path, + oid, + max_files=1, + max_bytes=1, + timeout_seconds=1, + ) + assert stopped == [timeout_process] + + failed_process = WaitFailure(1) + monkeypatch.setattr(materializer, "open_tree_reader", lambda *args: failed_process) + with pytest.raises(RuntimeError, match="tree failed"): + materializer.parse_tree( + tmp_path, + oid, + max_files=1, + max_bytes=1, + timeout_seconds=1, + ) + + +def test_batch_blob_reader_rejects_pipe_header_size_and_truncation() -> None: + """The batch protocol binds every blob header, type, size, and delimiter.""" + oid = "a" * 40 + + class Batch: + def __init__(self, payload: bytes, *, stdin=True): + self.stdin = io.BytesIO() if stdin else None + self.stdout = io.BytesIO(payload) + + with pytest.raises(RuntimeError, match="pipes"): + materializer.read_blob(Batch(b"", stdin=False), oid, 1) + for payload, size, reason in ( + (b"bad\n", 1, "unexpected object header"), + (f"{oid} tree 1\n".encode(), 1, "not a blob"), + (f"{oid} blob 2\n".encode(), 1, "size changed"), + (f"{oid} blob 2\nx".encode(), 2, "truncated blob"), + ): + with pytest.raises(RuntimeError, match=reason): + materializer.read_blob(Batch(payload), oid, size) + + +def test_inert_writer_closes_descriptor_after_fdopen_failure( + monkeypatch, tmp_path: Path +) -> None: + """A wrapper failure cannot leave a writable descriptor behind.""" + real_close = os.close + + def broken_fdopen(descriptor, *args, **kwargs): + real_close(descriptor) + raise RuntimeError("fdopen failed") + + monkeypatch.setattr(materializer.os, "fdopen", broken_fdopen) + with pytest.raises(RuntimeError, match="fdopen failed"): + materializer.write_inert_file(tmp_path / "inert", b"data") + + +def test_materialize_in_process_covers_gitlink_manifest_and_cli( + monkeypatch, tmp_path: Path, capsys +) -> None: + """The in-process path writes inert blobs, gitlink markers, provenance, and CLI output.""" + work, bare, _base_sha, fixture_head = build_repository(tmp_path) + git( + work, + "update-index", + "--add", + "--cacheinfo", + f"160000,{fixture_head},vendor/submodule", + ) + git(work, "commit", "--quiet", "-m", "gitlink fixture") + head_sha = git(work, "rev-parse", "HEAD") + git(work, "push", "--quiet", str(bare), f"{head_sha}:refs/heads/master") + + def argv(output: Path, manifest: Path) -> list[str]: + return [ + "--git-dir", + str(bare), + "--head-sha", + head_sha, + "--output-dir", + str(output), + "--manifest", + str(manifest), + ] + + output = tmp_path / "in-process-source" + manifest = tmp_path / "in-process-manifest.json" + metadata = materializer.materialize(materializer.parse_args(argv(output, manifest))) + assert metadata["written_files"] == 5 + assert (output / "vendor" / "submodule").read_text(encoding="utf-8") == ( + f"Submodule commit {fixture_head}\n" + ) + assert any( + entry["representation"] == "gitlink-marker" + for entry in metadata["special_representations"] + ) + assert stat.S_IMODE(manifest.stat().st_mode) == 0o444 + + cli_output = tmp_path / "cli-source" + cli_manifest = tmp_path / "cli-manifest.json" + monkeypatch.setattr(sys, "argv", [str(SCRIPT), *argv(cli_output, cli_manifest)]) + assert materializer.main() == 0 + assert "Materialized inert PR source blobs" in capsys.readouterr().out + + +def test_materialize_preconditions_unsupported_entry_and_batch_failure( + monkeypatch, tmp_path: Path +) -> None: + """Manifest overlap, unsupported modes, and batch failures remain blocking.""" + git_dir = tmp_path / "objects.git" + git_dir.mkdir() + head = "a" * 40 + + def args(output: Path, manifest: Path): + return materializer.parse_args( + [ + "--git-dir", + str(git_dir), + "--head-sha", + head, + "--output-dir", + str(output), + "--manifest", + str(manifest), + ] + ) + + monkeypatch.setattr(materializer, "validate_git_dir", lambda *unused: git_dir) + monkeypatch.setattr(materializer, "validate_output_path", lambda output, unused: output) + + existing_manifest = tmp_path / "existing-manifest" + existing_manifest.write_text("x", encoding="utf-8") + with pytest.raises(ValueError, match="manifest must not already exist"): + materializer.materialize(args(tmp_path / "out-existing", existing_manifest)) + output = tmp_path / "out-overlap" + with pytest.raises(ValueError, match="outside --output-dir"): + materializer.materialize(args(output, output / "manifest")) + + class Batch: + def __init__(self, *, result=0, stderr=None, stdin=True): + self.stdin = io.BytesIO() if stdin else None + self.stdout = io.BytesIO() + self.stderr = io.BytesIO(stderr) if stderr is not None else None + self.result = result + + def wait(self): + return self.result + + unsupported_output = tmp_path / "unsupported-output" + monkeypatch.setattr( + materializer, + "parse_tree", + lambda *args, **kwargs: ( + [("999999", "blob", head, 0, PurePosixPath("bad"))], + 0, + ), + ) + monkeypatch.setattr(materializer, "open_batch_reader", lambda unused: Batch()) + with pytest.raises(ValueError, match="unsupported Git entry"): + materializer.materialize( + args(unsupported_output, tmp_path / "unsupported-manifest") + ) + + monkeypatch.setattr(materializer, "parse_tree", lambda *args, **kwargs: ([], 0)) + monkeypatch.setattr( + materializer, + "open_batch_reader", + lambda unused: Batch(result=1, stderr=b"batch boom", stdin=False), + ) + with pytest.raises(RuntimeError, match="batch boom"): + materializer.materialize( + args(tmp_path / "batch-output", tmp_path / "batch-manifest") + ) + + +def test_materializer_main_failure_dunder_and_missing_git_import( + monkeypatch, tmp_path: Path, capsys +) -> None: + """CLI failures are bounded, the dunder exits, and Git discovery fails closed.""" + monkeypatch.setattr( + materializer, + "materialize", + lambda args: (_ for _ in ()).throw(ValueError("bounded failure")), + ) + argv = [ + "--git-dir", + str(tmp_path / "missing.git"), + "--head-sha", + "a" * 40, + "--output-dir", + str(tmp_path / "out"), + "--manifest", + str(tmp_path / "manifest"), + ] + assert materializer.main(argv) == 1 + assert "bounded failure" in capsys.readouterr().err + + monkeypatch.setattr(sys, "argv", [str(SCRIPT), *argv]) + with pytest.raises(SystemExit) as exc: + runpy.run_path(str(SCRIPT), run_name="__main__") + assert exc.value.code == 1 + + monkeypatch.setattr(materializer.shutil, "which", lambda unused: None) + with pytest.raises(RuntimeError, match="absolute Git executable"): + runpy.run_path(str(SCRIPT), run_name="materializer_without_git") diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 3afe0773..a71550a4 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -339,6 +339,7 @@ def test_check_helpers_and_existing_noema_review(): assert noema.check_label(status_context) == "ci" assert noema.check_label(check_run) == "CI / build" + assert not noema.existing_noema_review(make_pr(), "") blockers = noema.blocking_checks( make_pr( statusCheckRollup={ diff --git a/tests/test_opencode_existing_approval_gate.py b/tests/test_opencode_existing_approval_gate.py index 0fd7f6f0..bb62d896 100644 --- a/tests/test_opencode_existing_approval_gate.py +++ b/tests/test_opencode_existing_approval_gate.py @@ -165,6 +165,41 @@ def test_extract_adversarial_evidence_uses_last_parseable_block(): assert gate.extract_adversarial_evidence("none") is None +def test_extract_control_payload_rejects_ambiguous_and_malformed_blocks(): + """Reusable approval bodies contain exactly one object-shaped control block.""" + assert gate.extract_control_payload("none")[1] == ( + "review body must contain exactly one OpenCode control block" + ) + one = "" + assert gate.extract_control_payload(f"{one}\n{one}")[1] == ( + "review body must contain exactly one OpenCode control block" + ) + assert "parseable JSON" in gate.extract_control_payload( + "" + )[1] + assert "JSON object" in gate.extract_control_payload( + "" + )[1] + + body = valid_body() + control_start = body.index("", control_start) + len("-->") + missing_control = review(body=body[:control_start] + body[control_end:]) + assert "exactly one" in gate.review_rejection_reason( + missing_control, HEAD, BASE_REF, BASE_SHA + ) + wrong_head = review( + body=valid_body().replace( + f'"head_sha": "{HEAD}"', + f'"head_sha": "{"c" * 40}"', + 1, + ) + ) + assert "control head" in gate.review_rejection_reason( + wrong_head, HEAD, BASE_REF, BASE_SHA + ) + + @pytest.mark.parametrize( ("mutate", "reason"), [ @@ -512,6 +547,18 @@ def test_parse_args_and_main(monkeypatch, capsys): ) assert "40-character" in capsys.readouterr().err + monkeypatch.setattr(sys, "stdin", io.StringIO("[]")) + assert gate.main( + ["--head", HEAD, "--base-ref", "bad ref", "--base-sha", BASE_SHA] + ) == 2 + assert "valid base ref" in capsys.readouterr().err + + monkeypatch.setattr(sys, "stdin", io.StringIO("[]")) + assert gate.main( + ["--head", HEAD, "--base-ref", BASE_REF, "--base-sha", "short"] + ) == 2 + assert "40-character base SHA" in capsys.readouterr().err + monkeypatch.setattr(sys, "stdin", io.StringIO("[]")) assert gate.main(gate_args) == 1 diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index c81773ff..adc48b15 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -64,6 +64,12 @@ def test_current_token_actor_fails_closed(monkeypatch): assert fix.current_token_actor() == "scheduler[bot]" monkeypatch.setattr(fix, "run_json", lambda _args: []) assert fix.current_token_actor() == "" + monkeypatch.setattr( + fix, + "run_json", + lambda _args: (_ for _ in ()).throw(json.JSONDecodeError("bad", "x", 0)), + ) + assert fix.current_token_actor() == "" def test_needs_autofix_uses_current_head_evidence(): From f538d6dd7c0f824a05f9ddc24893e5d490a896a1 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 17 Jul 2026 13:19:02 +0900 Subject: [PATCH 9/9] test(review): isolate scheduler actor lookup --- tests/test_pr_review_fix_scheduler.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index adc48b15..b2ae63d2 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -170,6 +170,7 @@ def test_process_queue_dispatches_same_repo_current_head(monkeypatch, capsys): pr = make_pr() calls = [] + monkeypatch.setattr(fix, "current_token_actor", lambda: "scheduler[bot]") monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr]) monkeypatch.setattr(fix, "needs_autofix", lambda pr: (True, ("current-head OpenCode requested changes",))) monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) @@ -529,6 +530,7 @@ def test_dispatch_autofix_rejects_selectable_workflow_and_invalid_repository(): def test_inspect_pr_dispatches_conflict_resolution(monkeypatch): """An approved conflicting PR dispatches autofix in resolve_conflict mode.""" captured = {} + monkeypatch.setattr(fix, "current_token_actor", lambda: "scheduler[bot]") monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) monkeypatch.setattr( fix, @@ -548,6 +550,7 @@ def test_inspect_pr_dispatches_conflict_resolution(monkeypatch): def test_process_queue_includes_conflict_resolution_candidates(monkeypatch, capsys): """The queue pre-filter fetches comments for approved conflicting PRs too.""" pr = _approved_dirty_pr() + monkeypatch.setattr(fix, "current_token_actor", lambda: "scheduler[bot]") monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr]) monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) monkeypatch.setattr(