Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 44 additions & 8 deletions src/dependamerge/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ class _MergeContext:
netrc_optional: bool
github2gerrit_mode: str
include_human_prs: bool = False
rebase_local: bool = True

# Derived / mutable state
github_client: GitHubClient | None = None
Expand Down Expand Up @@ -728,8 +729,11 @@ def _run_parallel_merge(
concurrency: Maximum number of concurrent merge workers.
For org-wide merges (PRs spread across repos) the default
of 10 is fine. For repo-scoped merges (all PRs in the
same repo) use 1 to serialise operations and give GitHub
time to propagate approvals between merges.
same repo) ``AsyncMergeManager`` serialises the actual
``merge_pull_request`` API call via a per-repo dispatch
lock, so a value > 1 is safe and lets the worker pool
keep processing other PRs while one PR sits in Step 5.5's
auto-merge wait loop — see ``_get_merge_dispatch_lock``.
"""

async def _do_merge():
Expand All @@ -747,6 +751,7 @@ async def _do_merge():
github2gerrit_mode=ctx.github2gerrit_mode,
no_netrc=ctx.no_netrc,
netrc_file=ctx.netrc_file,
rebase_local=ctx.rebase_local,
) as merge_manager:
if not preview:
console.print(
Expand Down Expand Up @@ -1095,12 +1100,23 @@ def _is_auto(author: str | None) -> bool:
ctx.progress_tracker.start()

try:
# Serialise merges for repo-scoped operations: all PRs target
# the same repository, so parallel approve+merge would race
# against GitHub's branch-protection propagation and cause
# spurious "branch protection" failures.
# Per-repo merge dispatch is serialised inside
# ``AsyncMergeManager`` (see ``_get_merge_dispatch_lock``),
# so it is now safe to run multiple workers against PRs
# that target the same repository — only the actual
# ``merge_pull_request`` call queues, while approve,
# rebase, and the Step 5.5 auto-merge wait run in
# parallel.
merge_results = _run_parallel_merge(
ctx, all_prs_to_merge, preview=not ctx.no_confirm, concurrency=1
ctx,
all_prs_to_merge,
preview=not ctx.no_confirm,
# Allow parallel workers; the merge dispatch itself is
# serialised per repo by ``AsyncMergeManager`` so PRs
# parked in Step 5.5's wait loop no longer block other
# PRs in the batch. Cap by PR count so we don't spawn
# more workers than there is work.
concurrency=min(5, len(all_prs_to_merge)) or 1,
)
finally:
if ctx.show_progress and ctx.progress_tracker:
Expand Down Expand Up @@ -1213,7 +1229,12 @@ def _execute_repo_confirmed_merge(

try:
real_results = _run_parallel_merge(
ctx, mergeable_prs, preview=False, concurrency=1
ctx,
mergeable_prs,
preview=False,
# Per-repo merge dispatch lock makes parallel workers
# safe; cap by PR count.
concurrency=min(5, len(mergeable_prs)) or 1,
)
finally:
if ctx.show_progress and ctx.progress_tracker:
Expand Down Expand Up @@ -1482,6 +1503,19 @@ def merge(
"--no-fix",
help="Do not attempt to automatically fix out-of-date branches",
),
rebase_local: bool = typer.Option(
True,
"--rebase-local/--no-rebase-local",
help=(
"When rebasing a behind PR, prefer a local ``git`` clone + "
"rebase + force-push-with-lease over the GitHub REST "
"``update-branch`` endpoint when the base branch requires "
"verified signatures or the PR is from pre-commit-ci[bot]. "
"The local path inherits ``~/.gitconfig`` so commits stay "
"signed; the REST path is faster but produces unsigned "
"commits that break verification. Default: enabled."
),
),
merge_timeout: float = typer.Option(
DEFAULT_MERGE_TIMEOUT,
"--merge-timeout",
Expand Down Expand Up @@ -1699,6 +1733,7 @@ def merge(
netrc_optional=netrc_optional,
github2gerrit_mode=github2gerrit_mode,
include_human_prs=include_human_prs,
rebase_local=rebase_local,
)
try:
_handle_repo_merge(parsed_repo, repo_ctx)
Expand Down Expand Up @@ -1765,6 +1800,7 @@ def merge(
netrc_optional=netrc_optional,
github2gerrit_mode=github2gerrit_mode,
include_human_prs=include_human_prs,
rebase_local=rebase_local,
)

try:
Expand Down
91 changes: 47 additions & 44 deletions src/dependamerge/github_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -930,57 +930,60 @@ async def post_issue_comment(
async def check_pr_commit_signatures(
self, owner: str, repo: str, number: int
) -> tuple[bool, list[str]]:
"""
Check whether all commits on a pull request have verified signatures.
"""Check whether all commits on a pull request have verified signatures.

REST: GET /repos/{owner}/{repo}/pulls/{pull_number}/commits

Returns:
Tuple of (all_verified: bool, unverified_shas: list[str]).
``all_verified`` is True when every commit carries a valid
signature according to GitHub. ``unverified_shas`` contains
the abbreviated SHAs of any commits whose verification failed.
Tuple of ``(all_verified, unverified_shas)``.
``all_verified`` is True when every commit carries a
valid signature according to GitHub.
``unverified_shas`` contains the abbreviated SHAs of
any commits whose verification failed.

Raises:
Exception: surfaces the underlying API/network error
on failure rather than silently returning a default.
Callers that want fail-open or fail-closed semantics
should wrap the call in ``try``/``except`` and decide
for themselves — the previous fail-open default
(returning ``(True, [])``) collided with the
signature-preservation gate in ``rebase.py``, which
documents "verified" as a positive confirmation.
"""
unverified: list[str] = []
try:
# Iterate over all pages of commits to ensure we don't miss
# unverified commits on pull requests with >100 commits.
async for commits in self.get_paginated(
f"/repos/{owner}/{repo}/pulls/{number}/commits",
per_page=100,
):
if not isinstance(commits, list):
# Unexpected response shape – assume OK to avoid false positives
return True, []

for commit_data in commits:
if not isinstance(commit_data, dict):
continue
raw_sha = commit_data.get("sha")
sha = str(raw_sha)[:8] if isinstance(raw_sha, str) else "unknown"
commit_obj = commit_data.get("commit")
if not isinstance(commit_obj, dict):
unverified.append(sha)
continue
verification = commit_obj.get("verification")
if not isinstance(verification, dict):
unverified.append(sha)
continue
if not verification.get("verified", False):
unverified.append(sha)
# Iterate over all pages of commits to ensure we don't miss
# unverified commits on pull requests with >100 commits.
async for commits in self.get_paginated(
f"/repos/{owner}/{repo}/pulls/{number}/commits",
per_page=100,
):
if not isinstance(commits, list):
# Unexpected response shape — assume OK to avoid
# false positives. The API returned 200 OK but in
# an unexpected shape, which is distinct from a
# network/HTTP error and arguably means the page
# is empty.
return True, []

for commit_data in commits:
if not isinstance(commit_data, dict):
continue
raw_sha = commit_data.get("sha")
sha = str(raw_sha)[:8] if isinstance(raw_sha, str) else "unknown"
commit_obj = commit_data.get("commit")
if not isinstance(commit_obj, dict):
unverified.append(sha)
continue
verification = commit_obj.get("verification")
if not isinstance(verification, dict):
unverified.append(sha)
continue
if not verification.get("verified", False):
unverified.append(sha)

all_verified = len(unverified) == 0
return all_verified, unverified
except Exception as e:
self.log.debug(
"Failed to check commit signatures for PR %s/%s#%s: %s",
owner,
repo,
number,
e,
)
# On error, assume verified to avoid blocking merges unnecessarily
return True, []
all_verified = len(unverified) == 0
return all_verified, unverified

async def requires_commit_signatures(
self, owner: str, repo: str, branch: str = "main"
Expand Down
21 changes: 21 additions & 0 deletions src/dependamerge/github_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,27 @@ async def _run() -> PullRequestInfo:
repository_full_name=f"{owner}/{repo}",
html_url=pr.get("html_url") or "",
reviews=reviews,
# Populate head/base repo identity so the
# signature-preserving local-rebase path can
# tell whether the PR is from a fork (and
# which remote to push to). Without these,
# ``rebase.local_rebase_pr()`` fails closed
# to avoid pushing to the wrong repository.
head_repo_full_name=(
((pr.get("head") or {}).get("repo") or {}).get("full_name")
),
head_repo_clone_url=(
((pr.get("head") or {}).get("repo") or {}).get("clone_url")
),
base_repo_full_name=(
((pr.get("base") or {}).get("repo") or {}).get("full_name")
),
base_repo_clone_url=(
((pr.get("base") or {}).get("repo") or {}).get("clone_url")
),
is_fork=(
((pr.get("head") or {}).get("repo") or {}).get("fork")
),
)

return asyncio.run(_run()) # type: ignore[no-any-return]
Expand Down
4 changes: 4 additions & 0 deletions src/dependamerge/github_graphql.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@
baseRefName
headRefName
headRefOid
headRepository { nameWithOwner url isFork }
baseRepository { nameWithOwner url }
createdAt
updatedAt
files(first: 50) {
Expand Down Expand Up @@ -169,6 +171,8 @@
baseRefName
headRefName
headRefOid
headRepository { nameWithOwner url isFork }
baseRepository { nameWithOwner url }
createdAt
updatedAt
files(first: $filesPageSize) {
Expand Down
65 changes: 65 additions & 0 deletions src/dependamerge/github_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,47 @@
]


def _str_or_none(value: Any) -> str | None:
"""Return ``value`` as a string when truthy, else None.

Used when populating optional ``PullRequestInfo`` fields from
GraphQL responses where the field may be missing entirely or
explicitly null.
"""
if isinstance(value, str) and value:
return value
return None


def _bool_or_none(value: Any) -> bool | None:
"""Coerce ``value`` to bool when present, else None.

Mirrors :func:`_str_or_none` for boolean GraphQL fields
(``isFork``).
"""
if isinstance(value, bool):
return value
return None


def _clone_url_with_git_suffix(url: Any) -> str | None:
"""Synthesise a canonical ``.git`` clone URL from a GraphQL ``url``.

GraphQL's ``Repository.url`` returns the HTTPS URL without the
``.git`` suffix that REST's ``clone_url`` includes. This
helper appends ``.git`` so both code paths produce the same
string and downstream consumers (notably
:func:`rebase.local_rebase_pr`) can treat them uniformly.

Returns None when the input is missing or empty so the
PullRequestInfo field stays unset rather than holding a
bogus ``".git"`` string.
"""
if isinstance(url, str) and url:
return f"{url}.git"
return None


class GitHubService:
"""
Asynchronous service orchestrating GraphQL paging and mapping results
Expand Down Expand Up @@ -515,6 +556,30 @@ def to_pull_request_info(
repository_full_name=repo_full_name,
html_url=pr.get("url") or "",
reviews=reviews,
# Populate head/base repo identity from the GraphQL
# ``headRepository`` / ``baseRepository`` fields so the
# signature-preserving local-rebase path can tell
# whether the PR is from a fork (and which remote to
# push to). Without these, ``rebase.local_rebase_pr()``
# fails closed to avoid pushing to the wrong repository.
# GraphQL returns the HTTPS URL via ``url`` (without the
# ``.git`` suffix), so we synthesise the canonical
# ``clone_url`` form for parity with REST.
head_repo_full_name=_str_or_none(
(pr.get("headRepository") or {}).get("nameWithOwner")
),
head_repo_clone_url=_clone_url_with_git_suffix(
(pr.get("headRepository") or {}).get("url")
),
base_repo_full_name=_str_or_none(
(pr.get("baseRepository") or {}).get("nameWithOwner")
),
base_repo_clone_url=_clone_url_with_git_suffix(
(pr.get("baseRepository") or {}).get("url")
),
is_fork=_bool_or_none(
(pr.get("headRepository") or {}).get("isFork")
),
)

async def find_similar_prs(
Expand Down
Loading
Loading