diff --git a/.github/workflows/testing.yaml b/.github/workflows/testing.yaml index 044c6d4..4746055 100644 --- a/.github/workflows/testing.yaml +++ b/.github/workflows/testing.yaml @@ -89,9 +89,15 @@ jobs: DRY_RUN: 'true' AUTOMATION_ONLY: 'false' - # Failure testing is also important + # Failure testing is also important. + # + # Skipped for pull requests raised from a fork: the approval gate + # stops those before input validation is reached, so the step + # above exits successfully having transferred nothing. That is + # the gate working, not the failure case under test. - name: "Error if step above did NOT fail" - if: steps.failure.outcome == 'success' + # yamllint disable-line rule:line-length + if: steps.failure.outcome == 'success' && github.event.pull_request.head.repo.full_name == github.repository shell: bash run: | # Error if step above did NOT fail diff --git a/README.md b/README.md index e31dae6..64f7eb7 100644 --- a/README.md +++ b/README.md @@ -93,6 +93,9 @@ name: github2gerrit on: pull_request_target: types: [opened, reopened, edited, synchronize, closed] + # Re-runs when a maintainer approves a fork pull request + pull_request_review: + types: [submitted, dismissed] push: branches: [main, master] workflow_dispatch: @@ -140,6 +143,9 @@ name: github2gerrit on: pull_request_target: types: [opened, reopened, edited, synchronize, closed] + # Re-runs when a maintainer approves a fork pull request + pull_request_review: + types: [submitted, dismissed] workflow_dispatch: permissions: diff --git a/docs/features.md b/docs/features.md index 4193ed2..1262385 100644 --- a/docs/features.md +++ b/docs/features.md @@ -400,9 +400,87 @@ Same-repository pull requests keep their previous behaviour and still read ### Approval gating -The tool does not yet require a maintainer approval before it transfers a fork -pull request to Gerrit. Until it does, use `AUTOMATION_ONLY` (see above) to -control which pull requests the tool accepts. +A fork pull request does not transfer to Gerrit until a maintainer approves it. +The check runs before the tool fetches anything from the pull request and before +it unlocks the Gerrit SSH key, so a pull request awaiting review reaches +neither. + +Same-repository pull requests skip the check entirely. Pushing a branch to the +base repository already requires write access, and automation such as +Dependabot, pre-commit.ci and Copilot works that way, so the gate never affects +them. + +#### What counts as approval + +An approving review, where every one of the following holds: + +- the reviewer's `author_association` falls in the trusted set, the same one + used for [comment commands](#who-may-issue-commands); +- the review targets the pull request's **current head commit**; +- no trusted reviewer has since requested changes; and +- the reviewer is not the pull request author. + +A pull request whose provenance the tool cannot establish takes the same path as +a fork. Approval is a question about authority, and an absent signal is not an +answer. + +The gate applies to unattended runs. Running the CLI directly against a pull +request URL does not require an approval: the operator chose that pull request +and is using their own credentials, so they are already the authority the gate +looks for. It exists for the automated path, where the tool acts on a shared +identity with nobody watching. + +Binding to the head commit matters. GitHub keeps approvals across pushes unless +branch protection dismisses them, so an approval that was not checked against a +commit would let a contributor gain approval for one revision and then push a +different one. The cost is that maintainers re-approve after each push, which is +the correct trade. + +The author exclusion is belt and braces. GitHub already rejects approving your +own pull request, and that guarantee is a large part of why the tool uses +reviews rather than comment directives — on a Gerrit mirror the pull request +author often holds the same organization membership as the reviewers. + +#### Enabling re-runs on approval + +`pull_request_target` carries no event for a submitted review, so approving a +pull request does nothing unless the calling workflow listens for it: + +```yaml +on: + pull_request_target: + types: [opened, reopened, edited, synchronize, closed] + pull_request_review: + types: [submitted, dismissed] +``` + +Without that trigger the tool only reconsiders a pull request on its next +push, or when a maintainer dispatches the workflow by hand. + +#### When the gate blocks + +The run finishes successfully rather than failing. A pull request waiting for a +human is not broken, and a red check would suggest otherwise. + +The tool posts one comment explaining what is missing, and edits that same +comment on later runs rather than adding another. Where an approval exists but +covers an earlier commit, the comment says so, so a maintainer who did approve +is not told that nobody has. + +Once approval arrives, the tool edits that comment again to record it, so a +transferred pull request does not keep displaying a stale block. + +#### If the head moves mid-run + +The gate reads the head commit from the API, while the workspace fetch reads +`refs/pull//head`, which the contributor can move meanwhile. The tool +compares the commit it fetched against the commit the maintainer approved and +refuses a mismatch, so a push timed against a running workflow cannot slip an +unreviewed commit into Gerrit. + +That refusal fails the run, unlike the ordinary blocked case, because it falls +outside the normal course of events. The push that caused it triggers a fresh +run, which asks for approval of the new commit in the usual way. ## Duplicate Detection diff --git a/src/github2gerrit/cli.py b/src/github2gerrit/cli.py index 17f3038..f8463c6 100644 --- a/src/github2gerrit/cli.py +++ b/src/github2gerrit/cli.py @@ -69,6 +69,7 @@ from .gerrit_pr_closer import parse_pr_url from .gerrit_pr_closer import process_recent_commits_for_pr_closure from .github_api import build_client +from .github_api import create_pr_comment from .github_api import get_pr_title_body from .github_api import get_pull from .github_api import get_repo_from_env @@ -80,6 +81,11 @@ from .models import Inputs from .netrc import NetrcParseError from .netrc import get_credentials_for_host +from .pr_approval import APPROVAL_MARKER +from .pr_approval import ApprovalStatus +from .pr_approval import evaluate_fork_approval +from .pr_approval import render_blocked_comment +from .pr_approval import render_cleared_comment from .rich_display import RICH_AVAILABLE from .rich_display import DummyProgressTracker from .rich_display import G2GProgressTracker @@ -87,6 +93,7 @@ from .rich_display import safe_console_print from .rich_display import safe_typer_echo from .rich_logging import setup_rich_aware_logging +from .trust import describe_trust_policy from .utils import append_github_output from .utils import env_bool from .utils import env_str @@ -228,14 +235,222 @@ def _check_automation_only( ) +def _resolve_pr_for_gate(gh: GitHubContext, data: Inputs) -> Any | None: + """Fetch the pull request for the approval gate. + + Used when the display step returned nothing — most notably the + no-token branch, which returns early. Without this the gate would + fail open exactly when the environment is least well configured. + """ + if not gh.pr_number: + return None + try: + token = getattr(data, "github_token", "") or os.getenv( + "GITHUB_TOKEN", "" + ) + client = build_client(token) + repo = get_repo_from_env(client) + return get_pull(repo, int(gh.pr_number)) + except Exception as exc: + log.debug("Could not resolve PR for the approval gate: %s", exc) + return None + + +def _check_fork_approval( + pr_obj: Any | None, + gh: GitHubContext, + progress_tracker: Any = None, +) -> tuple[bool, str]: + """Report whether a pull request may transfer to Gerrit. + + Same-repository pull requests are unaffected: their head branch + already implies write access. Everything else — forks, and pull + requests whose provenance could not be established — requires an + approving review from a trusted maintainer, bound to the current + head commit. + + Only a head positively known to live in the base repository skips + the gate. Unknown provenance does not: `is_fork_pr` answers a + factual question and reports ``False`` when it cannot tell, whereas + ``head_is_trusted`` answers the authorisation question and reports + ``False`` in the same case. Using the latter keeps the gate closed + when metadata is missing, which is exactly when it matters most. + + The gate applies to unattended runs only. A direct CLI invocation + is a person acting deliberately with their own credentials, and + needs no second signature. + + Runs before any fork content is fetched and before the Gerrit key + is materialised, so a blocked pull request never reaches either. + + Returns: + ``(allowed, approved_sha)``. ``approved_sha`` is the commit the + gate authorised, empty when no gate applied. It is returned + rather than stored globally because bulk runs process several + pull requests concurrently, where shared state would let one + worker clear or overwrite another's constraint. + + Blocking is not an error: the pull request is simply waiting + for a human, and a failed check would misrepresent that. + """ + if gh.head_is_trusted: + return True, "" + + if not _is_github_actions_context(): + # Direct CLI invocation. The operator chose this pull request + # and is using their own credentials, so they are the authority + # the gate would otherwise be looking for. The gate exists for + # the unattended path, where the tool acts on a shared identity + # with nobody watching. + log.debug( + "Not running under GitHub Actions; approval gate not applied " + "to PR #%s", + gh.pr_number, + ) + return True, "" + + if pr_obj is None: + # Fail closed. Reaching here means the pull request could not + # be resolved, so approval cannot be established either way. + log.warning( + "🛑 Pull request #%s not transferred to Gerrit: the tool " + "could not resolve the pull request to check for approval", + gh.pr_number, + ) + return False, "" + + head_sha = str( + getattr(getattr(pr_obj, "head", None), "sha", "") or "" + ).strip() + author = str( + getattr(getattr(pr_obj, "user", None), "login", "") or "" + ).strip() + + status = evaluate_fork_approval( + pr_obj, head_sha=head_sha, author_login=author + ) + + if status.approved: + log.info( + "✅ Pull request #%s authorised: %s", + gh.pr_number, + status.reason, + ) + # The workspace fetch reads refs/pull//head, which the + # contributor can move after this check. Carry what was + # approved so the fetch can refuse anything else. + _clear_fork_approval_notice(pr_obj, status, head_sha) + return True, head_sha + + log.warning( + "🛑 Pull request #%s not transferred to Gerrit: %s. " + "Reviews count from: %s", + gh.pr_number, + status.reason, + describe_trust_policy(), + ) + safe_console_print( + f"🛑 Awaiting maintainer approval: {status.reason}", + style="yellow", + progress_tracker=progress_tracker, + ) + + _post_fork_approval_notice(pr_obj, status, head_sha) + return False, "" + + +def _edit_owned_marker_comment(pr_obj: Any, body: str) -> bool: + """Replace the tool's own approval notice, if one exists. + + The marker is not proof of authorship — anyone may paste it into a + comment. Ownership is established by *attempting the edit*: the API + refuses to edit another user's comment, so a failure means the + comment was not ours and the search continues. + + Returns: + ``True`` when a notice of ours was updated. + """ + try: + issue = pr_obj.as_issue() + comments = list(issue.get_comments()) + except Exception as exc: + log.debug("Could not read comments for approval notice: %s", exc) + return False + + # Newest first: if several carry the marker, keep the most recent. + for comment in reversed(comments): + if APPROVAL_MARKER not in (getattr(comment, "body", "") or ""): + continue + try: + comment.edit(body) + except Exception as exc: + log.debug( + "Could not edit comment carrying the approval marker; " + "it is probably not ours: %s", + exc, + ) + continue + else: + return True + return False + + +def _clear_fork_approval_notice( + pr_obj: Any, + status: ApprovalStatus, + head_sha: str, +) -> None: + """Retract an earlier block notice once approval arrives. + + Only edits an existing notice; it does not create one. A pull + request that was never blocked has nothing to retract, and adding a + comment to say so would be noise. + + Best-effort: failing to tidy a comment must not stop a transfer + that has been authorised. + """ + if env_bool("CI_TESTING", False): + return + _edit_owned_marker_comment( + pr_obj, render_cleared_comment(status, head_sha=head_sha) + ) + + +def _post_fork_approval_notice( + pr_obj: Any, + status: ApprovalStatus, + head_sha: str, +) -> None: + """Explain the block on the pull request, editing any prior notice. + + Best-effort throughout: a comment API failure must never turn a + block into a crash, nor a block into a pass. + """ + if env_bool("CI_TESTING", False): + return + + body = render_blocked_comment(status, head_sha=head_sha) + if _edit_owned_marker_comment(pr_obj, body): + return + + try: + create_pr_comment(pr_obj, body) + except Exception as exc: + log.debug("Could not post fork approval notice: %s", exc) + + def _extract_and_display_pr_info( gh: GitHubContext, data: Inputs, progress_tracker: Any = None, -) -> None: - """Extract PR information and display it with Rich formatting.""" +) -> Any | None: + """Extract PR information and display it with Rich formatting. + + Returns the pull request object when one was fetched, so callers + can reuse it instead of making a further API call. + """ if not gh.pr_number: - return + return None try: # Get GitHub token from inputs if available, fallback to environment @@ -250,7 +465,7 @@ def _extract_and_display_pr_info( style="yellow", progress_tracker=progress_tracker, ) - return + return None client = build_client(token) repo = get_repo_from_env(client) @@ -298,6 +513,9 @@ def _extract_and_display_pr_info( _exit_for_pr_not_found(gh.pr_number, gh.repository) else: _exit_for_pr_fetch_error(exc) + else: + return pr_obj + return None class ConfigurationError(Exception): @@ -1522,12 +1740,13 @@ def _submit_bulk_pr( per_ctx: models.GitHubContext, pr_number: int, progress_tracker: G2GProgressTracker | DummyProgressTracker, + approved_sha: str = "", ) -> _BulkPrResult: """Run the orchestrator for a single PR in multi-PR mode.""" try: with tempfile.TemporaryDirectory() as temp_dir: workspace = Path(temp_dir) - orch = Orchestrator(workspace=workspace) + orch = Orchestrator(workspace=workspace, approved_sha=approved_sha) result_multi = orch.execute(inputs=data, gh=per_ctx) _record_change_tracker_result(progress_tracker, result_multi) return "success", result_multi, None @@ -1576,13 +1795,20 @@ def _process_bulk_pr( log.debug("PR #%d rejected by automation_only check", pr_number) return "skipped", None, None + # Fork PRs need a maintainer's approval before anything is fetched + allowed, approved_sha = _check_fork_approval(pr, per_ctx, progress_tracker) + if not allowed: + return "skipped", None, None + skip_result = _check_bulk_pr_duplicates( data, per_ctx, pr_number, progress_tracker ) if skip_result is not None: return skip_result - return _submit_bulk_pr(data, per_ctx, pr_number, progress_tracker) + return _submit_bulk_pr( + data, per_ctx, pr_number, progress_tracker, approved_sha + ) def _record_bulk_success( @@ -2079,10 +2305,11 @@ def _process_single( data: Inputs, gh: GitHubContext, progress_tracker: G2GProgressTracker | DummyProgressTracker | None = None, + approved_sha: str = "", ) -> tuple[bool, SubmissionResult]: with tempfile.TemporaryDirectory() as temp_dir: workspace = Path(temp_dir) - orch = Orchestrator(workspace=workspace) + orch = Orchestrator(workspace=workspace, approved_sha=approved_sha) _prepare_single_checkout(orch, workspace, data, gh, progress_tracker) _log_single_pre_submit(data, progress_tracker) pipeline_success, result = _run_single_submission( @@ -2932,9 +3159,34 @@ def _handle_single_pr( # Augment PR refs via API when in URL mode and token present gh = _augment_pr_refs_if_needed(gh) + # A review only exists to unblock a gated pull request. On a + # same-repository head there is nothing to unblock, so stop rather + # than resubmitting an unchanged commit every time somebody + # comments on, approves or requests changes to a pull request. + if gh.event_name == "pull_request_review" and gh.head_is_trusted: + log.info( + "Review on PR #%s, whose head is in this repository; " + "nothing to unblock, so no transfer is needed", + gh.pr_number, + ) + sys.exit(int(ExitCode.SUCCESS)) + # Display PR information with Rich formatting + approved_sha = "" if gh.pr_number: - _extract_and_display_pr_info(gh, data, progress_tracker) + pr_obj = _extract_and_display_pr_info(gh, data, progress_tracker) + + # Fork PRs need a maintainer's approval before anything is + # fetched and before the Gerrit key is materialised. Resolve + # the PR here when the display step could not, so a missing + # token cannot skip the gate. + if pr_obj is None: + pr_obj = _resolve_pr_for_gate(gh, data) + allowed, approved_sha = _check_fork_approval( + pr_obj, gh, progress_tracker + ) + if not allowed: + sys.exit(int(ExitCode.SUCCESS)) # Check for duplicates in single-PR mode (before workspace setup) if gh.pr_number and not env_bool("SYNC_ALL_OPEN_PRS", False): @@ -2946,7 +3198,9 @@ def _handle_single_pr( log.debug("Processing PR #%s from %s", gh.pr_number, gh.repository) log.debug("Target Gerrit server: %s", data.gerrit_server) log.debug("Target Gerrit project: %s", data.gerrit_project) - pipeline_success, result = _process_single(data, gh, progress_tracker) + pipeline_success, result = _process_single( + data, gh, progress_tracker, approved_sha + ) # Run abandoned-PR and Gerrit cleanup if the pipeline was successful # Skip in G2G_NO_GERRIT: no Gerrit server to query diff --git a/src/github2gerrit/core.py b/src/github2gerrit/core.py index 4af69e6..9a47abf 100644 --- a/src/github2gerrit/core.py +++ b/src/github2gerrit/core.py @@ -1650,28 +1650,56 @@ def _should_create_missing( self, inputs: Inputs, gh: GitHubContext, - ) -> bool: + ) -> tuple[bool, str]: """Decide whether to fall back from UPDATE to CREATE. - Returns ``True`` when either the ``--create-missing`` CLI flag - is active **or** a ``@github2gerrit create missing change`` - comment is present on the PR *from a trusted author*. + Three things authorise the fallback: the ``--create-missing`` + flag, a review-triggered run on an untrusted head, and a + ``@github2gerrit create missing change`` comment from a trusted + author. Comment authorship is checked because these mirrors are public: without it, anyone able to comment could force creation of a Gerrit change that the UPDATE path deliberately declined to make. + + Returns: + ``(should_create, reason)``. The reason is returned rather + than restated by the notice, so the explanation the + contributor sees cannot drift from the condition that + actually fired. """ # 1. Explicit CLI / environment flag if inputs.create_missing: log.info( "✅ --create-missing flag is set; authorising CREATE fallback" ) - return True + return True, "Triggered by the `--create-missing` flag." + + # 2. A review is what unblocks a gated pull request, so a + # review-triggered run may legitimately be the first one to + # reach Gerrit. Treating a missing change as an error there + # would fail every gated PR on its first approval. + # + # Scoped to heads that actually pass through the gate. A + # same-repository PR was never gated, so a review on one + # must not quietly override CREATE_MISSING=false. + if gh.event_name == "pull_request_review" and not gh.head_is_trusted: + log.info( + "✅ Review-triggered run for PR #%s from an untrusted head; " + "authorising CREATE fallback because no Gerrit change " + "exists yet", + gh.pr_number, + ) + return True, ( + "Triggered by a maintainer's approving review, which is " + "the first run permitted to reach Gerrit for this pull " + "request." + ) - # 2. Scan PR comments for the directive + # 3. Scan PR comments for the directive if not gh.pr_number: - return False + return False, "" try: from .pr_commands import CMD_CREATE_MISSING @@ -1690,7 +1718,10 @@ def _should_create_missing( gh.pr_number, match.comment_index, ) - return True + return True, ( + "Triggered by the `@github2gerrit create missing " + "change` comment." + ) log.debug( "No @github2gerrit create-missing command found in " @@ -1703,13 +1734,21 @@ def _should_create_missing( exc, ) - return False + return False, "" def _post_create_missing_notice( self, gh: GitHubContext, + reason: str = "", ) -> None: - """Post a comment on the PR noting the CREATE fallback.""" + """Post a comment on the PR noting the CREATE fallback. + + Args: + gh: GitHub context. + reason: What authorised the fallback. Passed in rather than + restated here, so the notice cannot drift out of step + with the conditions in ``_should_create_missing``. + """ if not gh.pr_number: return # Respect CI_TESTING @@ -1719,6 +1758,7 @@ def _post_create_missing_notice( "yes", ): return + attribution = f"\n\n_{reason}_" if reason else "" try: client_gh = build_client() repo = get_repo_from_env(client_gh) @@ -1728,9 +1768,7 @@ def _post_create_missing_notice( "🔄 **GitHub2Gerrit**: No existing Gerrit change found for " "this PR.\n" "Creating a new Gerrit change (fallback from UPDATE " - "operation).\n\n" - "_Triggered by `@github2gerrit create missing change` " - "comment or `--create-missing` flag._", + f"operation).{attribution}", ) except Exception as exc: log.warning( @@ -2029,8 +2067,15 @@ def __init__( self, *, workspace: Path, + approved_sha: str = "", ) -> None: self.workspace = workspace + # Commit the fork approval gate authorised, when one applied. + # Passed explicitly rather than read from the environment: + # bulk runs process several pull requests concurrently, and a + # process-global value would let one worker clear or overwrite + # another's constraint. Empty means no gate applied. + self._approved_sha: str = approved_sha.strip() # SSH configuration paths (set by _setup_ssh) self._ssh_key_path: Path | None = None self._ssh_known_hosts_path: Path | None = None @@ -2593,7 +2638,9 @@ def _resolve_forced_reuse_ids( except OrchestratorError: # UPDATE found no existing Gerrit change — check whether # we should fall back to CREATE instead of failing. - should_create = self._should_create_missing(inputs, gh) + should_create, create_reason = self._should_create_missing( + inputs, gh + ) if not should_create: raise # propagate the original error @@ -2610,7 +2657,7 @@ def _resolve_forced_reuse_ids( os.environ["G2G_FALLBACK_CREATE"] = "true" # Notify on the PR that we are creating from scratch - self._post_create_missing_notice(gh) + self._post_create_missing_notice(gh, create_reason) return ( forced_reuse_ids, @@ -5975,11 +6022,72 @@ def _prepare_workspace_checkout( except Exception as exc: log.debug("PR fetch failed, will try API fallback: %s", exc) + if fetch_success: + self._enforce_approved_head(gh) + # Fallback to GitHub API archive if git fetch failed (CLI's resilience) if not fetch_success and pr_num_str and pr_num_str != "0": log.info("Git fetch failed, falling back to GitHub API archive") self._fallback_to_api_archive(self.workspace, gh, inputs) + def _enforce_approved_head(self, gh: GitHubContext) -> None: + """Refuse a head that differs from the one the gate approved. + + The approval gate reads the head SHA from the API, but the + workspace is populated from ``refs/pull//head`` or, on + fallback, from an archive of the PR's *current* head. Both can + move after the check. Without this comparison a push timed + against the workflow would put an unreviewed commit into + Gerrit. + + No constraint is recorded when no gate applied, so + same-repository pull requests and direct CLI runs are + unaffected. + """ + from .gitutils import run_cmd + + approved = self._approved_sha.strip().lower() + if not approved: + return + + result = run_cmd(["git", "rev-parse", "HEAD"], cwd=self.workspace) + fetched = (result.stdout or "").strip().lower() + + if fetched == approved: + log.debug("Fetched head matches the approved commit %s", approved) + return + + msg = ( + f"pull request head moved after approval: approved " + f"{approved[:12]}, fetched {fetched[:12]}. Refusing to " + f"transfer an unreviewed commit; the next run will ask for " + f"approval of the new head" + ) + raise OrchestratorError(msg) + + def _assert_archive_sha_approved(self, head_sha: str) -> None: + """Refuse an archive of a head the gate did not approve. + + The archive fallback re-reads the pull request's *current* head + from the API, so it must be checked before download rather than + after: unlike the git path there is no commit object to compare + once the files have landed. + """ + approved = self._approved_sha.strip().lower() + if not approved: + return + + if head_sha.strip().lower() == approved: + return + + msg = ( + f"pull request head moved after approval: approved " + f"{approved[:12]}, archive offers {head_sha[:12]}. Refusing " + f"to transfer an unreviewed commit; the next run will ask " + f"for approval of the new head" + ) + raise OrchestratorError(msg) + def _validate_and_get_api_base_url(self, server_url: str) -> str: """Validate server URL and return appropriate API base URL. @@ -6207,6 +6315,10 @@ def _fallback_to_api_archive( head_sha = pr_data["head"]["sha"] + # Checked before the download, not after: the archive yields a + # file tree with no commit object to compare against. + self._assert_archive_sha_approved(head_sha) + # Download archive archive_url = f"{api_base}/repos/{repo_full}/zipball/{head_sha}" diff --git a/src/github2gerrit/github_api.py b/src/github2gerrit/github_api.py index d49edf9..0a3aeeb 100644 --- a/src/github2gerrit/github_api.py +++ b/src/github2gerrit/github_api.py @@ -80,6 +80,21 @@ class GhUser(Protocol): login: str +class GhReview(Protocol): + """A submitted pull request review. + + ``state`` is one of ``APPROVED``, ``CHANGES_REQUESTED``, + ``COMMENTED``, ``DISMISSED`` or ``PENDING``. ``commit_id`` is the + head SHA the review was submitted against, which is what binds an + approval to the code that was actually reviewed. + """ + + state: str | None + commit_id: str | None + author_association: str | None + user: GhUser | None + + class GhIssueComment(Protocol): body: str | None author_association: str | None @@ -104,6 +119,10 @@ def as_issue(self) -> GhIssue: """Return the issue view of this pull request.""" raise NotImplementedError + def get_reviews(self) -> Iterable[GhReview]: + """Return the reviews submitted on this pull request.""" + raise NotImplementedError + def edit(self, *, state: str) -> None: """Edit the pull request state.""" @@ -133,6 +152,7 @@ def get_repo(self, full: str) -> GhRepository: "create_pr_comment", "get_pr_title_body", "get_pull", + "get_pull_request_reviews", "get_recent_change_ids_from_comments", "get_repo_from_env", "get_trusted_comment_bodies", @@ -327,6 +347,35 @@ def get_recent_change_ids_from_comments( return found +@external_api_call(ApiType.GITHUB, "get_pull_request_reviews") +def get_pull_request_reviews(pr: GhPullRequest) -> list[GhReview]: + """Return the reviews on a pull request, oldest first. + + The endpoint returns the full review *history*, not current state: + one user may appear several times. Callers must reduce to the + latest meaningful review per user themselves. + """ + try: + reviews = list(pr.get_reviews()) + except Exception as exc: + if is_github_api_permission_error(exc): + raise GitHub2GerritError( + ExitCode.GITHUB_API_ERROR, + message=( + "❌ GitHub API query failed; cannot read reviews for " + f"pull request #{pr.number}" + ), + details=( + "GITHUB_TOKEN needs pull-requests read access to " + "evaluate approvals" + ), + original_exception=exc, + ) from exc + raise + else: + return reviews + + @external_api_call(ApiType.GITHUB, "get_trusted_comment_bodies") def get_trusted_comment_bodies( pr: GhPullRequest, diff --git a/src/github2gerrit/models.py b/src/github2gerrit/models.py index 3297b29..e2dd451 100644 --- a/src/github2gerrit/models.py +++ b/src/github2gerrit/models.py @@ -142,14 +142,26 @@ class GitHubContext: def get_operation_mode(self) -> PROperationMode: """Determine the operation mode based on event type and action. - Supports both ``pull_request`` and ``pull_request_target`` triggers. - Using ``pull_request`` is preferred for security (avoids granting - secrets to untrusted fork code), while ``pull_request_target`` is - accepted for backward compatibility. + Supports both ``pull_request`` and ``pull_request_target`` + triggers. Using ``pull_request`` is preferred for security + (avoids granting secrets to untrusted fork code), while + ``pull_request_target`` is accepted for backward compatibility. + + ``pull_request_review`` maps to UPDATE. A review changes no + code, so an existing Gerrit change should gain a patchset + rather than a sibling. When the review is instead the event + that first unblocks a fork pull request, no change exists yet + and the create-missing fallback covers it — see + ``Orchestrator._should_create_missing``. Choosing CREATE here + instead would raise a duplicate for the far commoner case of a + re-approval after a push. Returns: PROperationMode enum indicating the type of operation """ + if self.event_name == "pull_request_review": + return PROperationMode.UPDATE + if self.event_name not in ("pull_request", "pull_request_target"): return PROperationMode.UNKNOWN diff --git a/src/github2gerrit/pr_approval.py b/src/github2gerrit/pr_approval.py new file mode 100644 index 0000000..65d592c --- /dev/null +++ b/src/github2gerrit/pr_approval.py @@ -0,0 +1,300 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2026 The Linux Foundation +"""Maintainer approval for fork pull requests. + +A pull request raised from a fork is written by someone without write +access to the base repository. Transferring it to Gerrit pushes that +content under the tool's own SSH identity, where Gerrit CI will then +execute it. This module decides whether a maintainer has authorised +that. + +Why a review rather than a comment directive +──────────────────────────────────────────── +On a Gerrit mirror the only usable trust signal is organisation +membership (see :mod:`github2gerrit.trust`), and the pull request author +frequently holds it. Under a comment scheme they could therefore +authorise their own change. GitHub structurally forbids approving your +own pull request, so a review carries a guarantee that a comment cannot. + +The evaluation is deliberately conservative: + +* the approval must come from a **trusted** association, +* it must be bound to the **current head SHA**, because GitHub only + dismisses stale approvals when branch protection says so — without + this an approve-then-force-push would slip through, +* a later ``CHANGES_REQUESTED`` from any trusted reviewer blocks, and +* the pull request author is excluded regardless, so a mirror + configured to accept self-review still cannot be used that way. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from dataclasses import field +from typing import Any + +from .github_api import get_pull_request_reviews +from .trust import describe_trust_policy +from .trust import is_trusted_association + + +__all__ = [ + "APPROVAL_MARKER", + "ApprovalStatus", + "evaluate_fork_approval", + "render_blocked_comment", + "render_cleared_comment", +] + +log = logging.getLogger("github2gerrit.pr_approval") + +APPROVAL_MARKER = "" +"""Sentinel identifying the gate's explanatory comment, so repeated +runs edit one comment rather than adding a new one each time.""" + +_STATE_APPROVED = "APPROVED" +_STATE_CHANGES_REQUESTED = "CHANGES_REQUESTED" +_STATE_DISMISSED = "DISMISSED" + +_DECISIVE_STATES = frozenset( + {_STATE_APPROVED, _STATE_CHANGES_REQUESTED, _STATE_DISMISSED} +) +"""States that express, revoke or withhold authorisation. + +``COMMENTED`` and ``PENDING`` deliberately do not: a reviewer who +leaves remarks without approving has not changed their position, and +must not displace their own earlier approval. + +``DISMISSED`` *is* included, and must be. It is how an approval is +revoked, so leaving it out would let a dismissed approval keep +authorising the transfer. +""" + + +@dataclass(frozen=True) +class ApprovalStatus: + """Outcome of evaluating a fork pull request's reviews. + + Attributes: + approved: Whether the transfer is authorised. + reason: Short human-readable explanation, used in logs and in + the pull request comment. + approvers: Logins whose current approval covers the head SHA. + stale_approvers: Logins who approved an earlier commit. Kept + separate so the comment can tell someone their approval + went stale rather than implying they never gave one. + blockers: Trusted logins currently requesting changes. + """ + + approved: bool + reason: str + approvers: list[str] = field(default_factory=list) + stale_approvers: list[str] = field(default_factory=list) + blockers: list[str] = field(default_factory=list) + + +def _latest_decisive_reviews( + reviews: list[Any], + *, + exclude_login: str, +) -> dict[str, Any]: + """Reduce a review history to each author's current position. + + The reviews endpoint returns history oldest-first, so later entries + overwrite earlier ones. Only decisive states participate, so a + trailing ``COMMENTED`` does not erase an approval. + """ + latest: dict[str, Any] = {} + excluded = exclude_login.strip().lower() + + for review in reviews: + state = str(getattr(review, "state", "") or "").strip().upper() + if state not in _DECISIVE_STATES: + continue + + login = str( + getattr(getattr(review, "user", None), "login", "") or "" + ).strip() + if not login: + continue + + if excluded and login.lower() == excluded: + # GitHub rejects self-approval, but the guarantee is worth + # holding locally too: it is the only structural check + # backing an otherwise weak trust signal. + log.debug("Ignoring self-review by PR author %s", login) + continue + + latest[login] = review + + return latest + + +def evaluate_fork_approval( + pr: Any, + *, + head_sha: str, + author_login: str = "", +) -> ApprovalStatus: + """Decide whether a fork pull request may transfer to Gerrit. + + Args: + pr: Pull request object. + head_sha: Current head commit of the pull request. Approvals + recorded against any other commit do not count. + author_login: Pull request author, excluded from reviewing. + + Returns: + An :class:`ApprovalStatus`. Any failure to read reviews yields + an unapproved result rather than an exception, so the gate + fails closed. + """ + try: + reviews = get_pull_request_reviews(pr) + except Exception as exc: + log.warning("Could not read reviews; treating as unapproved: %s", exc) + return ApprovalStatus( + approved=False, + reason="the tool could not read this pull request's reviews", + ) + + latest = _latest_decisive_reviews(reviews, exclude_login=author_login) + + approvers: list[str] = [] + stale_approvers: list[str] = [] + blockers: list[str] = [] + target = head_sha.strip().lower() + + for login, review in sorted(latest.items()): + association = str(getattr(review, "author_association", "") or "") + if not is_trusted_association(association): + log.debug( + "Ignoring review by %s (%s): not a trusted association", + login, + association or "unknown", + ) + continue + + state = str(getattr(review, "state", "") or "").strip().upper() + if state == _STATE_CHANGES_REQUESTED: + blockers.append(login) + continue + if state != _STATE_APPROVED: + # DISMISSED, or anything unrecognised. Only an explicit + # approval authorises; everything else withholds. + continue + + commit_id = str(getattr(review, "commit_id", "") or "").strip().lower() + if not target or not commit_id or commit_id != target: + # The guarantee is that the approval covers exactly the + # commit about to be transferred. Absent SHA metadata + # cannot establish that, so it withholds approval rather + # than being waved through. + stale_approvers.append(login) + continue + + approvers.append(login) + + if blockers: + return ApprovalStatus( + approved=False, + reason=( + "a maintainer has requested changes: " + ", ".join(blockers) + ), + approvers=approvers, + stale_approvers=stale_approvers, + blockers=blockers, + ) + + if approvers: + return ApprovalStatus( + approved=True, + reason="approved by " + ", ".join(approvers), + approvers=approvers, + stale_approvers=stale_approvers, + ) + + if stale_approvers: + return ApprovalStatus( + approved=False, + reason=( + "the approval from " + + ", ".join(stale_approvers) + + " does not cover the current commit" + ), + stale_approvers=stale_approvers, + ) + + return ApprovalStatus( + approved=False, + reason="no maintainer has approved this pull request", + ) + + +def render_cleared_comment( + status: ApprovalStatus, + *, + head_sha: str, +) -> str: + """Build the replacement for a notice whose block has lifted. + + Posted by editing the earlier notice rather than adding a second + comment, so the pull request does not keep telling a contributor + they are waiting for something that already happened. + """ + short_sha = head_sha[:7] if head_sha else "unknown" + + return "\n".join( + [ + APPROVAL_MARKER, + "### Approved", + "", + f"This pull request is {status.reason}, for commit " + f"`{short_sha}`, and transfers to Gerrit.", + "", + "Pushing further commits requires a fresh approval, because " + "an approval covers the commit it was given for.", + ] + ) + + +def render_blocked_comment( + status: ApprovalStatus, + *, + head_sha: str, +) -> str: + """Build the explanatory comment posted when the gate blocks.""" + short_sha = head_sha[:7] if head_sha else "unknown" + + lines = [ + APPROVAL_MARKER, + "### Awaiting maintainer approval", + "", + "This pull request does not transfer to Gerrit until a maintainer " + "approves it. That applies to pull requests raised from a fork, " + "and to any whose origin the tool could not establish.", + "", + f"**Status:** {status.reason}.", + "", + ] + + if status.stale_approvers: + lines += [ + "An earlier approval exists but covers a different commit. " + "GitHub keeps approvals across pushes unless branch protection " + "dismisses them, so this tool checks the approval against the " + f"commit it will transfer (`{short_sha}`). Re-approve to " + "refresh it.", + "", + ] + + lines += [ + "**To proceed:** submit an approving review. The tool re-runs on " + "approval and transfers the change to Gerrit.", + "", + f"Reviews count from: {describe_trust_policy()}. The pull request " + "author cannot approve their own pull request.", + ] + + return "\n".join(lines) diff --git a/tests/test_cli_outputs_file.py b/tests/test_cli_outputs_file.py index 83bcfe2..12048a1 100644 --- a/tests/test_cli_outputs_file.py +++ b/tests/test_cli_outputs_file.py @@ -46,7 +46,7 @@ def __init__( class _DummyOrchestratorSingle: - def __init__(self, workspace: Any) -> None: + def __init__(self, workspace: Any, approved_sha: str = "") -> None: self.workspace = workspace def _prepare_workspace_checkout(self, *, inputs: Any, gh: Any) -> None: @@ -69,7 +69,7 @@ def execute( class _DummyOrchestratorMulti: - def __init__(self, workspace: Any) -> None: + def __init__(self, workspace: Any, approved_sha: str = "") -> None: self.workspace = workspace def _prepare_workspace_checkout(self, *, inputs: Any, gh: Any) -> None: @@ -88,7 +88,16 @@ def execute( def _base_env_with_event(tmp_path: Path) -> dict[str, str]: event_path = tmp_path / "event.json" - event = {"action": "opened", "pull_request": {"number": 77}} + # Real pull_request/pull_request_target payloads always carry the + # head repository, which the fork approval gate reads to tell a + # same-repository pull request from a fork. + event = { + "action": "opened", + "pull_request": { + "number": 77, + "head": {"repo": {"full_name": "example/repo"}}, + }, + } event_path.write_text(json.dumps(event), encoding="utf-8") # Start with a minimal clean environment to avoid pollution diff --git a/tests/test_cli_url_and_dryrun.py b/tests/test_cli_url_and_dryrun.py index 417a5d0..0c3a995 100644 --- a/tests/test_cli_url_and_dryrun.py +++ b/tests/test_cli_url_and_dryrun.py @@ -52,7 +52,7 @@ class _DummyOrchestrator: Test stub for Orchestrator used to capture calls to execute(). """ - def __init__(self, workspace: Any) -> None: + def __init__(self, workspace: Any, approved_sha: str = "") -> None: self.workspace = workspace def _prepare_workspace_checkout(self, *, inputs: Any, gh: Any) -> None: diff --git a/tests/test_fork_approval_gate.py b/tests/test_fork_approval_gate.py new file mode 100644 index 0000000..28f5982 --- /dev/null +++ b/tests/test_fork_approval_gate.py @@ -0,0 +1,679 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2026 The Linux Foundation +"""Maintainer approval gate for fork pull requests. + +Transferring a fork pull request pushes someone else's code into Gerrit +under the tool's SSH identity, where Gerrit CI executes it. These tests +pin the conditions under which that is allowed. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock +from unittest.mock import patch + +import pytest + +from github2gerrit.models import GitHubContext +from github2gerrit.models import PROperationMode +from github2gerrit.pr_approval import APPROVAL_MARKER +from github2gerrit.pr_approval import ApprovalStatus +from github2gerrit.pr_approval import evaluate_fork_approval +from github2gerrit.pr_approval import render_blocked_comment +from github2gerrit.pr_approval import render_cleared_comment + + +BASE_REPO = "opendaylight/mdsal" +FORK_REPO = "contributor/mdsal" +HEAD_SHA = "0b2abdcf7bb2fb5ed6620f214968ae2b3c5e70e6" +OLD_SHA = "1111111111111111111111111111111111111111" + + +def _review( + state: str, + login: str, + association: str = "MEMBER", + commit_id: str = HEAD_SHA, +) -> Any: + review = MagicMock() + review.state = state + review.commit_id = commit_id + review.author_association = association + review.user = MagicMock() + review.user.login = login + return review + + +def _pr(reviews: list[Any], author: str = "contributor") -> Any: + pr = MagicMock() + pr.get_reviews.return_value = reviews + pr.user = MagicMock() + pr.user.login = author + pr.head = MagicMock() + pr.head.sha = HEAD_SHA + return pr + + +def _evaluate(reviews: list[Any], author: str = "contributor"): + return evaluate_fork_approval( + _pr(reviews, author), head_sha=HEAD_SHA, author_login=author + ) + + +class TestApprovalEvaluation: + """Which reviews authorise a transfer.""" + + def test_no_reviews_blocks(self) -> None: + status = _evaluate([]) + assert status.approved is False + assert "no maintainer has approved" in status.reason + + @pytest.mark.parametrize("association", ["OWNER", "MEMBER", "COLLABORATOR"]) + def test_trusted_approval_passes(self, association: str) -> None: + status = _evaluate([_review("APPROVED", "maintainer", association)]) + assert status.approved is True + assert status.approvers == ["maintainer"] + + @pytest.mark.parametrize( + "association", ["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "NONE"] + ) + def test_untrusted_approval_ignored(self, association: str) -> None: + status = _evaluate([_review("APPROVED", "outsider", association)]) + assert status.approved is False + + def test_missing_association_ignored(self) -> None: + status = _evaluate([_review("APPROVED", "ghost", "")]) + assert status.approved is False + + +class TestApprovalBindsToHeadSha: + """The approve-then-force-push bypass.""" + + def test_approval_of_older_commit_does_not_count(self) -> None: + """GitHub keeps approvals across pushes unless told otherwise.""" + status = _evaluate( + [_review("APPROVED", "maintainer", commit_id=OLD_SHA)] + ) + assert status.approved is False + assert status.stale_approvers == ["maintainer"] + assert "does not cover the current commit" in status.reason + + def test_approval_without_commit_id_does_not_count(self) -> None: + """Absent SHA metadata cannot establish what was reviewed.""" + status = _evaluate([_review("APPROVED", "maintainer", commit_id="")]) + assert status.approved is False + assert status.stale_approvers == ["maintainer"] + + def test_unknown_head_sha_blocks(self) -> None: + pr = _pr([_review("APPROVED", "maintainer")]) + status = evaluate_fork_approval( + pr, head_sha="", author_login="contributor" + ) + assert status.approved is False + + def test_reapproval_after_push_counts(self) -> None: + status = _evaluate( + [ + _review("APPROVED", "maintainer", commit_id=OLD_SHA), + _review("APPROVED", "maintainer", commit_id=HEAD_SHA), + ] + ) + assert status.approved is True + + def test_sha_comparison_is_case_insensitive(self) -> None: + status = _evaluate( + [_review("APPROVED", "maintainer", commit_id=HEAD_SHA.upper())] + ) + assert status.approved is True + + +class TestSelfApproval: + """GitHub forbids it; the tool does not rely on that alone.""" + + def test_author_cannot_approve_own_pr(self) -> None: + """Org membership is weak, so the author is excluded outright.""" + status = _evaluate( + [_review("APPROVED", "contributor", "MEMBER")], + author="contributor", + ) + assert status.approved is False + + def test_author_exclusion_is_case_insensitive(self) -> None: + status = _evaluate( + [_review("APPROVED", "Contributor", "MEMBER")], + author="contributor", + ) + assert status.approved is False + + def test_other_member_still_counts(self) -> None: + status = _evaluate( + [ + _review("APPROVED", "contributor", "MEMBER"), + _review("APPROVED", "maintainer", "MEMBER"), + ], + author="contributor", + ) + assert status.approved is True + assert status.approvers == ["maintainer"] + + +class TestReviewHistoryReduction: + """The endpoint returns history, not current state.""" + + def test_changes_requested_blocks(self) -> None: + status = _evaluate( + [ + _review("APPROVED", "one"), + _review("CHANGES_REQUESTED", "two"), + ] + ) + assert status.approved is False + assert status.blockers == ["two"] + + def test_later_approval_supersedes_changes_requested(self) -> None: + status = _evaluate( + [ + _review("CHANGES_REQUESTED", "maintainer"), + _review("APPROVED", "maintainer"), + ] + ) + assert status.approved is True + + def test_later_changes_requested_supersedes_approval(self) -> None: + status = _evaluate( + [ + _review("APPROVED", "maintainer"), + _review("CHANGES_REQUESTED", "maintainer"), + ] + ) + assert status.approved is False + + def test_trailing_comment_does_not_erase_approval(self) -> None: + """COMMENTED expresses no position and must not displace one.""" + status = _evaluate( + [ + _review("APPROVED", "maintainer"), + _review("COMMENTED", "maintainer"), + ] + ) + assert status.approved is True + + def test_dismissed_approval_does_not_count(self) -> None: + status = _evaluate( + [ + _review("APPROVED", "maintainer"), + _review("DISMISSED", "maintainer"), + ] + ) + assert status.approved is False + + def test_pending_review_does_not_count(self) -> None: + status = _evaluate([_review("PENDING", "maintainer")]) + assert status.approved is False + + +class TestEvaluationFailsClosed: + """An unreadable review list is not an approval.""" + + def test_api_failure_blocks(self) -> None: + pr = MagicMock() + pr.get_reviews.side_effect = RuntimeError("403") + + status = evaluate_fork_approval( + pr, head_sha=HEAD_SHA, author_login="contributor" + ) + + assert status.approved is False + assert "could not read" in status.reason + + +class TestBlockedComment: + """What the contributor is told.""" + + def test_carries_marker_for_idempotent_updates(self) -> None: + body = render_blocked_comment( + ApprovalStatus(approved=False, reason="no approval"), + head_sha=HEAD_SHA, + ) + assert body.startswith(APPROVAL_MARKER) + + def test_explains_stale_approval_distinctly(self) -> None: + """Being told 'no approval' when you approved is confusing.""" + body = render_blocked_comment( + ApprovalStatus( + approved=False, + reason="the approval from maintainer does not cover it", + stale_approvers=["maintainer"], + ), + head_sha=HEAD_SHA, + ) + assert "Re-approve" in body + assert HEAD_SHA[:7] in body + + def test_names_the_trust_policy(self) -> None: + body = render_blocked_comment( + ApprovalStatus(approved=False, reason="no approval"), + head_sha=HEAD_SHA, + ) + assert "MEMBER" in body + assert "cannot approve their own" in body + + +def _ctx( + *, + head_repo: str = FORK_REPO, + event_name: str = "pull_request_target", + pr_number: int | None = 29, +) -> GitHubContext: + return GitHubContext( + event_name=event_name, + event_action="opened", + event_path=None, + repository=BASE_REPO, + repository_owner="opendaylight", + server_url="https://github.com", + run_id="1", + sha=HEAD_SHA, + base_ref="master", + head_ref="topic/fix", + pr_number=pr_number, + head_repo=head_repo, + ) + + +class TestForkApprovalGate: + """The gate's decision at the CLI boundary.""" + + def _gate(self, ctx: GitHubContext, pr: Any) -> bool: + from github2gerrit.cli import _check_fork_approval + + with ( + patch("github2gerrit.cli._post_fork_approval_notice"), + patch( + "github2gerrit.cli._is_github_actions_context", + return_value=True, + ), + ): + return _check_fork_approval(pr, ctx)[0] + + def test_direct_cli_invocation_is_not_gated(self) -> None: + """The operator running the CLI is already the authority. + + The gate exists for the unattended path, where the tool acts on + a shared identity with nobody watching. + """ + from github2gerrit.cli import _check_fork_approval + + with ( + patch("github2gerrit.cli._post_fork_approval_notice"), + patch( + "github2gerrit.cli._is_github_actions_context", + return_value=False, + ), + ): + assert _check_fork_approval(_pr([]), _ctx())[0] is True + + def test_same_repo_pr_is_never_gated(self) -> None: + """A same-repo head branch already implies write access.""" + pr = _pr([]) + assert self._gate(_ctx(head_repo=BASE_REPO), pr) is True + + def test_fork_without_approval_blocked(self) -> None: + assert self._gate(_ctx(), _pr([])) is False + + def test_fork_with_approval_allowed(self) -> None: + pr = _pr([_review("APPROVED", "maintainer")]) + assert self._gate(_ctx(), pr) is True + + def test_unresolvable_pr_blocks(self) -> None: + """Fail closed: unknown provenance is not permission.""" + assert self._gate(_ctx(), None) is False + + def test_unresolvable_pr_on_same_repo_still_passes(self) -> None: + assert self._gate(_ctx(head_repo=BASE_REPO), None) is True + + def test_unknown_provenance_is_gated(self) -> None: + """An absent signal is not an answer to a question of authority. + + ``is_fork_pr`` reports ``False`` when provenance is unknown + because it states a fact. The gate uses ``head_is_trusted`` + instead, which reports ``False`` in the same case, so a pull + request whose head could not be resolved is gated rather than + waved through. + """ + assert self._gate(_ctx(head_repo=""), _pr([])) is False + + def test_unknown_provenance_with_approval_passes(self) -> None: + pr = _pr([_review("APPROVED", "maintainer")]) + assert self._gate(_ctx(head_repo=""), pr) is True + + +class TestApprovalNoticeOwnership: + """The marker is not proof of authorship. + + Anyone may paste it into a comment. Ownership is established by + attempting the edit, which the API refuses on another user's + comment. + """ + + def _post(self, comments: list[Any]) -> tuple[bool, list[Any]]: + from github2gerrit.cli import _post_fork_approval_notice + + issue = MagicMock() + issue.get_comments.return_value = comments + pr = MagicMock() + pr.as_issue.return_value = issue + + with ( + patch("github2gerrit.cli.env_bool", return_value=False), + patch("github2gerrit.cli.create_pr_comment") as created, + ): + _post_fork_approval_notice( + pr, + ApprovalStatus(approved=False, reason="no approval"), + HEAD_SHA, + ) + return created.called, comments + + def _marker_comment(self, *, editable: bool) -> Any: + comment = MagicMock() + comment.body = f"{APPROVAL_MARKER}\nolder text" + if not editable: + comment.edit.side_effect = RuntimeError("403 Forbidden") + return comment + + def test_own_notice_is_edited_not_duplicated(self) -> None: + own = self._marker_comment(editable=True) + + created, _ = self._post([own]) + + own.edit.assert_called_once() + assert created is False + + def test_planted_marker_does_not_suppress_the_notice(self) -> None: + """A comment we cannot edit is not ours; post our own.""" + planted = self._marker_comment(editable=False) + + created, _ = self._post([planted]) + + assert created is True + + def test_planted_marker_alongside_our_own(self) -> None: + """Newest first, so our own notice is found and edited.""" + planted = self._marker_comment(editable=False) + own = self._marker_comment(editable=True) + + created, _ = self._post([planted, own]) + + own.edit.assert_called_once() + assert created is False + + def test_no_prior_notice_creates_one(self) -> None: + other = MagicMock() + other.body = "unrelated chatter" + + created, _ = self._post([other]) + + assert created is True + + def test_comment_failure_does_not_raise(self) -> None: + """A block must never become a crash.""" + pr = MagicMock() + pr.as_issue.side_effect = RuntimeError("boom") + + from github2gerrit.cli import _post_fork_approval_notice + + with ( + patch("github2gerrit.cli.env_bool", return_value=False), + patch( + "github2gerrit.cli.create_pr_comment", + side_effect=RuntimeError("boom"), + ), + ): + _post_fork_approval_notice( + pr, + ApprovalStatus(approved=False, reason="no approval"), + HEAD_SHA, + ) + + +class TestApprovedHeadPinning: + """Closing the window between the check and the fetch. + + ``refs/pull//head`` is mutable, so a contributor can push + between the gate reading the head SHA and the workspace fetch + reading the ref. The fetch compares what it got against what was + approved. + """ + + def _enforce(self, approved: str, fetched: str) -> None: + from github2gerrit.core import Orchestrator + + orch = Orchestrator( + workspace=Path("/nonexistent"), approved_sha=approved + ) + + result = MagicMock() + result.stdout = fetched + + with patch("github2gerrit.gitutils.run_cmd", return_value=result): + orch._enforce_approved_head(MagicMock()) + + def test_matching_head_passes(self) -> None: + self._enforce(HEAD_SHA, HEAD_SHA) + + def test_comparison_is_case_insensitive(self) -> None: + self._enforce(HEAD_SHA, HEAD_SHA.upper()) + + def test_moved_head_refuses(self) -> None: + from github2gerrit.core import OrchestratorError + + with pytest.raises(OrchestratorError, match="moved after approval"): + self._enforce(HEAD_SHA, OLD_SHA) + + def test_no_recorded_approval_imposes_no_constraint(self) -> None: + """Same-repo PRs and CLI runs record nothing and are unaffected.""" + self._enforce("", OLD_SHA) + + def test_archive_fallback_is_also_checked(self) -> None: + """The archive path re-reads the PR's current head. + + It must be checked before download: unlike the git path there + is no commit object left to compare afterwards. + """ + from github2gerrit.core import Orchestrator + from github2gerrit.core import OrchestratorError + + orch = Orchestrator( + workspace=Path("/nonexistent"), approved_sha=HEAD_SHA + ) + + orch._assert_archive_sha_approved(HEAD_SHA) + with pytest.raises(OrchestratorError, match="moved after approval"): + orch._assert_archive_sha_approved(OLD_SHA) + + def test_archive_fallback_unconstrained_when_no_gate(self) -> None: + from github2gerrit.core import Orchestrator + + orch = Orchestrator(workspace=Path("/nonexistent")) + orch._assert_archive_sha_approved(OLD_SHA) + + +class TestApprovedShaIsPerPullRequest: + """Bulk runs process several pull requests concurrently. + + The approved commit travels with the pull request rather than + through shared state, so one worker cannot clear or overwrite + another's constraint. + """ + + def _gate(self, ctx: GitHubContext, pr: Any) -> tuple[bool, str]: + from github2gerrit.cli import _check_fork_approval + + with ( + patch("github2gerrit.cli._post_fork_approval_notice"), + patch( + "github2gerrit.cli._is_github_actions_context", + return_value=True, + ), + ): + return _check_fork_approval(pr, ctx) + + def test_approval_returns_the_head(self) -> None: + pr = _pr([_review("APPROVED", "maintainer")]) + assert self._gate(_ctx(), pr) == (True, HEAD_SHA) + + def test_block_returns_no_constraint(self) -> None: + assert self._gate(_ctx(), _pr([])) == (False, "") + + def test_trusted_head_returns_no_constraint(self) -> None: + pr = _pr([]) + assert self._gate(_ctx(head_repo=BASE_REPO), pr) == (True, "") + + def test_nothing_is_written_to_the_environment(self) -> None: + """Shared state is what made concurrent runs unsafe.""" + before = dict(os.environ) + self._gate(_ctx(), _pr([_review("APPROVED", "maintainer")])) + assert os.environ == before + + +class TestApprovalNoticeRetraction: + """A lifted block must stop saying it is blocking.""" + + def _clear(self, comments: list[Any]) -> None: + from github2gerrit.cli import _clear_fork_approval_notice + + issue = MagicMock() + issue.get_comments.return_value = comments + pr = MagicMock() + pr.as_issue.return_value = issue + + with patch("github2gerrit.cli.env_bool", return_value=False): + _clear_fork_approval_notice( + pr, + ApprovalStatus(approved=True, reason="approved by maintainer"), + HEAD_SHA, + ) + + def test_existing_notice_is_retracted(self) -> None: + notice = MagicMock() + notice.body = f"{APPROVAL_MARKER}\nAwaiting maintainer approval" + + self._clear([notice]) + + notice.edit.assert_called_once() + assert "Approved" in notice.edit.call_args[0][0] + + def test_no_notice_creates_nothing(self) -> None: + """A PR that was never blocked has nothing to retract.""" + other = MagicMock() + other.body = "unrelated chatter" + + with patch("github2gerrit.cli.create_pr_comment") as created: + self._clear([other]) + + assert created.called is False + + def test_planted_marker_is_not_edited(self) -> None: + planted = MagicMock() + planted.body = APPROVAL_MARKER + planted.edit.side_effect = RuntimeError("403 Forbidden") + + self._clear([planted]) + + def test_cleared_body_warns_that_pushes_reset_approval(self) -> None: + body = render_cleared_comment( + ApprovalStatus(approved=True, reason="approved by maintainer"), + head_sha=HEAD_SHA, + ) + assert body.startswith(APPROVAL_MARKER) + assert "fresh approval" in body + + +class TestReviewEventOperationMode: + """A review must not create a sibling change.""" + + def test_review_event_maps_to_update(self) -> None: + ctx = _ctx(event_name="pull_request_review") + assert ctx.get_operation_mode() is PROperationMode.UPDATE + + def test_pull_request_events_unchanged(self) -> None: + ctx = _ctx(event_name="pull_request_target") + assert ctx.get_operation_mode() is PROperationMode.CREATE + + +class TestBlockedCommentWording: + """The notice must not assert more than the tool established.""" + + def test_does_not_claim_the_pr_is_from_a_fork(self) -> None: + """Unknown provenance is gated too, and is not known to be a fork.""" + body = render_blocked_comment( + ApprovalStatus(approved=False, reason="no approval"), + head_sha=HEAD_SHA, + ) + assert "comes from a fork" not in body + assert "could not establish" in body + + +class TestReviewTriggersCreateMissing: + """First approval of a fork PR has no change to update.""" + + def _should_create( + self, event_name: str, head_repo: str = FORK_REPO + ) -> bool: + from github2gerrit.core import Orchestrator + + orch = Orchestrator.__new__(Orchestrator) + inputs = MagicMock() + inputs.create_missing = False + gh = _ctx(event_name=event_name, head_repo=head_repo) + + with patch("github2gerrit.core.build_client", side_effect=OSError): + return orch._should_create_missing(inputs, gh)[0] + + def test_review_event_authorises_create(self) -> None: + assert self._should_create("pull_request_review") is True + + def test_reason_names_the_review(self) -> None: + """The notice must not claim a comment or flag triggered it.""" + from github2gerrit.core import Orchestrator + + orch = Orchestrator.__new__(Orchestrator) + inputs = MagicMock() + inputs.create_missing = False + + with patch("github2gerrit.core.build_client", side_effect=OSError): + _ok, reason = orch._should_create_missing( + inputs, _ctx(event_name="pull_request_review") + ) + + assert "approving review" in reason + assert "--create-missing" not in reason + + def test_flag_reason_names_the_flag(self) -> None: + from github2gerrit.core import Orchestrator + + orch = Orchestrator.__new__(Orchestrator) + inputs = MagicMock() + inputs.create_missing = True + + _ok, reason = orch._should_create_missing(inputs, _ctx()) + + assert "--create-missing" in reason + + def test_synchronize_event_does_not(self) -> None: + assert self._should_create("pull_request_target") is False + + def test_same_repo_review_does_not_override_policy(self) -> None: + """A same-repo PR was never gated, so a review is not consent. + + Otherwise any review on any pull request would quietly defeat + ``CREATE_MISSING=false``. + """ + assert ( + self._should_create("pull_request_review", head_repo=BASE_REPO) + is False + ) diff --git a/tests/test_pr_command_authorisation.py b/tests/test_pr_command_authorisation.py index 012f176..123f856 100644 --- a/tests/test_pr_command_authorisation.py +++ b/tests/test_pr_command_authorisation.py @@ -323,7 +323,7 @@ def _run(self, comments: list[Any]) -> bool: patch("github2gerrit.core.get_repo_from_env"), patch("github2gerrit.core.get_pull", return_value=pr), ): - return orch._should_create_missing(inputs, gh) + return orch._should_create_missing(inputs, gh)[0] def test_directive_from_outsider_ignored(self) -> None: assert self._run([_comment(DIRECTIVE, "NONE", "outsider")]) is False diff --git a/tests/test_pr_commands.py b/tests/test_pr_commands.py index df8fd1a..949f05b 100644 --- a/tests/test_pr_commands.py +++ b/tests/test_pr_commands.py @@ -467,7 +467,7 @@ def test_cli_flag_returns_true(self, tmp_path): inputs = self._make_inputs(create_missing=True) gh = self._make_gh() - result = orch._should_create_missing(inputs, gh) + result, _reason = orch._should_create_missing(inputs, gh) assert result is True def test_no_flag_no_comment_returns_false(self, tmp_path): @@ -502,7 +502,7 @@ def test_no_flag_no_comment_returns_false(self, tmp_path): ), patch("github2gerrit.core.get_pull", return_value=mock_pr), ): - result = orch._should_create_missing(inputs, gh) + result, _reason = orch._should_create_missing(inputs, gh) assert result is False def test_comment_directive_returns_true(self, tmp_path): @@ -543,7 +543,7 @@ def test_comment_directive_returns_true(self, tmp_path): ), patch("github2gerrit.core.get_pull", return_value=mock_pr), ): - result = orch._should_create_missing(inputs, gh) + result, _reason = orch._should_create_missing(inputs, gh) assert result is True def test_no_pr_number_returns_false(self, tmp_path): @@ -554,7 +554,7 @@ def test_no_pr_number_returns_false(self, tmp_path): inputs = self._make_inputs(create_missing=False) gh = self._make_gh(pr_number=None) - result = orch._should_create_missing(inputs, gh) + result, _reason = orch._should_create_missing(inputs, gh) assert result is False def test_github_api_failure_returns_false(self, tmp_path): @@ -569,7 +569,7 @@ def test_github_api_failure_returns_false(self, tmp_path): "github2gerrit.core.build_client", side_effect=RuntimeError("API unavailable"), ): - result = orch._should_create_missing(inputs, gh) + result, _reason = orch._should_create_missing(inputs, gh) assert result is False def test_alias_in_comment_returns_true(self, tmp_path): @@ -606,7 +606,7 @@ def test_alias_in_comment_returns_true(self, tmp_path): ), patch("github2gerrit.core.get_pull", return_value=mock_pr), ): - result = orch._should_create_missing(inputs, gh) + result, _reason = orch._should_create_missing(inputs, gh) assert result is True