diff --git a/.github/ARCHITECTS b/.github/ARCHITECTS new file mode 100644 index 000000000000..a53b12602381 --- /dev/null +++ b/.github/ARCHITECTS @@ -0,0 +1,10 @@ +# Instructions for ARCHITECTS file format: +# This file follows the CODEOWNERS pattern style. Each non-comment line maps a +# repository path pattern to the GitHub user responsible for API architecture review. + +########### +# SDK +########### + +# Catch all +/sdk/ @kashifkhan diff --git a/.github/skills/create-api-review-pr/SKILL.md b/.github/skills/create-api-review-pr/SKILL.md index f0b999ed3c8e..817551fe8ba1 100644 --- a/.github/skills/create-api-review-pr/SKILL.md +++ b/.github/skills/create-api-review-pr/SKILL.md @@ -18,6 +18,7 @@ If the user asks to create an API review PR for a new package, explain that new 3. Python 3.10 or later must be available. 4. `azpysdk` must be installed (`pip install -e ./eng/tools/azure-sdk-tools`). 5. ApiView stub generator dependencies must be installed (`pip install -r ./eng/apiview_reqs.txt`). +6. `azsdk` CLI may be needed for package work item lookup. Do not proactively check or install it before running `create_api_review_pr.py`; the script detects supported install locations and reports when installation or update is necessary. ## Information to Gather @@ -47,6 +48,8 @@ Before running the script: 3. **Validate the target tag when applicable**: If the user provided a target version or tag, construct or validate the full tag as `_` and run `git tag -l ""`. 4. **Confirm the working tree is clean**: Run `git status --porcelain` and warn if there are uncommitted changes. +Do not proactively run `azsdk --help`, `azsdk package find-work-item --help`, or the `azure-sdk-mcp.ps1` installer as a validation step. If `create_api_review_pr.py` fails with an error saying the `azsdk` CLI is not found or the `package find-work-item` command is unavailable, then run `pwsh ./eng/common/mcp/azure-sdk-mcp.ps1` from the repository root to install or update it, and rerun the same `create_api_review_pr.py` command once. If the script still reports an `azsdk` error after that, stop and report the failure. + ## Execution This is a long-running operation. The script may take several minutes because it generates API surfaces for both the baseline and target, creates or reuses review branches, pushes branches, and then opens the draft PR. Do not treat quiet terminal periods during `apistub` generation as failure unless the command exits, prints an error, or waits for input. diff --git a/scripts/api_md_workflow/create_api_review_pr.py b/scripts/api_md_workflow/create_api_review_pr.py index e9e823682cf5..35124cb5f914 100644 --- a/scripts/api_md_workflow/create_api_review_pr.py +++ b/scripts/api_md_workflow/create_api_review_pr.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 import argparse +import fnmatch import json import os import re @@ -14,7 +15,6 @@ from urllib.parse import quote, urlencode from urllib.request import Request, urlopen - REPO_ROOT = Path(__file__).resolve().parents[2] REPO_OWNER = "Azure" REPO_NAME = "azure-sdk-for-python" @@ -24,6 +24,8 @@ SYNC_METADATA_MARKER = "api-md-review-sync" SYNC_METADATA_WARNING = "DO NOT MODIFY THESE CONTENTS!" GITHUB_API_TIMEOUT_SECONDS = 30 +AZSDK_INSTALL_SCRIPT = REPO_ROOT / "eng" / "common" / "mcp" / "azure-sdk-mcp.ps1" +ARCHITECTS_PATH = REPO_ROOT / ".github" / "ARCHITECTS" class GitHubApiError(Exception): @@ -87,7 +89,14 @@ def log_error(message: str) -> None: print(message, file=sys.stderr) -def run(args: list[str], *, cwd: Path = REPO_ROOT, check: bool = True, capture: bool = False, shell: bool = False) -> CommandResult: +def run( + args: list[str], + *, + cwd: Path = REPO_ROOT, + check: bool = True, + capture: bool = False, + shell: bool = False, +) -> CommandResult: printable = " ".join(args) log_info(f"$ {printable}") completed = subprocess.run( @@ -98,9 +107,17 @@ def run(args: list[str], *, cwd: Path = REPO_ROOT, check: bool = True, capture: text=True, shell=shell, ) - result = CommandResult(completed.returncode, completed.stdout or "", completed.stderr or "") + result = CommandResult( + completed.returncode, completed.stdout or "", completed.stderr or "" + ) if check and result.status != 0: - raise RuntimeError(f"Command failed ({result.status}): {printable}") + details = "\n".join( + part for part in [result.stderr.strip(), result.stdout.strip()] if part + ) + raise RuntimeError( + f"Command failed ({result.status}): {printable}" + + (f"\n{details}" if details else "") + ) return result @@ -108,7 +125,9 @@ def git(args: list[str], *, check: bool = True) -> CommandResult: if _git_runner: result = _git_runner(args, check) if check and result.status != 0: - raise RuntimeError(f"Command failed ({result.status}): {' '.join(['git', *args])}") + raise RuntimeError( + f"Command failed ({result.status}): {' '.join(['git', *args])}" + ) return result return run(["git", *args], check=check, capture=True) @@ -143,12 +162,19 @@ def normalize_pull_request(pr: dict[str, Any] | None) -> dict[str, Any] | None: return None owner_login = None + author_login = None if isinstance(pr.get("headRepositoryOwner"), dict): owner_login = pr["headRepositoryOwner"].get("login") elif isinstance(pr.get("head"), dict): - repo = pr["head"].get("repo") if isinstance(pr["head"].get("repo"), dict) else {} + repo = ( + pr["head"].get("repo") if isinstance(pr["head"].get("repo"), dict) else {} + ) owner = repo.get("owner") if isinstance(repo.get("owner"), dict) else {} owner_login = owner.get("login") + if isinstance(pr.get("author"), dict): + author_login = pr["author"].get("login") + elif isinstance(pr.get("user"), dict): + author_login = pr["user"].get("login") return { "number": pr.get("number"), @@ -158,6 +184,7 @@ def normalize_pull_request(pr: dict[str, Any] | None) -> dict[str, Any] | None: "body": pr.get("body"), "headRefName": pr.get("headRefName") or (pr.get("head") or {}).get("ref"), "headRepositoryOwner": {"login": owner_login}, + "authorLogin": author_login, } @@ -165,7 +192,9 @@ class GitHubApi: def __init__(self, token: str | None): self.token = token - def _request(self, method: str, url: str, payload: dict[str, Any] | None = None) -> Any: + def _request( + self, method: str, url: str, payload: dict[str, Any] | None = None + ) -> Any: data = json.dumps(payload).encode("utf-8") if payload is not None else None headers = { "Accept": "application/vnd.github+json", @@ -204,7 +233,9 @@ def list_pull_requests_by_head(self, head: str, limit: int) -> list[dict[str, An {"head": head, "state": "open", "per_page": limit}, ), ) - return [pr for pr in (normalize_pull_request(item) for item in data or []) if pr] + return [ + pr for pr in (normalize_pull_request(item) for item in data or []) if pr + ] def search_pull_requests(self, query: str, limit: int) -> list[dict[str, Any]]: graphql_query = """ @@ -219,6 +250,7 @@ def search_pull_requests(self, query: str, limit: int) -> list[dict[str, Any]]: body headRefName headRepositoryOwner { login } + author { login } } } } @@ -232,26 +264,64 @@ def search_pull_requests(self, query: str, limit: int) -> list[dict[str, Any]]: nodes = ((data or {}).get("data") or {}).get("search", {}).get("nodes", []) return [pr for pr in (normalize_pull_request(item) for item in nodes) if pr] - def list_pull_requests_by_branches(self, base: str, head: str, limit: int) -> list[dict[str, Any]]: + def list_pull_requests_by_branches( + self, base: str, head: str, limit: int + ) -> list[dict[str, Any]]: data = self._request( "GET", self._rest_url( f"/repos/{REPO_OWNER}/{REPO_NAME}/pulls", - {"base": base, "head": f"{REPO_OWNER}:{head}", "state": "open", "per_page": limit}, + { + "base": base, + "head": f"{REPO_OWNER}:{head}", + "state": "open", + "per_page": limit, + }, ), ) - return [pr for pr in (normalize_pull_request(item) for item in data or []) if pr] + return [ + pr for pr in (normalize_pull_request(item) for item in data or []) if pr + ] def update_pull_request_body(self, number: int, body: str) -> None: - self._request("PATCH", self._rest_url(f"/repos/{REPO_OWNER}/{REPO_NAME}/pulls/{number}"), {"body": body}) + self._request( + "PATCH", + self._rest_url(f"/repos/{REPO_OWNER}/{REPO_NAME}/pulls/{number}"), + {"body": body}, + ) - def create_draft_pull_request(self, base: str, head: str, title: str, body: str) -> dict[str, Any]: + def create_draft_pull_request( + self, base: str, head: str, title: str, body: str + ) -> dict[str, Any]: return self._request( "POST", self._rest_url(f"/repos/{REPO_OWNER}/{REPO_NAME}/pulls"), {"base": base, "head": head, "title": title, "body": body, "draft": True}, ) + def request_reviewers(self, pr_number: int, reviewers: list[str]) -> None: + url = self._rest_url( + f"/repos/{REPO_OWNER}/{REPO_NAME}/pulls/{pr_number}/requested_reviewers" + ) + log_info( + f"API review architect diagnostics: requesting PR reviewers at {url}: {reviewers}" + ) + response = self._request( + "POST", + url, + {"reviewers": reviewers}, + ) + requested_reviewers = [] + if isinstance(response, dict): + requested_reviewers = [ + reviewer.get("login") + for reviewer in response.get("requested_reviewers", []) + if isinstance(reviewer, dict) and reviewer.get("login") + ] + log_info( + f"API review architect diagnostics: GitHub response requested_reviewers={requested_reviewers}" + ) + def get_github_api() -> GitHubApi: global _github_api @@ -263,7 +333,9 @@ def get_github_api() -> GitHubApi: def ensure_clean_worktree() -> None: status = git_out(["status", "--porcelain"]) if status: - raise RuntimeError(f"ERROR: working tree is not clean. Commit or stash changes before running.\n{status}") + raise RuntimeError( + f"ERROR: working tree is not clean. Commit or stash changes before running.\n{status}" + ) def current_branch() -> str: @@ -271,20 +343,31 @@ def current_branch() -> str: def tag_exists(tag: str) -> bool: - return git(["rev-parse", "--verify", "--quiet", f"refs/tags/{tag}"], check=False).status == 0 + return ( + git( + ["rev-parse", "--verify", "--quiet", f"refs/tags/{tag}"], check=False + ).status + == 0 + ) def validate_base_tag(package_name: str, base_tag: str) -> str: prefix = f"{package_name}_" if not base_tag.startswith(prefix): - raise RuntimeError(f"ERROR: --base tag '{base_tag}' must start with '{prefix}'.") + raise RuntimeError( + f"ERROR: --base tag '{base_tag}' must start with '{prefix}'." + ) version = base_tag[len(prefix) :] if not version: - raise RuntimeError(f"ERROR: --base tag '{base_tag}' is missing the version suffix.") + raise RuntimeError( + f"ERROR: --base tag '{base_tag}' is missing the version suffix." + ) if not tag_exists(base_tag): - raise RuntimeError(f"ERROR: tag '{base_tag}' does not exist in this repository.") + raise RuntimeError( + f"ERROR: tag '{base_tag}' does not exist in this repository." + ) return version @@ -331,15 +414,21 @@ def resolve_target_ref(target: str, package_name: str | None = None) -> str: if branch_ref: return branch_ref - raise RuntimeError(f"ERROR: --target '{target}' is neither a branch on {REMOTE} nor a tag in this repository.") + raise RuntimeError( + f"ERROR: --target '{target}' is neither a branch on {REMOTE} nor a tag in this repository." + ) owner, branch = target.split(":", 1) if not owner or not branch: - raise RuntimeError(f"ERROR: invalid --target '{target}'. Expected 'tag', 'branch', or 'owner:branch'.") + raise RuntimeError( + f"ERROR: invalid --target '{target}'. Expected 'tag', 'branch', or 'owner:branch'." + ) branch_ref = try_fork_branch_ref(owner, branch) if not branch_ref: - raise RuntimeError(f"ERROR: branch '{branch}' does not exist in fork '{owner}'.") + raise RuntimeError( + f"ERROR: branch '{branch}' does not exist in fork '{owner}'." + ) return branch_ref @@ -364,12 +453,16 @@ def find_package_dir(package_name: str) -> Path: if not matches: raise RuntimeError(f"ERROR: package '{package_name}' not found under sdk/*/") if len(matches) > 1: - raise RuntimeError(f"ERROR: multiple matches for '{package_name}': {', '.join(str(match) for match in matches)}") + raise RuntimeError( + f"ERROR: multiple matches for '{package_name}': {', '.join(str(match) for match in matches)}" + ) return matches[0] def read_version(package_dir: Path) -> str: - version_regex = re.compile(r"^\s*VERSION\s*[:=]\s*[\"']([^\"']+)[\"']", re.MULTILINE) + version_regex = re.compile( + r"^\s*VERSION\s*[:=]\s*[\"']([^\"']+)[\"']", re.MULTILINE + ) candidates: list[Path] = [] for file_path in walk_files(package_dir): if file_path.name not in {"_version.py", "version.py"}: @@ -391,7 +484,9 @@ def read_version(package_dir: Path) -> str: raise RuntimeError(f"ERROR: could not find a version string in {package_dir}") -def generate_api_for_package(package_name: str, runtime_executable: str | None, ref_label: str | None = None) -> None: +def generate_api_for_package( + package_name: str, runtime_executable: str | None, ref_label: str | None = None +) -> None: if ref_label: log_info(f"--- Generating api.md on {ref_label} ---") @@ -415,7 +510,15 @@ def generate_api_for_package(package_name: str, runtime_executable: str | None, return run( - ["azpysdk", "apistub", "--md", "--extract-metadata", "--dest-dir", str(package_dir), package_name], + [ + "azpysdk", + "apistub", + "--md", + "--extract-metadata", + "--dest-dir", + str(package_dir), + package_name, + ], check=True, shell=sys.platform == "win32", ) @@ -461,6 +564,130 @@ def parse_simple_yaml(text: str) -> dict[str, str]: return result +def write_simple_yaml(file_path: Path, metadata: dict[str, str]) -> None: + existing_text = file_path.read_text(encoding="utf-8") if file_path.exists() else "" + line_ending = "\r\n" if "\r\n" in existing_text else "\n" + file_path.write_text( + line_ending.join(f"{key}: {metadata[key]}" for key in sorted(metadata)) + + line_ending, + encoding="utf-8", + ) + + +def package_version_major_minor(package_version: str) -> str: + match = re.match(r"^(\d+)(?:\.(\d+))?", package_version.strip()) + if not match: + raise RuntimeError( + f"ERROR: could not derive major/minor version from package version '{package_version}'." + ) + major = match.group(1) + minor = match.group(2) or "0" + return f"{major}.{minor}" + + +def parse_package_work_item_id(output: str) -> int | None: + try: + parsed = json.loads(output) + if isinstance(parsed, dict): + value = parsed.get("work_item_id") or parsed.get("workItemId") + if isinstance(value, int): + return value + if isinstance(value, str) and value.isdigit(): + return int(value) + except json.JSONDecodeError: + pass + + match = re.search(r"Work Item ID:\s*(\d+)", output) + return int(match.group(1)) if match else None + + +def find_azsdk_executable() -> str | None: + azsdk_from_path = shutil.which("azsdk") + if azsdk_from_path: + return azsdk_from_path + + for install_dir in [Path.home() / "bin", Path.home() / ".azure-sdk-mcp"]: + for executable_name in ["azsdk.exe", "azsdk"]: + candidate = install_dir / executable_name + if candidate.is_file(): + return str(candidate) + + return None + + +def combined_command_output(result: CommandResult) -> str: + return "\n".join( + part.strip() for part in [result.stdout, result.stderr] if part.strip() + ) + + +def resolve_azsdk_executable() -> str: + azsdk_executable = find_azsdk_executable() + if azsdk_executable: + return azsdk_executable + + raise RuntimeError( + "ERROR: azsdk CLI not found. Install it by running " + f"'pwsh {AZSDK_INSTALL_SCRIPT}', or run with '-UpdatePathInProfile' to add it to PATH." + ) + + +def ensure_azsdk_find_work_item_available() -> None: + azsdk_executable = resolve_azsdk_executable() + result = run( + [azsdk_executable, "package", "find-work-item", "--help"], + check=False, + capture=True, + ) + if result.status == 0: + return + + details = combined_command_output(result) + raise RuntimeError( + "ERROR: azsdk CLI is installed, but the 'package find-work-item' command is unavailable. " + f"Update it by running 'pwsh {AZSDK_INSTALL_SCRIPT}'." + + (f"\n{details}" if details else "") + ) + + +def package_work_item_id(package_name: str, package_version: str) -> int | None: + package_version_major_minor_value = package_version_major_minor(package_version) + result = run( + [ + resolve_azsdk_executable(), + "package", + "find-work-item", + "--package-name", + package_name, + "--package-version", + package_version_major_minor_value, + "--language", + "Python", + ], + check=False, + capture=True, + ) + output = combined_command_output(result) + + if result.status != 0: + log_warning( + f"WARNING: failed to resolve package work item ID for {package_name} {package_version_major_minor_value}. " + "PR body sync metadata will omit packageWorkItemId." + + (f"\n{output}" if output else "") + ) + return None + + work_item_id = parse_package_work_item_id(result.stdout) + if work_item_id is None: + log_warning( + f"WARNING: azsdk package find-work-item completed for {package_name} {package_version_major_minor_value} " + "but did not return a work item ID. PR body sync metadata will omit packageWorkItemId." + + (f"\n{output}" if output else "") + ) + return None + return work_item_id + + def metadata_sha_or_none(metadata_bytes: bytes | None) -> str | None: if not metadata_bytes: return None @@ -468,6 +695,66 @@ def metadata_sha_or_none(metadata_bytes: bytes | None) -> str | None: return metadata.get("apiMdSha256") +def codeowners_style_pattern_matches(pattern: str, path: str) -> bool: + normalized_path = path.replace("\\", "/").strip("/") + normalized_pattern = pattern.replace("\\", "/").strip() + if not normalized_pattern or normalized_pattern.startswith("!"): + return False + if normalized_pattern.startswith("/"): + normalized_pattern = normalized_pattern[1:] + if normalized_pattern.endswith("/"): + directory_pattern = normalized_pattern.rstrip("/") + return normalized_path == directory_pattern or normalized_path.startswith( + f"{directory_pattern}/" + ) + return fnmatch.fnmatchcase(normalized_path, normalized_pattern) + + +def github_user_from_owner(owner: str) -> str | None: + if not owner.startswith("@") or "/" in owner: + return None + user = owner[1:].strip() + return user or None + + +def architects_for_package( + package_dir: Path | str, architects_path: Path | None = None +) -> list[str]: + architects_path = architects_path or ARCHITECTS_PATH + package_relative = normalize_package_dir(package_dir) + log_info( + "API review architect diagnostics: " + f"package_dir={package_dir}, normalized={package_relative}, architects_path={architects_path}" + ) + if not architects_path.exists(): + log_warning( + f"API review architect diagnostics: ARCHITECTS file does not exist: {architects_path}" + ) + return [] + + matching_architects: list[str] = [] + for line in architects_path.read_text(encoding="utf-8").splitlines(): + stripped_line = line.strip() + if not stripped_line or stripped_line.startswith("#"): + continue + parts = stripped_line.split() + if len(parts) < 2: + continue + pattern = parts[0] + if codeowners_style_pattern_matches(pattern, package_relative): + matching_architects = [ + user for owner in parts[1:] if (user := github_user_from_owner(owner)) + ] + log_info( + "API review architect diagnostics: " + f"matched ARCHITECTS pattern={pattern!r}, owners={parts[1:]}, users={matching_architects}" + ) + log_info( + f"API review architect diagnostics: resolved architects={matching_architects}" + ) + return matching_architects + + def branch_remote_ref(branch: str) -> str: return f"{REMOTE}/{branch}" @@ -503,7 +790,9 @@ def read_ref_file_bytes(ref: str, relative_path: str) -> bytes | None: def desired_branch_state(result: ApiResult | None) -> BranchState: if result is None: return BranchState(False, False, None) - return BranchState(True, bool(result.metadata), metadata_sha_or_none(result.metadata)) + return BranchState( + True, bool(result.metadata), metadata_sha_or_none(result.metadata) + ) def api_results_have_api_diff(base_result: ApiResult, target_result: ApiResult) -> bool: @@ -517,7 +806,9 @@ def branch_state_matches_desired(actual: BranchState, desired: BranchState) -> b def read_branch_state(ref: str, api_relative: str, meta_relative: str) -> BranchState: metadata_bytes = read_ref_file_bytes(ref, meta_relative) api_md_bytes = read_ref_file_bytes(ref, api_relative) - return BranchState(bool(api_md_bytes), bool(metadata_bytes), metadata_sha_or_none(metadata_bytes)) + return BranchState( + bool(api_md_bytes), bool(metadata_bytes), metadata_sha_or_none(metadata_bytes) + ) def branch_suffix_from_index(index: int) -> str: @@ -530,7 +821,9 @@ def branch_suffix_from_index(index: int) -> str: return suffix -def next_available_branch_name(preferred_branch: str, existing_branches: set[str]) -> str: +def next_available_branch_name( + preferred_branch: str, existing_branches: set[str] +) -> str: if preferred_branch not in existing_branches: return preferred_branch @@ -541,7 +834,12 @@ def next_available_branch_name(preferred_branch: str, existing_branches: set[str def is_ancestor_ref(ancestor_ref: str, branch_ref: str) -> bool: - return git(["merge-base", "--is-ancestor", ancestor_ref, branch_ref], check=False).status == 0 + return ( + git( + ["merge-base", "--is-ancestor", ancestor_ref, branch_ref], check=False + ).status + == 0 + ) def resolve_branch_selection( @@ -553,27 +851,42 @@ def resolve_branch_selection( required_ancestor_ref: str | None = None, ) -> BranchSelection: existing_branches = set(list_remote_branches_with_prefix(preferred_branch)) - ordered_candidates = sorted(existing_branches, key=lambda branch: (branch != preferred_branch, branch)) + ordered_candidates = sorted( + existing_branches, key=lambda branch: (branch != preferred_branch, branch) + ) for candidate_branch in ordered_candidates: remote_ref = fetch_remote_branch(candidate_branch) actual_state = read_branch_state(remote_ref, api_relative, meta_relative) if not branch_state_matches_desired(actual_state, desired_state): continue - if required_ancestor_ref and not is_ancestor_ref(required_ancestor_ref, remote_ref): + if required_ancestor_ref and not is_ancestor_ref( + required_ancestor_ref, remote_ref + ): continue return BranchSelection(candidate_branch, True, remote_ref) - return BranchSelection(next_available_branch_name(preferred_branch, existing_branches), False, None) + return BranchSelection( + next_available_branch_name(preferred_branch, existing_branches), False, None + ) def ensure_branch_state_has_metadata_sha(branch_label: str, state: BranchState) -> None: if state.has_api_md and not state.api_md_sha256: - raise RuntimeError(f"ERROR: {branch_label} is missing apiMdSha256 in api.metadata.yml.") + raise RuntimeError( + f"ERROR: {branch_label} is missing apiMdSha256 in api.metadata.yml." + ) def select_best_pr(prs: list[dict[str, Any]]) -> dict[str, Any] | None: - candidates = [pr for pr in prs if pr.get("number") is not None and pr.get("url") and pr.get("state") and pr.get("updatedAt")] + candidates = [ + pr + for pr in prs + if pr.get("number") is not None + and pr.get("url") + and pr.get("state") + and pr.get("updatedAt") + ] if not candidates: return None open_prs = [pr for pr in candidates if str(pr.get("state", "")).lower() == "open"] @@ -597,7 +910,9 @@ def target_branch_exists(head_selector: str) -> bool: return bool(try_fork_branch_ref(parts["owner"], parts["branch"])) -def sync_working_branch_info(head_selector: str | None, package_name: str | None = None) -> dict[str, str] | None: +def sync_working_branch_info( + head_selector: str | None, package_name: str | None = None +) -> dict[str, str] | None: if not head_selector: return None if resolve_target_tag(head_selector, package_name): @@ -615,6 +930,7 @@ def build_sync_metadata_object( base_branch: str, review_branch: str, head_selector: str, + package_work_item_id_value: int | None = None, ) -> dict[str, Any] | None: working_branch = sync_working_branch_info(head_selector, package_name) if not working_branch: @@ -631,7 +947,13 @@ def build_sync_metadata_object( "workingBranch": working_branch["branch"], } working_pr = find_open_pr_for_head(head_selector) - metadata["workingPrNumber"] = working_pr.get("number") if working_pr and isinstance(working_pr.get("number"), int) else None + metadata["workingPrNumber"] = ( + working_pr.get("number") + if working_pr and isinstance(working_pr.get("number"), int) + else None + ) + if package_work_item_id_value: + metadata["packageWorkItemId"] = package_work_item_id_value return metadata @@ -649,7 +971,9 @@ def build_sync_metadata_block(metadata: dict[str, Any] | None) -> str | None: def replace_sync_metadata_block(body: str | None, metadata_block: str | None) -> str: - cleaned_body = re.sub(rf"\s*", "", str(body or "")).rstrip() + cleaned_body = re.sub( + rf"\s*", "", str(body or "") + ).rstrip() if not metadata_block: return cleaned_body return f"{cleaned_body}\n\n{metadata_block}" @@ -686,7 +1010,9 @@ def update_pr_body(pr_number: int, body: str) -> None: get_github_api().update_pull_request_body(pr_number, body) -def ensure_pr_body_sync_metadata(pr: dict[str, Any] | None, metadata_block: str | None) -> None: +def ensure_pr_body_sync_metadata( + pr: dict[str, Any] | None, metadata_block: str | None +) -> None: if not metadata_block or not pr or not isinstance(pr.get("number"), int): return desired_body = replace_sync_metadata_block(pr.get("body") or "", metadata_block) @@ -697,7 +1023,10 @@ def ensure_pr_body_sync_metadata(pr: dict[str, Any] | None, metadata_block: str log_info(f"Updated API review sync metadata on PR #{pr['number']}.") except Exception as error: # pylint: disable=broad-except details = str(error) - log_warning(f"WARNING: failed to update API review sync metadata on PR #{pr['number']}." + (f"\n {details}" if details else "")) + log_warning( + f"WARNING: failed to update API review sync metadata on PR #{pr['number']}." + + (f"\n {details}" if details else "") + ) def find_open_pr_for_head(head_selector: str) -> dict[str, Any] | None: @@ -712,7 +1041,11 @@ def find_open_pr_for_head(head_selector: str) -> dict[str, Any] | None: pass try: - all_prs.extend(github.search_pull_requests(f"repo:{REPO_SLUG} is:pr is:open head:{parts['branch']}", 50)) + all_prs.extend( + github.search_pull_requests( + f"repo:{REPO_SLUG} is:pr is:open head:{parts['branch']}", 50 + ) + ) except Exception: # pylint: disable=broad-except pass @@ -727,7 +1060,9 @@ def find_open_pr_for_head(head_selector: str) -> dict[str, Any] | None: return select_best_pr(list(deduped.values())) -def find_open_pr_for_branches(base_branch: str, head_branch: str) -> dict[str, Any] | None: +def find_open_pr_for_branches( + base_branch: str, head_branch: str +) -> dict[str, Any] | None: github = get_github_api() try: prs = github.list_pull_requests_by_branches(base_branch, head_branch, 20) @@ -737,20 +1072,94 @@ def find_open_pr_for_branches(base_branch: str, head_branch: str) -> dict[str, A pass try: - prs = github.search_pull_requests(f"repo:{REPO_SLUG} is:pr is:open head:{head_branch} base:{base_branch}", 20) + prs = github.search_pull_requests( + f"repo:{REPO_SLUG} is:pr is:open head:{head_branch} base:{base_branch}", 20 + ) return select_best_pr(prs) except Exception: # pylint: disable=broad-except return None -def create_draft_pr(base_branch: str, head_branch: str, title: str, body: str) -> dict[str, Any]: +def create_draft_pr( + base_branch: str, head_branch: str, title: str, body: str +) -> dict[str, Any]: try: - created_pr = get_github_api().create_draft_pull_request(base_branch, head_branch, title, body) - return {"ok": True, "url": created_pr.get("html_url") or created_pr.get("url") or "", "stderr": "", "stdout": ""} + created_pr = get_github_api().create_draft_pull_request( + base_branch, head_branch, title, body + ) + return { + "ok": True, + "number": created_pr.get("number"), + "url": created_pr.get("html_url") or created_pr.get("url") or "", + "authorLogin": ( + (created_pr.get("user") or {}).get("login") + if isinstance(created_pr.get("user"), dict) + else None + ), + "stderr": "", + "stdout": "", + } except GitHubApiError as error: return {"ok": False, "status": error.status, "stdout": "", "stderr": str(error)} +def single_line_output(value: Any) -> str: + return str(value or "").replace(chr(10), " ").replace(chr(13), " ").strip() + + +def assign_architects_to_pr( + pr_number: int | None, + package_dir: Path | str, + pr_author_login: str | None = None, + architects: list[str] | None = None, +) -> None: + if not pr_number: + log_warning( + "API review architect diagnostics: no PR number available; skipping reviewer request" + ) + return + architects = ( + architects_for_package(package_dir) if architects is None else architects + ) + if not architects: + log_warning( + "API review architect diagnostics: " + f"no architects resolved for package_dir={package_dir}; skipping reviewer request" + ) + return + log_info(f"API review architect diagnostics: PR author login={pr_author_login}") + reviewers = architects + if pr_author_login: + author_login = pr_author_login.lower() + self_reviewers = [ + architect for architect in architects if architect.lower() == author_login + ] + reviewers = [ + architect for architect in architects if architect.lower() != author_login + ] + if self_reviewers: + log_warning( + "WARNING: GitHub does not allow requesting the PR author as a reviewer; " + f"skipping architect reviewer(s) on PR #{pr_number}: " + f"{', '.join('@' + user for user in self_reviewers)}" + ) + if not reviewers: + log_warning( + "API review architect diagnostics: " + f"all resolved architects are the PR author; no reviewer request will be sent for PR #{pr_number}" + ) + return + try: + get_github_api().request_reviewers(pr_number, reviewers) + log_info( + f"Requested API review from architect(s) on PR #{pr_number}: {', '.join('@' + user for user in reviewers)}" + ) + except GitHubApiError as error: + log_warning( + f"WARNING: failed to request API review from architect(s) on PR #{pr_number}: {error}" + ) + + def branch_reference_markdown(head_selector: str) -> str: parts = branch_reference_parts(head_selector) branch_url = f"https://github.com/{parts['owner']}/{REPO_NAME}/tree/{quote(parts['branch'], safe='')}" @@ -765,18 +1174,32 @@ def baseline_reference_markdown(base_tag: str | None) -> str: return f"[tag `{base_tag}`]({commit_url})" -def target_reference_info(head_selector: str, package_name: str | None = None) -> dict[str, str]: +def target_reference_info( + head_selector: str, package_name: str | None = None +) -> dict[str, str]: target_tag = resolve_target_tag(head_selector, package_name) if target_tag: - return {"label": "Target tag", "markdown": baseline_reference_markdown(target_tag)} + return { + "label": "Target tag", + "markdown": baseline_reference_markdown(target_tag), + } if target_branch_exists(head_selector): pr = find_open_pr_for_head(head_selector) if pr: - return {"label": "Working PR", "markdown": f"[PR #{pr['number']}]({pr['url']})"} - return {"label": "Working branch", "markdown": branch_reference_markdown(head_selector)} + return { + "label": "Working PR", + "markdown": f"[PR #{pr['number']}]({pr['url']})", + } + return { + "label": "Working branch", + "markdown": branch_reference_markdown(head_selector), + } - return {"label": "Working branch", "markdown": branch_reference_markdown(head_selector)} + return { + "label": "Working branch", + "markdown": branch_reference_markdown(head_selector), + } def write_bytes(file_path: Path, contents: bytes) -> None: @@ -804,7 +1227,11 @@ def generate_api_bytes_for_ref( if not output_path.exists(): raise RuntimeError(f"ERROR: did not produce {output_path}") - metadata = metadata_path(package_dir).read_bytes() if metadata_path(package_dir).exists() else None + metadata = ( + metadata_path(package_dir).read_bytes() + if metadata_path(package_dir).exists() + else None + ) return ApiResult(output_path.read_bytes(), metadata, version) finally: git(["reset", "--", package_relative], check=False) @@ -813,18 +1240,28 @@ def generate_api_bytes_for_ref( def parse_args(argv: list[str]) -> argparse.Namespace: - parser = argparse.ArgumentParser(description="Create an API review PR for a Python package api.md diff.") + parser = argparse.ArgumentParser( + description="Create an API review PR for a Python package api.md diff." + ) parser.add_argument("--package-name", required=True) parser.add_argument("--base", required=True) parser.add_argument("--target") - parser.add_argument("--python", "--runtime", dest="runtime_executable", default=os.environ.get("RUNTIME_EXECUTABLE")) + parser.add_argument( + "--python", + "--runtime", + dest="runtime_executable", + default=os.environ.get("RUNTIME_EXECUTABLE"), + ) return parser.parse_args(argv) def main(argv: list[str] | None = None) -> int: args = parse_args(argv or sys.argv[1:]) + ensure_azsdk_find_work_item_available() + package_dir = find_package_dir(args.package_name) log_info(f"Found package at: {package_dir}") + architect_reviewers = architects_for_package(package_dir) ensure_clean_worktree() original_branch = current_branch() @@ -833,7 +1270,9 @@ def main(argv: list[str] | None = None) -> int: git(["fetch", REMOTE, "main"]) base_version = validate_base_tag(args.package_name, args.base) - target_ref = resolve_target_ref(args.target, args.package_name) if args.target else MAIN_REF + target_ref = ( + resolve_target_ref(args.target, args.package_name) if args.target else MAIN_REF + ) try: log_info(f"\n=== Capturing baseline api.md from tag {args.base} ===") @@ -874,7 +1313,9 @@ def main(argv: list[str] | None = None) -> int: ensure_branch_state_has_metadata_sha("target API result", desired_review_state) base_selection = resolve_branch_selection( - preferred_branch=api_review_branch_name("base", args.package_name, base_version), + preferred_branch=api_review_branch_name( + "base", args.package_name, base_version + ), desired_state=desired_base_state, api_relative=api_relative, meta_relative=meta_relative, @@ -892,11 +1333,19 @@ def main(argv: list[str] | None = None) -> int: if base_result.metadata: write_bytes(meta_file_path, base_result.metadata) git(["add", meta_relative]) - git(["commit", "-m", f"[API Review] Baseline api.md for {args.package_name} {base_version}"]) + git( + [ + "commit", + "-m", + f"[API Review] Baseline api.md for {args.package_name} {base_version}", + ] + ) git(["push", "--force-with-lease", REMOTE, base_branch]) review_selection = resolve_branch_selection( - preferred_branch=api_review_branch_name("review", args.package_name, target_version), + preferred_branch=api_review_branch_name( + "review", args.package_name, target_version + ), desired_state=desired_review_state, api_relative=api_relative, meta_relative=meta_relative, @@ -915,19 +1364,33 @@ def main(argv: list[str] | None = None) -> int: if target_result.metadata: write_bytes(meta_file_path, target_result.metadata) git(["add", meta_relative]) - git(["commit", "-m", f"[API Review] api.md for {args.package_name} {target_version}"]) + git( + [ + "commit", + "-m", + f"[API Review] api.md for {args.package_name} {target_version}", + ] + ) git(["push", "--force-with-lease", REMOTE, review_branch]) - title = f"[API Review] {args.package_name} {target_version} (base {base_version})" + title = ( + f"[API Review] {args.package_name} {target_version} (base {base_version})" + ) working_selector = args.target or "main" working_reference = target_reference_info(working_selector, args.package_name) baseline_ref = baseline_reference_markdown(args.base) + package_work_item_id_value = None + if sync_working_branch_info(working_selector, args.package_name): + package_work_item_id_value = package_work_item_id( + args.package_name, target_version + ) sync_metadata = build_sync_metadata_object( package_name=args.package_name, package_dir=package_dir, base_branch=base_branch, review_branch=review_branch, head_selector=working_selector, + package_work_item_id_value=package_work_item_id_value, ) sync_metadata_block = build_sync_metadata_block(sync_metadata) body = build_review_pr_body( @@ -943,6 +1406,12 @@ def main(argv: list[str] | None = None) -> int: existing_pr = find_open_pr_for_branches(base_branch, review_branch) if existing_pr: ensure_pr_body_sync_metadata(existing_pr, sync_metadata_block) + assign_architects_to_pr( + existing_pr.get("number"), + package_dir, + existing_pr.get("authorLogin"), + architect_reviewers, + ) log_info(f"\n=== Reusing existing PR #{existing_pr['number']} ===") log_info(existing_pr["url"]) return 0 @@ -951,12 +1420,24 @@ def main(argv: list[str] | None = None) -> int: compare_url = f"https://github.com/{REPO_SLUG}/compare/{base_branch}...{review_branch}?expand=1" pr_create = create_draft_pr(base_branch, review_branch, title, body) if pr_create["ok"]: + assign_architects_to_pr( + pr_create.get("number"), + package_dir, + pr_create.get("authorLogin"), + architect_reviewers, + ) if pr_create.get("url"): log_info(pr_create["url"]) else: existing_pr = find_open_pr_for_branches(base_branch, review_branch) if existing_pr: ensure_pr_body_sync_metadata(existing_pr, sync_metadata_block) + assign_architects_to_pr( + existing_pr.get("number"), + package_dir, + existing_pr.get("authorLogin"), + architect_reviewers, + ) log_info(f"\n=== Reusing existing PR #{existing_pr['number']} ===") log_info(existing_pr["url"]) return 0 @@ -965,21 +1446,26 @@ def main(argv: list[str] | None = None) -> int: item for item in [ f"Exit code: {pr_create.get('status')}", - f"stderr: {str(pr_create.get('stderr') or '').replace(chr(10), ' ').replace(chr(13), ' ').strip()}" - if pr_create.get("stderr") - else "", - f"stdout: {str(pr_create.get('stdout') or '').replace(chr(10), ' ').replace(chr(13), ' ').strip()}" - if pr_create.get("stdout") - else "", - f"Debug repro: use the GitHub REST API endpoint POST /repos/{REPO_SLUG}/pulls with base/head/title/body/draft=true.", + ( + f"stderr: {single_line_output(pr_create.get('stderr'))}" + if pr_create.get("stderr") + else "" + ), + ( + f"stdout: {single_line_output(pr_create.get('stdout'))}" + if pr_create.get("stdout") + else "" + ), + "Debug repro: use the GitHub REST API endpoint POST " + f"/repos/{REPO_SLUG}/pulls with base/head/title/body/draft=true.", ] if item ) log_warning( - "\nWARNING: GitHub PR creation failed. Both branches were pushed successfully -- open the PR manually here:\n" + "\nWARNING: GitHub PR creation failed. Both branches were pushed successfully -- " + "open the PR manually here:\n" f" {compare_url}\n" - f" Title: {title}" - + (f"\n {error_details}" if error_details else "") + f" Title: {title}" + (f"\n {error_details}" if error_details else "") ) return 0 finally: diff --git a/scripts/api_md_workflow/create_api_review_pr_test.py b/scripts/api_md_workflow/create_api_review_pr_test.py index 89dd5211d2b4..9554a10b7f58 100644 --- a/scripts/api_md_workflow/create_api_review_pr_test.py +++ b/scripts/api_md_workflow/create_api_review_pr_test.py @@ -1,5 +1,7 @@ import json +import tempfile import unittest +from pathlib import Path from unittest.mock import MagicMock, patch from scripts.api_md_workflow import create_api_review_pr as workflow @@ -13,7 +15,11 @@ def stub_git_branches(branches): branch_set = set(branches) def runner(args, check): - if args[0] == "fetch" and len(args) > 2 and args[2].split(":", 1)[0] in branch_set: + if ( + args[0] == "fetch" + and len(args) > 2 + and args[2].split(":", 1)[0] in branch_set + ): return command_result() return command_result(status=1) @@ -25,6 +31,7 @@ def __init__(self, head_results=None, search_results=None, on_lookup=None): self.head_results = head_results or [] self.search_results = search_results or [] self.on_lookup = on_lookup + self.reviewers = [] def _lookup(self, results): if self.on_lookup: @@ -44,7 +51,13 @@ def update_pull_request_body(self, _number, _body): return None def create_draft_pull_request(self, _base, _head, _title, _body): - return {"html_url": "https://github.com/Azure/azure-sdk-for-python/pull/1"} + return { + "number": 1, + "html_url": "https://github.com/Azure/azure-sdk-for-python/pull/1", + } + + def request_reviewers(self, pr_number, reviewers): + self.reviewers.append((pr_number, reviewers)) class ApiReviewPrTests(unittest.TestCase): @@ -58,26 +71,37 @@ def test_github_api_request_uses_timeout(self): response_context = MagicMock() response_context.__enter__.return_value = response - with patch.object(workflow, "urlopen", return_value=response_context) as urlopen: - self.assertEqual(workflow.GitHubApi(None)._request("GET", "https://example.test"), {"ok": True}) + with patch.object( + workflow, "urlopen", return_value=response_context + ) as urlopen: + self.assertEqual( + workflow.GitHubApi(None)._request("GET", "https://example.test"), + {"ok": True}, + ) urlopen.assert_called_once() - self.assertEqual(urlopen.call_args.kwargs["timeout"], workflow.GITHUB_API_TIMEOUT_SECONDS) + self.assertEqual( + urlopen.call_args.kwargs["timeout"], workflow.GITHUB_API_TIMEOUT_SECONDS + ) def test_target_reference_info_links_matching_open_pr_from_direct_head_query(self): - workflow.set_command_runner_for_test(stub_git_branches(["users/example/direct-feature"])) - workflow.set_github_api_for_test(StubGithubApi( - head_results=[ - { - "number": 45678, - "url": "https://github.com/Azure/azure-sdk-for-python/pull/45678", - "state": "OPEN", - "updatedAt": "2026-06-05T00:00:00Z", - "headRefName": "users/example/direct-feature", - "headRepositoryOwner": {"login": "example"}, - } - ] - )) + workflow.set_command_runner_for_test( + stub_git_branches(["users/example/direct-feature"]) + ) + workflow.set_github_api_for_test( + StubGithubApi( + head_results=[ + { + "number": 45678, + "url": "https://github.com/Azure/azure-sdk-for-python/pull/45678", + "state": "OPEN", + "updatedAt": "2026-06-05T00:00:00Z", + "headRefName": "users/example/direct-feature", + "headRepositoryOwner": {"login": "example"}, + } + ] + ) + ) self.assertEqual( workflow.target_reference_info("example:users/example/direct-feature"), @@ -89,18 +113,20 @@ def test_target_reference_info_links_matching_open_pr_from_direct_head_query(sel def test_target_reference_info_keeps_origin_main_as_branch(self): workflow.set_command_runner_for_test(stub_git_branches(["main"])) - workflow.set_github_api_for_test(StubGithubApi( - search_results=[ - { - "number": 23456, - "url": "https://github.com/Azure/azure-sdk-for-python/pull/23456", - "state": "OPEN", - "updatedAt": "2026-06-05T00:00:00Z", - "headRefName": "main", - "headRepositoryOwner": {"login": "example"}, - } - ] - )) + workflow.set_github_api_for_test( + StubGithubApi( + search_results=[ + { + "number": 23456, + "url": "https://github.com/Azure/azure-sdk-for-python/pull/23456", + "state": "OPEN", + "updatedAt": "2026-06-05T00:00:00Z", + "headRefName": "main", + "headRepositoryOwner": {"login": "example"}, + } + ] + ) + ) self.assertEqual( workflow.target_reference_info("origin/main"), @@ -131,7 +157,8 @@ def on_lookup(): workflow.target_reference_info("azure-example_1.2.3"), { "label": "Target tag", - "markdown": "[tag `azure-example_1.2.3`](https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", + "markdown": "[tag `azure-example_1.2.3`]" + "(https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", }, ) self.assertEqual(pr_lookup_count, 0) @@ -140,11 +167,20 @@ def test_explicit_package_tag_target_wins_over_same_named_remote_branch(self): pr_lookup_count = 0 def runner(args, check): - if args == ["rev-parse", "--verify", "--quiet", "refs/tags/azure-example_1.2.3"]: + if args == [ + "rev-parse", + "--verify", + "--quiet", + "refs/tags/azure-example_1.2.3", + ]: return command_result() if args == ["rev-list", "-n", "1", "azure-example_1.2.3"]: return command_result("abc123def456\n") - if args == ["fetch", "origin", "azure-example_1.2.3:refs/remotes/origin/azure-example_1.2.3"]: + if args == [ + "fetch", + "origin", + "azure-example_1.2.3:refs/remotes/origin/azure-example_1.2.3", + ]: return command_result() return command_result(status=1) @@ -155,31 +191,41 @@ def on_lookup(): workflow.set_command_runner_for_test(runner) workflow.set_github_api_for_test(StubGithubApi(on_lookup=on_lookup)) - self.assertEqual(workflow.resolve_target_ref("azure-example_1.2.3", "azure-example"), "azure-example_1.2.3") - self.assertIsNone(workflow.sync_working_branch_info("azure-example_1.2.3", "azure-example")) + self.assertEqual( + workflow.resolve_target_ref("azure-example_1.2.3", "azure-example"), + "azure-example_1.2.3", + ) + self.assertIsNone( + workflow.sync_working_branch_info("azure-example_1.2.3", "azure-example") + ) self.assertEqual( workflow.target_reference_info("azure-example_1.2.3", "azure-example"), { "label": "Target tag", - "markdown": "[tag `azure-example_1.2.3`](https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", + "markdown": "[tag `azure-example_1.2.3`]" + "(https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", }, ) self.assertEqual(pr_lookup_count, 0) def test_build_sync_metadata_object_records_fork_owner_and_branch(self): - workflow.set_command_runner_for_test(stub_git_branches(["users/example/feature"])) - workflow.set_github_api_for_test(StubGithubApi( - search_results=[ - { - "number": 47204, - "url": "https://github.com/Azure/azure-sdk-for-python/pull/47204", - "state": "OPEN", - "updatedAt": "2026-06-05T00:00:00Z", - "headRefName": "users/example/feature", - "headRepositoryOwner": {"login": "example"}, - } - ] - )) + workflow.set_command_runner_for_test( + stub_git_branches(["users/example/feature"]) + ) + workflow.set_github_api_for_test( + StubGithubApi( + search_results=[ + { + "number": 47204, + "url": "https://github.com/Azure/azure-sdk-for-python/pull/47204", + "state": "OPEN", + "updatedAt": "2026-06-05T00:00:00Z", + "headRefName": "users/example/feature", + "headRepositoryOwner": {"login": "example"}, + } + ] + ) + ) metadata = workflow.build_sync_metadata_object( package_name="azure-example", @@ -187,11 +233,13 @@ def test_build_sync_metadata_object_records_fork_owner_and_branch(self): base_branch="apireview/base_azure-example_1.0.0", review_branch="apireview/review_azure-example_1.1.0", head_selector="example:users/example/feature", + package_work_item_id_value=31370, ) self.assertEqual(metadata["workingOwner"], "example") self.assertEqual(metadata["workingBranch"], "users/example/feature") self.assertEqual(metadata["workingPrNumber"], 47204) + self.assertEqual(metadata["packageWorkItemId"], 31370) def test_build_sync_metadata_object_omits_metadata_for_tag_targets(self): pr_lookup_count = 0 @@ -240,7 +288,9 @@ def test_build_review_pr_body_calls_out_static_tag_to_tag_reviews(self): self.assertNotIn("Update behavior", body) self.assertNotIn("api-md-review-sync", body) - def test_build_review_pr_body_includes_sync_metadata_for_working_branch_reviews(self): + def test_build_review_pr_body_includes_sync_metadata_for_working_branch_reviews( + self, + ): metadata_block = workflow.build_sync_metadata_block( { "schemaVersion": 1, @@ -252,6 +302,7 @@ def test_build_review_pr_body_includes_sync_metadata_for_working_branch_reviews( "workingOwner": "Azure", "workingBranch": "main", "workingPrNumber": None, + "packageWorkItemId": "31370", } ) @@ -271,15 +322,283 @@ def test_build_review_pr_body_includes_sync_metadata_for_working_branch_reviews( self.assertNotIn("Static tag-to-tag review", body) self.assertIn("", "") - self.assertEqual(json.loads(json_text), {"schemaVersion": 1, "workingPrNumber": None}) + block = workflow.build_sync_metadata_block( + {"schemaVersion": 1, "workingPrNumber": None} + ) + json_text = ( + block.replace("", "") + ) + self.assertEqual( + json.loads(json_text), {"schemaVersion": 1, "workingPrNumber": None} + ) if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main()