From 2a37305297bf952844944ab672ebe3f4193c52c1 Mon Sep 17 00:00:00 2001 From: Dan Serbanoiu <32792904+dserbano@users.noreply.github.com> Date: Tue, 3 Nov 2026 09:53:06 -0400 Subject: [PATCH] Finish parked builds when GitHub PRs merge --- .../contrib/software_factory/contracts.py | 6 ++-- .../druks/contrib/software_factory/models.py | 5 ++- .../contrib/software_factory/subscribers.py | 16 ++++++--- .../contrib/software_factory/workflows.py | 2 ++ .../tests/software_factory/test_activity.py | 11 +++++- .../software_factory/test_lane_reactions.py | 35 +++++++++++++++++++ .../test_webhooks_pull_request.py | 24 +++++++++---- .../apps/software_factory/activity.test.ts | 2 ++ .../src/apps/software_factory/activity.ts | 2 ++ 9 files changed, 84 insertions(+), 19 deletions(-) diff --git a/backend/druks/contrib/software_factory/contracts.py b/backend/druks/contrib/software_factory/contracts.py index 9cc136eae..e18aa206a 100644 --- a/backend/druks/contrib/software_factory/contracts.py +++ b/backend/druks/contrib/software_factory/contracts.py @@ -15,11 +15,11 @@ class ReviewWork(Gate): - """The verdict on the built work: approve/request_changes (PR webhook) or - revise_contract (UI).""" + """The verdict on the built work: approve/request_changes (PR review webhook), + revise_contract (UI), or merged (pr.closed webhook).""" name = "review_work" - action: Literal["approve", "request_changes", "revise_contract"] + action: Literal["approve", "request_changes", "revise_contract", "merged"] reviewer: str | None = None body: str | None = None diff --git a/backend/druks/contrib/software_factory/models.py b/backend/druks/contrib/software_factory/models.py index 77b178cea..7c3a878c5 100644 --- a/backend/druks/contrib/software_factory/models.py +++ b/backend/druks/contrib/software_factory/models.py @@ -8,6 +8,7 @@ from sqlalchemy.orm import Mapped, mapped_column, relationship, selectinload from druks.accounts import Account +from druks.contrib.software_factory.contracts import ReviewWork from druks.contrib.software_factory.enums import Priority, Resolution, Status from druks.contrib.software_factory.exceptions import PrefixTakenError from druks.contrib.software_factory.policy import RepoPolicy @@ -307,10 +308,8 @@ async def resolve(self, resolution: Resolution, *, at: datetime) -> None: if resolution in (Resolution.MERGED, Resolution.CLOSED): await self.announce(resolution, repo=self.repo, pr_number=self.pr_number) if resolution == Resolution.MERGED: - # A merge strands a build that is parked on review. A running build finishes - # by itself, because its merge step finds the PR closed. if (await self.get_status(workflow=Build)).is_parked: - await Build.cancel(self, failure="pr merged while parked") + await ReviewWork.answer(self, action="merged") await self.set_ticket_status(TicketStatus.DONE) if resolution == Resolution.CLOSED: # The attempt ended, not the ticket, so the ticket goes back to its resting diff --git a/backend/druks/contrib/software_factory/subscribers.py b/backend/druks/contrib/software_factory/subscribers.py index a588a7397..ec447afbf 100644 --- a/backend/druks/contrib/software_factory/subscribers.py +++ b/backend/druks/contrib/software_factory/subscribers.py @@ -33,15 +33,21 @@ async def pr_open_fills_the_item(*, repo: str, pr_number: int, payload: dict) -> @subscribe(WorkflowEvent.RUNNING, workflow=Build) -async def build_start_marks_ticket_in_progress(*, subject: WorkItem, **_: object) -> None: - # Every (re)start and gate-resume of a build means the ticket is in progress — - # including the return from a rework loop that had parked it In Review. - await subject.set_ticket_status(TicketStatus.IN_PROGRESS) +async def build_start_marks_ticket_in_progress( + *, subject: WorkItem, result: dict | None = None, **_: object +) -> None: + # A merged reply ends the build, and the merge already moved the ticket to Done. + if not result or result["action"] != "merged": + await subject.set_ticket_status(TicketStatus.IN_PROGRESS) @subscribe(WorkflowEvent.PARKED, workflow=Build, gate=ReviewWork) async def review_park_marks_ticket_in_review(*, subject: WorkItem, **_: object) -> None: - await subject.set_ticket_status(TicketStatus.IN_REVIEW) + # A merge can arrive before the gate parks, when there is no ask to answer yet. + if subject.resolution == Resolution.MERGED: + await ReviewWork.answer(subject, action="merged") + else: + await subject.set_ticket_status(TicketStatus.IN_REVIEW) @subscribe("repo.pushed", to_default_branch=True) diff --git a/backend/druks/contrib/software_factory/workflows.py b/backend/druks/contrib/software_factory/workflows.py index 0f38c4e61..55afd3d7e 100644 --- a/backend/druks/contrib/software_factory/workflows.py +++ b/backend/druks/contrib/software_factory/workflows.py @@ -341,6 +341,8 @@ async def _work_gate(self) -> bool: "url": f"https://github.com/{(await self.subject).repo}/pull/{self.pr_number}", } ) + if decision.action == "merged": + return True if decision.action == "approve": return await self._approved_work() if decision.action == "request_changes": diff --git a/backend/tests/software_factory/test_activity.py b/backend/tests/software_factory/test_activity.py index 7b5d452bb..532458d42 100644 --- a/backend/tests/software_factory/test_activity.py +++ b/backend/tests/software_factory/test_activity.py @@ -1,4 +1,5 @@ from datetime import UTC, datetime +from unittest.mock import AsyncMock import pytest from conftest import installation_key @@ -7,6 +8,7 @@ FindingOutput, PlanOutput, ReviewReport, + ReviewWork, ) from druks.contrib.software_factory.datastructures import PullRequest from druks.contrib.software_factory.enums import Resolution @@ -173,7 +175,14 @@ async def test_operator_stop_records_no_owner_close(druks_db, druks_client, pr_n @pytest.mark.parametrize("state", ["parked", "failed"]) @pytest.mark.parametrize("merged", [True, False]) -async def test_owner_outcome_records_once_without_an_operator_stop(druks_db, state, merged): +async def test_owner_outcome_records_once_without_an_operator_stop( + druks_db, + monkeypatch, + state, + merged, +): + answer = AsyncMock() + monkeypatch.setattr(ReviewWork, "answer", answer) item = await make_test_work_item(repo="acme/widget", title="Merged work") await item.update(pr_number=42, branch="agent/merged") await seed_run( diff --git a/backend/tests/software_factory/test_lane_reactions.py b/backend/tests/software_factory/test_lane_reactions.py index f7fbaa703..72a6d61b1 100644 --- a/backend/tests/software_factory/test_lane_reactions.py +++ b/backend/tests/software_factory/test_lane_reactions.py @@ -1,4 +1,5 @@ from datetime import UTC, datetime +from unittest.mock import AsyncMock import druks.contrib.software_factory.subscribers # noqa: F401 — registers the lane reactions import pytest @@ -201,3 +202,37 @@ async def test_a_fork_pr_on_the_items_branch_leaves_the_item_alone(druks_db): ) assert not item.pr_number + + +async def test_merged_reply_keeps_done_before_resolution_commit(druks_db, monkeypatch): + item = await make_test_work_item(repo="acme/widget", title="Merged") + push = AsyncMock() + monkeypatch.setattr(WorkItem, "set_ticket_status", push) + + await publish( + WorkflowEvent.RUNNING, + subject=item.identity, + kind=Build.kind, + result={"action": "merged"}, + ) + + push.assert_not_awaited() + + +async def test_merge_before_park_answers_review(druks_db, monkeypatch): + item = await make_test_work_item(repo="acme/widget", title="Merged before park") + item.resolution = "merged" + await druks_db.flush() + push, answer = AsyncMock(), AsyncMock() + monkeypatch.setattr(WorkItem, "set_ticket_status", push) + monkeypatch.setattr(ReviewWork, "answer", answer) + + await publish( + WorkflowEvent.PARKED, + subject=item.identity, + kind=Build.kind, + gate=ReviewWork.name, + ) + + answer.assert_awaited_once_with(item, action="merged") + push.assert_not_awaited() diff --git a/backend/tests/software_factory/test_webhooks_pull_request.py b/backend/tests/software_factory/test_webhooks_pull_request.py index 18df5cd74..2bb7c727e 100644 --- a/backend/tests/software_factory/test_webhooks_pull_request.py +++ b/backend/tests/software_factory/test_webhooks_pull_request.py @@ -1,8 +1,10 @@ from datetime import UTC, datetime from types import SimpleNamespace +from unittest.mock import AsyncMock import druks.contrib.software_factory.subscribers # noqa: F401 — registers the pr.closed subscriber import pytest +from druks.contrib.software_factory.contracts import ReviewWork from druks.contrib.software_factory.models import WorkItem from druks.core.webhooks.github import GitHubEvents from druks.durable import Run @@ -85,9 +87,11 @@ async def _fresh_run(run_id): @pytest.mark.asyncio -async def test_external_merge_stores_githubs_verdict_and_ends_involvement(druks_db, tmp_path): +async def test_external_merge_answers_review_without_cancelling(druks_db, tmp_path, monkeypatch): + answer = AsyncMock() + monkeypatch.setattr(ReviewWork, "answer", answer) repo, pr_number, branch = "ClawHaven/acme-app", 42, "agent/eng-1" - work_item_id, run_id = await _park_work_item(repo=repo, pr_number=pr_number, branch=branch) + work_item_id, _ = await _park_work_item(repo=repo, pr_number=pr_number, branch=branch) await _fire_closed(repo=repo, pr_number=pr_number, branch=branch, tmp_path=tmp_path) @@ -97,8 +101,7 @@ async def test_external_merge_stores_githubs_verdict_and_ends_involvement(druks_ assert item.resolved_at == datetime(2026, 7, 25, 21, 59, 9, tzinfo=UTC) # ...announced as the milestone... assert await _milestone_count(work_item_id, "merged") == 1 - # ...and involvement ended: the parked build run is cancelled. - assert not (await _fresh_run(run_id)).is_active + answer.assert_awaited_once_with(item, action="merged") @pytest.mark.asyncio @@ -122,10 +125,12 @@ async def test_merge_ships_but_leaves_a_running_build_to_converge(druks_db, tmp_ @pytest.mark.asyncio -async def test_a_redelivered_webhook_does_not_rewrite_the_verdict(druks_db, tmp_path): +async def test_a_redelivered_webhook_does_not_rewrite_the_verdict(druks_db, tmp_path, monkeypatch): """GitHub redelivers webhooks, and druks's own merge echoes back through the same path. The stored verdict is the first one; a contradicting redelivery neither overwrites it nor records a second milestone.""" + answer = AsyncMock() + monkeypatch.setattr(ReviewWork, "answer", answer) repo, pr_number, branch = "ClawHaven/acme-app", 44, "agent/eng-3" work_item_id, _ = await _park_work_item(repo=repo, pr_number=pr_number, branch=branch) await _fire_closed(repo=repo, pr_number=pr_number, branch=branch, tmp_path=tmp_path) @@ -144,6 +149,7 @@ async def test_a_redelivered_webhook_does_not_rewrite_the_verdict(druks_db, tmp_ assert item.resolved_at == datetime(2026, 7, 25, 21, 59, 9, tzinfo=UTC) assert await _milestone_count(work_item_id, "merged") == 1 assert await _milestone_count(work_item_id, "closed") == 0 + answer.assert_awaited_once_with(item, action="merged") @pytest.mark.asyncio @@ -215,7 +221,9 @@ async def test_a_remerge_after_redispatch_records_a_fresh_verdict(druks_db, tmp_ from druks.db import db_session as ds repo, pr_number, branch = "ClawHaven/acme-app", 77, "agent/eng-9" - work_item_id, _ = await _park_work_item(repo=repo, pr_number=pr_number, branch=branch) + work_item_id, _ = await _park_work_item( + repo=repo, pr_number=pr_number, branch=branch, state="running" + ) await _fire_closed(repo=repo, pr_number=pr_number, branch=branch, tmp_path=tmp_path) # Redispatched: a newer build run owns the item, and its PR is a fresh one. await seed_build_run(ds(), work_item_id=work_item_id, state="running") @@ -267,7 +275,9 @@ async def _record(self, status): monkeypatch.setattr(WorkItem, "set_ticket_status", _record) repo, pr_number, branch = "ClawHaven/acme-app", 92, "agent/eng-21" - work_item_id, _ = await _park_work_item(repo=repo, pr_number=pr_number, branch=branch) + work_item_id, _ = await _park_work_item( + repo=repo, pr_number=pr_number, branch=branch, state="running" + ) await _fire_closed( repo=repo, pr_number=pr_number, branch=branch, tmp_path=tmp_path, merged=True diff --git a/frontend/src/apps/software_factory/activity.test.ts b/frontend/src/apps/software_factory/activity.test.ts index a9fb7619a..c8b0411d6 100644 --- a/frontend/src/apps/software_factory/activity.test.ts +++ b/frontend/src/apps/software_factory/activity.test.ts @@ -13,6 +13,7 @@ describe('Factory Activity', () => { ['review.completed', 'software_factory.pull_request_review', 'Review completed'], ['merged', null, 'Pull request merged'], ['closed', null, 'Pull request closed'], + ['workflow.finished', 'software_factory.build', 'Build finished'], ['workflow.failed', 'software_factory.build', 'Build failed'], ['build.rejected', null, 'Build could not start'], ['workflow.cancelled', 'software_factory.build', 'Build cancelled'], @@ -44,6 +45,7 @@ it.each([ }) it.each([ + ['merged', 'Merged on GitHub'], ['approve', 'Approve'], ['request_changes', 'Request changes'], ['revise_contract', 'Revise contract'], ])('describes receipt of the actual %s reply', (action, label) => { const line = eventLine({ id: 'event:1', seq: 1, at: '2026-09-09T12:00:00Z', diff --git a/frontend/src/apps/software_factory/activity.ts b/frontend/src/apps/software_factory/activity.ts index cc39fc3f7..78c71dead 100644 --- a/frontend/src/apps/software_factory/activity.ts +++ b/frontend/src/apps/software_factory/activity.ts @@ -17,6 +17,7 @@ function label(event: ActivityEvent): string | undefined { if (event.payload?.kind === 'software_factory.build') { switch (event.topic) { case 'workflow.scheduled': return 'Build queued' + case 'workflow.finished': return 'Build finished' case 'workflow.failed': return 'Build failed' case 'workflow.cancelled': return 'Build cancelled' case 'workflow.parked': @@ -43,6 +44,7 @@ const TOPIC_ICONS: Record = { } const REPLY_ACTIONS: Record = { + merged: 'Merged on GitHub', approve: 'Approve', request_changes: 'Request changes', revise_contract: 'Revise contract',