Skip to content
Merged
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
24 changes: 13 additions & 11 deletions src/repo_sync/stack/gh_ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,18 +291,20 @@ def get_pr_for_commit(self, sha: str) -> PullRequest | None:
lookup, avoiding false-positive text matches from gh pr list --search.
Filters for merged PRs to avoid picking up open/closed PRs that happen
to include the same commit.

Raises subprocess.CalledProcessError if the GitHub API call fails.
Callers in non-critical paths should catch this and degrade gracefully;
callers in safety-critical paths (e.g. loop detection) must let it
propagate to abort the workflow.
"""
try:
output = self._run(
[
"api",
f"repos/{self.repo}/commits/{sha}/pulls",
"--jq",
'[.[] | select(.merged_at != null)] | .[0]',
],
)
except subprocess.CalledProcessError:
return None
output = self._run(
[
"api",
f"repos/{self.repo}/commits/{sha}/pulls",
"--jq",
'[.[] | select(.merged_at != null)] | .[0]',
],
)
if not output or output == "null":
return None
pr = json.loads(output)
Expand Down
8 changes: 7 additions & 1 deletion src/repo_sync/stack/loop_detection.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,15 @@ def is_sync_originated(
return False

# Check 2: was the commit merged from a repo-sync/ branch?
#
# get_pr_for_commit raises on API failure, which intentionally aborts
# the sync run. We must not fail-open here: if we cannot verify the
# PR branch, we cannot safely determine whether this commit is
# sync-originated.
pr = gh.get_pr_for_commit(commit_sha)
if pr is None:
# No PR found -- this could be a direct push with a spoofed trailer.
# API succeeded but no merged PR was found — this could be a
# direct push with a spoofed trailer.
return False

if not is_sync_branch(pr.head_branch):
Expand Down
35 changes: 30 additions & 5 deletions src/repo_sync/workflows/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from __future__ import annotations

import logging
import subprocess
from dataclasses import dataclass

from repo_sync.stack.branches import check_idempotency
Expand Down Expand Up @@ -94,9 +95,17 @@ def enumerate_unsynced_commits(
# Filter out sync-originated commits.
result = []
for sha in all_commits:
if is_sync_originated(source_git, source_gh, sha):
logger.info("Skipping sync-originated commit %s.", sha[:12])
continue
try:
if is_sync_originated(source_git, source_gh, sha):
logger.info("Skipping sync-originated commit %s.", sha[:12])
continue
except subprocess.CalledProcessError:
logger.error(
"GitHub API failure during loop detection for commit %s. "
"Aborting sync to avoid creating an infinite loop.",
sha[:12],
)
raise
result.append(sha)

return result
Expand Down Expand Up @@ -146,7 +155,15 @@ def build_public_to_private_description(
direct pushes.
"""
source_repo_name = source_repo.split("/")[-1]
source_pr = source_gh.get_pr_for_commit(source_sha)
try:
source_pr = source_gh.get_pr_for_commit(source_sha)
except subprocess.CalledProcessError:
logger.warning(
"GitHub API error looking up PR for commit %s; "
"falling back to commit-based description.",
source_sha[:12],
)
source_pr = None

if source_pr is not None:
return public_to_private_from_pr(
Expand Down Expand Up @@ -180,7 +197,15 @@ def determine_sync_reviewer(
Tries the merger of the source PR first, then the commit author (via
GitHub API), then the fallback team.
"""
source_pr = source_gh.get_pr_for_commit(source_sha)
try:
source_pr = source_gh.get_pr_for_commit(source_sha)
except subprocess.CalledProcessError:
logger.warning(
"GitHub API error looking up PR for commit %s; "
"falling back to commit author or team for reviewer.",
source_sha[:12],
)
source_pr = None
pr_number = source_pr.number if source_pr else None

# For direct pushes (no source PR), look up the commit author via the API.
Expand Down
19 changes: 19 additions & 0 deletions tests/test_loop_detection.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

from __future__ import annotations

import subprocess
from unittest.mock import MagicMock

import pytest

from repo_sync.stack.gh_ops import GhOps, PullRequest
from repo_sync.stack.git_ops import GitOps
from repo_sync.stack.loop_detection import is_sync_originated
Expand Down Expand Up @@ -99,6 +102,22 @@ def test_trailer_present_but_no_pr_found(self) -> None:

assert is_sync_originated(git, gh, "directpush") is False

def test_api_failure_propagates_when_trailer_present(self) -> None:
"""An API failure during PR lookup aborts rather than failing-open."""
git = MagicMock(spec=GitOps)
git.commit_message.return_value = (
"sync commit\n\n"
"Repo-Sync-Origin: warpdotdev/warp-internal@abc123"
)

gh = MagicMock(spec=GhOps)
gh.get_pr_for_commit.side_effect = subprocess.CalledProcessError(
1, ["gh", "api", "repos/org/repo/commits/abc123/pulls"]
)

with pytest.raises(subprocess.CalledProcessError):
is_sync_originated(git, gh, "abc123")

def test_public_to_private_sync_branch_recognized(self) -> None:
"""Public-to-private sync branches are also recognized."""
git = MagicMock(spec=GitOps)
Expand Down
Loading