From 89dbb81d3e34dedfe9330d5d7062701996f7f45a Mon Sep 17 00:00:00 2001 From: Dan Serbanoiu <32792904+dserbano@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:06:38 -0400 Subject: [PATCH] DRU-1044 -- Wait for CI before the review, and keep a PR that is not ready in draft --- .../contrib/software_factory/constants.py | 8 + .../contrib/software_factory/contracts.py | 56 +++++- .../druks/contrib/software_factory/journal.py | 19 ++ .../contrib/software_factory/subscribers.py | 9 + .../templates/build/implement.md | 4 +- .../contrib/software_factory/workflows.py | 75 +++++++- backend/druks/core/apis/github.py | 21 +++ backend/druks/core/webhooks/github.py | 14 ++ backend/druks/workflows.py | 11 +- .../tests/software_factory/test_build_ci.py | 62 +++++++ .../software_factory/test_build_journal.py | 21 +++ .../software_factory/test_build_plan_phase.py | 170 +++++++++++++++++- .../software_factory/test_lane_reactions.py | 13 ++ backend/tests/test_durable_sdk.py | 38 ++++ backend/tests/test_webhooks_github.py | 34 ++++ docs/configuration.md | 4 + docs/writing-an-app.md | 3 + 17 files changed, 541 insertions(+), 21 deletions(-) create mode 100644 backend/tests/software_factory/test_build_ci.py diff --git a/backend/druks/contrib/software_factory/constants.py b/backend/druks/contrib/software_factory/constants.py index f323fdba..0cbd1ff1 100644 --- a/backend/druks/contrib/software_factory/constants.py +++ b/backend/druks/contrib/software_factory/constants.py @@ -13,3 +13,11 @@ ) # A review takes minutes. A build still waiting after this long was never answered. REVIEW_TTL_SECONDS = 2 * 60 * 60 +# A repo with CI registers a check within this long of a push. With none by then, the +# repo has no CI, and the build goes on as if CI were green. +CI_REGISTER_SECONDS = 3 * 60 +# CI still running after this long goes to the review unfinished. +CI_TTL_SECONDS = 60 * 60 +# A webhook wakes the CI wait early. The poll catches a check that finished before the +# wait began, and a delivery that never came. +CI_POLL_SECONDS = 60 diff --git a/backend/druks/contrib/software_factory/contracts.py b/backend/druks/contrib/software_factory/contracts.py index b3a2f3e6..007d5123 100644 --- a/backend/druks/contrib/software_factory/contracts.py +++ b/backend/druks/contrib/software_factory/contracts.py @@ -3,6 +3,7 @@ from pydantic import BaseModel, Field, model_validator from druks.agents import AgentOutput +from druks.contrib.software_factory.constants import CI_REGISTER_SECONDS, CI_TTL_SECONDS from druks.contrib.software_factory.enums import HumanFeedbackAction, ReviewDecision from druks.workflows import Gate, OperatorReply, Workflow @@ -22,7 +23,10 @@ class ReviewWork(Gate): @classmethod async def on_wait(cls, workflow: Workflow) -> None: build = cast("Build", workflow) - await build.set_pr_draft(draft=False) + # A round parks for a person even when its CI is not green or its review requested + # changes, but its PR is not ready to merge. + if build.journal.is_ready_for_review: + await build.set_pr_draft(draft=False) await build.request_assignee_review() @@ -300,3 +304,53 @@ def to_artifact(self) -> dict[str, str]: def to_event(self) -> dict[str, str]: return {"topic": "review.completed", "summary": self.summary} + + +# A suite or status that passed or did not apply. +_GREEN_CONCLUSIONS = frozenset({"success", "neutral", "skipped"}) + + +class CiReport(BaseModel): + """CI's verdict on one delivered head. ``red`` sends the round back to implement. + ``unfinished`` is CI that did not finish in time.""" + + head_sha: str + conclusion: Literal["green", "red", "unfinished"] + + @classmethod + def judge( + cls, + head_sha: str, + suites: list[dict[str, Any]], + statuses: list[dict[str, Any]], + *, + waited: float, + ) -> "CiReport | None": + """The verdict on ``head_sha`` from its check suites and commit statuses after + ``waited`` seconds, or None while CI can still change.""" + registering = waited < CI_REGISTER_SECONDS + # Some apps leave a queued suite with no check runs for good. + suites = [suite for suite in suites if suite["latest_check_runs_count"] or registering] + if not suites and not statuses: + return None if registering else cls(head_sha=head_sha, conclusion="green") + # A suite stays open until its workflow run completes, while the check run of a + # ``needs:`` job appears only once the jobs it needs finish. + finished = all( + suite["latest_check_runs_count"] and suite["status"] == "completed" for suite in suites + ) and all(status["state"] != "pending" for status in statuses) + if not finished: + if waited < CI_TTL_SECONDS: + return None + return cls(head_sha=head_sha, conclusion="unfinished") + green = all(suite["conclusion"] in _GREEN_CONCLUSIONS for suite in suites) and all( + status["state"] == "success" for status in statuses + ) + return cls(head_sha=head_sha, conclusion="green" if green else "red") + + @property + def revision_request(self) -> str: + return ( + f"CI on `{self.head_sha}` is red. Read it with `gh pr checks` and " + "`gh run view --log-failed`, fix what your diff caused, and report the rest " + "in `checks`." + ) diff --git a/backend/druks/contrib/software_factory/journal.py b/backend/druks/contrib/software_factory/journal.py index 2ab3fb4a..49594e16 100644 --- a/backend/druks/contrib/software_factory/journal.py +++ b/backend/druks/contrib/software_factory/journal.py @@ -2,6 +2,7 @@ from typing import Any from druks.contrib.software_factory.contracts import ( + CiReport, ImplementationOutput, PlanData, ReviewOutput, @@ -56,8 +57,26 @@ def assignee_github_login(self) -> str | None: return plan.assignee_github_login return + @property + def is_ready_for_review(self) -> bool: + """The round's review of a head with green CI requested no changes.""" + ci_report = self.latest(CiReport) + if not ci_report or ci_report.conclusion != "green": + return False + reviews = self.filter(ReviewReport, after=ci_report) + return bool(reviews) and reviews[-1].decision != "request_changes" + @property def revision_request(self) -> str | None: + # Red CI sends the round back before its review, so a review recorded after it + # means the round moved on. + ci_report = self.latest(CiReport) + if ( + ci_report + and ci_report.conclusion == "red" + and not self.filter(ReviewReport, after=ci_report) + ): + return ci_report.revision_request # The newest triage's instructions drive exactly one implement round. # A review recorded after that triage means the round already ran — # the review's findings are the current request, not these. diff --git a/backend/druks/contrib/software_factory/subscribers.py b/backend/druks/contrib/software_factory/subscribers.py index dbec5b76..5a22246b 100644 --- a/backend/druks/contrib/software_factory/subscribers.py +++ b/backend/druks/contrib/software_factory/subscribers.py @@ -106,6 +106,15 @@ async def pr_review_answers_the_gate(*, repo: str, pr_number: int, payload: dict ) +@subscribe("ci.updated") +async def ci_update_wakes_the_build(*, repo: str, payload: dict) -> None: + """A build waiting on CI reads its checks again. A commit no build delivered wakes + no one.""" + for branch in payload["branches"]: + if item := await WorkItem.get_for_branch(repo=repo, branch=branch): + await Build.answer(item, "ci.updated") + + @subscribe("pr.closed") async def pr_close_settles_the_item(*, repo: str, pr_number: int, payload: dict) -> None: """GitHub announcing the verdict on a PR druks owns — one path for every merge, diff --git a/backend/druks/contrib/software_factory/templates/build/implement.md b/backend/druks/contrib/software_factory/templates/build/implement.md index a9f239b5..2a791d20 100644 --- a/backend/druks/contrib/software_factory/templates/build/implement.md +++ b/backend/druks/contrib/software_factory/templates/build/implement.md @@ -45,11 +45,11 @@ acceptance criterion — nothing more, nothing less. {% endif %}When the checkout has `.druks/software_factory/review.md`, read it before you write: it is the repo's standing rules for code and prose, and the reviewer holds your diff to it. -Implement the approved plan (rendered above as **Current plan**) on the PR branch (see "Delivering your work" below). When a **Revision request** section is rendered above, implement it on top of the plan — it is the reviewer's feedback, already triaged into instructions. Verification is yours: before pushing, run the commands in the **Verification profile** above. Fix a failure your diff causes. A failure you cannot attribute to your own diff is environment noise to report, not a gate to chase. Do not run ad hoc install, build, or smoke commands beyond that unless the plan explicitly requires changing those commands or generated outputs. Never leave dependency lockfile, generated, or cache changes unless they are part of the requested implementation. Return structured evidence for every acceptance criterion, every check you ran or intentionally did not run, changed files, and known risks. If a check was not run, include status not_run and a reason. +Implement the approved plan (rendered above as **Current plan**) on the PR branch (see "Delivering your work" below). When a **Revision request** section is rendered above, implement it on top of the plan — it is the reviewer's feedback, already triaged into instructions, or a note that CI on your last push is red. Verification is yours: before every push, run the lint and typecheck commands in the **Verification profile** above, and its test commands narrowed to the files you changed. Report each result in `checks`. Fix a failure your diff causes. A failure you cannot attribute to your own diff is environment noise to report, not a gate to chase. Do not run ad hoc install, build, or smoke commands beyond that unless the plan explicitly requires changing those commands or generated outputs. Never leave dependency lockfile, generated, or cache changes unless they are part of the requested implementation. Return structured evidence for every acceptance criterion, every check you ran or intentionally did not run, changed files, and known risks. If a check was not run, include status not_run and a reason. ## Delivering your work -When the implementation is complete you MUST commit and push it to the PR branch — the pushed commit is the deliverable. The repo is checked out on the PR branch `{{ build.branch }}` already. Stay on it: Druks finds the PR by this name. Run, from the repo root: +When the implementation is complete you MUST commit and push it to the PR branch — the pushed commit is the deliverable. The one exception is a revision request for red CI that your diff did not cause: then push nothing, report each red check in `checks` with the reason it is not yours, and report the branch's current head as `base_sha`, `head_sha`, and `commit_sha`. The repo is checked out on the PR branch `{{ build.branch }}` already. Stay on it: Druks finds the PR by this name. Run, from the repo root: ``` git status --short # review what changed diff --git a/backend/druks/contrib/software_factory/workflows.py b/backend/druks/contrib/software_factory/workflows.py index f25182e0..6a74e584 100644 --- a/backend/druks/contrib/software_factory/workflows.py +++ b/backend/druks/contrib/software_factory/workflows.py @@ -1,10 +1,13 @@ import logging +import time +from contextlib import suppress from typing import Any from pydantic import BaseModel, Field from druks.accounts import Account from druks.contrib.software_factory.contracts import ( + CiReport, ImplementationOutput, ReviewReport, ReviewWork, @@ -20,11 +23,18 @@ from druks.services.exceptions import ServiceNotConnectedError from druks.settings import get_settings from druks.skills.models import Skill -from druks.workflows import FatalError, Workflow, step +from druks.workflows import FatalError, WaitTimeout, Workflow, step from druks.workspaces import RepoWorkspace from .app import SoftwareFactory -from .constants import GITHUB_MCP_NAME, GITHUB_MCP_URL, REVIEW_TTL_SECONDS, TICKET_TOOLS +from .constants import ( + CI_POLL_SECONDS, + CI_TTL_SECONDS, + GITHUB_MCP_NAME, + GITHUB_MCP_URL, + REVIEW_TTL_SECONDS, + TICKET_TOOLS, +) from .datastructures import PullRequest from .exceptions import TicketAlreadyMerged, TicketNotRoutable from .github import get_review_actor @@ -288,15 +298,54 @@ async def _plan_phase(self) -> bool: async def _implement_phase(self) -> None: await self._create_branch() while True: - await self.implement() - review = await self._review_work() - if review.decision == "request_changes" and ( + previous = self.journal.last_implementation + delivery = await self.implement() + can_revise = ( self.journal.implementation_revision < self._settings.max_implementation_revisions - ): + ) + # The implementer pushes nothing when no red check is its diff's, and CI on + # that head has already been read. + if not previous or delivery.head_sha != previous.head_sha: + ci_report = await self._wait_for_ci() + if ci_report.conclusion == "red" and can_revise: + continue + review = await self._review_work() + if review.decision == "request_changes" and can_revise: continue if await self._work_gate(): return + async def _wait_for_ci(self) -> CiReport: + """Wait until CI has finished on the delivered head, and return its verdict.""" + head_sha = self.journal.last_implementation.head_sha + started_at = await self._now() + while not (ci_report := await self._read_ci(head_sha, started_at)): + with suppress(WaitTimeout): + await self.wait( + "ci.updated", + label="Waiting for CI", + ttl_seconds=CI_POLL_SECONDS, + release_sandbox=True, + ) + self.journal.add(ci_report) + return ci_report + + @step + async def _now(self) -> float: + return time.time() + + @step + async def _read_ci(self, head_sha: str, started_at: float) -> CiReport | None: + """CI's verdict on ``head_sha``, or None while it can still change.""" + repo = (await self.subject).repo + github = await Github.get_client(repo) + return CiReport.judge( + head_sha, + await github.list_check_suites(repo, head_sha), + await github.list_commit_statuses(repo, head_sha), + waited=time.time() - started_at, + ) + async def _review_work(self) -> ReviewReport: """Hand the pull request to the standalone review and wait for its decision.""" # The body starts the child run, so a replay gets back the review it started. @@ -315,16 +364,28 @@ async def _work_gate(self) -> bool: goes back to implementation.""" if self._policy.implementation_approval_gate() == "none": return await self._approved_work() + label = f"Review implementation on GitHub: PR #{self.pr_number}" + ci_report = self.journal.latest(CiReport) + if ci_report.conclusion == "red": + label = f"{label}. CI on {ci_report.head_sha[:7]} is red" + if ci_report.conclusion == "unfinished": + label = ( + f"{label}. CI on {ci_report.head_sha[:7]} did not finish within " + f"{CI_TTL_SECONDS // 60} minutes" + ) decision = await ReviewWork.wait( input_request={ "presentation": "external", - "label": f"Review implementation on GitHub: PR #{self.pr_number}", + "label": label, "url": f"https://github.com/{(await self.subject).repo}/pull/{self.pr_number}", } ) if decision.action == "merged": return True if decision.action == "approve": + # A person's approval stands over CI, and GitHub merges no draft. + if self._policy.on_approval == "merge": + await self._clear_draft() return await self._approved_work() if decision.action == "request_changes": return await self._triage() diff --git a/backend/druks/core/apis/github.py b/backend/druks/core/apis/github.py index fa1afcd3..7221c769 100644 --- a/backend/druks/core/apis/github.py +++ b/backend/druks/core/apis/github.py @@ -550,6 +550,27 @@ async def post_check_run( body["output"] = {**output, "annotations": list(annotations[start : start + limit])} await checks.async_update(owner, repo_name, check_run_id, data=body) + async def list_check_suites(self, repo: str, ref: str) -> list[dict[str, Any]]: + """The check suites on ``ref``: one per Actions workflow run, or per other app.""" + owner, name = repo.split("/", 1) + suites = self._github.rest.paginate( + self._github.rest.checks.async_list_suites_for_ref, + map_func=lambda response: response.parsed_data.check_suites, + owner=owner, + repo=name, + ref=ref, + per_page=100, + ) + return [suite.model_dump() async for suite in suites] + + async def list_commit_statuses(self, repo: str, ref: str) -> list[dict[str, Any]]: + """The latest status of each context on ``ref``.""" + owner, name = repo.split("/", 1) + response = await self._github.rest.repos.async_get_combined_status_for_ref( + owner, name, ref, per_page=100 + ) + return [status.model_dump() for status in response.parsed_data.statuses] + async def get_file_content( self, repo: str, diff --git a/backend/druks/core/webhooks/github.py b/backend/druks/core/webhooks/github.py index c69acb3b..96d0c898 100644 --- a/backend/druks/core/webhooks/github.py +++ b/backend/druks/core/webhooks/github.py @@ -234,6 +234,20 @@ async def on_pull_request_closed(self) -> Response: ) return _accepted() + async def on_check_run_completed(self) -> Response: + # A check on a fork's commit or a tag has no head branch. + branch = self.data["check_run"]["check_suite"]["head_branch"] + return await self._publish_ci_update([branch] if branch else []) + + async def on_status(self) -> Response: + return await self._publish_ci_update([branch["name"] for branch in self.data["branches"]]) + + async def _publish_ci_update(self, branches: list[str]) -> Response: + """Publish that a check changed on the commit these branches head. A subscriber + reads the checks themselves.""" + await publish("ci.updated", repo=_repo_name(self.data), payload={"branches": branches}) + return _accepted() + async def on_push(self) -> Response: repository = self.data["repository"] repo = repository["full_name"] diff --git a/backend/druks/workflows.py b/backend/druks/workflows.py index fc662c73..333f09ea 100644 --- a/backend/druks/workflows.py +++ b/backend/druks/workflows.py @@ -962,10 +962,15 @@ async def review( self.journal.add(reply) return reply - async def wait(self, topic: str, *, label: str, ttl_seconds: float) -> dict[str, Any]: + async def wait( + self, topic: str, *, label: str, ttl_seconds: float, release_sandbox: bool = False + ) -> dict[str, Any]: """Wait for the facts that ``answer`` delivers on ``topic``, and return them. The run - stays running, shows ``label``, and keeps its warm sandbox. A failure answered - instead raises ``WaitFailed``.""" + stays running and shows ``label``. It keeps its warm sandbox, or with + ``release_sandbox`` releases it as a gate park does. A failure answered instead + raises ``WaitFailed``.""" + if release_sandbox: + await self._reap_run() # The wait takes answers before its label shows, so an answer sent on seeing it arrives. await DBOS.set_event_async("waiting_on", topic) await set_run_phase(label) diff --git a/backend/tests/software_factory/test_build_ci.py b/backend/tests/software_factory/test_build_ci.py new file mode 100644 index 00000000..b522935b --- /dev/null +++ b/backend/tests/software_factory/test_build_ci.py @@ -0,0 +1,62 @@ +from druks.contrib.software_factory.constants import CI_REGISTER_SECONDS, CI_TTL_SECONDS +from druks.contrib.software_factory.contracts import CiReport + +HEAD = "0f3a2c1d" + + +def _suite(*, status="completed", conclusion="success", runs=1): + return { + "status": status, + "conclusion": conclusion if status == "completed" else None, + "latest_check_runs_count": runs, + } + + +def _status(state): + return {"context": "external/lint", "state": state} + + +def _judge(suites, statuses=(), *, waited=0.0): + return CiReport.judge(HEAD, list(suites), list(statuses), waited=waited) + + +def test_a_repo_without_ci_is_green_once_no_check_registers(): + assert _judge([]) is None + assert _judge([], waited=CI_REGISTER_SECONDS).conclusion == "green" + + +def test_a_workflow_between_its_needs_waves_is_still_running(): + # The jobs of the first wave finished, and the jobs that need them have no check + # run yet, but the workflow run's suite is still open. + assert _judge([_suite(status="in_progress", runs=3)]) is None + assert _judge([_suite(runs=9)]).conclusion == "green" + + +def test_a_suite_with_no_check_runs_counts_only_while_checks_register(): + suites = [_suite(), _suite(status="queued", runs=0)] + + assert _judge(suites) is None + assert _judge(suites, waited=CI_REGISTER_SECONDS).conclusion == "green" + + +def test_running_ci_is_waited_on_until_its_time_limit(): + running = [_suite(status="in_progress")] + + assert _judge(running, waited=CI_TTL_SECONDS - 1) is None + assert _judge(running, waited=CI_TTL_SECONDS).conclusion == "unfinished" + + +def test_a_pending_commit_status_keeps_ci_running(): + assert _judge([_suite()], [_status("pending")]) is None + assert _judge([_suite()], [_status("success")]).conclusion == "green" + + +def test_neutral_and_skipped_suites_are_green(): + assert _judge([_suite(conclusion="neutral"), _suite(conclusion="skipped")]).conclusion == ( + "green" + ) + + +def test_a_failed_suite_or_status_is_red(): + assert _judge([_suite(), _suite(conclusion="failure")]).conclusion == "red" + assert _judge([_suite()], [_status("error")]).conclusion == "red" diff --git a/backend/tests/software_factory/test_build_journal.py b/backend/tests/software_factory/test_build_journal.py index e3ba5d8e..ecaf9770 100644 --- a/backend/tests/software_factory/test_build_journal.py +++ b/backend/tests/software_factory/test_build_journal.py @@ -1,4 +1,5 @@ from druks.contrib.software_factory.contracts import ( + CiReport, ImplementationOutput, PlanData, ReviewReport, @@ -106,3 +107,23 @@ def test_human_feedback_lists_every_reply_and_marks_the_untriaged_one_pending(): {"reviewer": "alice", "body": "raw review text", "triage": first_triage}, {"reviewer": "dana", "body": "second review", "triage": None}, ] + + +def _review(decision: str) -> ReviewReport: + return ReviewReport(decision=decision, summary="", findings=[], context_repos=[]) + + +def test_red_ci_is_the_revision_request_until_a_review_supersedes_it(): + red = CiReport(head_sha="b", conclusion="red") + assert "CI on `b` is red" in _journal(_implementation(), red).revision_request + assert _journal(_implementation(), red, _review("approve")).revision_request is None + + +def test_a_review_after_green_ci_that_requests_no_changes_is_ready_for_review(): + green = CiReport(head_sha="b", conclusion="green") + unfinished = CiReport(head_sha="b", conclusion="unfinished") + assert _journal(green, _review("approve")).is_ready_for_review + assert _journal(green, _review("comment")).is_ready_for_review + assert not _journal(green, _review("request_changes")).is_ready_for_review + assert not _journal(unfinished, _review("approve")).is_ready_for_review + assert not _journal(_review("approve"), green).is_ready_for_review diff --git a/backend/tests/software_factory/test_build_plan_phase.py b/backend/tests/software_factory/test_build_plan_phase.py index 52dc6c05..492564e3 100644 --- a/backend/tests/software_factory/test_build_plan_phase.py +++ b/backend/tests/software_factory/test_build_plan_phase.py @@ -4,6 +4,7 @@ from druks.contrib.software_factory.app import SoftwareFactory from druks.contrib.software_factory.contracts import ( AcceptanceCriterionOutput, + CiReport, PlanData, PlanOutput, QuestionOptionOutput, @@ -625,7 +626,9 @@ async def test_blocked_redraft_fails_the_run_instead_of_implementing(monkeypatch async def test_review_work_ask_renders_a_notification_body(monkeypatch): workflow = _flow() workflow.subject = SimpleNamespace(repo="org/repo") - workflow.journal = SimpleNamespace(implementations=[SimpleNamespace(pr_number=42)]) + workflow.journal = BuildJournal() + workflow.journal.add(CiReport(head_sha="0f3a2c1d", conclusion="green")) + monkeypatch.setattr(Build, "pr_number", property(lambda self: 42)) input_requests: list[dict] = [] async def fake_wait(*, input_request): @@ -646,6 +649,62 @@ async def fake_approved_work(): assert input_requests[0]["label"] == "Review implementation on GitHub: PR #42" +@pytest.mark.parametrize( + ("conclusion", "note"), + [ + ("red", "CI on 0f3a2c1 is red"), + ("unfinished", "CI on 0f3a2c1 did not finish within 60 minutes"), + ], +) +async def test_a_round_without_green_ci_parks_saying_so(monkeypatch, conclusion, note): + workflow = _flow() + workflow.subject = SimpleNamespace(repo="org/repo") + workflow.journal = BuildJournal() + workflow.journal.add(CiReport(head_sha="0f3a2c1d", conclusion=conclusion)) + monkeypatch.setattr(Build, "pr_number", property(lambda self: 42)) + input_requests: list[dict] = [] + + async def fake_wait(*, input_request): + input_requests.append(input_request) + return ReviewWork(action="merged") + + monkeypatch.setattr(ReviewWork, "wait", fake_wait) + + await workflow._work_gate() + + assert input_requests[0]["label"] == f"Review implementation on GitHub: PR #42. {note}" + + +@pytest.mark.parametrize( + ("gate", "calls"), [("human", ["clear_draft", "merge"]), ("none", ["merge"])] +) +async def test_only_a_person_s_approval_clears_the_draft_before_a_merge(monkeypatch, gate, calls): + flow = Build() + flow._policy = RepoPolicy(on_approval="merge", gates={"implementation_approval": gate}) + flow.subject = SimpleNamespace(repo="org/repo") + flow.journal = BuildJournal() + flow.journal.add(CiReport(head_sha="0f3a2c1d", conclusion="red")) + monkeypatch.setattr(Build, "pr_number", property(lambda self: 42)) + recorded: list[str] = [] + + async def fake_wait(*, input_request): + return ReviewWork(action="approve") + + async def clear_draft(): + recorded.append("clear_draft") + + async def merge(): + recorded.append("merge") + return True + + monkeypatch.setattr(ReviewWork, "wait", fake_wait) + monkeypatch.setattr(flow, "_clear_draft", clear_draft) + monkeypatch.setattr(flow, "declare_merge_intent", merge) + + assert await flow._work_gate() is True + assert recorded == calls + + async def test_needs_clarification_delivery_stops_the_run(monkeypatch): """The implementer bailing (needs_clarification) fails the run with its own reason — the stop is a workflow decision now, not a contract side effect.""" @@ -745,24 +804,34 @@ async def test_a_build_hands_its_pull_request_to_the_review(monkeypatch): assert flow.journal.reviews == [review] -@pytest.mark.parametrize(("max_revisions", "implemented"), [(5, 2), (1, 1)]) -async def test_requested_changes_go_back_to_implement_until_the_cap( - monkeypatch, max_revisions, implemented -): +def _implementing_build( + monkeypatch, *, max_revisions, heads, conclusions, decisions +) -> tuple[Build, list[str]]: + """A build whose implementer delivers ``heads``, whose CI concludes ``conclusions``, + and whose review decides ``decisions``, each in turn. It returns the calls made.""" flow = Build() flow._settings = Build.Settings(max_implementation_revisions=max_revisions) - flow.journal = SimpleNamespace(implementation_revision=0) - decisions = iter(["request_changes", "approve"]) + flow.journal = SimpleNamespace(implementation_revision=0, last_implementation=None) + heads, conclusions, decisions = iter(heads), iter(conclusions), iter(decisions) calls: list[str] = [] async def create_branch(): return None async def implement(): + delivery = SimpleNamespace(head_sha=next(heads)) flow.journal.implementation_revision += 1 + flow.journal.last_implementation = delivery calls.append("implement") + return delivery + + async def wait_for_ci(): + conclusion = next(conclusions) + calls.append(f"ci:{conclusion}") + return CiReport(head_sha=flow.journal.last_implementation.head_sha, conclusion=conclusion) async def review_work(): + calls.append("review") return ReviewReport(decision=next(decisions), summary="", findings=[], context_repos=[]) async def work_gate(): @@ -771,9 +840,94 @@ async def work_gate(): monkeypatch.setattr(flow, "_create_branch", create_branch) monkeypatch.setattr(flow, "implement", implement) + monkeypatch.setattr(flow, "_wait_for_ci", wait_for_ci) monkeypatch.setattr(flow, "_review_work", review_work) monkeypatch.setattr(flow, "_work_gate", work_gate) + return flow, calls + + +@pytest.mark.parametrize( + ("max_revisions", "expected"), + [ + (5, ["implement", "ci:green", "review", "implement", "ci:green", "review", "work_gate"]), + (1, ["implement", "ci:green", "review", "work_gate"]), + ], +) +async def test_requested_changes_go_back_to_implement_until_the_cap( + monkeypatch, max_revisions, expected +): + flow, calls = _implementing_build( + monkeypatch, + max_revisions=max_revisions, + heads=["a", "b"], + conclusions=["green", "green"], + decisions=["request_changes", "approve"], + ) + + await flow._implement_phase() + + assert calls == expected + + +@pytest.mark.parametrize( + ("max_revisions", "expected"), + [ + (5, ["implement", "ci:red", "implement", "ci:green", "review", "work_gate"]), + (1, ["implement", "ci:red", "review", "work_gate"]), + ], +) +async def test_red_ci_goes_back_to_implement_before_the_review_until_the_cap( + monkeypatch, max_revisions, expected +): + flow, calls = _implementing_build( + monkeypatch, + max_revisions=max_revisions, + heads=["a", "b"], + conclusions=["red", "green"], + decisions=["approve"], + ) await flow._implement_phase() - assert calls == ["implement"] * implemented + ["work_gate"] + assert calls == expected + + +async def test_a_round_that_pushes_nothing_goes_to_the_review_without_a_ci_wait(monkeypatch): + flow, calls = _implementing_build( + monkeypatch, max_revisions=5, heads=["a", "a"], conclusions=["red"], decisions=["approve"] + ) + + await flow._implement_phase() + + assert calls == ["implement", "ci:red", "implement", "review", "work_gate"] + + +@pytest.mark.parametrize( + ("conclusion", "decision", "leaves_draft"), + [ + ("green", "approve", True), + ("green", "comment", True), + ("red", "approve", False), + ("green", "request_changes", False), + ], +) +async def test_the_work_park_takes_the_pr_out_of_draft_only_when_ready( + monkeypatch, conclusion, decision, leaves_draft +): + flow = Build() + flow.journal.add(CiReport(head_sha="0f3a2c1d", conclusion=conclusion)) + flow.journal.add(ReviewReport(decision=decision, summary="", findings=[], context_repos=[])) + drafts: list[bool] = [] + + async def set_pr_draft(*, draft): + drafts.append(draft) + + async def request_assignee_review(): + pass + + monkeypatch.setattr(flow, "set_pr_draft", set_pr_draft) + monkeypatch.setattr(flow, "request_assignee_review", request_assignee_review) + + await ReviewWork.on_wait(flow) + + assert drafts == ([False] if leaves_draft else []) diff --git a/backend/tests/software_factory/test_lane_reactions.py b/backend/tests/software_factory/test_lane_reactions.py index 8bab00dd..40d33008 100644 --- a/backend/tests/software_factory/test_lane_reactions.py +++ b/backend/tests/software_factory/test_lane_reactions.py @@ -300,3 +300,16 @@ async def test_a_review_of_a_pull_request_no_build_owns_answers_no_one(druks_db, ) assert answers == [] + + +async def test_a_ci_update_wakes_the_build_of_its_branch(druks_db, monkeypatch): + item = await _item_with_pr("ACME-32", 44) + answers = await _answers_to_the_build(monkeypatch) + + await publish( + "ci.updated", + repo="acme/widget", + payload={"branches": ["agent/acme-32", "agent/nobody"]}, + ) + + assert answers == [(item.id, "ci.updated", {})] diff --git a/backend/tests/test_durable_sdk.py b/backend/tests/test_durable_sdk.py index 92cf759b..af711d4b 100644 --- a/backend/tests/test_durable_sdk.py +++ b/backend/tests/test_durable_sdk.py @@ -566,6 +566,44 @@ async def waiting() -> None: workflows._items.pop(WaitFlow.kind) +async def test_only_a_wait_that_releases_the_sandbox_reaps_it(runtime, monkeypatch): + reaped: list[str] = [] + + async def reap(self) -> None: + reaped.append(self.workflow_id) + + monkeypatch.setattr(Workflow, "_reap_run", reap) + + class ReleasingWaitFlow(Workflow): + subject = Widget + + async def run_multistep(self) -> None: + await self.wait("review.completed", label="Waiting for the review", ttl_seconds=9) + await self.wait( + "checks.finished", label="Waiting for checks", ttl_seconds=9, release_sandbox=True + ) + + subject = Widget(id=770772) + try: + workflow_id = await ReleasingWaitFlow.start(subject=subject) + + async def showing(label: str) -> None: + while await get_run_phase(workflow_id) != label: + await asyncio.sleep(0.1) + + await asyncio.wait_for(showing("Waiting for the review"), 15) + assert reaped == [] + async with session_scope(runtime.engine): + await ReleasingWaitFlow.answer(subject, "review.completed") + await asyncio.wait_for(showing("Waiting for checks"), 15) + assert reaped == [workflow_id] + async with session_scope(runtime.engine): + await ReleasingWaitFlow.answer(subject, "checks.finished") + await _wait_for_run(runtime.engine, workflow_id, lambda run: run.state == RunState.FINISHED) + finally: + workflows._items.pop(ReleasingWaitFlow.kind) + + async def test_wait_fails_with_the_failure_it_is_answered(runtime): class FailingWaitFlow(Workflow): subject = Widget diff --git a/backend/tests/test_webhooks_github.py b/backend/tests/test_webhooks_github.py index 93914b26..154605c4 100644 --- a/backend/tests/test_webhooks_github.py +++ b/backend/tests/test_webhooks_github.py @@ -214,3 +214,37 @@ async def test_a_review_says_whether_it_is_of_the_latest_commit( [(event_type, published_event)] = published assert event_type == "pr.review_submitted" assert published_event["payload"]["is_latest_commit"] is is_latest_commit + + +async def test_a_finished_check_names_the_branch_of_its_commit(tmp_path, monkeypatch): + published = _capture(monkeypatch) + payload = { + "action": "completed", + "repository": {"full_name": "acme/widget"}, + "check_run": {"head_sha": "0f3a2c1d", "check_suite": {"head_branch": "agent/eng-1"}}, + } + + await _events(tmp_path, payload=payload).on_check_run_completed() + + assert published == [ + ( + "ci.updated", + { + "repo": "acme/widget", + "payload": {"branches": ["agent/eng-1"]}, + }, + ) + ] + + +async def test_a_status_names_the_branches_its_commit_heads(tmp_path, monkeypatch): + published = _capture(monkeypatch) + payload = { + "repository": {"full_name": "acme/widget"}, + "sha": "0f3a2c1d", + "branches": [{"name": "agent/eng-1"}, {"name": "agent/eng-1-2"}], + } + + await _events(tmp_path, payload=payload).on_status() + + assert published[0][1]["payload"] == {"branches": ["agent/eng-1", "agent/eng-1-2"]} diff --git a/docs/configuration.md b/docs/configuration.md index bcb123c5..3d7271cf 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -516,6 +516,10 @@ reviewer approves the pull request: | `none` | Default. Druks marks the pull request ready for review and ends the build. The repository's own process merges it. Druks records the merge when GitHub announces it. | | `merge` | Druks squash-merges the pull request, or arms auto-merge when checks are still running. | +After each push, the build waits for CI on the head before the review, and red +CI goes back to the implementer. The pull request leaves draft when the review of +a head with green CI requests no changes, or when a person approves it. + When Druks receives a push to the default branch that changes `config.yml`, the next build uses the new file. diff --git a/docs/writing-an-app.md b/docs/writing-an-app.md index b87a4d3b..4ed85362 100644 --- a/docs/writing-an-app.md +++ b/docs/writing-an-app.md @@ -1194,6 +1194,9 @@ The run stays running. The run page shows the label. Druks sends no notification and keeps the warm sandbox. When the wait is longer than `ttl_seconds`, the run fails with `WaitTimeout`. +A wait that can last long can pass `release_sandbox=True`. The wait releases the +warm sandbox, as a gate park does, and the next agent call boots a new one. + A subscriber answers the wait through the workflow class and the subject. The facts it sends are the result of `wait()`: