From 58ef82860b742db306e94643ca41e48ef6e2ab20 Mon Sep 17 00:00:00 2001 From: Paulo Date: Mon, 27 Jul 2026 17:00:58 +0200 Subject: [PATCH] The pull request is the subject, not the input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting a review wrote the same two facts twice: once as the subject identity and once as run input. The subject class knows its own repo and number, so the input goes — `run()` takes only what the request itself knows, and `get_workspace_kwargs()`, `get_prompt_context()` and the reviewer's prompt read the pull request off `self.subject`. A review already queued carries the old pair in its durable payload; extra keys are ignored and the subject rides separately, so it replays through the declaration. --- .../review/templates/review_pull_request.md | 2 +- backend/druks/contrib/review/workflows.py | 8 ++-- backend/tests/test_review.py | 45 +++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/backend/druks/contrib/review/templates/review_pull_request.md b/backend/druks/contrib/review/templates/review_pull_request.md index 752916e..7229ea1 100644 --- a/backend/druks/contrib/review/templates/review_pull_request.md +++ b/backend/druks/contrib/review/templates/review_pull_request.md @@ -1,6 +1,6 @@ # Pull Request Reviewer -You are reviewing pull request #{{ workflow.input.pr_number }} on `{{ workflow.input.repo }}`, +You are reviewing pull request #{{ workflow.subject.number }} on `{{ workflow.subject.repo }}`, at {{ workflow.input.requested_by }}'s request. The repo is cloned at `{{ workspace.repo_path }}`, and `gh` is authenticated there as the diff --git a/backend/druks/contrib/review/workflows.py b/backend/druks/contrib/review/workflows.py index dde3e18..047aa44 100644 --- a/backend/druks/contrib/review/workflows.py +++ b/backend/druks/contrib/review/workflows.py @@ -45,12 +45,10 @@ async def dispatch(cls, *, repo: str, pr_number: int, requested_by: str) -> str: return await cls.start( subject=PullRequest.get(repo, pr_number), account_id=account.id if account else None, - repo=repo, - pr_number=pr_number, requested_by=requested_by, ) - async def run(self, repo: str, pr_number: int, requested_by: str) -> None: + async def run(self, requested_by: str) -> None: await Review.review_pull_request() async def get_workspace_kwargs(self, sandbox: "Sandbox") -> dict[str, Any]: @@ -58,7 +56,7 @@ async def get_workspace_kwargs(self, sandbox: "Sandbox") -> dict[str, Any]: # The token is the reviewer app's: it authenticates the clone and ``gh``, so the # review is authored under that identity. Siblings stay uncloned — the reviewer # clones the ones it opens, into a directory that must exist for the grant to hold. - repo = self.input.repo + repo = self.subject.repo github_token = await get_reviewer_github_client(load_settings()).token_for_repo(repo) await sandbox.write_secret( secret=github_token, remote=get_github_token_remote_path(sandbox.ssh_username) @@ -77,7 +75,7 @@ async def get_workspace_kwargs(self, sandbox: "Sandbox") -> dict[str, Any]: } async def get_prompt_context(self, **context: Any) -> dict[str, Any]: - target = ProjectRepo.get_for_repo(self.input.repo, raise_on_missing=True) + target = ProjectRepo.get_for_repo(self.subject.repo, raise_on_missing=True) return { "siblings": target.siblings(), **await super().get_prompt_context(**context), diff --git a/backend/tests/test_review.py b/backend/tests/test_review.py index b2d4187..b8c89ae 100644 --- a/backend/tests/test_review.py +++ b/backend/tests/test_review.py @@ -1,11 +1,16 @@ from pathlib import Path +from types import SimpleNamespace import pytest from druks.contrib.review.datastructures import PullRequest from druks.contrib.review.workflows import PullRequestReview +from druks.prompts import render_prompt from druks.testing import seed_run +from druks.workflows import _bind_instance from fastapi.testclient import TestClient +pytestmark = pytest.mark.usefixtures("druks_without_remote_config") + @pytest.fixture def client(tmp_path: Path, druks_db, monkeypatch): @@ -53,3 +58,43 @@ def test_the_pull_request_board_and_page_mount(client: TestClient, druks_db): assert detail["summary"]["pullRequestUrl"] == "https://github.com/acme/app/pull/7" assert [entry["kind"] for entry in detail["timeline"]] == [PullRequestReview.kind] assert client.get("/api/review/pull_request/acme/app").status_code == 404 + + +def test_the_run_carries_the_pull_request_once(): + # The repo and the number are the subject, so they are not also input: the body + # takes what only the request knows. + assert list(PullRequestReview._run_input_model.model_fields) == ["requested_by"] + + +def test_a_queued_run_replays_through_its_subject(): + # A review enqueued before the repo and number came off the input still carries + # them in its durable payload. The extra keys are ignored and the subject rides + # separately, so the body binds and reads the pull request off the declaration. + instance, run_kwargs = _bind_instance( + PullRequestReview, + PullRequest.get("acme/app", 7).identity, + {"repo": "acme/app", "pr_number": 7, "requested_by": "dev@example.com"}, + ) + + assert run_kwargs == {"requested_by": "dev@example.com"} + assert (instance.subject.repo, instance.subject.number) == ("acme/app", 7) + + +async def test_the_reviewer_prompt_names_the_pull_request_it_is_about(): + workflow = SimpleNamespace( + subject=PullRequest.get("acme/app", 7), + input=SimpleNamespace(requested_by="dev@example.com"), + ) + workspace = SimpleNamespace( + repo_path="/home/agent/work/repo", related_root="/home/agent/related" + ) + + output = await render_prompt( + "review/review_pull_request.md", + workflow=workflow, + workspace=workspace, + siblings=[], + ) + + assert "pull request #7 on `acme/app`" in output + assert "at dev@example.com's request" in output