diff --git a/docs/Architecture.md b/docs/Architecture.md index e107f70..a5698ce 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -726,9 +726,12 @@ Installing on any other host is dangerous. - **The PR may have moved on meanwhile.** After pushing a fix, the agent checks the PR: if it merged before the fix landed, the fix goes out as a new PR; if it was closed, the agent says so and asks. -- **Merging follows the trust dial** (L3). Today the operator merges. If the dial - lets the agent merge its own PRs once checks pass, branch protection must require - an approval the agent's account can't give, so Gitea enforces the rule. +- **Merging follows the trust dial** (L3), set per project (`trust.merge`). By + default the operator merges. Set to `approved`, the agent merges a PR once an + operator has approved its current commit and its checks pass; branch + protection requires that approval from an operators-only whitelist and + dismisses stale approvals, so Gitea enforces the rule, and the agent checks at + start that its own unapproved merge is refused. ### D6. Action classes diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index 319a323..0b9b2db 100644 --- a/docs/Development-Plan.md +++ b/docs/Development-Plan.md @@ -25,7 +25,7 @@ Only `nisse-dev` has real code; `nisse-core` and `nisse-ops` are placeholders. |---|---|---| | **L1** Work model | mostly done | Work items with an active pointer (M1.1); issue intake (M2.1); queue, parking and blocked items (M2.2) on the new loop; no grace period yet | | **L2** Agent loop | mostly done | The loop (Phase 2), the only one since the switch-over (M2.8): driver runs, waits, `stop`, `abandon`, question runs | -| **L3** Authority | partial | Go-aheads bound to the restatement's revision, on the issue and its conversation (M2.1); action classes and the fast path, with `allow` bound to one dangerous action (M3.1); no trust dial yet (M3.3) | +| **L3** Authority | partial | Go-aheads bound to the restatement's revision, on the issue and its conversation (M2.1); action classes and the fast path, with `allow` bound to one dangerous action (M3.1); the trust dial, `trust.merge` (M3.3) | | **L4** Classifier | mostly done | On the classifier role (the local model), for what the fast path doesn't know; fails closed; overrides export as an evaluation set (M3.2). Promoting its verdicts into rules is by hand | | **L5** Conversation | partial | Matrix transport, mentions, lenient commands; on the new loop, a thread per item, named and ambiguous commands, plain-language go-aheads (M2.3); little explanation | | **L6** Context | partial | Context fitting, per-role views and orchestrator facts (M1.2); no scope or agent memory | @@ -355,8 +355,25 @@ fail-closed verdicts, overrides as an evaluation set. - Deferred: promoting verdicts the classifier keeps agreeing on into rules (by hand, from exported cases); the trust dial (M3.3). -**M3.3 The trust dial.** *Needs discussion.* Per project (D8); agent merging behind -protection it can't satisfy, verified by a test (D4, D5, #65). +**M3.3 The trust dial.** *Needs discussion.* **Done** (this PR). Per project (D8); +agent merging behind protection it can't satisfy, verified by a test (D4, D5, #65). +- `trust.merge` per project: `operator` (the default) or `approved`: the harness + (never the model) merges a PR in the PR watch once an approving review from a + `gitea.operators` account is on the PR's current commit, its checks pass (no + checks at all counts as passing; pending waits) and it's mergeable; then the + item goes on as after any merge. Each reason to hold off is said once. +- Protection, verified at start per `approved` project: a throwaway branch and + PR made through the API (never the workspace), whose merge must be refused; + if it isn't, the operator merges until a restart. Real Gitea is tested too + (`test_trust_live.py`): refused unapproved, merged once approved, refused + again after a commit lands on the approved PR. +- Test and bring-up repositories get the protection: approvals from an + operators-only whitelist, stale approvals dismissed, the agent on the merge + whitelist. +- Decided: two levels only (no merging on checks alone, D5); verification by + behaviour, since a collaborator can't read protection and must never set it. + Deferred: a project-setup script for the operator (protection, labels) and a + label check, #130. ### Phase 4 — The environment diff --git a/nisse-dev.example.yaml b/nisse-dev.example.yaml index 755d1ac..a1eb63b 100644 --- a/nisse-dev.example.yaml +++ b/nisse-dev.example.yaml @@ -29,6 +29,10 @@ projects: # DATABASE_URL: {value: "${DEMO_DATABASE_URL}", visible: commands} # DEPLOY_TOKEN: {value: "${DEMO_DEPLOY_TOKEN}", visible: harness} # egress: [pypi.org] # beyond the default; recorded, not yet enforced + # trust: # the trust dial (L3, D5) + # merge: approved # I merge PRs an operator approved; default operator. + # # Needs protection: approvals only operators can give, + # # stale approvals dismissed, me on the merge whitelist. llm: # L9: LiteLLM is the only LLM destination base_url: http://litellm.example:4000 diff --git a/nisse-dev/src/nisse_dev/config.py b/nisse-dev/src/nisse_dev/config.py index 8e184fd..d4f0d21 100644 --- a/nisse-dev/src/nisse_dev/config.py +++ b/nisse-dev/src/nisse_dev/config.py @@ -67,7 +67,12 @@ PROJECT_OPTIONAL = ( "labels", "secrets", "egress", + "trust.merge", ) +# The trust dial (L3, D5): who merges a project's PRs. "approved": the +# harness merges once an operator approved the PR's current commit and its +# checks pass, behind protection that requires that approval. +MERGE_MODES = ("operator", "approved") # Who may see a project secret (D7, D8): the model's commands get it as an # environment variable, redacted from what comes back; harness-only ones are # never in a command the model chose. @@ -315,6 +320,9 @@ def _parse_project( errors.append(f"{where}: set a push token or a push key, not both") labels = _strings(values.get("labels", []), f"{where}.labels", errors) egress = _strings(values.get("egress", []), f"{where}.egress", errors) + merge = str(values.get("trust.merge") or "operator") + if merge not in MERGE_MODES: + errors.append(f"{where}.trust.merge: must be one of {', '.join(MERGE_MODES)}") secrets = _parse_secrets(entry.get("secrets"), where, env, errors) return Project( name=name, @@ -333,6 +341,7 @@ def _parse_project( labels=labels, secrets=secrets, egress=egress, + merge=merge, ) @@ -454,6 +463,7 @@ class Project: labels: tuple[str, ...] = () # added to every issue's own (D1) secrets: Mapping[str, Secret] = field(default_factory=dict) egress: tuple[str, ...] = () # beyond the default; recorded, not yet enforced + merge: str = "operator" # the trust dial (L3): who merges its PRs def command_secrets(self) -> dict[str, str]: """The secrets the model's commands may see, by name.""" diff --git a/nisse-dev/src/nisse_dev/gitea.py b/nisse-dev/src/nisse_dev/gitea.py index 96a317e..4641f3e 100644 --- a/nisse-dev/src/nisse_dev/gitea.py +++ b/nisse-dev/src/nisse_dev/gitea.py @@ -10,6 +10,7 @@ idempotent across a crash: boot reconciliation (L8) asks Gitea whether the effect already happened rather than repeating it. """ +import base64 from dataclasses import dataclass from typing import Any @@ -33,6 +34,18 @@ class PullRequest: head: str url: str merged: bool = False + head_sha: str = "" # the commit the PR is at now + mergeable: bool | None = None # None: Gitea didn't say + + +@dataclass(frozen=True) +class Approval: + """An approving review, and the commit it approved (L3: approvals bind + to exactly what was approved).""" + + author: str + commit: str + stale: bool = False # Gitea's mark: commits were pushed since @dataclass(frozen=True) @@ -286,6 +299,47 @@ class GiteaClient: break return when + def approvals(self, repo: str, index: int) -> list[Approval]: + """The PR's approving reviews that still stand: not dismissed.""" + return [ + Approval( + author=(review.get("user") or {}).get("login", ""), + commit=str(review.get("commit_id") or ""), + stale=bool(review.get("stale")), + ) + for review in ( + self._request("GET", f"/repos/{repo}/pulls/{index}/reviews") or [] + ) + if review.get("state") == "APPROVED" and not review.get("dismissed") + ] + + def checks(self, repo: str, sha: str) -> str: + """A commit's combined check status: success, pending, failure or + error; "none" when no check reported on it at all.""" + data = self._request("GET", f"/repos/{repo}/commits/{sha}/status") or {} + if not data.get("total_count") and not data.get("statuses"): + return "none" + return str(data.get("state") or "pending") + + def branch_with_file( + self, repo: str, *, base: str, branch: str, path: str, text: str, message: str + ) -> None: + """A new branch off ``base`` with one file added, made through the + API: no clone, no workspace.""" + self._request( + "POST", + f"/repos/{repo}/contents/{path}", + json={ + "content": base64.b64encode(text.encode()).decode(), + "message": message, + "branch": base, + "new_branch": branch, + }, + ) + + def delete_branch(self, repo: str, name: str) -> None: + self._request("DELETE", f"/repos/{repo}/branches/{name}") + def repository(self, repo: str) -> dict[str, Any]: """The repository as this account sees it, permissions included.""" return dict(self._request("GET", f"/repos/{repo}")) @@ -320,6 +374,8 @@ def _pull_request(data: dict[str, Any]) -> PullRequest: head=(data.get("head") or {}).get("ref", ""), url=data.get("html_url", ""), merged=bool(data.get("merged")), + head_sha=(data.get("head") or {}).get("sha", ""), + mergeable=data.get("mergeable"), ) diff --git a/nisse-dev/src/nisse_dev/loop/agent.py b/nisse-dev/src/nisse_dev/loop/agent.py index 2102755..ab4457c 100644 --- a/nisse-dev/src/nisse_dev/loop/agent.py +++ b/nisse-dev/src/nisse_dev/loop/agent.py @@ -42,7 +42,7 @@ from nisse_dev.config import GENERAL from nisse_dev.delegation import DELEGATION_INTENT, Delegation from nisse_dev.expert import BudgetExhausted, ExpertDesk from nisse_dev.git import GitError -from nisse_dev.gitea import GiteaClient, GiteaError, Issue +from nisse_dev.gitea import GiteaClient, GiteaError, Issue, PullRequest from nisse_dev.llm import LLMError, RoleMap from nisse_dev.loop.actions import classify from nisse_dev.loop.classifier import IN_SCOPE, ActionClassifier, Scope @@ -227,6 +227,8 @@ class Project: workspace: Workspace | None = None milestones: Milestones | None = None labels: tuple[str, ...] = () + base: str = "main" + merge: str = "operator" # the trust dial (L3): who merges its PRs def _restatement_problem(args: dict[str, object]) -> str | None: @@ -297,6 +299,9 @@ class LoopAgent: self._helpers = helpers # L4: judges what the fast path doesn't know; without one, it runs. self._classifier = classifier + # The trust dial (M3.3): projects whose protection was checked, and + # whether the harness may merge their approved PRs. + self._merging: dict[str, bool] = {} # Verdicts by (item, agreed revision, action): judged once (M3.2). self._judged: dict[tuple[str, int, str], tuple[Verdict, tuple[str, str]]] = {} self._clock = clock @@ -385,6 +390,9 @@ class LoopAgent: """Resume waiting points as they were, and never resume interrupted work blindly: an item cut off mid-run waits on the operator.""" self._store.set_active(None, cause="boot") + for project in self._projects.values(): + if project.merge == "approved": + self._may_merge(project) # D4: verified at start, not assumed self._reconcile_publishing() self._reconcile_filing() self._reconcile_delegations() @@ -408,6 +416,126 @@ class LoopAgent: thread=loop.thread, ) + # -- the trust dial (L3, D5; M3.3) -------------------------------------------- + + def _merge_if_approved( + self, item: WorkItem, loop: Loop, repo: str, pr: PullRequest + ) -> tuple[PullRequest, Loop]: + """Merge the PR when an operator approved its current commit and its + checks pass. Gitea enforces the approval (protection); this only + decides when to ask it. Each reason to hold off is said once.""" + try: + approvals = self._gitea.approvals(repo, pr.number) + checks = self._gitea.checks(repo, pr.head_sha) + except GiteaError as exc: + self._oplog.emit("loop.watch_unavailable", cause=str(exc)) + return pr, loop + by = [ + a.author + for a in approvals + if a.author in self._gitea_operators + and a.commit == pr.head_sha + and not a.stale + ] + if not by: + return pr, loop + held = "" + if checks in ("failure", "error"): + held = f"its checks failed ({checks}), so I'll leave it unmerged" + elif checks not in ("success", "none"): + return pr, loop # pending: wait for them + elif pr.mergeable is False: + held = "it has conflicts with the base branch, so I can't merge it" + elif not self._may_merge(self._at(item)): + return pr, loop # protection unverified: said when checked + if not held: + try: + self._gitea.merge_pull_request(repo, pr.number) + except GiteaError as exc: + held = ( + f"Gitea refused my merge ({exc}); I'll try again, or you " + "can merge it" + ) + else: + self._fact(item, f"merged PR #{pr.number} after {by[0]}'s approval") + self._transport.post( + f"Merged PR #{pr.number} after {by[0]}'s approval.", + thread=loop.thread, + ) + return replace(pr, merged=True, state="closed"), loop + mark = f"{pr.head_sha}:{held}" + if loop.merge_held != mark: + self._transport.post( + f"{by[0]} approved PR #{pr.number}, but {held}.", thread=loop.thread + ) + loop = replace(loop, merge_held=mark) + return pr, loop + + def _may_merge(self, project: Project) -> bool: + """D4: protection verified, not assumed. Once per start, a check + that Gitea refuses the agent's merge without an approval; if it + doesn't, the operator merges until it's fixed.""" + if project.merge != "approved": + return False + if project.name not in self._merging: + problem = self._merge_protection_problem(project) + self._merging[project.name] = problem is None + self._oplog.emit( + "loop.merge_protection", + cause="start" if problem is None else problem, + project=project.name, + verified=problem is None, + ) + if problem is not None: + self._transport.post( + f"{project.repo} is set to have me merge approved PRs, but " + f"{problem}. I'll leave merging to you until that's fixed " + "and I've restarted." + ) + return self._merging[project.name] + + def _merge_protection_problem(self, project: Project) -> str | None: + """A throwaway branch and PR, made through the API (never the + workspace, which may hold an item's work): merging it must be + refused. Closed and deleted either way.""" + branch = f"nisse-check-{uuid.uuid4().hex[:6]}" + try: + self._gitea.branch_with_file( + project.repo, + base=project.base, + branch=branch, + path=".nisse-merge-check", + text="Checks that merging needs an approval; never merged.\n", + message="nisse-dev: merge check", + ) + except GiteaError as exc: + return f"I couldn't make a branch to check merging: {exc}" + pr = None + try: + pr = self._gitea.open_pull_request( + project.repo, + head=branch, + base=project.base, + title="nisse-dev merge check (never merged)", + body="Checks that merging needs an approval; closed straight away.", + ) + try: + self._gitea.merge_pull_request(project.repo, pr.number) + except GiteaError: + return None # refused, as it should be + return ( + f"I could merge my own PR #{pr.number} without an approval " + f"(it added .nisse-merge-check to {project.base})" + ) + except GiteaError as exc: + return f"I couldn't open a PR to check merging: {exc}" + finally: + if pr is not None: + with contextlib.suppress(GiteaError): + self._gitea.close_pull_request(project.repo, pr.number) + with contextlib.suppress(GiteaError): + self._gitea.delete_branch(project.repo, branch) + def _reconcile_delegations(self) -> None: """L8, L9: a delegation cut off by a crash is discarded, its branch deleted and the milestone's left as it was. The item, cut off mid- @@ -613,6 +741,15 @@ class LoopAgent: ) new = True position = item.position + if ( + not pr.merged + and pr.state == "open" + and not new + and item.position == WAITING + and loop.wait == MERGE + and self._at(item).merge == "approved" + ): + pr, loop = self._merge_if_approved(item, loop, repo, pr) if pr.merged: self._fact(item, f"PR #{loop.pr} merged") loop, position = replace(loop, pr=0, pr_url="", branch=""), READY diff --git a/nisse-dev/src/nisse_dev/loop/build.py b/nisse-dev/src/nisse_dev/loop/build.py index beac127..31cadd6 100644 --- a/nisse-dev/src/nisse_dev/loop/build.py +++ b/nisse-dev/src/nisse_dev/loop/build.py @@ -230,6 +230,8 @@ def project_runtime( name=entry.name, repo=entry.repo, labels=entry.labels, + base=entry.base_branch, + merge=entry.merge, workspace=Workspace( host, entry.workspace, diff --git a/nisse-dev/src/nisse_dev/loop/items.py b/nisse-dev/src/nisse_dev/loop/items.py index 3c08725..bf29ee5 100644 --- a/nisse-dev/src/nisse_dev/loop/items.py +++ b/nisse-dev/src/nisse_dev/loop/items.py @@ -74,6 +74,9 @@ class Loop: # exactly that action, by its key). pending_action: str = "" allowed: str = "" + # The trust dial (M3.3): why an approved PR was last left unmerged, so + # it's said once per commit and reason. + merge_held: str = "" def to_data(self) -> dict[str, Any]: return asdict(self) diff --git a/nisse-dev/tests/test_config.py b/nisse-dev/tests/test_config.py index 1b2f9b6..fe52c72 100644 --- a/nisse-dev/tests/test_config.py +++ b/nisse-dev/tests/test_config.py @@ -511,3 +511,10 @@ def test_capability_profiles_are_read() -> None: def test_a_bad_profile_is_reported() -> None: with pytest.raises(ConfigError, match="profiles.m.colour: unknown setting"): build(profiles={"m": {"colour": "red"}}) + + +def test_the_trust_dial_defaults_to_the_operator_merging() -> None: + assert only(build()).merge == "operator" + assert only(build(**{f"{P}trust__merge": "approved"})).merge == "approved" + with pytest.raises(ConfigError, match="trust.merge: must be one of operator"): + build(**{f"{P}trust__merge": "always"}) diff --git a/nisse-dev/tests/test_gitea.py b/nisse-dev/tests/test_gitea.py index cdad09e..8340093 100644 --- a/nisse-dev/tests/test_gitea.py +++ b/nisse-dev/tests/test_gitea.py @@ -248,3 +248,74 @@ def test_assigned_at_is_the_last_assignment_across_pages() -> None: assert client(handler).assigned_at("o/r", 6, "dev-agent") == "t4" assert client(handler).assigned_at("o/r", 6, "nobody") is None + + +def test_approvals_are_the_standing_approving_reviews() -> None: + reviews = [ + {"state": "APPROVED", "user": {"login": "don"}, "commit_id": "abc"}, + { + "state": "APPROVED", + "user": {"login": "don"}, + "commit_id": "old", + "stale": True, + }, + { + "state": "APPROVED", + "user": {"login": "x"}, + "commit_id": "abc", + "dismissed": True, + }, + {"state": "REQUEST_CHANGES", "user": {"login": "don"}, "commit_id": "abc"}, + {"state": "COMMENT", "user": {"login": "don"}, "commit_id": "abc"}, + ] + handler = lambda request: httpx.Response(200, json=reviews) + approvals = client(handler).approvals("o/r", 3) + assert [(a.author, a.commit, a.stale) for a in approvals] == [ + ("don", "abc", False), + ("don", "old", True), + ] + + +def test_checks_are_the_combined_status_or_none() -> None: + def handler(request: httpx.Request) -> httpx.Response: + assert request.url.path == "/api/v1/repos/o/r/commits/abc/status" + return httpx.Response(200, json=answer) + + answer: dict = {"state": "", "total_count": 0, "statuses": []} + assert client(handler).checks("o/r", "abc") == "none" + answer = {"state": "pending", "total_count": 1, "statuses": [{}]} + assert client(handler).checks("o/r", "abc") == "pending" + answer = {"state": "success", "total_count": 2, "statuses": [{}, {}]} + assert client(handler).checks("o/r", "abc") == "success" + + +def test_a_pull_request_carries_its_head_commit_and_mergeability() -> None: + data = { + **pr_json(5, "m-1"), + "head": {"ref": "m-1", "sha": "f00"}, + "mergeable": True, + } + pr = client(lambda request: httpx.Response(200, json=data)).pull_request("o/r", 5) + assert (pr.head_sha, pr.mergeable) == ("f00", True) + + +def test_a_check_branch_is_made_through_the_api() -> None: + seen: list = [] + + def handler(request: httpx.Request) -> httpx.Response: + seen.append((request.method, request.url.path, request.content)) + return httpx.Response(201, json={}) + + gitea = client(handler) + gitea.branch_with_file( + "o/r", base="main", branch="chk", path="f", text="x", message="m" + ) + gitea.delete_branch("o/r", "chk") + body = json.loads(seen[0][2]) + assert (seen[0][0], seen[0][1]) == ("POST", "/api/v1/repos/o/r/contents/f") + assert (body["branch"], body["new_branch"], body["content"]) == ( + "main", + "chk", + "eA==", + ) + assert seen[1][:2] == ("DELETE", "/api/v1/repos/o/r/branches/chk") diff --git a/nisse-dev/tests/test_loop_milestones.py b/nisse-dev/tests/test_loop_milestones.py index 196343d..c2f8d48 100644 --- a/nisse-dev/tests/test_loop_milestones.py +++ b/nisse-dev/tests/test_loop_milestones.py @@ -14,7 +14,14 @@ from nisse_dev.cancel import Cancel from nisse_dev.canonical import ToolCall from nisse_dev.execution import SSHHost from nisse_dev.git import GitRepo -from nisse_dev.gitea import Comment, GiteaError, Issue, PullRequest, ReviewNote +from nisse_dev.gitea import ( + Approval, + Comment, + GiteaError, + Issue, + PullRequest, + ReviewNote, +) from nisse_dev.loop.agent import LoopAgent from nisse_dev.loop.driver import Driver from nisse_dev.loop.items import ANSWER, MERGE, WAITING, loop_of @@ -62,6 +69,10 @@ class Forge: self.deps: dict[int, list[int]] = {} # issue -> the issues it depends on self.assigned: list[int] = [] # issues the agent assigned to itself self.reassigned: dict[int, str] = {} # issue -> when assigned again + # The trust dial (M3.3): reviews, check statuses, merges by the agent. + self.approved: dict[int, list[Approval]] = {} + self.status: dict[str, str] = {} # commit -> combined check state + self.agent_merged: list[int] = [] # intake def assigned_issues(self, repo: str, assignee: str) -> list[Issue]: @@ -91,7 +102,40 @@ class Forge: # PRs def _pr(self, n: int) -> PullRequest: p = self.prs[n] - return PullRequest(n, p["state"], p["head"], f"https://g/pr/{n}", p["merged"]) + return PullRequest( + n, + p["state"], + p["head"], + f"https://g/pr/{n}", + p["merged"], + head_sha=self.head_sha(n), + ) + + def head_sha(self, n: int) -> str: + ref = f"refs/heads/{self.prs[n]['head']}" + found = subprocess.run( + ["git", "--git-dir", str(self.bare), "rev-parse", "--verify", "-q", ref], + capture_output=True, + text=True, + check=False, + ) + return found.stdout.strip() + + def approve(self, n: int, by: str = "don", sha: str = "") -> None: + """A review approving the PR as it is now (or at ``sha``).""" + self.approved.setdefault(n, []).append(Approval(by, sha or self.head_sha(n))) + + def approvals(self, repo: str, index: int) -> list[Approval]: + return self.approved.get(index, []) + + def checks(self, repo: str, sha: str) -> str: + return self.status.get(sha, "none") + + def branch_with_file(self, repo: str, *, base, branch, path, text, message) -> None: + git("--git-dir", str(self.bare), "update-ref", f"refs/heads/{branch}", base) + + def delete_branch(self, repo: str, name: str) -> None: + git("--git-dir", str(self.bare), "update-ref", "-d", f"refs/heads/{name}") def find_pull_request(self, repo: str, head: str) -> PullRequest | None: for n, p in self.prs.items(): @@ -150,8 +194,15 @@ class Forge: return {"protected": self.protected, "user_can_push": not self.protected} def merge_pull_request(self, repo: str, index: int) -> None: - if self.protected: + """As protection would have it: an operator's approval of the + PR's current commit, or no merge.""" + head = self.head_sha(index) + if self.protected and not any( + a.author == "don" and a.commit == head and not a.stale + for a in self.approved.get(index, []) + ): raise GiteaError("gitea POST merge returned 405: approvals needed") + self.agent_merged.append(index) self.merge(index) # what the operator does in Gitea diff --git a/nisse-dev/tests/test_loop_properties.py b/nisse-dev/tests/test_loop_properties.py index b388b7c..99d46b1 100644 --- a/nisse-dev/tests/test_loop_properties.py +++ b/nisse-dev/tests/test_loop_properties.py @@ -243,8 +243,33 @@ class Walk: self.steps: list[str] = [] self.shown_at_go: list[str] = [] # item:revision on screen at each go self.now = 1000.0 + # The trust dial (M3.3): half the walks have the agent merge approved + # PRs, behind protection that refuses anything else. + self.merge = "approved" if seed % 2 == 0 else "operator" + self.agent_merges: list[tuple[int, bool]] = [] # (PR, approved at head) + if self.merge == "approved": + for w in worlds.values(): + self._watch_merges(w["forge"]) self.agent, self.transport = self._agent() + def _watch_merges(self, forge: Forge) -> None: + """Protected, and each merge the agent asks for recorded with + whether an operator had approved the PR as it was.""" + forge.protect() + merge = forge.merge_pull_request + + def recorded(repo: str, index: int) -> None: + if not forge.prs[index]["head"].startswith("nisse-check-"): + head = forge.head_sha(index) + approved = any( + a.author == "don" and a.commit == head and not a.stale + for a in forge.approved.get(index, []) + ) + self.agent_merges.append((index, approved)) + merge(repo, index) + + forge.merge_pull_request = recorded # type: ignore[method-assign] + def reads_yes(self, proposal: str, reply: str) -> bool: """The scripted go-ahead reader: a plain yes usually reads as one, and nothing else ever does.""" @@ -270,6 +295,7 @@ class Walk: repo=w["repo_name"], workspace=Workspace(w["host"], w["repo"].root, timeout_s=30), milestones=w["milestones"], + merge=self.merge, ) for name, w in self.worlds.items() ], @@ -342,6 +368,8 @@ class Walk: weights.update( {"comment": 4, "merge": 2, "close-pr": 1, "comment-merge": 2} ) + if self.merge == "approved": + weights.update({"approve": 4}) action = self.rng.choices(list(weights), list(weights.values()))[0] self.steps.append(f"{action}{self.name(item)}" if item else action) if action == "intake": @@ -455,6 +483,16 @@ class Walk: self.agent.watch() forge.merge(loop.pr) self.agent._schedule() + elif action == "approve" and loop and forge.prs[loop.pr]["state"] == "open": + # An operator's approval, or someone else's; of the PR as it is, + # or of an earlier commit. + forge.approve( + loop.pr, + by=self.rng.choice(["don", "don", "mallory"]), + sha=self.rng.choice(["", "0" * 40]), + ) + self.agent.watch() + self.agent._schedule() elif action == "merge" and loop and forge.prs[loop.pr]["state"] == "open": forge.merge(loop.pr) self.agent.watch() @@ -497,6 +535,10 @@ class Walk: def check(self) -> None: where = f"seed {self.seed}, steps {self.steps}" + # D5: the agent asks Gitea to merge only what an operator approved + # as it is (Gitea would refuse the rest; the harness mustn't ask). + unapproved = [pr for pr, approved in self.agent_merges if not approved] + assert not unapproved, f"merges_only_approved: {unapproved}: {where}" self._dangerous_only_when_allowed(where) store = self.world["store"] items = self.items() diff --git a/nisse-dev/tests/test_loop_trust.py b/nisse-dev/tests/test_loop_trust.py new file mode 100644 index 0000000..c899b30 --- /dev/null +++ b/nisse-dev/tests/test_loop_trust.py @@ -0,0 +1,187 @@ +"""The trust dial (L3, D5; M3.3): a project set to `merge: approved` has its +PRs merged by the harness once an operator approved the PR's current commit +and its checks pass. Gitea enforces the approval (protection); the agent +checks at start that it does, and otherwise leaves merging to the operator.""" + +import io +from typing import Any + +from test_loop_milestones import ( + DON, + FINISH, + RESTATE, + WORK, + Forge, + ScriptedModel, + call, + say, +) +from test_loop_milestones import item as the_item +from test_loop_milestones import world as world # noqa: PLC0414 -- the fixture + +from nisse_dev.cancel import Cancel +from nisse_dev.loop.agent import LoopAgent, Project +from nisse_dev.loop.driver import Driver +from nisse_dev.loop.items import MERGE, loop_of +from nisse_dev.oplog import OpLog +from nisse_dev.tools import Workspace +from nisse_dev.transport import InMemoryTransport +from nisse_dev.worker import InlineWorker + +CLOSE = [call("close_item", summary="added and merged")] + + +def agent_for( + world: dict[str, Any], model: ScriptedModel, merge: str = "approved" +) -> tuple[LoopAgent, InMemoryTransport]: + transport = InMemoryTransport() + cancel = Cancel() + project = Project( + "sandbox", + "don/sandbox", + Workspace(world["host"], world["repo"].root, timeout_s=30), + world["milestones"], + merge=merge, + ) + loop = LoopAgent( + world["store"], + transport, + world["forge"], + projects=[project], + account="nisse-dev", + operators={DON}, + gitea_operators={"don"}, + driver=Driver(model, "m", cancel=cancel), # type: ignore[arg-type] + worker=InlineWorker(), + cancel=cancel, + oplog=OpLog("nisse-dev", io.StringIO()), + ) + transport.on_message(loop.on_message) + return loop, transport + + +def to_review(world: dict[str, Any], merge: str = "approved", *more: Any) -> Any: + """An item whose PR is up, waiting for the merge.""" + world["forge"].protect() + model = ScriptedModel(RESTATE, [*WORK, *FINISH], *more) + agent, transport = agent_for(world, model, merge) + agent.boot() + agent.intake() + say(transport, world, "go") + item = the_item(world) + assert loop_of(item).wait == MERGE, (item.position, loop_of(item), said(transport)) + return agent, transport + + +def said(transport: InMemoryTransport) -> list[str]: + return [m.text for m in transport.outbound] + + +def test_an_approved_pr_is_merged_and_the_item_goes_on(world: dict[str, Any]) -> None: + forge: Forge = world["forge"] + agent, transport = to_review(world, "approved", CLOSE) + pr = loop_of(the_item(world)).pr + agent.watch() + assert forge.agent_merged == [] # not approved yet + forge.approve(pr) + agent.watch() + agent._schedule() + assert forge.agent_merged == [pr] + assert f"Merged PR #{pr} after don's approval." in said(transport) + assert the_item(world).status == "closed" + + +def test_the_operator_merges_by_default(world: dict[str, Any]) -> None: + forge: Forge = world["forge"] + agent, _ = to_review(world, "operator") + pr = loop_of(the_item(world)).pr + forge.approve(pr) + agent.watch() + assert forge.agent_merged == [] + assert loop_of(the_item(world)).wait == MERGE + + +def test_an_approval_of_an_older_commit_doesnt_count(world: dict[str, Any]) -> None: + forge: Forge = world["forge"] + agent, _ = to_review(world) + pr = loop_of(the_item(world)).pr + forge.approve(pr, sha="0" * 40) # what the PR was before a fix + agent.watch() + assert forge.agent_merged == [] + + +def test_only_an_operators_approval_counts(world: dict[str, Any]) -> None: + forge: Forge = world["forge"] + agent, _ = to_review(world) + pr = loop_of(the_item(world)).pr + forge.approve(pr, by="mallory") + agent.watch() + assert forge.agent_merged == [] + + +def test_pending_checks_wait_and_failed_ones_are_said_once( + world: dict[str, Any], +) -> None: + forge: Forge = world["forge"] + agent, transport = to_review(world) + pr = loop_of(the_item(world)).pr + forge.approve(pr) + forge.status[forge.head_sha(pr)] = "pending" + agent.watch() + assert forge.agent_merged == [] + forge.status[forge.head_sha(pr)] = "failure" + agent.watch() + agent.watch() + held = [t for t in said(transport) if "its checks failed" in t] + assert len(held) == 1 and forge.agent_merged == [] + forge.status[forge.head_sha(pr)] = "success" + agent.watch() + assert forge.agent_merged == [pr] + + +def test_protection_is_checked_at_start(world: dict[str, Any]) -> None: + forge: Forge = world["forge"] + forge.protect() + agent, transport = agent_for(world, ScriptedModel()) + agent.boot() + assert agent._merging == {"sandbox": True} + # The check's branch and PR are gone again. + assert all(p["state"] == "closed" for p in forge.prs.values()) + assert forge.agent_merged == [] + assert not [t for t in said(transport) if "leave merging to you" in t] + + +def test_unprotected_means_the_operator_merges(world: dict[str, Any]) -> None: + forge: Forge = world["forge"] # not protected: the agent could merge anything + model = ScriptedModel(RESTATE, [*WORK, *FINISH]) + agent, transport = agent_for(world, model) + agent.boot() + assert agent._merging == {"sandbox": False} + assert any("I could merge my own PR" in t for t in said(transport)) + agent.intake() + say(transport, world, "go") + pr = loop_of(the_item(world)).pr + forge.approve(pr) + agent.watch() + assert pr not in forge.agent_merged # left to the operator + + +def test_a_refused_merge_is_said_once_and_left_to_the_operator( + world: dict[str, Any], +) -> None: + from nisse_dev.gitea import GiteaError + + forge: Forge = world["forge"] + agent, transport = to_review(world) + pr = loop_of(the_item(world)).pr + forge.approve(pr) + + def refuse(repo: str, index: int) -> None: + raise GiteaError("gitea POST merge returned 405: not mergeable") + + forge.merge_pull_request = refuse # type: ignore[method-assign] + agent.watch() + agent.watch() + refused = [t for t in said(transport) if "Gitea refused my merge" in t] + assert len(refused) == 1 + assert loop_of(the_item(world)).wait == MERGE diff --git a/pyproject.toml b/pyproject.toml index dc8ee20..1d63d3e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,6 +21,7 @@ markers = [ "integration: end-to-end tests that exercise installed entry points or external services", "e2e: the whole agent against real services, run after each milestone (scripts/e2e.sh)", "smoke: the end-to-end tests every PR runs; the rest on e2e:full and before a release", + "merge(mode): the end-to-end project's trust dial, trust.merge (default operator)", ] [tool.ruff] diff --git a/tests/integration/e2e_support.py b/tests/integration/e2e_support.py index f2d31f2..5200abd 100644 --- a/tests/integration/e2e_support.py +++ b/tests/integration/e2e_support.py @@ -138,13 +138,20 @@ def make_repo( "POST", f"/repos/{full}/branch_protections", # Pushes only by the operator (who sets up fixtures, as an owner - # can); the agent's are refused, and merging needs an approval. + # can); the agent's are refused. Merging needs an approval only + # the operator can give, on the PR as it is now (D5, M3.3): the + # agent may merge once it has one. json={ "branch_name": "main", "enable_push": True, "enable_push_whitelist": True, "push_whitelist_usernames": [OWNER], "required_approvals": 1, + "enable_approvals_whitelist": True, + "approvals_whitelist_username": [OWNER], + "dismiss_stale_approvals": True, + "enable_merge_whitelist": True, + "merge_whitelist_usernames": [OWNER, agent], }, ) # The labels the agent reads (D1), as a project would have them. diff --git a/tests/integration/test_end_to_end_loop.py b/tests/integration/test_end_to_end_loop.py index fbaf78d..1e4b09e 100644 --- a/tests/integration/test_end_to_end_loop.py +++ b/tests/integration/test_end_to_end_loop.py @@ -61,7 +61,9 @@ def running( ssh_host: SSHHost, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + request: pytest.FixtureRequest, ) -> Iterator[tuple[RunningLoop, InMemoryTransport, str]]: + mode = request.node.get_closest_marker("merge") run = uuid.uuid4().hex[:8] stack = contextlib.ExitStack() # Kept outside pytest's temp directories, which pytest prunes, so a @@ -80,6 +82,7 @@ def running( push_key=fresh.push_key, workspace=Path(f"/tmp/nisse-e2e-{run}/sandbox"), build_host=build_host(ssh_host), + merge=mode.args[0] if mode else "operator", ) config = replace( live_config, @@ -229,6 +232,27 @@ class Operator: state = gitea._request("GET", f"/repos/{repo}/issues/{issue}")["state"] assert state == "closed" + def approve_and_wait(self, issue: int) -> None: + """Approve the item's PR, as the operator, and see the agent merge + it and close the item (trust.merge: approved).""" + gitea, repo = self.agent.gitea, self.agent.config.projects[0].repo + as_operator = operator_gitea(self.agent.config.gitea_url) + pr = loop_of(self.item(issue)).pr + as_operator._request( + "POST", f"/repos/{repo}/pulls/{pr}/reviews", json={"event": "APPROVED"} + ) + deadline = time.monotonic() + DEADLINE_S + nudges = 0 + while self.item(issue).status != "closed" and time.monotonic() < deadline: + self.tick() + if loop_of(self.item(issue)).wait == ANSWER: + nudges += 1 + assert nudges <= MAX_ANSWERS, f"never closed: {self.last()}" + self.say("It's merged; please close the item.", issue) + self.settle() + assert gitea._request("GET", f"/repos/{repo}/pulls/{pr}")["merged"] + assert self.item(issue).outcome == "completed" + def tidy(self, issues: list[int], prs: list[int]) -> None: """Leave the sandbox tidy: whatever is still open gets closed.""" gitea, repo = self.agent.gitea, self.agent.config.projects[0].repo @@ -690,3 +714,28 @@ def test_a_question_leaves_the_restatement_up( assert loop_of(operator.item(issues[0])).started finally: operator.tidy(issues, []) + + +@pytest.mark.merge("approved") +def test_an_approved_pr_is_merged_by_the_agent( + running: tuple[RunningLoop, InMemoryTransport, str], +) -> None: + """M3.3: on a project set to merge approved PRs, the operator only + approves; the agent merges and closes the item.""" + agent, transport, run = running + operator = Operator(agent, transport) + repo = agent.config.projects[0].repo + as_operator = operator_gitea(agent.config.gitea_url) + issue = None + pr = 0 + try: + issue = as_operator.create_issue(repo, f"vowels_{run}", task(run)) + as_operator.assign_issue(repo, issue, agent.account) + operator.tick() + item = operator.drive_until(MERGE, issue) + pr = loop_of(item).pr + operator.approve_and_wait(issue) + said = " ".join(m.text for m in transport.outbound) + assert f"Merged PR #{pr} after" in said + finally: + operator.tidy([issue] if issue is not None else [], [pr] if pr else []) diff --git a/tests/integration/test_trust_live.py b/tests/integration/test_trust_live.py new file mode 100644 index 0000000..364abc9 --- /dev/null +++ b/tests/integration/test_trust_live.py @@ -0,0 +1,75 @@ +"""The trust dial's hard wall, on a real Gitea (D4, D5; M3.3): with the +protection a `merge: approved` project has, the agent's own token can't +merge its PR until an operator approves the PR as it is now; it can then; +and a commit pushed after the approval needs approving again.""" + +import base64 +import uuid +from pathlib import Path + +import pytest +from e2e_support import fresh_repo, operator_gitea + +from nisse_dev.config import AgentConfig +from nisse_dev.gitea import GiteaClient, GiteaError + +pytestmark = pytest.mark.integration + + +def add_file(gitea: GiteaClient, repo: str, branch: str, path: str) -> None: + gitea._request( + "POST", + f"/repos/{repo}/contents/{path}", + json={ + "content": base64.b64encode(f"{path}\n".encode()).decode(), + "message": f"add {path}", + "branch": branch, + }, + ) + + +def approve(operator: GiteaClient, repo: str, pr: int) -> None: + operator._request( + "POST", f"/repos/{repo}/pulls/{pr}/reviews", json={"event": "APPROVED"} + ) + + +def test_merging_needs_the_operators_approval_of_the_pr_as_it_is( + tmp_path: Path, live_config: AgentConfig, gitea: GiteaClient +) -> None: + operator = operator_gitea(live_config.gitea_url) + run = uuid.uuid4().hex[:8] + with fresh_repo(operator, run, tmp_path / "keys", agent=gitea.whoami()) as repo: + name = repo.name + # The agent's PR, its branch made through the API. + gitea.branch_with_file( + name, base="main", branch="m-1", path="a.txt", text="a\n", message="a" + ) + pr = gitea.open_pull_request(name, head="m-1", base="main", title="a", body="") + with pytest.raises(GiteaError): + gitea.merge_pull_request(name, pr.number) # no approval yet + + approve(operator, name, pr.number) + [approval] = gitea.approvals(name, pr.number) + head = gitea.pull_request(name, pr.number).head_sha + assert (approval.author, approval.commit) == (operator.whoami(), head) + gitea.merge_pull_request(name, pr.number) # approved: the agent may + assert gitea.pull_request(name, pr.number).merged + + # Approved, then changed: the approval no longer stands. + gitea.branch_with_file( + name, base="main", branch="m-2", path="b.txt", text="b\n", message="b" + ) + second = gitea.open_pull_request( + name, head="m-2", base="main", title="b", body="" + ) + approve(operator, name, second.number) + add_file(gitea, name, "m-2", "c.txt") # a fix pushed after the approval + after = gitea.pull_request(name, second.number).head_sha + assert not [ + a + for a in gitea.approvals(name, second.number) + if a.commit == after and not a.stale + ] + with pytest.raises(GiteaError): + gitea.merge_pull_request(name, second.number)