From 8897f70174ab6eb4bf48680c032038aca5842899 Mon Sep 17 00:00:00 2001 From: ellmos-multi-bot Date: Sun, 25 Oct 2026 21:17:17 +0100 Subject: [PATCH 1/2] feat(core): close only proven integrated pull-request heads Add opt-in pr.close with explicit repository scope, pinned default-branch ancestry, repeated identity and head checks, preview-plan binding and independent state readback. Keep branches intact and ambiguous writes unconfirmed. Isolate Windows launcher tests from installed packages. Agent-Session: 01a16e5b-dcd2-7521-8d7a-e78fe113b4f1 --- CHANGELOG.md | 4 + README.md | 18 +- README_de.md | 19 +- src/githubbot_core/default_config.json | 4 + src/githubbot_core/multibot.py | 3 +- src/githubbot_core/prompts/MULTI-BOT.de.md | 6 + src/githubbot_core/typed_actions.py | 100 ++++++++- tests/test_multibot_windows.py | 3 + tests/test_pr_close_action.py | 236 +++++++++++++++++++++ 9 files changed, 385 insertions(+), 8 deletions(-) create mode 100644 tests/test_pr_close_action.py diff --git a/CHANGELOG.md b/CHANGELOG.md index fde6c48..b890039 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Format: [Keep a Changelog](https://keepachangelog.com/de/1.1.0/), [Semantic Vers ### Hinzugefügt / Added +- Opt-in-Aktion `pr.close` für nachweislich integrierte Heads: explizite Repository-Liste, + eigenes Policy-Opt-in, exakte Head-/Default-SHAs, Kontoprüfung und unabhängiges Rücklesen. + Keine Branch-Löschung, keine pauschalen Schließungen. / Typed opt-in closure of heads + proven contained in the pinned default branch, with explicit repository scope and readback. - Opt-in-Aktion `pr.review` für formale GitHub-Reviews am exakten Head mit Kontoprüfung, verpflichtendem Event/Text, Duplikatprüfung und unabhängigem Review-/Head-Readback. Standardmäßig deaktiviert; braucht `permissions.enabled` und `policy.publish_reviews`. diff --git a/README.md b/README.md index c5b9490..9a72683 100644 --- a/README.md +++ b/README.md @@ -113,20 +113,34 @@ not a silent override. `permissions.enabled` must stay inside the ceiling. | `pr.merge` | disabled | re-reads the PR (open, exact `sha`, default base, not draft, `mergeable_state` clean, configured merge account), `PUT …/pulls/{n}/merge` with `sha`, reads `merged` back | | `pr.ready` | disabled | re-reads the draft at the exact `sha`, GraphQL `markPullRequestReadyForReview`, reads back | | `pr.review` | disabled | submits `COMMENT`, `APPROVE` or `REQUEST_CHANGES` to `POST …/pulls/{n}/reviews` with mandatory `sha`/body; requires the configured account and reads review ID, actor, commit, body and state back | +| `pr.close` | disabled | closes a PR only after its exact head is proven contained in the pinned current default branch; requires an explicit repository allowlist, configured account and closed-state readback | | `actions.dispatch` | disabled | `POST …/actions/workflows/{workflow}/dispatches` for an active workflow | | `actions.rerun` | disabled | `POST …/actions/runs/{id}/rerun` for a completed, non-fork run of the exact `sha` | | `pr.create` | disabled | `POST …/pulls` from a bot-prefixed branch onto the default branch | | `git.push_branch` | disabled | `git push origin :refs/heads/` without force, bot prefix only (`typed_actions.push_branch_prefix`, default `bot/`), never the default branch, verified with `ls-remote` | -The seven typed actions sit in the default ceiling but are never enabled by default. Each one also +The eight typed actions sit in the default ceiling but are never enabled by default. Each one also needs its `multi_bot` switch (`merge_enabled`, `policy.draft_*`, `policy.trigger_missing_ci`, -`policy.open_fix_prs`, `policy.publish_reviews`); a permission alone opens nothing. One action at a time: +`policy.open_fix_prs`, `policy.publish_reviews`, `policy.close_integrated_prs`); a permission alone opens nothing. One action at a time: ```bash githubbot-core --config instance.json act pr.merge --params '{"repo": "o/r", "number": 7, "sha": "<40-hex>"}' githubbot-core --config instance.json act pr.merge --params '…' --apply --expect-plan ``` +Closing integrated PRs additionally requires `typed_actions.close_repositories`, an explicit list +of exact `owner/repo` names. Enable `pr.close` and `multi_bot.policy.close_integrated_prs`, set +`multi_bot.merge_account`, then preview: +`act pr.close --params '{"repo":"o/r","number":7,"sha":"<40-hex-head>","integration_sha":"<40-hex-current-default>","reason":"integrated"}'`. +Apply only the inspected `--expect-plan`. Drafts and stacked PRs are eligible only when their +head is proven contained in that default-branch commit. The action never deletes a branch, +posts a comment or closes an unintegrated head. Repository/account/head/default branch are +rechecked immediately before PATCH; PR state and head are read again afterwards. GitHub's +[update endpoint](https://docs.github.com/en/rest/pulls/pulls#update-a-pull-request) has no atomic +expected-head condition: a race detected after writing is an unconfirmed failure, not success. +Do not blindly retry an ambiguous write; inspect the current PR first. Scope locks and an +independent review of the integration proof remain operator responsibilities. + Formal reviews require an explicit instance opt-in, for example `{"permissions":{"enabled":["pr.review"]},"multi_bot":{"merge_account":"example-bot","policy":{"publish_reviews":true}}}`. Then preview `act pr.review --params '{"repo":"o/r","number":7,"sha":"<40-hex>","event":"COMMENT","body":"Review scope and evidence"}'` diff --git a/README_de.md b/README_de.md index 05ef534..21d3a05 100644 --- a/README_de.md +++ b/README_de.md @@ -115,14 +115,15 @@ stille Überschreibung. `permissions.enabled` muss innerhalb der Obergrenze blei | `pr.merge` | aus | liest den PR frisch (offen, exakter `sha`, Default-Base, kein Draft, `mergeable_state` clean, konfiguriertes Merge-Konto), `PUT …/pulls/{n}/merge` mit `sha`, liest `merged` zurück | | `pr.ready` | aus | liest den Draft am exakten `sha` frisch, GraphQL `markPullRequestReadyForReview`, liest zurück | | `pr.review` | aus | veröffentlicht `COMMENT`, `APPROVE` oder `REQUEST_CHANGES` über `POST …/pulls/{n}/reviews`, mit verpflichtendem `sha` und Text; prüft Konto und liest Review-ID, Autor, Commit, Text und Zustand zurück | +| `pr.close` | aus | schließt nur einen PR, dessen exakter Head im gebundenen aktuellen Default-Branch nachweislich enthalten ist; braucht explizite Repository-Liste, konfiguriertes Konto und Rücklesen des geschlossenen Zustands | | `actions.dispatch` | aus | `POST …/actions/workflows/{workflow}/dispatches` für einen aktiven Workflow | | `actions.rerun` | aus | `POST …/actions/runs/{id}/rerun` für einen abgeschlossenen Lauf des exakten `sha`, nie aus Forks | | `pr.create` | aus | `POST …/pulls` von einem Branch mit Bot-Präfix auf den Default-Branch | | `git.push_branch` | aus | `git push origin :refs/heads/` ohne Force, nur Bot-Präfix (`typed_actions.push_branch_prefix`, Standard `bot/`), nie der Default-Branch, Prüfung per `ls-remote` | -Die sieben getypten Aktionen stehen in der Standard-Obergrenze, sind aber nie standardmäßig aktiv. +Die acht getypten Aktionen stehen in der Standard-Obergrenze, sind aber nie standardmäßig aktiv. Jede braucht zusätzlich ihren `multi_bot`-Schalter (`merge_enabled`, `policy.draft_*`, -`policy.trigger_missing_ci`, `policy.open_fix_prs`, `policy.publish_reviews`); eine Berechtigung allein öffnet nichts. +`policy.trigger_missing_ci`, `policy.open_fix_prs`, `policy.publish_reviews`, `policy.close_integrated_prs`); eine Berechtigung allein öffnet nichts. Eine Aktion je Aufruf: ```bash @@ -130,6 +131,20 @@ githubbot-core --config instanz.json act pr.merge --params '{"repo": "o/r", "num githubbot-core --config instanz.json act pr.merge --params '…' --apply --expect-plan ``` +Das Schließen integrierter PRs braucht zusätzlich `typed_actions.close_repositories`, eine Liste +exakter Namen im Format `owner/repo`. `pr.close` und `multi_bot.policy.close_integrated_prs` +aktivieren, `multi_bot.merge_account` setzen und zunächst die Vorschau ausführen: +`act pr.close --params '{"repo":"o/r","number":7,"sha":"<40-hex-head>","integration_sha":"<40-hex-aktueller-default>","reason":"integrated"}'`. +Nur den geprüften Plan mit `--expect-plan` anwenden. Auch Drafts und gestapelte PRs sind nur +dann geeignet, wenn ihr Head nachweislich in diesem Default-Branch-Commit enthalten ist. +Die Aktion löscht keine Branches, veröffentlicht keine Kommentare und schließt keine noch +nicht integrierten Heads. Konto, Repository, Head und Default-Branch werden unmittelbar vor +PATCH erneut geprüft, PR-Zustand und Head danach zurückgelesen. GitHubs +[Update-Endpunkt](https://docs.github.com/en/rest/pulls/pulls#update-a-pull-request) bietet keine +atomare Head-Bedingung: Ein erst nach dem Schreiben erkanntes Rennen bleibt ein unbestätigter +Fehler. Unklare Schreibversuche nie blind wiederholen; zuerst den aktuellen PR lesen. +Scope-Sperren und unabhängige Prüfung des Integrationsnachweises bleiben Operator-Aufgaben. + Formale Reviews brauchen ein ausdrückliches Instanz-Opt-in, beispielsweise `{"permissions":{"enabled":["pr.review"]},"multi_bot":{"merge_account":"example-bot","policy":{"publish_reviews":true}}}`. Danach `act pr.review --params '{"repo":"o/r","number":7,"sha":"<40-hex>","event":"COMMENT","body":"Prüfumfang und Belege"}'` diff --git a/src/githubbot_core/default_config.json b/src/githubbot_core/default_config.json index 717f5d9..4ef65b9 100644 --- a/src/githubbot_core/default_config.json +++ b/src/githubbot_core/default_config.json @@ -11,6 +11,7 @@ "pr.merge", "pr.ready", "pr.review", + "pr.close", "actions.dispatch", "actions.rerun", "pr.create", @@ -18,6 +19,9 @@ ], "enabled": ["issue.add_labels", "issue.comment"] }, + "typed_actions": { + "close_repositories": [] + }, "label_schema": { "bug": {"color": "d73a4a", "description": "Something is not working", "auto_apply": true}, "feature": {"color": "0075ca", "description": "New feature or enhancement", "auto_apply": true}, diff --git a/src/githubbot_core/multibot.py b/src/githubbot_core/multibot.py index aa6a43c..213ef0b 100644 --- a/src/githubbot_core/multibot.py +++ b/src/githubbot_core/multibot.py @@ -59,6 +59,7 @@ def permission_contract(config, layers): "draft_ready_when_mature": False, "trigger_missing_ci": False, "publish_reviews": False, + "close_integrated_prs": False, "open_fix_prs": False, } _POLICY_CHOICES = { @@ -83,7 +84,7 @@ def policy_settings(raw): hours = policy["draft_auto_ready_hours"] if hours is not None and (type(hours) not in (int, float) or hours <= 0): raise ValueError("multi_bot.policy.draft_auto_ready_hours must be null or a positive number") - for key in ("draft_ready_when_mature", "trigger_missing_ci", "publish_reviews", "open_fix_prs"): + for key in ("draft_ready_when_mature", "trigger_missing_ci", "publish_reviews", "open_fix_prs", "close_integrated_prs"): if type(policy[key]) is not bool: raise ValueError(f"multi_bot.policy.{key} must be a boolean") return policy diff --git a/src/githubbot_core/prompts/MULTI-BOT.de.md b/src/githubbot_core/prompts/MULTI-BOT.de.md index 964c4b8..63cabe9 100644 --- a/src/githubbot_core/prompts/MULTI-BOT.de.md +++ b/src/githubbot_core/prompts/MULTI-BOT.de.md @@ -212,6 +212,12 @@ und ersetzen weder Merge-Opt-in noch Identitätsprüfung. Prüfumfang (gelesen/nicht gelesen), Testergebnis mit Befehl, Befunde mit Datei und Zeile. Ein Beitrag pro (PR, Head); keine Secrets, keine lokalen Pfade. Ohne diesen Schalter bleibt das Ergebnis im Bericht. +- `close_integrated_prs` (bool, standardmäßig aus): erlaubt ausschließlich `pr.close` für + nachweislich im aktuellen Default-Branch integrierte Heads. Zusätzlich braucht die Aktion + eine explizite Liste `typed_actions.close_repositories`, ihre Berechtigung und das konfigurierte + Konto. Exakte Head-/Integrations-SHAs, Vorschau/Planbindung und frische Integrationsprüfung + sind Pflicht. Keine Branch-Löschung oder pauschalen Schließungen. Bei unklarem Schreibversuch + zuerst unabhängig zurücklesen; niemals blind wiederholen. - `open_fix_prs` (bool): erlaubt Fix-Branches zu pushen (Core-Aktion `git.push_branch`: nur Bot-Präfix `typed_actions.push_branch_prefix`, nie Default-Branch, nie Force) und PRs zu öffnen (`pr.create`, Base immer der Default-Branch). Ohne diesen Schalter bleiben Fixes lokal diff --git a/src/githubbot_core/typed_actions.py b/src/githubbot_core/typed_actions.py index 83ade3b..3f2746d 100644 --- a/src/githubbot_core/typed_actions.py +++ b/src/githubbot_core/typed_actions.py @@ -13,6 +13,7 @@ import re import subprocess from typing import Any, Optional +from urllib.parse import quote SHA = re.compile(r"[0-9a-f]{40}") REPO = re.compile(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+") @@ -21,7 +22,7 @@ MERGE_METHODS = ("merge", "squash", "rebase") DEFAULT_PUSH_PREFIX = "bot/" -KINDS = ("pr.merge", "pr.ready", "pr.review", "actions.dispatch", "actions.rerun", "pr.create", "git.push_branch") +KINDS = ("pr.merge", "pr.ready", "pr.review", "pr.close", "actions.dispatch", "actions.rerun", "pr.create", "git.push_branch") REVIEW_STATES = {"COMMENT": "COMMENTED", "APPROVE": "APPROVED", "REQUEST_CHANGES": "CHANGES_REQUESTED"} _git = subprocess.run # replaced in tests @@ -46,6 +47,7 @@ def switch_denial(kind: str, config: dict) -> Optional[str]: "pr.merge": policy["merge_enabled"], "pr.ready": policy["draft_auto_ready_hours"] is not None or policy["draft_ready_when_mature"], "pr.review": policy["publish_reviews"], + "pr.close": policy["close_integrated_prs"], "actions.dispatch": policy["trigger_missing_ci"], "actions.rerun": policy["trigger_missing_ci"], "pr.create": policy["open_fix_prs"], @@ -65,11 +67,31 @@ def param_denial(kind: str, params: dict, config: dict) -> Optional[str]: return None if not REPO.fullmatch(str(params.get("repo", ""))): return "repo must be owner/name" - need_sha = kind in ("pr.merge", "pr.ready", "pr.review", "actions.rerun", "git.push_branch") + need_sha = kind in ("pr.merge", "pr.ready", "pr.review", "pr.close", "actions.rerun", "git.push_branch") if need_sha and not SHA.fullmatch(str(params.get("sha", ""))): return "sha must be the exact reviewed 40-character lowercase commit" - if kind in ("pr.merge", "pr.ready", "pr.review") and not str(params.get("number", "")).isdecimal(): + if kind in ("pr.merge", "pr.ready", "pr.review", "pr.close") and not str(params.get("number", "")).isdecimal(): return "number must be a pull request number" + if kind == "pr.close": + if int(params["number"]) < 1: + return "number must be a positive pull request number" + if params.get("reason") != "integrated": + return "pr.close only supports reason='integrated'" + if not SHA.fullmatch(str(params.get("integration_sha", ""))): + return "integration_sha must be the exact current default-branch commit" + account = (config.get("multi_bot") or {}).get("merge_account") + if not isinstance(account, str) or not account.strip(): + return "pr.close requires the configured multi_bot.merge_account" + settings = config.get("typed_actions") or {} + if not isinstance(settings, dict): + return "typed_actions must be an object" + scopes = settings.get("close_repositories") + if not isinstance(scopes, list) or not scopes or any( + not isinstance(repo, str) or not REPO.fullmatch(repo) for repo in scopes + ): + return "pr.close requires explicit typed_actions.close_repositories" + if params["repo"] not in scopes: + return "repository is outside typed_actions.close_repositories" if kind == "pr.review": if int(params["number"]) < 1: return "number must be a positive pull request number" @@ -283,6 +305,77 @@ def current_pull(): return {"readback": {"review_id": review_id, "head": sha, "state": after["state"], "account": account}}, None +def exec_close(transport, params: dict, config: dict): + """Close only heads proven contained in the pinned current default branch. + + GitHub's close endpoint has no atomic expected-head argument. Recheck before + PATCH and detect races afterwards; ambiguous results must never be retried + blindly. Locks and independent source review remain operator responsibilities. + """ + permissions = config.get("permissions") or {} + if "pr.close" not in (permissions.get("enabled") or []) or "pr.close" not in (permissions.get("ceiling") or []): + return None, "pr.close is not enabled within the permission ceiling" + denial = switch_denial("pr.close", config) or param_denial("pr.close", params, config) + if denial: + return None, denial + account = config["multi_bot"]["merge_account"].strip() + repo, number, sha = params["repo"], int(params["number"]), params["sha"] + integrated = params["integration_sha"] + + def current_target(): + user, error = transport.get("user") + if error or not isinstance(user, dict) or user.get("login") != account: + return None, "identity is not the configured close account" + branch, error = _default_branch(transport, repo) + if error or not isinstance(branch, str): + return None, error or "default branch unreadable" + ref, error = transport.get(f"repos/{repo}/git/ref/heads/{quote(branch, safe='')}") + obj = ref.get("object") if isinstance(ref, dict) else None + if error or not isinstance(obj, dict) or obj.get("type") != "commit" or obj.get("sha") != integrated: + return None, "default branch moved or unreadable; nothing written" + pull, error = _pull(transport, repo, number) + head = pull.get("head") + base = pull.get("base") + base_repo = base.get("repo") if isinstance(base, dict) else None + if ( + error or pull.get("number") != number or pull.get("state") != "open" + or pull.get("merged") is not False or not isinstance(head, dict) or head.get("sha") != sha + or not isinstance(base_repo, dict) or base_repo.get("full_name") != repo + ): + return None, "pull is not open and unmerged at the exact head/repository; nothing written" + return branch, None + + branch, error = current_target() + if error: + return None, error + proof, error = transport.get(f"repos/{repo}/compare/{sha}...{integrated}") + if ( + error or not isinstance(proof, dict) or proof.get("status") not in ("ahead", "identical") + or (proof.get("status") == "identical") != (sha == integrated) + or not isinstance(proof.get("base_commit"), dict) or proof["base_commit"].get("sha") != sha + or not isinstance(proof.get("merge_base_commit"), dict) or proof["merge_base_commit"].get("sha") != sha + ): + return None, "head is not proven integrated in the pinned default branch" + latest_branch, error = current_target() + if error or latest_branch != branch: + return None, error or "default branch name changed; nothing written" + _, error = transport.request("PATCH", f"repos/{repo}/pulls/{number}", {"state": "closed"}) + if error: + return None, "close write unconfirmed; independently read PR state before any retry: " + str(error) + after, error = _pull(transport, repo, number) + head = after.get("head") + base = after.get("base") + base_repo = base.get("repo") if isinstance(base, dict) else None + if ( + error or after.get("number") != number or after.get("state") != "closed" + or after.get("merged") is not False or not isinstance(head, dict) or head.get("sha") != sha + or not isinstance(base_repo, dict) or base_repo.get("full_name") != repo + ): + return None, "close readback unconfirmed or head changed; do not blindly retry" + return {"readback": {"closed": True, "merged": False, "head": sha, + "integration_sha": integrated, "reason": "integrated", "account": account}}, None + + def exec_dispatch(transport, params: dict, config: dict): repo, workflow = params["repo"], params["workflow"] data, error = transport.get(f"repos/{repo}/actions/workflows/{workflow}") @@ -371,6 +464,7 @@ def exec_push_branch(transport, params: dict, config: dict): "pr.merge": exec_merge, "pr.ready": exec_ready, "pr.review": exec_review, + "pr.close": exec_close, "actions.dispatch": exec_dispatch, "actions.rerun": exec_rerun, "pr.create": exec_create_pr, diff --git a/tests/test_multibot_windows.py b/tests/test_multibot_windows.py index bd5dd32..d82efba 100644 --- a/tests/test_multibot_windows.py +++ b/tests/test_multibot_windows.py @@ -15,6 +15,9 @@ def env_for(tmp_path, code=23): package = tmp_path / "githubbot_core" package.mkdir() + # A namespace package would lose to an installed regular githubbot_core and + # accidentally exercise the real CLI instead of this isolated no-provider stub. + (package / "__init__.py").write_text("", encoding="ascii") (package / "__main__.py").write_text( "import sys\nprint('[TEST] no provider started')\nsys.exit(" + str(code) + ")\n", encoding="ascii" ) diff --git a/tests/test_pr_close_action.py b/tests/test_pr_close_action.py new file mode 100644 index 0000000..fec6633 --- /dev/null +++ b/tests/test_pr_close_action.py @@ -0,0 +1,236 @@ +"""Closing is opt-in and limited to heads proven contained in current main.""" + +import copy + +import pytest + +from githubbot_core import actions, typed_actions +from githubbot_core.actions import Action +from githubbot_core.config import default_config, merge_layers +from githubbot_core.multibot import permission_contract, policy_settings + +REPO = "example-org/example-repo" +SHA, MAIN, OTHER = "a" * 40, "b" * 40, "c" * 40 +ENDPOINT = f"repos/{REPO}/pulls/7" + + +def config(*, enabled=True, policy=True, repos=None): + return merge_layers( + [ + ("default", default_config()), + ( + "instance", + { + "permissions": {"enabled": ["pr.close"] if enabled else []}, + "multi_bot": {"merge_account": "example-bot", "policy": {"close_integrated_prs": policy}}, + "typed_actions": {"close_repositories": [REPO] if repos is None else repos}, + }, + ), + ] + ) + + +def action(**overrides): + return Action( + "pr.close", + {"repo": REPO, "number": 7, "sha": SHA, "integration_sha": MAIN, "reason": "integrated", **overrides}, + ) + + +class Transport: + def __init__(self): + self.login = "example-bot" + self.branch = "main" + self.ref = {"object": {"sha": MAIN, "type": "commit"}} + self.pull = { + "number": 7, + "state": "open", + "merged": False, + "head": {"sha": SHA}, + "draft": True, + "base": {"ref": "stacked-worker", "repo": {"full_name": REPO}}, + } + self.proof = {"status": "ahead", "base_commit": {"sha": SHA}, "merge_base_commit": {"sha": SHA}} + self.pull_answers = [] + self.ref_answers = [] + self.errors = {} + self.write_error = None + self.readback_changes = {} + self.writes, self.reads = [], [] + + def get(self, endpoint): + self.reads.append(endpoint) + if endpoint == "user": + data = {"login": self.login} + elif endpoint == f"repos/{REPO}": + data = {"default_branch": self.branch} + elif endpoint.startswith(f"repos/{REPO}/git/ref/heads/"): + data = self.ref_answers.pop(0) if self.ref_answers else self.ref + elif endpoint == ENDPOINT: + data = self.pull_answers.pop(0) if self.pull_answers else self.pull + elif endpoint.startswith(f"repos/{REPO}/compare/{SHA}..."): + data = self.proof + else: + raise AssertionError(endpoint) + return copy.deepcopy(data), self.errors.get(endpoint) + + def request(self, method, endpoint, body): + assert (method, endpoint, body) == ("PATCH", ENDPOINT, {"state": "closed"}) + self.writes.append((method, endpoint, body)) + self.pull.update(state="closed", **self.readback_changes) + return copy.deepcopy(self.pull), self.write_error + + +def apply(transport, item=None, cfg=None, **kwargs): + return actions.apply([item or action()], transport, cfg or config(), **kwargs) + + +def test_opt_in_preview_and_permission_contract(): + assert "pr.close" not in default_config()["permissions"]["enabled"] + assert policy_settings({})["close_integrated_prs"] is False + assert actions.preview([action()], config())["writes"] == 0 + assert permission_contract(config(), [])["permissions"]["effective_enabled"] == ["pr.close"] + for cfg in [config(enabled=False), config(policy=False), config(repos=[]), config(repos=["other/repo"])]: + t = Transport() + assert apply(t, cfg=cfg)["receipts"][0]["status"] == "denied" + assert not t.reads and not t.writes + with pytest.raises(ValueError): + policy_settings({"close_integrated_prs": "true"}) + + +@pytest.mark.parametrize( + "overrides", + [ + {"sha": "short"}, + {"integration_sha": "short"}, + {"integration_sha": MAIN.upper()}, + {"number": 0}, + {"number": -1}, + {"number": True}, + {"reason": "obsolete"}, + {"reason": None}, + {"repo": "../bad/repo"}, + ], +) +def test_invalid_parameters_never_read_or_write(overrides): + t = Transport() + assert apply(t, action(**overrides))["receipts"][0]["status"] == "denied" + assert not t.reads and not t.writes + + +@pytest.mark.parametrize("repos", ["*", ["*"], [None], [REPO, "../bad/repo"], {}]) +def test_malformed_scopes_are_not_wildcards(repos): + t = Transport() + assert apply(t, cfg=config(repos=repos))["receipts"][0]["status"] == "denied" + assert not t.reads and not t.writes + + +def test_integrated_draft_or_stack_can_close_with_exact_readback(): + t = Transport() + receipt = apply(t)["receipts"][0] + assert receipt["status"] == "applied" + assert receipt["readback"] == { + "closed": True, + "merged": False, + "head": SHA, + "integration_sha": MAIN, + "reason": "integrated", + "account": "example-bot", + } + assert t.writes == [("PATCH", ENDPOINT, {"state": "closed"})] + assert t.reads.count(ENDPOINT) == 3 + assert t.reads.count("user") == 2 + + +def test_identical_head_and_default_branch_can_close(): + t = Transport() + t.ref["object"]["sha"] = SHA + t.proof["status"] = "identical" + assert apply(t, action(integration_sha=SHA))["receipts"][0]["status"] == "applied" + + +@pytest.mark.parametrize( + "change", + [ + {"status": "diverged"}, + {"status": "behind"}, + {"merge_base_commit": {"sha": OTHER}}, + {"base_commit": {"sha": OTHER}}, + {"merge_base_commit": None}, + {"status": "identical"}, + ], +) +def test_unproven_ancestry_never_closes(change): + t = Transport() + t.proof.update(change) + assert apply(t)["receipts"][0]["status"] == "failed" + assert not t.writes + + +@pytest.mark.parametrize( + "change", + [ + {"head": {"sha": OTHER}}, + {"head": None}, + {"state": "closed"}, + {"merged": True}, + {"merged": None}, + {"number": 8}, + {"base": {"repo": {"full_name": "foreign/repo"}}}, + ], +) +def test_initial_and_last_moment_head_or_target_changes_stop_before_patch(change): + for last_moment in [False, True]: + t = Transport() + changed = {**t.pull, **change} + if last_moment: + t.pull_answers = [copy.deepcopy(t.pull), changed] + else: + t.pull = changed + assert apply(t)["receipts"][0]["status"] == "failed" + assert not t.writes + + +def test_identity_default_branch_movement_and_api_errors_fail_closed(): + for last_moment in [False, True]: + t = Transport() + changed = {"object": {"sha": OTHER, "type": "commit"}} + t.ref_answers = [copy.deepcopy(t.ref), changed] if last_moment else [changed] + assert apply(t)["receipts"][0]["status"] == "failed" + assert not t.writes + for endpoint in ["user", f"repos/{REPO}", ENDPOINT, f"repos/{REPO}/compare/{SHA}...{MAIN}"]: + t = Transport() + t.errors[endpoint] = "unreadable" + assert apply(t)["receipts"][0]["status"] == "failed" + assert not t.writes + t = Transport() + t.login = "other-account" + assert apply(t)["receipts"][0]["status"] == "failed" + assert not t.writes + + +def test_write_timeout_or_readback_race_never_claims_success_or_retries(): + t = Transport() + t.write_error = "timeout" + assert apply(t)["receipts"][0]["status"] == "failed" + assert len(t.writes) == 1 + assert apply(t)["receipts"][0]["status"] == "failed" + assert len(t.writes) == 1 + for changes in [{"head": {"sha": OTHER}}, {"merged": True}, {"number": 8}]: + t = Transport() + t.readback_changes = changes + assert apply(t)["receipts"][0]["status"] == "failed" + assert len(t.writes) == 1 + + +def test_plan_binding_duplicate_receipt_and_direct_executor_permission(tmp_path): + t = Transport() + result = apply(t, expected_plan_sha256="wrong") + assert result["writes"] == 0 and not t.reads and not t.writes + store = tmp_path / "receipts.jsonl" + assert apply(t, receipt_store=store)["receipts"][0]["status"] == "applied" + assert apply(t, receipt_store=store)["receipts"][0]["status"] == "skipped_duplicate" + assert len(t.writes) == 1 + t = Transport() + _, error = typed_actions.exec_close(t, action().params, config(enabled=False)) + assert error and not t.reads and not t.writes From ec4e7f05273e4227280309500c078ca4c44bacf0 Mon Sep 17 00:00:00 2001 From: ellmos-multi-bot Date: Sun, 25 Oct 2026 21:28:02 +0100 Subject: [PATCH 2/2] style(core): format close action and policy switch Keep Python AST unchanged while satisfying the repository formatter gate. Agent-Session: 01a16e5b-dcd2-7521-8d7a-e78fe113b4f1 --- src/githubbot_core/multibot.py | 8 +++- src/githubbot_core/typed_actions.py | 61 ++++++++++++++++++++++------- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/src/githubbot_core/multibot.py b/src/githubbot_core/multibot.py index 213ef0b..9f029ae 100644 --- a/src/githubbot_core/multibot.py +++ b/src/githubbot_core/multibot.py @@ -84,7 +84,13 @@ def policy_settings(raw): hours = policy["draft_auto_ready_hours"] if hours is not None and (type(hours) not in (int, float) or hours <= 0): raise ValueError("multi_bot.policy.draft_auto_ready_hours must be null or a positive number") - for key in ("draft_ready_when_mature", "trigger_missing_ci", "publish_reviews", "open_fix_prs", "close_integrated_prs"): + for key in ( + "draft_ready_when_mature", + "trigger_missing_ci", + "publish_reviews", + "open_fix_prs", + "close_integrated_prs", + ): if type(policy[key]) is not bool: raise ValueError(f"multi_bot.policy.{key} must be a boolean") return policy diff --git a/src/githubbot_core/typed_actions.py b/src/githubbot_core/typed_actions.py index 3f2746d..7e16a40 100644 --- a/src/githubbot_core/typed_actions.py +++ b/src/githubbot_core/typed_actions.py @@ -22,7 +22,16 @@ MERGE_METHODS = ("merge", "squash", "rebase") DEFAULT_PUSH_PREFIX = "bot/" -KINDS = ("pr.merge", "pr.ready", "pr.review", "pr.close", "actions.dispatch", "actions.rerun", "pr.create", "git.push_branch") +KINDS = ( + "pr.merge", + "pr.ready", + "pr.review", + "pr.close", + "actions.dispatch", + "actions.rerun", + "pr.create", + "git.push_branch", +) REVIEW_STATES = {"COMMENT": "COMMENTED", "APPROVE": "APPROVED", "REQUEST_CHANGES": "CHANGES_REQUESTED"} _git = subprocess.run # replaced in tests @@ -86,8 +95,10 @@ def param_denial(kind: str, params: dict, config: dict) -> Optional[str]: if not isinstance(settings, dict): return "typed_actions must be an object" scopes = settings.get("close_repositories") - if not isinstance(scopes, list) or not scopes or any( - not isinstance(repo, str) or not REPO.fullmatch(repo) for repo in scopes + if ( + not isinstance(scopes, list) + or not scopes + or any(not isinstance(repo, str) or not REPO.fullmatch(repo) for repo in scopes) ): return "pr.close requires explicit typed_actions.close_repositories" if params["repo"] not in scopes: @@ -338,9 +349,14 @@ def current_target(): base = pull.get("base") base_repo = base.get("repo") if isinstance(base, dict) else None if ( - error or pull.get("number") != number or pull.get("state") != "open" - or pull.get("merged") is not False or not isinstance(head, dict) or head.get("sha") != sha - or not isinstance(base_repo, dict) or base_repo.get("full_name") != repo + error + or pull.get("number") != number + or pull.get("state") != "open" + or pull.get("merged") is not False + or not isinstance(head, dict) + or head.get("sha") != sha + or not isinstance(base_repo, dict) + or base_repo.get("full_name") != repo ): return None, "pull is not open and unmerged at the exact head/repository; nothing written" return branch, None @@ -350,10 +366,14 @@ def current_target(): return None, error proof, error = transport.get(f"repos/{repo}/compare/{sha}...{integrated}") if ( - error or not isinstance(proof, dict) or proof.get("status") not in ("ahead", "identical") + error + or not isinstance(proof, dict) + or proof.get("status") not in ("ahead", "identical") or (proof.get("status") == "identical") != (sha == integrated) - or not isinstance(proof.get("base_commit"), dict) or proof["base_commit"].get("sha") != sha - or not isinstance(proof.get("merge_base_commit"), dict) or proof["merge_base_commit"].get("sha") != sha + or not isinstance(proof.get("base_commit"), dict) + or proof["base_commit"].get("sha") != sha + or not isinstance(proof.get("merge_base_commit"), dict) + or proof["merge_base_commit"].get("sha") != sha ): return None, "head is not proven integrated in the pinned default branch" latest_branch, error = current_target() @@ -367,13 +387,26 @@ def current_target(): base = after.get("base") base_repo = base.get("repo") if isinstance(base, dict) else None if ( - error or after.get("number") != number or after.get("state") != "closed" - or after.get("merged") is not False or not isinstance(head, dict) or head.get("sha") != sha - or not isinstance(base_repo, dict) or base_repo.get("full_name") != repo + error + or after.get("number") != number + or after.get("state") != "closed" + or after.get("merged") is not False + or not isinstance(head, dict) + or head.get("sha") != sha + or not isinstance(base_repo, dict) + or base_repo.get("full_name") != repo ): return None, "close readback unconfirmed or head changed; do not blindly retry" - return {"readback": {"closed": True, "merged": False, "head": sha, - "integration_sha": integrated, "reason": "integrated", "account": account}}, None + return { + "readback": { + "closed": True, + "merged": False, + "head": sha, + "integration_sha": integrated, + "reason": "integrated", + "account": account, + } + }, None def exec_dispatch(transport, params: dict, config: dict):