From a26849e6919a6eb7d64b5d2ec80214f8091ecdfe Mon Sep 17 00:00:00 2001 From: Alexander Lanin Date: Fri, 28 Aug 2026 09:03:01 +0200 Subject: [PATCH 1/6] feat(repo-policy-sync): discover policy PRs by label instead of branch head PR ownership lookup used `gh pr list --head ` once per candidate branch. This tied ownership to the branch-naming scheme, so a policy rename or a previous tool's differently-named branches were invisible to lookup. Query PRs once via the `repo-policy-sync` label, then match ownership by the marker in the PR body. Add a legacy `repo-sync` marker/label lookup so PRs created by the tool's predecessor are still recognized, and record former policy IDs (`legacy_names`) for policies that were renamed during that migration. Also report the discovered policy PR state (open/merged/closed/number) in the terminal table, not just in JSON/Markdown output, and request it for `plan` runs so it is visible without writing a report file. Move repo_policy_sync/operations/README.md to src/operations/ alongside the operation modules it documents. --- repo_policy_sync/README.md | 14 +- .../docs/explanation/architecture.md | 4 +- .../docs/explanation/pull-request-safety.md | 22 ++- repo_policy_sync/docs/how-to/run-a-policy.md | 5 +- repo_policy_sync/docs/reference/cli.md | 4 +- .../docs/reference/policy-format.md | 7 +- .../docs-as-code-gitignore/policy.yml | 5 +- .../policy.yml | 3 + .../policies/minimum-bazel-version/policy.yml | 1 + repo_policy_sync/src/cli.py | 4 +- repo_policy_sync/src/github.py | 155 ++++++++++-------- .../{ => src}/operations/README.md | 0 repo_policy_sync/src/reporting.py | 50 +++++- repo_policy_sync/src/runner.py | 27 +-- repo_policy_sync/tests/test_cli.py | 17 ++ repo_policy_sync/tests/test_github.py | 68 ++++---- repo_policy_sync/tests/test_reporting.py | 49 ++++++ repo_policy_sync/tests/test_runner.py | 37 +++++ 18 files changed, 328 insertions(+), 144 deletions(-) rename repo_policy_sync/{ => src}/operations/README.md (100%) diff --git a/repo_policy_sync/README.md b/repo_policy_sync/README.md index acb031e..6452577 100644 --- a/repo_policy_sync/README.md +++ b/repo_policy_sync/README.md @@ -87,7 +87,9 @@ Policy options can be kept in the optional `score-repo-policy-sync.toml` file. Explicit CLI values override the file; see the [configuration reference](docs/reference/configuration.md). -The CLI always prints a compact table to standard output. Pass +The CLI always prints a compact table to standard output, including the +discovered policy pull-request state (`open`, `merged`, `closed`, or `none`) +and number. Pass `--json-output PATH` and/or `--markdown-output PATH` to write additional versioned JSON and Markdown reports during the same policy run. Markdown is suited for pull requests, issues, and wikis. Its cells use `✅` for compliant, @@ -153,11 +155,11 @@ policy concurrency, and recovery from stale or conflicting policy branches. Compatibility notes: -- Callers must use `score-repo-policy-sync`, `--policy-dir`, the current policy - IDs, and `repo-policy-sync/` branches. Legacy command aliases, - `--policy-directory`, and historical policy IDs are not supported. -- Renaming a policy changes its branch and pull-request identity. Existing - policy branches or pull requests must be handled before adopting the new ID. +- Callers must use `score-repo-policy-sync` and `--policy-dir`; legacy command + aliases and `--policy-directory` are not supported. +- Policy migrations can list historical policy IDs in `legacy_names`. Matching + labelled historical pull requests are reported and can only be modified when + their body contains a verifiable branch-head marker. - The first version intentionally does not provide dynamic operation plugins, persistent result storage, generalized retries/rate-limit handling, or non-GitHub providers. diff --git a/repo_policy_sync/docs/explanation/architecture.md b/repo_policy_sync/docs/explanation/architecture.md index 96b8b1a..8d10c78 100644 --- a/repo_policy_sync/docs/explanation/architecture.md +++ b/repo_policy_sync/docs/explanation/architecture.md @@ -79,8 +79,8 @@ pre-authenticated `gh` CLI and Git. applicability and compliance. - Policy operations use repository-relative paths and are idempotent. - Each policy maps to one deterministic branch per repository. -- A branch is treated as tool-owned only when its open pull request contains - the policy marker. +- A labelled pull request is associated with a policy only when its body + contains the policy marker. - An apply run updates the owned pull request’s title and body from the current policy before pushing any required commit. diff --git a/repo_policy_sync/docs/explanation/pull-request-safety.md b/repo_policy_sync/docs/explanation/pull-request-safety.md index 8252b3f..9a60f62 100644 --- a/repo_policy_sync/docs/explanation/pull-request-safety.md +++ b/repo_policy_sync/docs/explanation/pull-request-safety.md @@ -18,18 +18,22 @@ The policy directory name is the policy ID and maps to the deterministic branch `repo-policy-sync/`, so subsequent runs update the same pull request instead of creating duplicates. -The first version recognizes only the current policy ID and the -`repo-policy-sync/` branch and marker. Renaming a policy -is a breaking change: update any existing policy branch or pull request before -using the new ID. +PR discovery uses the `repo-policy-sync` label and then matches the policy +marker in the body. A policy can list former IDs in `legacy_names`, allowing +historical branch names and the former `repo-sync` marker format to remain +visible during a policy migration. New pull requests always use the current +policy ID and branch scheme. ## Ownership and safety -The PR body contains invisible markers for the policy ID and the branch head -created by Repository Policy Sync. Before reusing a branch, the tool verifies -that its remote head still matches the stored value. A missing marker or a -mismatch fails the run without updating, closing, or otherwise taking over the -pull request. This protects a policy branch that someone has changed manually. +The label identifies the tool and the PR body contains an invisible marker for +the policy. The body may also contain a marker for the branch head created by +Repository Policy Sync. Before reusing or closing a PR in apply mode, the tool +verifies that its remote head still matches the stored value. A missing head +marker or a mismatch fails the run without updating, closing, or otherwise +taking over the pull request. This protects a policy branch that someone has +changed manually. Plan mode can report a matching historical PR even when its +old body has no head marker. If the refreshed default branch is compliant while an owned PR remains open, apply mode closes that PR after verifying the same branch-head marker used for diff --git a/repo_policy_sync/docs/how-to/run-a-policy.md b/repo_policy_sync/docs/how-to/run-a-policy.md index 511e279..b713b8c 100644 --- a/repo_policy_sync/docs/how-to/run-a-policy.md +++ b/repo_policy_sync/docs/how-to/run-a-policy.md @@ -13,8 +13,9 @@ # How to run a policy -Use plan mode first. It refreshes disposable local checkouts and reports drift, -but never changes remote repositories: +Use plan mode first. It refreshes disposable local checkouts, reports drift, +and includes the status of matching policy pull requests, but never changes +remote repositories: ```bash uv run score-repo-policy-sync plan --org eclipse-score diff --git a/repo_policy_sync/docs/reference/cli.md b/repo_policy_sync/docs/reference/cli.md index ca6207f..809a314 100644 --- a/repo_policy_sync/docs/reference/cli.md +++ b/repo_policy_sync/docs/reference/cli.md @@ -27,7 +27,7 @@ are CLI-only. | Command | Description | | --- | --- | -| `plan` | Evaluate policies without changing repositories. Exit status `1` indicates drift. | +| `plan` | Evaluate policies and report matching policy pull requests without changing repositories. Exit status `1` indicates drift. | | `apply` | Apply policies and create or update policy-owned pull requests. | ## Typical @@ -37,7 +37,7 @@ are CLI-only. | `--org NAME` | GitHub organization to scan. May be set in TOML. | | `--policy NAME` | Select a policy by directory name from local or bundled policies. Repeat to select more than one; when present, only the selected policies run. Defaults to all local and bundled policies. | | `--repo NAME` | Restrict the run to an exact repository name. Repeat to select more than one. | -| *(stdout)* | Always prints the terminal policy-evaluation table. | +| *(stdout)* | Always prints the terminal policy-evaluation table, including policy pull-request state and number. | ## Rare diff --git a/repo_policy_sync/docs/reference/policy-format.md b/repo_policy_sync/docs/reference/policy-format.md index 3ea9252..171ca49 100644 --- a/repo_policy_sync/docs/reference/policy-format.md +++ b/repo_policy_sync/docs/reference/policy-format.md @@ -60,8 +60,11 @@ ensure: `title` and a non-empty `ensure` list are required. `description`, `legacy_names`, and `when` are optional. `legacy_names` is a list of former -policy IDs whose branches and pull-request markers are still recognized for -this policy. New pull requests use the current directory name and marker. +policy IDs whose pull-request markers are still recognized for this policy. +Pull requests are located through the `repo-policy-sync` label and then matched +by marker, so this also supports historical branch names and the former +`repo-sync` marker format. New pull requests use the current directory name +and marker. Paths must be non-empty and repository-relative; absolute paths and paths containing `..` are rejected. diff --git a/repo_policy_sync/policies/docs-as-code-gitignore/policy.yml b/repo_policy_sync/policies/docs-as-code-gitignore/policy.yml index ba80e8d..3d9bd0b 100644 --- a/repo_policy_sync/policies/docs-as-code-gitignore/policy.yml +++ b/repo_policy_sync/policies/docs-as-code-gitignore/policy.yml @@ -11,7 +11,10 @@ # SPDX-License-Identifier: Apache-2.0 # ******************************************************************************* title: "chore(docs): update score_docs_as_code gitignore configuration" -legacy_names: [docs-as-code-legacy-configuration-removal] +legacy_names: + - docs-as-code-legacy-configuration-removal + - score-docs-as-code.gitignore-and-cleanup + - score-docs-as-code.legacy-documentation-cleanup description: > Update the `.gitignore` configuration for `score_docs_as_code`. diff --git a/repo_policy_sync/policies/minimal-bazel-module-declaration/policy.yml b/repo_policy_sync/policies/minimal-bazel-module-declaration/policy.yml index d98f24f..b467cf1 100644 --- a/repo_policy_sync/policies/minimal-bazel-module-declaration/policy.yml +++ b/repo_policy_sync/policies/minimal-bazel-module-declaration/policy.yml @@ -11,6 +11,9 @@ # SPDX-License-Identifier: Apache-2.0 # ******************************************************************************* title: "chore(bazel): use a minimal module declaration" +legacy_names: + - bazel.minimal-module-declaration + - bazel.module-one-line description: Keep the Bazel module declaration limited to its repository-owned name. when: diff --git a/repo_policy_sync/policies/minimum-bazel-version/policy.yml b/repo_policy_sync/policies/minimum-bazel-version/policy.yml index 8645024..ad1f23b 100644 --- a/repo_policy_sync/policies/minimum-bazel-version/policy.yml +++ b/repo_policy_sync/policies/minimum-bazel-version/policy.yml @@ -11,6 +11,7 @@ # SPDX-License-Identifier: Apache-2.0 # ******************************************************************************* title: "chore(bazel): keep the declared version at 8.6.0 or newer" +legacy_names: [bazel.minimum-version] description: | Bazel versions older than `8.6.0` are no longer supported, due to a breaking change in how they handle `compatibility_level`. diff --git a/repo_policy_sync/src/cli.py b/repo_policy_sync/src/cli.py index 63bd1a1..babe8f4 100644 --- a/repo_policy_sync/src/cli.py +++ b/repo_policy_sync/src/cli.py @@ -286,7 +286,9 @@ def main(argv: Sequence[str] | None = None) -> int: sync_workers=sync_workers, policy_workers=policy_workers, include_pull_request_status=( - args.json_output is not None or args.markdown_output is not None + args.command == "plan" + or args.json_output is not None + or args.markdown_output is not None ), progress=_discard_progress if quiet else _write_progress, ) diff --git a/repo_policy_sync/src/github.py b/repo_policy_sync/src/github.py index d7a617d..4dfa266 100644 --- a/repo_policy_sync/src/github.py +++ b/repo_policy_sync/src/github.py @@ -28,6 +28,10 @@ from .models import Change, Policy, policy_branch_slug TOOL_SLUG = "repo-policy-sync" +# PRs created by the previous implementation used this slug in their body and +# branch names. Keep accepting it while the label remains the stable tool +# ownership signal. +LEGACY_TOOL_SLUG = "repo-sync" AUTOMATION_LABELS = ("automation", TOOL_SLUG) AUTOMATION_LABEL_COLOR = "EDEDED" _PRE_COMMIT_ENVIRONMENT_KEYS = { @@ -150,90 +154,90 @@ def _find_policy_pull_requests( legacy_policy_ids: tuple[str, ...], state: str, ) -> tuple[PullRequest, ...]: - """Find policy-owned PRs in one GitHub state across its branch.""" + """Find policy-owned PRs in one GitHub state using the tool label.""" owned: list[PullRequest] = [] - accepted_markers = { - _policy_marker(identifier) for identifier in (policy_id, *legacy_policy_ids) - } + accepted_markers = _policy_markers((policy_id, *legacy_policy_ids)) fields = ( - "number,url,body,mergedAt" + "number,url,body,headRefName,mergedAt" if state == "merged" - else "number,url,body,mergeable" + else "number,url,body,headRefName,mergeable" ) - for branch in branches: - output = self._run( - [ - "gh", - "pr", - "list", - "--repo", - repository, - "--head", - branch, - "--state", - state, - "--json", - fields, - ] + output = self._run( + [ + "gh", + "pr", + "list", + "--repo", + repository, + "--label", + TOOL_SLUG, + "--state", + state, + "--limit", + "1000", + "--json", + fields, + ] + ) + try: + pull_requests = json.loads(output) + except json.JSONDecodeError as exc: + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" + ) from exc + if not isinstance(pull_requests, list): + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" ) - try: - pull_requests = json.loads(output) - except json.JSONDecodeError as exc: + for pull_request in pull_requests: + if not isinstance(pull_request, dict): raise CommandError( f"gh returned invalid pull-request JSON for {repository}" - ) from exc - if not isinstance(pull_requests, list): + ) + raw_body = pull_request.get("body", "") + body = "" if raw_body is None else raw_body + branch = pull_request.get("headRefName") + if not isinstance(body, str) or not isinstance(branch, str) or not branch: raise CommandError( f"gh returned invalid pull-request JSON for {repository}" ) - for pull_request in pull_requests: - if not isinstance(pull_request, dict): - raise CommandError( - f"gh returned invalid pull-request JSON for {repository}" - ) - raw_body = pull_request.get("body", "") - body = "" if raw_body is None else raw_body - if not isinstance(body, str): - raise CommandError( - f"gh returned invalid pull-request JSON for {repository}" - ) - if not any(marker in body for marker in accepted_markers): - if state == "merged": - # Merged history may contain an unrelated PR from a - # previous branch user; only an open PR can block reuse. - continue + if not any(marker in body for marker in accepted_markers): + if state == "open" and branch in branches: raise CommandError( f"refusing to reuse {repository} branch {branch}: its {state} pull request " f"is not owned by policy {policy_id}" ) - number = pull_request.get("number") - url = pull_request.get("url") - if not isinstance(number, int) or not isinstance(url, str): - raise CommandError( - f"gh returned invalid pull-request JSON for {repository}" - ) - merged_at = pull_request.get("mergedAt") - if merged_at is not None and not isinstance(merged_at, str): - raise CommandError( - f"gh returned invalid pull-request JSON for {repository}" - ) - mergeable = pull_request.get("mergeable") - if mergeable is not None and not isinstance(mergeable, str): - raise CommandError( - f"gh returned invalid pull-request JSON for {repository}" - ) - owned.append( - PullRequest( - number=number, - url=url, - expected_head_oid=_policy_head_marker_from_body(body), - branch=branch, - merged_at=merged_at, - body=body, - mergeable=mergeable, - ) + # A labelled PR for another policy is expected in a repository + # that is managed by the tool; it is not ours to reuse. + continue + number = pull_request.get("number") + url = pull_request.get("url") + if not isinstance(number, int) or not isinstance(url, str): + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" + ) + merged_at = pull_request.get("mergedAt") + if merged_at is not None and not isinstance(merged_at, str): + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" ) + mergeable = pull_request.get("mergeable") + if mergeable is not None and not isinstance(mergeable, str): + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" + ) + owned.append( + PullRequest( + number=number, + url=url, + expected_head_oid=_policy_head_marker_from_body(body), + branch=branch, + merged_at=merged_at, + body=body, + mergeable=mergeable, + ) + ) return tuple(owned) def switch_to_policy_branch( @@ -625,12 +629,25 @@ def _policy_marker(policy_id: str) -> str: return f"" +def _policy_markers(policy_ids: tuple[str, ...]) -> tuple[str, ...]: + """Return current and historical ownership markers for policy IDs.""" + + return tuple( + f"" + for tool_slug in (TOOL_SLUG, LEGACY_TOOL_SLUG) + for policy_id in policy_ids + ) + + def _policy_head_marker(head_oid: str) -> str: return f"" def _policy_head_marker_from_body(body: str) -> str | None: - match = re.search(rf"", body) + tool_slugs = "|".join( + re.escape(tool_slug) for tool_slug in (TOOL_SLUG, LEGACY_TOOL_SLUG) + ) + match = re.search(rf"", body) return match.group(1) if match else None diff --git a/repo_policy_sync/operations/README.md b/repo_policy_sync/src/operations/README.md similarity index 100% rename from repo_policy_sync/operations/README.md rename to repo_policy_sync/src/operations/README.md diff --git a/repo_policy_sync/src/reporting.py b/repo_policy_sync/src/reporting.py index 4f75f1d..a0c7d76 100644 --- a/repo_policy_sync/src/reporting.py +++ b/repo_policy_sync/src/reporting.py @@ -31,7 +31,7 @@ def render_table(report: RunReport) -> str: ( outcome.policy_id, outcome.repository, - _status_label(outcome.status), + _status_label(outcome), _actions_label(outcome), ) for outcome in report.outcomes @@ -44,7 +44,7 @@ def render_table(report: RunReport) -> str: _render_box_table( ("Policy", "Repository", "Status", "Actions"), rows, - column_limits=(24, 24, 32, 48), + column_limits=(24, 24, 44, 48), ), ] lines.extend(_summary_lines(report)) @@ -389,8 +389,8 @@ def _failure_table(title: str, failures: tuple[RepositoryOutcome, ...]) -> list[ return lines -def _status_label(status: str) -> str: - return { +def _status_label(outcome: RepositoryOutcome) -> str: + label = { "compliant": "✅", "changes-required": _changes_required_marker(), "not-applicable": "⚪", @@ -402,7 +402,14 @@ def _status_label(status: str) -> str: "pull-request-recreated": "♻ pull request recreated", "pull-request-recreated-no-changes": "♻ pull request recreated (no changes)", "pull-request-closed": "✅ pull request closed", - }.get(status, status) + }.get(outcome.status, outcome.status) + if _matching_pull_request_state_should_replace_compliance(outcome): + return _pull_request_label(outcome) + if outcome.status.startswith("pull-request-") and outcome.pull_request_url: + number = outcome.pull_request_url.rstrip("/").rsplit("/", 1)[-1] + if number.isdigit(): + return f"{label} #{number}" + return label def _changes_required_marker() -> str: @@ -413,13 +420,42 @@ def _actions_label(outcome: RepositoryOutcome) -> str: if outcome.error: return outcome.error actions = _format_changes(outcome.changes) - if outcome.pull_request_url: - actions = f"{actions}; PR: {outcome.pull_request_url}" if outcome.warnings: actions = f"{actions}; {'; '.join(outcome.warnings)}" return actions or "-" +def _pull_request_label(outcome: RepositoryOutcome) -> str: + """Render the discovered policy PR state and number for terminal users.""" + + labels = { + "open": "🔄 open", + "merged": "🔗 merged", + "closed": "✅ closed", + "none": "— no PR", + } + if outcome.policy_pr_status is None: + return "? not checked" + label = labels.get(outcome.policy_pr_status, outcome.policy_pr_status) + if outcome.pull_request_url: + number = outcome.pull_request_url.rstrip("/").rsplit("/", 1)[-1] + if number.isdigit(): + return f"{label} #{number}" + return label + + +def _matching_pull_request_state_should_replace_compliance( + outcome: RepositoryOutcome, +) -> bool: + """Return whether the PR state is the useful status for this outcome.""" + + if outcome.policy_pr_status == "open": + return outcome.status == "changes-required" + if outcome.policy_pr_status in {"merged", "closed"}: + return outcome.status in {"compliant", "pull-request-closed"} + return False + + def _format_changes(changes: tuple[Change, ...]) -> str: by_path: dict[str, list[str]] = {} for change in changes: diff --git a/repo_policy_sync/src/runner.py b/repo_policy_sync/src/runner.py index f95c5a2..4804e98 100644 --- a/repo_policy_sync/src/runner.py +++ b/repo_policy_sync/src/runner.py @@ -414,6 +414,15 @@ def _run_repository( include_pull_request_status: bool = False, ) -> RepositoryOutcome: full_name = f"{org}/{repository}" + policy_pr_status = ( + _find_policy_pull_request_status( + client=client, + repository=full_name, + policy=policy, + ) + if include_pull_request_status + else None + ) try: evaluation = evaluate_policy(checkout, policy, organization=org) except RepoPolicySyncError as exc: @@ -471,7 +480,14 @@ def _run_repository( pull_request_url=existing_pr.url, policy_pr_status="closed", ) - return RepositoryOutcome(repository, policy.id, "no (live)", "not-applicable") + return RepositoryOutcome( + repository, + policy.id, + "no (live)", + "not-applicable", + pull_request_url=_policy_pr_url(policy_pr_status), + policy_pr_status=_policy_pr_label(policy_pr_status), + ) if recreate: return _recreate_repository( client=client, @@ -482,15 +498,6 @@ def _run_repository( checkout=checkout, allow_dirty_pr=allow_dirty_pr, ) - policy_pr_status = ( - _find_policy_pull_request_status( - client=client, - repository=full_name, - policy=policy, - ) - if include_pull_request_status - else None - ) if not evaluation.changes: existing_pr = ( policy_pr_status.open diff --git a/repo_policy_sync/tests/test_cli.py b/repo_policy_sync/tests/test_cli.py index 299377c..0381b5b 100644 --- a/repo_policy_sync/tests/test_cli.py +++ b/repo_policy_sync/tests/test_cli.py @@ -175,6 +175,23 @@ def test_json_report_requests_pull_request_status( capsys.readouterr() +def test_plan_requests_pull_request_status_for_terminal_report( + monkeypatch, capsys +) -> None: + observed = {} + monkeypatch.setattr(cli, "load_policies", lambda _: ()) + monkeypatch.setattr( + cli, + "run_policies", + lambda **kwargs: observed.update(kwargs) or _empty_report(), + ) + + assert cli.main(("plan", "--org", "eclipse-score", "--quiet")) == 0 + + assert observed["include_pull_request_status"] is True + capsys.readouterr() + + def test_recreate_selects_only_the_requested_bundled_policy( monkeypatch, tmp_path: Path ) -> None: diff --git a/repo_policy_sync/tests/test_github.py b/repo_policy_sync/tests/test_github.py index 05c76e6..a2d4496 100644 --- a/repo_policy_sync/tests/test_github.py +++ b/repo_policy_sync/tests/test_github.py @@ -838,18 +838,19 @@ def test_policy_pull_request_without_head_marker_is_not_safe_to_reuse( branch = policy_branches(policy)[0] def run(command: list[str]) -> str: - if command[command.index("--head") + 1] == branch: - return json.dumps( - [ - { - "number": 1, - "url": "https://github.example/owner/repo/pull/1", - "body": "", - "mergeable": "CONFLICTING", - } - ] - ) - return "[]" + assert command[command.index("--label") + 1] == "repo-policy-sync" + assert command[command.index("--limit") + 1] == "1000" + return json.dumps( + [ + { + "number": 1, + "url": "https://github.example/owner/repo/pull/1", + "body": "", + "headRefName": branch, + "mergeable": "CONFLICTING", + } + ] + ) monkeypatch.setattr(GitHubCli, "_run", staticmethod(run)) @@ -869,18 +870,17 @@ def test_pre_existing_user_pull_request_is_not_reused(monkeypatch) -> None: branch = policy_branches(policy)[0] def run(command: list[str]) -> str: - if command[command.index("--head") + 1] == branch: - return json.dumps( - [ - { - "number": 1, - "url": "https://github.example/owner/repo/pull/1", - "body": "A pull request opened by a maintainer.", - "mergeable": "MERGEABLE", - } - ] - ) - return "[]" + return json.dumps( + [ + { + "number": 1, + "url": "https://github.example/owner/repo/pull/1", + "body": "A pull request opened by a maintainer.", + "headRefName": branch, + "mergeable": "MERGEABLE", + } + ] + ) monkeypatch.setattr(GitHubCli, "_run", staticmethod(run)) @@ -904,19 +904,18 @@ def test_legacy_policy_pull_request_is_recognized_on_its_old_branch( legacy_names=("old-policy",), ) branches = policy_branches(policy) + old_branch = "repo-sync/old-policy" def run(command: list[str]) -> str: - branch = command[command.index("--head") + 1] - if branch == branches[0]: - return "[]" - assert branch == branches[1] + assert command[command.index("--label") + 1] == "repo-policy-sync" return json.dumps( [ { "number": 1, "url": "https://github.example/owner/repo/pull/1", - "body": "\n" - "", + "body": "\n" + "", + "headRefName": old_branch, "mergeable": "MERGEABLE", } ] @@ -932,7 +931,7 @@ def run(command: list[str]) -> str: ) assert pull_request is not None - assert pull_request.branch == branches[1] + assert pull_request.branch == old_branch def test_policy_pull_request_status_includes_latest_merged_pull_request( @@ -942,7 +941,6 @@ def test_policy_pull_request_status_includes_latest_merged_pull_request( branch = policy_branches(policy)[0] def run(command: list[str]) -> str: - assert command[command.index("--head") + 1] == branch state = command[command.index("--state") + 1] if state == "open": return json.dumps( @@ -952,6 +950,7 @@ def run(command: list[str]) -> str: "url": "https://github.example/owner/repo/pull/3", "body": "\n" "", + "headRefName": branch, } ] ) @@ -962,12 +961,14 @@ def run(command: list[str]) -> str: "url": "https://github.example/owner/repo/pull/2", "body": "\n" "", + "headRefName": branch, "mergedAt": "2026-01-01T00:00:00Z", }, { "number": 99, "url": "https://github.example/owner/repo/pull/99", "body": "a historical PR owned by another tool", + "headRefName": "other-tool/branch", "mergedAt": "2026-02-01T00:00:00Z", }, ] @@ -997,18 +998,19 @@ def test_multiple_policy_pull_requests_fail_instead_of_choosing(monkeypatch) -> ) def run(command: list[str]) -> str: - assert command[command.index("--head") + 1] == branch return json.dumps( [ { "number": 1, "url": "https://github.example/owner/repo/pull/1", "body": body, + "headRefName": branch, }, { "number": 2, "url": "https://github.example/owner/repo/pull/2", "body": body, + "headRefName": "repo-sync/example", }, ] ) diff --git a/repo_policy_sync/tests/test_reporting.py b/repo_policy_sync/tests/test_reporting.py index 2d4151b..f52b0cd 100644 --- a/repo_policy_sync/tests/test_reporting.py +++ b/repo_policy_sync/tests/test_reporting.py @@ -157,6 +157,55 @@ def test_render_table_uses_red_changes_required_marker() -> None: assert "🔴" in output +def test_render_table_shows_pull_request_state_and_number_in_status() -> None: + report = _report() + outcome = report.outcomes[0] + report = RunReport( + summary=report.summary, + outcomes=( + RepositoryOutcome( + outcome.repository, + outcome.policy_id, + outcome.when, + outcome.status, + changes=outcome.changes, + pull_request_url="https://github.example/owner/repo/pull/1", + policy_pr_status="open", + ), + ), + ) + + output = render_table(report) + + assert "Pull request" not in output + assert "🔄 open #1" in output + assert "https://github.example/owner/repo/pull/1" not in output + + +def test_render_table_keeps_compliance_status_for_mismatched_pull_request() -> None: + report = _report() + outcome = report.outcomes[0] + report = RunReport( + summary=report.summary, + outcomes=( + RepositoryOutcome( + outcome.repository, + outcome.policy_id, + "yes (live)", + "compliant", + pull_request_url="https://github.example/owner/repo/pull/1", + policy_pr_status="open", + ), + ), + ) + + output = render_table(report).split("\n\n📊 Summary", 1)[0] + + assert "│ ✅ │" in output + assert "open" not in output + assert "#1" not in output + + def test_render_json_is_machine_readable_and_versioned() -> None: output = render_json(_report()) diff --git a/repo_policy_sync/tests/test_runner.py b/repo_policy_sync/tests/test_runner.py index 79d672e..f9a1695 100644 --- a/repo_policy_sync/tests/test_runner.py +++ b/repo_policy_sync/tests/test_runner.py @@ -31,6 +31,7 @@ from repo_policy_sync.src.models import ( BazelCondition, EnsureLine, + FileExistsCondition, RemoveFile, Evaluation, Policy, @@ -371,6 +372,42 @@ def test_runner_adds_policy_pull_request_status_for_markdown_reports( assert report.outcomes[0].pull_request_url.endswith("/7") +def test_runner_reports_pull_request_status_when_policy_is_not_applicable( + tmp_path: Path, +) -> None: + """Plan reports an existing PR even when the live policy no longer applies.""" + policy = Policy( + "example", + "Example", + None, + None, + (), + file_exists_condition=FileExistsCondition(Path("does-not-exist")), + ) + client = PolicyStatusClient( + tmp_path, + (Repository("repo", "main"),), + PolicyPullRequestStatus( + open=PullRequest(1, "https://github.example/owner/repo/pull/1") + ), + ) + + outcome = _run_repository( + client=client, + org="eclipse-score", + repository="repo", + default_branch="main", + policy=policy, + checkout=tmp_path, + apply=False, + include_pull_request_status=True, + ) + + assert outcome.status == "not-applicable" + assert outcome.policy_pr_status == "open" + assert outcome.pull_request_url == "https://github.example/owner/repo/pull/1" + + def test_runner_counts_closed_pull_requests_as_compliant( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From 2d964e81500b90e84df78953380e2c936aade283 Mon Sep 17 00:00:00 2001 From: Alexander Lanin Date: Fri, 28 Aug 2026 09:23:59 +0200 Subject: [PATCH 2/6] docs(repo-policy-sync): explain why apply skips the merged-PR prefetch --- repo_policy_sync/src/cli.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/repo_policy_sync/src/cli.py b/repo_policy_sync/src/cli.py index babe8f4..47db12d 100644 --- a/repo_policy_sync/src/cli.py +++ b/repo_policy_sync/src/cli.py @@ -285,6 +285,10 @@ def main(argv: Sequence[str] | None = None) -> int: allow_dirty_pr=allow_dirty_pr, sync_workers=sync_workers, policy_workers=policy_workers, + # apply already looks up an open PR itself whenever it might act on + # one, so this only buys the extra merged-PR history used for + # display. Skip that extra `gh pr list` call in a bare apply run; + # still fetch it for plan (its only output) and for report files. include_pull_request_status=( args.command == "plan" or args.json_output is not None From 973ff1f0f739e8edaeb68da9c1ce0b3b2813e5b5 Mon Sep 17 00:00:00 2001 From: Alexander Lanin Date: Fri, 28 Aug 2026 13:26:20 +0200 Subject: [PATCH 3/6] fix(repo-policy-sync): restore branch-collision guard and closed-PR status The label-based PR lookup cannot see a PR that never got the tool label (a maintainer's own PR on the expected branch, or a tool-created PR whose label application silently failed), so it could push over it or open a duplicate. Restore a per-branch collision check independent of the label filter, and make applying the tool label in create_pull_request mandatory instead of a swallowed warning, since that label is the only signal later runs use to recognize ownership. Also propagate the newly-tracked closed PR state through runner.py's policy_pr_status/url helpers, which previously could only report open/merged/none even though PolicyPullRequestStatus.closed and the reporting/README text already assumed a "closed" value existed. Update the README's first-version compatibility note, which still claimed no historical policy-ID compatibility while the same PR had already added legacy_names support for renamed policies. --- repo_policy_sync/README.md | 6 +- repo_policy_sync/src/github.py | 102 ++++++++++++++++++++++-- repo_policy_sync/src/runner.py | 4 +- repo_policy_sync/tests/test_github.py | 107 ++++++++++++++++++++++---- repo_policy_sync/tests/test_runner.py | 37 +++++++++ 5 files changed, 234 insertions(+), 22 deletions(-) diff --git a/repo_policy_sync/README.md b/repo_policy_sync/README.md index 6452577..c659d46 100644 --- a/repo_policy_sync/README.md +++ b/repo_policy_sync/README.md @@ -142,8 +142,10 @@ quadrants: The supported executable is `score-repo-policy-sync`. Policy IDs are the directory names containing each `policy.yml`, and policy-owned branches use the `repo-policy-sync/` naming scheme. The first version does not -provide command aliases or historical policy-ID compatibility; update callers -to the supported command and current policy IDs before rollout. +provide command aliases; update callers to the supported command before +rollout. Renaming a policy is supported by listing its former directory name +in `legacy_names` (see Compatibility notes below) so the existing branch or +pull request is located and updated instead of duplicated. ## First-version change summary diff --git a/repo_policy_sync/src/github.py b/repo_policy_sync/src/github.py index 4dfa266..41926be 100644 --- a/repo_policy_sync/src/github.py +++ b/repo_policy_sync/src/github.py @@ -56,6 +56,7 @@ class PullRequest: warnings: tuple[str, ...] = () branch: str = "" merged_at: str | None = None + closed_at: str | None = None body: str | None = None mergeable: str | None = None @@ -66,6 +67,7 @@ class PolicyPullRequestStatus: open: PullRequest | None = None merged: PullRequest | None = None + closed: PullRequest | None = None @dataclass(frozen=True) @@ -111,7 +113,7 @@ def find_policy_pull_request_status( policy_id: str, legacy_policy_ids: tuple[str, ...] = (), ) -> PolicyPullRequestStatus: - """Find the open PR and latest merged PR owned by a repository policy.""" + """Find the open, latest merged, and latest closed PR owned by a policy.""" open_pull_requests = self._find_policy_pull_requests( repository=repository, @@ -140,9 +142,25 @@ def find_policy_pull_request_status( ), default=None, ) + closed_pull_requests = self._find_policy_pull_requests( + repository=repository, + branches=branches, + policy_id=policy_id, + legacy_policy_ids=legacy_policy_ids, + state="closed", + ) + latest_closed = max( + closed_pull_requests, + key=lambda pull_request: ( + pull_request.closed_at or "", + pull_request.number, + ), + default=None, + ) return PolicyPullRequestStatus( open=open_pull_requests[0] if open_pull_requests else None, merged=latest_merged, + closed=latest_closed, ) def _find_policy_pull_requests( @@ -158,11 +176,12 @@ def _find_policy_pull_requests( owned: list[PullRequest] = [] accepted_markers = _policy_markers((policy_id, *legacy_policy_ids)) - fields = ( - "number,url,body,headRefName,mergedAt" - if state == "merged" - else "number,url,body,headRefName,mergeable" - ) + if state == "merged": + fields = "number,url,body,headRefName,mergedAt" + elif state == "closed": + fields = "number,url,body,headRefName,closedAt" + else: + fields = "number,url,body,headRefName,mergeable" output = self._run( [ "gh", @@ -222,6 +241,11 @@ def _find_policy_pull_requests( raise CommandError( f"gh returned invalid pull-request JSON for {repository}" ) + closed_at = pull_request.get("closedAt") + if closed_at is not None and not isinstance(closed_at, str): + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" + ) mergeable = pull_request.get("mergeable") if mergeable is not None and not isinstance(mergeable, str): raise CommandError( @@ -234,12 +258,71 @@ def _find_policy_pull_requests( expected_head_oid=_policy_head_marker_from_body(body), branch=branch, merged_at=merged_at, + closed_at=closed_at, body=body, mergeable=mergeable, ) ) + if state == "open": + self._verify_no_unlabeled_branch_collision( + repository=repository, + branches=branches, + owned_branches={pull_request.branch for pull_request in owned}, + policy_id=policy_id, + ) return tuple(owned) + def _verify_no_unlabeled_branch_collision( + self, + *, + repository: str, + branches: tuple[str, ...], + owned_branches: set[str], + policy_id: str, + ) -> None: + """Refuse to reuse an expected branch whose open PR lacks the tool label. + + The `--label` lookup above cannot see a PR that never got the tool + label: a maintainer's own PR on the branch, or a tool-created PR whose + label application failed. Without this check either would be silently + invisible and a later apply could push over it or attempt a duplicate + pull request instead of refusing safely. + """ + + for branch in branches: + if branch in owned_branches: + continue + output = self._run( + [ + "gh", + "pr", + "list", + "--repo", + repository, + "--head", + branch, + "--state", + "open", + "--json", + "number", + ] + ) + try: + pull_requests = json.loads(output) + except json.JSONDecodeError as exc: + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" + ) from exc + if not isinstance(pull_requests, list): + raise CommandError( + f"gh returned invalid pull-request JSON for {repository}" + ) + if pull_requests: + raise CommandError( + f"refusing to reuse {repository} branch {branch}: its open pull " + f"request is not owned by policy {policy_id}" + ) + def switch_to_policy_branch( self, *, checkout: Path, branch: str, exists_remotely: bool ) -> None: @@ -447,8 +530,15 @@ def create_pull_request( output = self._run(create_command).strip() if not output: raise CommandError(f"gh did not return a pull-request URL for {repository}") + # The tool label is the sole ownership signal `_find_policy_pull_requests` + # relies on, so a PR missing it would be invisible to every later run + # and could be duplicated or overwritten. Only the cosmetic "automation" + # label may fail without aborting. warnings: list[str] = [] for label in AUTOMATION_LABELS: + if label == TOOL_SLUG: + self._run(["gh", "pr", "edit", output, "--add-label", label]) + continue try: self._run(["gh", "pr", "edit", output, "--add-label", label]) except CommandError as exc: diff --git a/repo_policy_sync/src/runner.py b/repo_policy_sync/src/runner.py index 4804e98..fef0858 100644 --- a/repo_policy_sync/src/runner.py +++ b/repo_policy_sync/src/runner.py @@ -741,13 +741,15 @@ def _policy_pr_label(status: PolicyPullRequestStatus | None) -> str | None: return "open" if status.merged is not None: return "merged" + if status.closed is not None: + return "closed" return "none" def _policy_pr_url(status: PolicyPullRequestStatus | None) -> str | None: if status is None: return None - pull_request = status.open or status.merged + pull_request = status.open or status.merged or status.closed return pull_request.url if pull_request is not None else None diff --git a/repo_policy_sync/tests/test_github.py b/repo_policy_sync/tests/test_github.py index a2d4496..7ddcfa6 100644 --- a/repo_policy_sync/tests/test_github.py +++ b/repo_policy_sync/tests/test_github.py @@ -529,6 +529,40 @@ def run(command: list[str]) -> str: ) +def test_create_pull_request_fails_when_tool_label_cannot_be_applied( + monkeypatch, +) -> None: + def run(command: list[str]) -> str: + if command[:5] == [ + "gh", + "api", + "--paginate", + "--slurp", + "/repos/owner/repo/labels?per_page=100", + ]: + return '[[{"name":"automation"},{"name":"repo-policy-sync"}]]' + if command[:3] == ["gh", "pr", "create"]: + return "https://github.example/owner/repo/pull/1\n" + if command[:3] == ["gh", "pr", "edit"] and command[-1] == "repo-policy-sync": + raise CommandError("gh pr edit: permission denied") + if command[:3] == ["gh", "pr", "edit"]: + return "" + raise AssertionError(command) + + monkeypatch.setattr(GitHubCli, "_run", staticmethod(run)) + policy = Policy("example", "Example", None, None, ()) + + with pytest.raises(CommandError, match="permission denied"): + GitHubCli().create_pull_request( + repository="owner/repo", + base="main", + branch="repo-policy-sync/example", + policy=policy, + changes=(), + head_oid="a" * 40, + ) + + def test_create_pull_request_can_create_a_draft(monkeypatch) -> None: commands: list[list[str]] = [] @@ -892,6 +926,35 @@ def run(command: list[str]) -> str: ) +def test_unlabeled_pull_request_on_expected_branch_is_not_reused(monkeypatch) -> None: + """A PR the `--label` lookup cannot see must still block branch reuse. + + This covers both a maintainer's own PR opened directly on the expected + branch and a tool-created PR whose label application previously failed: + neither carries the tool label, so the `--label` query alone would miss + them and the branch could otherwise be silently pushed over. + """ + + policy = Policy("example", "Example", None, None, ()) + branch = policy_branches(policy)[0] + + def run(command: list[str]) -> str: + if "--label" in command: + return "[]" + assert command[:3] == ["gh", "pr", "list"] + assert command[command.index("--head") + 1] == branch + return json.dumps([{"number": 7}]) + + monkeypatch.setattr(GitHubCli, "_run", staticmethod(run)) + + with pytest.raises(CommandError, match="is not owned by policy example"): + GitHubCli().find_open_pull_request( + repository="owner/repo", + branches=policy_branches(policy), + policy_id=policy.id, + ) + + def test_legacy_policy_pull_request_is_recognized_on_its_old_branch( monkeypatch, ) -> None: @@ -907,19 +970,22 @@ def test_legacy_policy_pull_request_is_recognized_on_its_old_branch( old_branch = "repo-sync/old-policy" def run(command: list[str]) -> str: - assert command[command.index("--label") + 1] == "repo-policy-sync" - return json.dumps( - [ - { - "number": 1, - "url": "https://github.example/owner/repo/pull/1", - "body": "\n" - "", - "headRefName": old_branch, - "mergeable": "MERGEABLE", - } - ] - ) + if "--label" in command: + assert command[command.index("--label") + 1] == "repo-policy-sync" + return json.dumps( + [ + { + "number": 1, + "url": "https://github.example/owner/repo/pull/1", + "body": "\n" + "", + "headRefName": old_branch, + "mergeable": "MERGEABLE", + } + ] + ) + assert command[:3] == ["gh", "pr", "list"] + return "[]" monkeypatch.setattr(GitHubCli, "_run", staticmethod(run)) @@ -954,6 +1020,19 @@ def run(command: list[str]) -> str: } ] ) + if state == "closed": + return json.dumps( + [ + { + "number": 4, + "url": "https://github.example/owner/repo/pull/4", + "body": "\n" + "", + "headRefName": branch, + "closedAt": "2026-03-01T00:00:00Z", + } + ] + ) return json.dumps( [ { @@ -986,6 +1065,8 @@ def run(command: list[str]) -> str: assert status.open.url.endswith("/3") assert status.merged is not None assert status.merged.url.endswith("/2") + assert status.closed is not None + assert status.closed.url.endswith("/4") def test_multiple_policy_pull_requests_fail_instead_of_choosing(monkeypatch) -> None: diff --git a/repo_policy_sync/tests/test_runner.py b/repo_policy_sync/tests/test_runner.py index f9a1695..165d48f 100644 --- a/repo_policy_sync/tests/test_runner.py +++ b/repo_policy_sync/tests/test_runner.py @@ -372,6 +372,43 @@ def test_runner_adds_policy_pull_request_status_for_markdown_reports( assert report.outcomes[0].pull_request_url.endswith("/7") +def test_runner_reports_a_pre_existing_closed_pull_request_for_plan( + tmp_path: Path, +) -> None: + """Plan surfaces a policy PR someone closed without merging, not just none.""" + source = tmp_path / "repository" + source.mkdir() + policy = Policy( + "example", "Example", None, None, (EnsureLine(Path("required.txt"), "yes", ()),) + ) + client = PolicyStatusClient( + source, + (Repository("candidate", "main"),), + PolicyPullRequestStatus( + closed=PullRequest( + 8, + "https://github.example/owner/candidate/pull/8", + closed_at="2026-01-01", + ) + ), + ) + + report = run_policies( + client=client, + org="eclipse-score", + policies=(policy,), + repository_names=(), + checkout_cache_directory=tmp_path / "cache", + apply=False, + sync_workers=1, + policy_workers=1, + include_pull_request_status=True, + ) + + assert report.outcomes[0].policy_pr_status == "closed" + assert report.outcomes[0].pull_request_url.endswith("/8") + + def test_runner_reports_pull_request_status_when_policy_is_not_applicable( tmp_path: Path, ) -> None: From c3a69aa52677c38cd5ef6c2e77dc8e0d363c2f79 Mon Sep 17 00:00:00 2001 From: Alexander Lanin Date: Fri, 28 Aug 2026 14:13:00 +0200 Subject: [PATCH 4/6] fix(repo-policy-sync): fake sync_org in the new closed-PR runner test The test called run_policies without _install_fake_sync, so it hit the real gh auth check via sync_org and only passed locally because gh was already authenticated. CI has no GitHub auth, so the tests job failed with "You are not logged into any GitHub hosts". --- repo_policy_sync/tests/test_runner.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/repo_policy_sync/tests/test_runner.py b/repo_policy_sync/tests/test_runner.py index 165d48f..d129386 100644 --- a/repo_policy_sync/tests/test_runner.py +++ b/repo_policy_sync/tests/test_runner.py @@ -373,7 +373,7 @@ def test_runner_adds_policy_pull_request_status_for_markdown_reports( def test_runner_reports_a_pre_existing_closed_pull_request_for_plan( - tmp_path: Path, + tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """Plan surfaces a policy PR someone closed without merging, not just none.""" source = tmp_path / "repository" @@ -392,6 +392,7 @@ def test_runner_reports_a_pre_existing_closed_pull_request_for_plan( ) ), ) + _install_fake_sync(monkeypatch, client) report = run_policies( client=client, From 188a5993dbd02c0a06c038b1df850ab14e9313ec Mon Sep 17 00:00:00 2001 From: Alexander Lanin Date: Fri, 28 Aug 2026 15:15:27 +0200 Subject: [PATCH 5/6] fix(repo-policy-sync): stop misclassifying merged PRs as closed gh pr list --state closed returns merged PRs too (GitHub's PR state only distinguishes open/closed; merged is a separate flag). Querying for "merged" and "closed" separately therefore duplicated the closed scan and let a merged PR appear as PolicyPullRequestStatus.closed whenever its closedAt happened to be set. Query --state closed once and classify locally by mergedAt, which fixes the misclassification and drops one gh pr list call per policy/repository status check. Update the docs and the outdated cli.py comment that described the terminal table's PR-status column as unconditional instead of shown only when that status is actionable. --- repo_policy_sync/README.md | 9 +++-- repo_policy_sync/docs/how-to/run-a-policy.md | 5 ++- repo_policy_sync/docs/reference/cli.md | 2 +- repo_policy_sync/src/cli.py | 8 ++-- repo_policy_sync/src/github.py | 31 ++++++++------- repo_policy_sync/tests/test_github.py | 40 ++++++++++---------- 6 files changed, 53 insertions(+), 42 deletions(-) diff --git a/repo_policy_sync/README.md b/repo_policy_sync/README.md index c659d46..c7d893a 100644 --- a/repo_policy_sync/README.md +++ b/repo_policy_sync/README.md @@ -87,9 +87,12 @@ Policy options can be kept in the optional `score-repo-policy-sync.toml` file. Explicit CLI values override the file; see the [configuration reference](docs/reference/configuration.md). -The CLI always prints a compact table to standard output, including the -discovered policy pull-request state (`open`, `merged`, `closed`, or `none`) -and number. Pass +The CLI always prints a compact table to standard output. Its status column +shows the discovered policy pull-request state (`open`, `merged`, or `closed`) +and number instead of the plain compliance status whenever that PR state is +actionable — an open PR while changes are still required, or a merged/closed +PR alongside current compliance; other combinations, including no matching +PR, keep the plain compliance status. Pass `--json-output PATH` and/or `--markdown-output PATH` to write additional versioned JSON and Markdown reports during the same policy run. Markdown is suited for pull requests, issues, and wikis. Its cells use `✅` for compliant, diff --git a/repo_policy_sync/docs/how-to/run-a-policy.md b/repo_policy_sync/docs/how-to/run-a-policy.md index b713b8c..2c34085 100644 --- a/repo_policy_sync/docs/how-to/run-a-policy.md +++ b/repo_policy_sync/docs/how-to/run-a-policy.md @@ -14,8 +14,9 @@ # How to run a policy Use plan mode first. It refreshes disposable local checkouts, reports drift, -and includes the status of matching policy pull requests, but never changes -remote repositories: +and surfaces an actionable matching policy pull request (for example an open +PR while changes are still required, or a merged/closed PR alongside current +compliance), but never changes remote repositories: ```bash uv run score-repo-policy-sync plan --org eclipse-score diff --git a/repo_policy_sync/docs/reference/cli.md b/repo_policy_sync/docs/reference/cli.md index 809a314..ee28464 100644 --- a/repo_policy_sync/docs/reference/cli.md +++ b/repo_policy_sync/docs/reference/cli.md @@ -37,7 +37,7 @@ are CLI-only. | `--org NAME` | GitHub organization to scan. May be set in TOML. | | `--policy NAME` | Select a policy by directory name from local or bundled policies. Repeat to select more than one; when present, only the selected policies run. Defaults to all local and bundled policies. | | `--repo NAME` | Restrict the run to an exact repository name. Repeat to select more than one. | -| *(stdout)* | Always prints the terminal policy-evaluation table, including policy pull-request state and number. | +| *(stdout)* | Always prints the terminal policy-evaluation table; its status column shows policy pull-request state and number instead of the plain compliance status when that state is actionable. | ## Rare diff --git a/repo_policy_sync/src/cli.py b/repo_policy_sync/src/cli.py index 47db12d..b75217d 100644 --- a/repo_policy_sync/src/cli.py +++ b/repo_policy_sync/src/cli.py @@ -286,9 +286,11 @@ def main(argv: Sequence[str] | None = None) -> int: sync_workers=sync_workers, policy_workers=policy_workers, # apply already looks up an open PR itself whenever it might act on - # one, so this only buys the extra merged-PR history used for - # display. Skip that extra `gh pr list` call in a bare apply run; - # still fetch it for plan (its only output) and for report files. + # one, so this only buys the merged/closed history used for + # display, at the cost of extra `gh pr list` calls (a closed-state + # scan and, for a branch outside the label lookup, a per-branch + # collision check). Skip that in a bare apply run; still fetch it + # for plan (its only output) and for report files. include_pull_request_status=( args.command == "plan" or args.json_output is not None diff --git a/repo_policy_sync/src/github.py b/repo_policy_sync/src/github.py index 41926be..ef1770f 100644 --- a/repo_policy_sync/src/github.py +++ b/repo_policy_sync/src/github.py @@ -127,12 +127,26 @@ def find_policy_pull_request_status( raise CommandError( f"multiple open pull requests match policy {policy_id} in {repository}: {urls}" ) - merged_pull_requests = self._find_policy_pull_requests( + # `gh pr list --state closed` returns every non-open PR, merged ones + # included (GitHub's PR "state" only distinguishes open/closed; merged + # is a separate flag). One query covers both merged and closed history, + # classified below by whether `mergedAt` is set. + resolved_pull_requests = self._find_policy_pull_requests( repository=repository, branches=branches, policy_id=policy_id, legacy_policy_ids=legacy_policy_ids, - state="merged", + state="closed", + ) + merged_pull_requests = tuple( + pull_request + for pull_request in resolved_pull_requests + if pull_request.merged_at is not None + ) + closed_pull_requests = tuple( + pull_request + for pull_request in resolved_pull_requests + if pull_request.merged_at is None ) latest_merged = max( merged_pull_requests, @@ -142,13 +156,6 @@ def find_policy_pull_request_status( ), default=None, ) - closed_pull_requests = self._find_policy_pull_requests( - repository=repository, - branches=branches, - policy_id=policy_id, - legacy_policy_ids=legacy_policy_ids, - state="closed", - ) latest_closed = max( closed_pull_requests, key=lambda pull_request: ( @@ -176,10 +183,8 @@ def _find_policy_pull_requests( owned: list[PullRequest] = [] accepted_markers = _policy_markers((policy_id, *legacy_policy_ids)) - if state == "merged": - fields = "number,url,body,headRefName,mergedAt" - elif state == "closed": - fields = "number,url,body,headRefName,closedAt" + if state == "closed": + fields = "number,url,body,headRefName,mergedAt,closedAt" else: fields = "number,url,body,headRefName,mergeable" output = self._run( diff --git a/repo_policy_sync/tests/test_github.py b/repo_policy_sync/tests/test_github.py index 7ddcfa6..9c9c461 100644 --- a/repo_policy_sync/tests/test_github.py +++ b/repo_policy_sync/tests/test_github.py @@ -1021,8 +1021,19 @@ def run(command: list[str]) -> str: ] ) if state == "closed": + # `gh pr list --state closed` also returns merged PRs (GitHub only + # distinguishes open/closed; merged is a separate flag), so both + # the merged and the closed-only PR come back from this one query. return json.dumps( [ + { + "number": 2, + "url": "https://github.example/owner/repo/pull/2", + "body": "\n" + "", + "headRefName": branch, + "mergedAt": "2026-01-01T00:00:00Z", + }, { "number": 4, "url": "https://github.example/owner/repo/pull/4", @@ -1030,28 +1041,17 @@ def run(command: list[str]) -> str: "", "headRefName": branch, "closedAt": "2026-03-01T00:00:00Z", - } + }, + { + "number": 99, + "url": "https://github.example/owner/repo/pull/99", + "body": "a historical PR owned by another tool", + "headRefName": "other-tool/branch", + "mergedAt": "2026-02-01T00:00:00Z", + }, ] ) - return json.dumps( - [ - { - "number": 2, - "url": "https://github.example/owner/repo/pull/2", - "body": "\n" - "", - "headRefName": branch, - "mergedAt": "2026-01-01T00:00:00Z", - }, - { - "number": 99, - "url": "https://github.example/owner/repo/pull/99", - "body": "a historical PR owned by another tool", - "headRefName": "other-tool/branch", - "mergedAt": "2026-02-01T00:00:00Z", - }, - ] - ) + raise AssertionError(f"unexpected state: {state}") monkeypatch.setattr(GitHubCli, "_run", staticmethod(run)) From f3ab2a989fce74c88f2845bad334c62a443a76db Mon Sep 17 00:00:00 2001 From: Alexander Lanin Date: Fri, 28 Aug 2026 15:32:10 +0200 Subject: [PATCH 6/6] fix(repo-policy-sync): don't count a historical closed PR as closed by this run Plan reporting for a not-applicable policy with a pre-existing closed PR produced the same status/policy_pr_status combination as an apply run that just closed that PR, so run_policies incremented pull_requests_closed during plan too. Add a pull_request_closed_by_run flag set only where the run actually calls close_pull_request, and aggregate on that instead of inferring the action from status + policy_pr_status. --- repo_policy_sync/src/runner.py | 8 ++--- repo_policy_sync/tests/test_runner.py | 45 +++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/repo_policy_sync/src/runner.py b/repo_policy_sync/src/runner.py index fef0858..32d185d 100644 --- a/repo_policy_sync/src/runner.py +++ b/repo_policy_sync/src/runner.py @@ -169,6 +169,7 @@ class RepositoryOutcome: warnings: tuple[str, ...] = () error: str | None = None policy_pr_status: str | None = None + pull_request_closed_by_run: bool = False def run_policies( @@ -273,10 +274,7 @@ def run_policies( "pull-request-recreated-no-changes", }: pull_requests_recreated += 1 - if outcome.status == "pull-request-closed" or ( - outcome.status == "not-applicable" - and outcome.policy_pr_status == "closed" - ): + if outcome.pull_request_closed_by_run: pull_requests_closed += 1 return RunReport( RunSummary( @@ -479,6 +477,7 @@ def _run_repository( "not-applicable", pull_request_url=existing_pr.url, policy_pr_status="closed", + pull_request_closed_by_run=True, ) return RepositoryOutcome( repository, @@ -528,6 +527,7 @@ def _run_repository( "pull-request-closed", pull_request_url=existing_pr.url, policy_pr_status="closed", + pull_request_closed_by_run=True, ) return RepositoryOutcome( repository, diff --git a/repo_policy_sync/tests/test_runner.py b/repo_policy_sync/tests/test_runner.py index d129386..26d628b 100644 --- a/repo_policy_sync/tests/test_runner.py +++ b/repo_policy_sync/tests/test_runner.py @@ -408,6 +408,51 @@ def test_runner_reports_a_pre_existing_closed_pull_request_for_plan( assert report.outcomes[0].policy_pr_status == "closed" assert report.outcomes[0].pull_request_url.endswith("/8") + assert report.summary.pull_requests_closed == 0 + + +def test_runner_does_not_count_a_pre_existing_closed_pr_as_closed_by_plan( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Plan reporting a historical closed PR must not claim this run closed one.""" + source = tmp_path / "repository" + source.mkdir() + policy = Policy( + "example", + "Example", + None, + None, + (), + file_exists_condition=FileExistsCondition(Path("does-not-exist")), + ) + client = PolicyStatusClient( + source, + (Repository("candidate", "main"),), + PolicyPullRequestStatus( + closed=PullRequest( + 8, + "https://github.example/owner/candidate/pull/8", + closed_at="2026-01-01", + ) + ), + ) + _install_fake_sync(monkeypatch, client) + + report = run_policies( + client=client, + org="eclipse-score", + policies=(policy,), + repository_names=(), + checkout_cache_directory=tmp_path / "cache", + apply=False, + sync_workers=1, + policy_workers=1, + include_pull_request_status=True, + ) + + assert report.outcomes[0].status == "not-applicable" + assert report.outcomes[0].policy_pr_status == "closed" + assert report.summary.pull_requests_closed == 0 def test_runner_reports_pull_request_status_when_policy_is_not_applicable(