Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions backend/druks/contrib/software_factory/contracts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions backend/druks/contrib/software_factory/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
16 changes: 11 additions & 5 deletions backend/druks/contrib/software_factory/subscribers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions backend/druks/contrib/software_factory/workflows.py
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down
11 changes: 10 additions & 1 deletion backend/tests/software_factory/test_activity.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from datetime import UTC, datetime
from unittest.mock import AsyncMock

import pytest
from conftest import installation_key
Expand All @@ -7,6 +8,7 @@
FindingOutput,
PlanOutput,
ReviewReport,
ReviewWork,
)
from druks.contrib.software_factory.datastructures import PullRequest
from druks.contrib.software_factory.enums import Resolution
Expand Down Expand Up @@ -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(
Expand Down
35 changes: 35 additions & 0 deletions backend/tests/software_factory/test_lane_reactions.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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()
24 changes: 17 additions & 7 deletions backend/tests/software_factory/test_webhooks_pull_request.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/apps/software_factory/activity.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand Down Expand Up @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/apps/software_factory/activity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand All @@ -43,6 +44,7 @@ const TOPIC_ICONS: Record<string, LucideIcon> = {
}

const REPLY_ACTIONS: Record<string, string> = {
merged: 'Merged on GitHub',
approve: 'Approve',
request_changes: 'Request changes',
revise_contract: 'Revise contract',
Expand Down
Loading