diff --git a/eden/scm/sapling/ext/github/consts/query.py b/eden/scm/sapling/ext/github/consts/query.py index ef111962c8153..5b5a4882d1410 100644 --- a/eden/scm/sapling/ext/github/consts/query.py +++ b/eden/scm/sapling/ext/github/consts/query.py @@ -81,6 +81,22 @@ } """ +# Like GRAPHQL_UPDATE_PULL_REQUEST, but does not touch the base branch. +# GitHub rejects updatePullRequest mutations that include baseRefName for +# pull requests that are part of a native stack (the stack manages base +# branches itself), so this variant is used to update only the title/body. +GRAPHQL_UPDATE_PULL_REQUEST_NO_BASE = """ +mutation ($pullRequestId: ID!, $title: String!, $body: String!) { + updatePullRequest( + input: {pullRequestId: $pullRequestId, title: $title, body: $body} + ) { + pullRequest { + id + } + } +} +""" + GRAPHQL_CREATE_BRANCH = """ mutation ($repositoryId: ID!, $name: String!, $oid: GitObjectID!) { createRef(input: {repositoryId: $repositoryId, name: $name, oid: $oid}) { diff --git a/eden/scm/sapling/ext/github/gh_submit.py b/eden/scm/sapling/ext/github/gh_submit.py index 6004ef4959eec..12a36052b693c 100644 --- a/eden/scm/sapling/ext/github/gh_submit.py +++ b/eden/scm/sapling/ext/github/gh_submit.py @@ -12,14 +12,14 @@ import enum from dataclasses import dataclass -from typing import Dict, Optional, Tuple, Union +from typing import Dict, List, Optional, Tuple, Union from sapling.i18n import _ from sapling.result import Err, Ok, Result from . import github_gh_cli as gh_cli from .consts import query -from .github_gh_cli import JsonDict +from .github_gh_cli import JsonDict, ParamValue from .pullrequest import PullRequestId _Params = Union[str, int, bool] @@ -336,18 +336,28 @@ async def update_pull_request( node_id: str, title: str, body: str, - base: str, + base: Optional[str], ) -> Result[str, str]: """Returns an "ID!" for the pull request, which should match the node_id that was passed in. + + If base is None, the base branch is left untouched. This is required for + pull requests that are part of a native GitHub stack: GitHub rejects + updatePullRequest mutations that include baseRefName for such pull + requests, as the stack manages base branches itself. """ params: Dict[str, _Params] = { - "query": query.GRAPHQL_UPDATE_PULL_REQUEST, + "query": ( + query.GRAPHQL_UPDATE_PULL_REQUEST + if base is not None + else query.GRAPHQL_UPDATE_PULL_REQUEST_NO_BASE + ), "pullRequestId": node_id, "title": title, "body": body, - "base": base, } + if base is not None: + params["base"] = base result = await gh_cli.make_request(params, hostname=hostname) if result.is_err(): return Err(result.unwrap_err()) @@ -408,3 +418,159 @@ async def get_username(hostname: str) -> Result[str, str]: return Err(result.unwrap_err()) else: return Ok(result.unwrap()["data"]["viewer"]["login"]) + + +# Native GitHub "pull request stack" REST endpoints. The stacks API is in +# public preview and requires an explicit API version header: +# https://docs.github.com/en/rest/pulls/stacks +_STACKS_API_HEADERS = {"X-GitHub-Api-Version": "2026-03-10"} + + +@dataclass +class StackDetails: + """A native GitHub pull request stack. + + https://docs.github.com/en/rest/pulls/stacks + """ + + # Number that identifies the stack within the repo. Note that GitHub + # allocates stack numbers and pull request/issue numbers from disjoint + # ranges, so a stack number never collides with a pull request number. + number: int + # URL for the stack. + url: str + # True if the stack is still open. + is_open: bool + # Numbers of the *open* pull requests in the stack, ordered from the + # bottom of the stack (closest to the trunk) to the top. Merged and + # closed pull requests are excluded. + pull_requests: List[int] + + +def _parse_stack_from_dict(stack_obj: JsonDict) -> StackDetails: + """Parses a "Pull Request Stack" object from the REST API. + + Note that merged (and otherwise closed) pull requests are excluded from + `pull_requests`: + + >>> _parse_stack_from_dict({ + ... "id": 1, + ... "number": 7, + ... "node_id": "PRS_1", + ... "url": "https://api.github.com/repos/facebook/sapling/stacks/7", + ... "open": True, + ... "base": {"ref": "main"}, + ... "created_at": "2026-07-30T00:00:00Z", + ... "pull_requests": [ + ... {"number": 101, "state": "closed", + ... "merged_at": "2026-07-30T01:00:00Z", "draft": False, + ... "head": {"ref": "pr101", "sha": "0" * 40}}, + ... {"number": 102, "state": "open", "merged_at": None, + ... "draft": False, "head": {"ref": "pr102", "sha": "1" * 40}}, + ... {"number": 103, "state": "open", "merged_at": None, + ... "draft": True, "head": {"ref": "pr103", "sha": "2" * 40}}, + ... ], + ... }) + StackDetails(number=7, url='https://api.github.com/repos/facebook/sapling/stacks/7', is_open=True, pull_requests=[102, 103]) + """ + return StackDetails( + number=stack_obj["number"], + url=stack_obj["url"], + is_open=stack_obj["open"], + pull_requests=[ + pr["number"] for pr in stack_obj["pull_requests"] if pr["state"] == "open" + ], + ) + + +async def get_stack_for_pull_request( + hostname: str, owner: str, name: str, number: int +) -> Result[Optional[StackDetails], str]: + """Returns the stack containing the specified pull request, or None if the + pull request is not part of a stack. + """ + endpoint = f"repos/{owner}/{name}/stacks?pull_request={number}" + result = await gh_cli.make_request( + {}, hostname=hostname, endpoint=endpoint, headers=_STACKS_API_HEADERS + ) + if result.is_err(): + return Err(result.unwrap_err()) + + # The response is a JSON array of stacks. Because a pull request can be in + # at most one stack, the `pull_request` filter yields at most one entry. + stacks = result.unwrap() + if not stacks: + return Ok(None) + return Ok(_parse_stack_from_dict(stacks[0])) + + +async def create_stack( + hostname: str, owner: str, name: str, pr_numbers: List[int] +) -> Result[StackDetails, str]: + """Creates a native GitHub stack from the specified pull requests. + + `pr_numbers` must be ordered from the bottom of the stack to the top: the + bottom pull request's base must be the trunk, and each subsequent pull + request's base branch must match the head branch of the one below it. The + caller is responsible for having set up the base branches accordingly. + """ + endpoint = f"repos/{owner}/{name}/stacks" + params: Dict[str, ParamValue] = {"pull_requests": pr_numbers} + result = await gh_cli.make_request( + params, + hostname=hostname, + endpoint=endpoint, + method="POST", + headers=_STACKS_API_HEADERS, + ) + if result.is_err(): + return Err(result.unwrap_err()) + return Ok(_parse_stack_from_dict(result.unwrap())) + + +async def add_prs_to_stack( + hostname: str, owner: str, name: str, stack_number: int, pr_numbers: List[int] +) -> Result[StackDetails, str]: + """Appends pull requests onto the top of an existing stack. + + `pr_numbers` must contain only the pull requests to add, ordered from the + current top of the stack upward: the first one's base branch must match + the head branch of the stack's current top pull request. + """ + endpoint = f"repos/{owner}/{name}/stacks/{stack_number}/add" + params: Dict[str, ParamValue] = {"pull_requests": pr_numbers} + result = await gh_cli.make_request( + params, + hostname=hostname, + endpoint=endpoint, + method="POST", + headers=_STACKS_API_HEADERS, + ) + if result.is_err(): + return Err(result.unwrap_err()) + return Ok(_parse_stack_from_dict(result.unwrap())) + + +async def unstack( + hostname: str, owner: str, name: str, stack_number: int +) -> Result[Optional[StackDetails], str]: + """Removes the unmerged pull requests from a stack. + + Pull requests that cannot be unstacked (e.g., merged or queued for merge) + are left in place. Returns the updated stack if pull requests remain in + it; returns None if the stack was dissolved entirely (HTTP 204). + """ + endpoint = f"repos/{owner}/{name}/stacks/{stack_number}/unstack" + result = await gh_cli.make_request( + {}, + hostname=hostname, + endpoint=endpoint, + method="POST", + headers=_STACKS_API_HEADERS, + ) + if result.is_err(): + return Err(result.unwrap_err()) + data = result.unwrap() + if not data: + return Ok(None) + return Ok(_parse_stack_from_dict(data)) diff --git a/eden/scm/sapling/ext/github/github_gh_cli.py b/eden/scm/sapling/ext/github/github_gh_cli.py index d87248f6c8dea..a50fde86c81e6 100644 --- a/eden/scm/sapling/ext/github/github_gh_cli.py +++ b/eden/scm/sapling/ext/github/github_gh_cli.py @@ -16,32 +16,43 @@ JsonDict = Dict[str, Any] +# Scalar value that can be passed as a field to `gh api`. +_ScalarParam = Union[str, int, bool] +# `gh api` also supports array fields via repeated `key[]=value` args. +ParamValue = Union[_ScalarParam, List[_ScalarParam]] + async def make_request( - params: Dict[str, Union[str, int, bool]], + params: Dict[str, ParamValue], hostname: str, endpoint="graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """If successful, returns a Result whose value is parsed JSON returned by the request. """ - return await _make_request(params, hostname, endpoint, method) + return await _make_request(params, hostname, endpoint, method, headers) # Unexported extension/mock point. async def _make_request( - params: Dict[str, Union[str, int, bool]], + params: Dict[str, ParamValue], hostname: str, endpoint: str, method: Optional[str], + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: if method: endpoint_args = ["-X", method.upper(), endpoint] else: endpoint_args = [endpoint] + header_args = list( + itertools.chain(*[["-H", f"{k}: {v}"] for (k, v) in (headers or {}).items()]) + ) args = ( ["gh", "api", "--hostname", hostname] + + header_args + endpoint_args + list(itertools.chain(*[_format_param(k, v) for (k, v) in params.items()])) ) @@ -68,7 +79,13 @@ async def _make_request( response = None if proc.returncode == 0: - assert response is not None + if response is None: + # Some REST endpoints return "204 No Content" on success (e.g., + # dissolving a pull request stack), in which case `gh api` prints + # no JSON to parse. + if not stdout.strip(): + return Ok({}) + return Err(f"could not parse JSON from response: {stdout.decode()}") assert "errors" not in response return Ok(response) elif response is not None: @@ -82,7 +99,38 @@ async def _make_request( ) -def _format_param(key: str, value: Union[str, int, bool]) -> List[str]: +def _format_param(key: str, value: ParamValue) -> List[str]: + r"""Formats a param as a list of arguments to pass to `gh api`. + + >>> _format_param("body", "hello") + ['-f', 'body=hello'] + >>> _format_param("number", 42) + ['-F', 'number=42'] + >>> _format_param("draft", True) + ['-F', 'draft=true'] + + Array values use the `gh api` repeated-field syntax, e.g. + `-F "pull_requests[]=101" -F "pull_requests[]=102"`: + + >>> _format_param("pull_requests", [101, 102]) + ['-F', 'pull_requests[]=101', '-F', 'pull_requests[]=102'] + >>> _format_param("labels", ["bug", "help wanted"]) + ['-f', 'labels[]=bug', '-f', 'labels[]=help wanted'] + + An empty array is passed explicitly as `key[]` without a value (per the + `gh api` manual) rather than dropped: an empty array and a missing field + can have different meanings to an endpoint: + + >>> _format_param("empty", []) + ['-F', 'empty[]'] + """ + if isinstance(value, list): + if not value: + return ["-F", f"{key}[]"] + return list( + itertools.chain(*[_format_param(f"{key}[]", v) for v in value]) + ) + # In Python, bool is a subclass of int, so check it first. if isinstance(value, bool): opt = "-F" diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index added9869489b..06e31e5452888 100644 --- a/eden/scm/sapling/ext/github/mock_utils.py +++ b/eden/scm/sapling/ext/github/mock_utils.py @@ -28,8 +28,11 @@ REPO_ID = "R_test_github_repo" USER_NAME = "facebook_username" -ParamsType = Dict[str, Union[bool, int, str]] -MakeRequestType = Callable[[ParamsType, str, str, Optional[str]], Result[JsonDict, str]] +ParamsType = Dict[str, Union[bool, int, str, List[bool], List[int], List[str]]] +MakeRequestType = Callable[ + [ParamsType, str, str, Optional[str], Optional[Dict[str, str]]], + Result[JsonDict, str], +] RunGitCommandType = Callable[[List[str], str], bytes] @@ -75,6 +78,7 @@ class MockGitHubServer: def __init__(self, hostname: str = GITHUB_HOSTNAME): self.hostname: str = hostname self.requests: Dict[str, MockRequest] = {} + self._consumed_keys: set = set() async def make_request( self, @@ -83,10 +87,15 @@ async def make_request( hostname: str, endpoint: str = "graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """Wrapper function for `github_gh_cli.make_request`. It reads mock data from `self.requests` instead of sending network requests. + + Note that `headers` is intentionally not part of the request key: the + headers we send (e.g., X-GitHub-Api-Version) do not affect which mock + response should be returned. """ assert real_make_request.__name__ == "_make_request", ( f"expected '_make_request', but got '{real_make_request.__name__}'" @@ -96,11 +105,45 @@ async def make_request( if key not in self.requests: raise MockRequestNotFound(key, self.requests) + self._consumed_keys.add(key) return self.requests[key].get_response() def _add_request(self, request_key: str, request: "MockRequest") -> None: self.requests[request_key] = request + def unconsumed_requests(self) -> List[str]: + """Keys of expectations that were never requested.""" + return sorted(k for k in self.requests if k not in self._consumed_keys) + + def report_unconsumed(self, ui) -> None: + """Prints a warning for every expectation that was never requested. + + Intended to be called after the command under test has finished (see + wrap_with_consumption_check). Tests that use this produce no extra + output when all expectations were consumed, so any warning makes the + test fail: this catches code paths that silently stopped making a + request the test author expected. + """ + for key in self.unconsumed_requests(): + first_line = key.splitlines()[0] + ui.status_err(f"warning, unconsumed mock request: {first_line}\n") + + +def wrap_with_consumption_check(server: "MockGitHubServer", module, funcname) -> None: + """Wraps `module.funcname` (a command function taking `ui` as its first + argument) so that unconsumed mock expectations are reported after the + command finishes, even if it aborts. + """ + from sapling import extensions + + def wrapped(orig, ui, *args, **kwargs): + try: + return orig(ui, *args, **kwargs) + finally: + server.report_unconsumed(ui) + + extensions.wrapfunction(module, funcname, wrapped) + def expect_get_repository_request( self, owner: str = OWNER, name: str = REPO_NAME ) -> "GetRepositoryRequest": diff --git a/eden/scm/tests/test-doctest.py b/eden/scm/tests/test-doctest.py index e0f5d40b354df..d8e4ac5cb92f5 100644 --- a/eden/scm/tests/test-doctest.py +++ b/eden/scm/tests/test-doctest.py @@ -32,6 +32,8 @@ def testmod(name, optionflags=0, testtarget=None): testmod("sapling.pathlog") testmod("sapling.ext.github.archive_commit") +testmod("sapling.ext.github.gh_submit") +testmod("sapling.ext.github.github_gh_cli") testmod("sapling.ext.github.github_repo_util") testmod("sapling.ext.github.pr_parser") testmod("sapling.ext.github.pull_request_arg")