Fix #1011: Finish parked builds when GitHub PR is merged by dserbano · Pull Request #1070 · czpython/druks

dserbano · 2026-11-02T19:37:45Z

When GitHub merges a pull request while its build waits for implementation review, answer the review gate with merged so the workflow returns successfully and the build finishes. Also answer the gate when a merge is already recorded as the build parks, and keep resolved tickets from moving back to In Progress on resume.

Closing a pull request without merging still cancels the build with pr closed without merge. Activity shows “Build finished” and “Merged on GitHub” for the successful path.

Closes #1011

czpython

Thanks, the fix works. Answering ReviewWork with merged lets the build return, so DBOS records SUCCESS. I checked the parts that worried me:

resolve and the park-time subscriber can both answer the same round. Run.resume keys each reply on the parked round, so the second send collapses.
DBOS accepts send from inside a step, so the answer from run.parked:propagate reaches the gate.
An operator cannot post merged from the UI. ReviewWork asks on GitHub and offers no in-app controls.
The result.action == "merged" check in build_start_marks_ticket_in_progress is needed. The DBOS send commits before the webhook transaction does, so the subscriber can still read an empty resolution.

Before merge, please cut the PR down. The inline comments have the details:

Remove _is_merged.
Revert .gitignore.
Remove the new test in test_durable_sdk.py.
Patch ReviewWork.answer, not DBOS.send_async.
Rewrite the PR description. It should say what the change does and end with Closes #1011. The test report, the local paths, and the MCP recovery tooling are not part of this diff.

One limit, not blocking: the park-time check makes a stranded build rare, but it does not exclude it. If the workflow commits its park after resolve reads the status but before the webhook commits, both sides miss it. That window is milliseconds.

czpython · 2026-11-03T07:55:45Z

                "url": f"https://github.com/{(await self.subject).repo}/pull/{self.pr_number}",
            }
        )
+        if decision.action == "merged" or await self._is_merged():


Please drop _is_merged and keep if decision.action == "merged": return True. When a merge lands while the build runs, the PARKED subscriber already answers merged at the next park. The step only covers the moment between the reply and this line, and it adds a step to every review reply. Dropping it also removes the __wrapped__ patches in test_build_plan_phase.py and test_merged_work_exits_review_without_more_work.

czpython · 2026-11-03T07:55:45Z

 /dist/
 # ADRs are private, kept out of the repo.
 /docs/adr/
+python_combine.py


These are local files. Please revert this hunk. It also drops the trailing newline.

czpython · 2026-11-03T07:55:45Z

+
+
+@pytest.mark.parametrize("merge_timing", ["parked", "before_park", "after_review"])
+async def test_merged_build_finishes_successfully(runtime, monkeypatch, merge_timing):


Please remove this test. test_durable_sdk.py tests the platform, and this test makes it import software_factory and the dashboard. It mocks eight Build internals, and most of what it checks is DBOS behavior. The tests in software_factory/ already cover the policy. If you want one real-DBOS check, keep only the parked case, put it in backend/tests/software_factory/, and drop the dashboard asserts.

czpython · 2026-11-03T07:55:45Z



+@pytest.fixture(autouse=True)
+def merged_reply(monkeypatch):


Patch ReviewWork.answer instead of DBOS.send_async, and only in the two tests that merge a parked run. test_lane_reactions.py already does it that way. Then assert on the answer call, not on the DBOS send arguments.

czpython · 2026-11-03T07:55:45Z

+    merged,
+):
+    send = AsyncMock()
+    monkeypatch.setattr("dbos.DBOS.send_async", send)


Same here: patch ReviewWork.answer, not DBOS.

czpython · 2026-11-03T07:55:45Z

 class ReviewWork(Gate):
    """The verdict on the built work: approve/request_changes (PR webhook) or
-    revise_contract (UI)."""
+    revise_contract (UI), or merged (GitHub closed the PR with a merge)."""


Nit: "approve/request_changes (PR review webhook), revise_contract (UI), or merged (pr.closed webhook)."

dserbano · 2026-11-03T13:16:01Z

Implemented the requested review cleanup and pushed:

Removed _is_merged; the review gate now returns successfully only for decision.action == "merged". Removed the associated __wrapped__ test patches.
Reverted the .gitignore changes.
Removed the software-factory integration test and its imports from test_durable_sdk.py.
Replaced DBOS mocks with ReviewWork.answer mocks in the two parked-merge webhook tests and the Activity test, with assertions on the answer call. The redispatch and ticket-status tests now use running builds so they do not depend on a global gate mock.
Updated the ReviewWork docstring as suggested.

Another full local UI test isn’t necessary for this cleanup. Most changes removed unnecessary code or adjusted tests. The 69 focused tests passed.

czpython

Thanks for the quick turn. One more pass, and it is all removal.

After the implementer opens the PR, a Build can park only on review_work. _plan_phase has the only other park, and the build never goes back to it after _implement_phase starts. So three branches in this PR cover states that cannot occur:

elif status.is_parked: await Build.cancel(self) in resolve.
not subject.resolution in the RUNNING subscriber.
elif not subject.resolution in the PARKED subscriber.

Please remove them and the tests that pin them. The inline comments have the details. After this, the resolve hunk is one line from main: the cancel becomes the answer.

czpython · 2026-11-03T14:07:59Z

+            status = await self.get_status(workflow=Build)
+            if status.is_parked and status.gate == ReviewWork.name:
+                await ReviewWork.answer(self, action="merged")
+            elif status.is_parked:


A Build with a PR can only be parked on review_work, so this branch never runs. Please remove it:

if (await self.get_status(workflow=Build)).is_parked: await ReviewWork.answer(self, action="merged")

If a park on another gate ever appears, ReviewWork.answer raises, and that is what we want.

czpython · 2026-11-03T14:07:59Z

+    *, subject: WorkItem, result: dict | None = None, **_: object
+) -> None:
+    # The gate reply can arrive before the merge webhook commits its resolution.
+    if not subject.resolution and (not result or result.get("action") != "merged"):


The result check is the real fix: the DBOS send commits before the webhook commits resolution. The not subject.resolution half never matters. A closed or cancelled build is cancelled and never resumes. A merged build resumes only through the merged reply. Every Build reply has action, so read it directly:

# 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)

czpython · 2026-11-03T14:07:59Z

+    # 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")
+    elif not subject.resolution:


A review_work park never happens with a closed or cancelled resolution, so this can be a plain else.

czpython · 2026-11-03T14:07:59Z

    assert reparked == [True]
+
+
+async def test_merged_work_exits_review_without_more_work(monkeypatch):


Please remove this test. It only shows that if decision.action == "merged": return True returns True.

czpython · 2026-11-03T14:07:59Z

+
+
+@pytest.mark.parametrize("resolution", ["merged", "closed", "cancelled"])
+async def test_resolved_build_resume_keeps_ticket_status(druks_db, monkeypatch, resolution):


Please remove this test. It pins the not subject.resolution guard, which goes away. Keep the two tests below it.

czpython · 2026-11-03T14:08:00Z

-    assert not (await _fresh_run(run_id)).is_active
+    answer.assert_awaited_once_with(item, action="merged")
+    run = await _fresh_run(run_id)
+    assert run.state == "parked"


With answer mocked, these three checks only show that cancel did not run. The answer.assert_awaited_once_with(...) above is enough. Please remove them and the _fresh_run call.

czpython · 2026-11-03T14:08:00Z

    assert await _milestone_count(work_item_id, "merged") == 0
-    assert not (await _fresh_run(run_id)).is_active
+    run = await _fresh_run(run_id)
+    assert run.state == "cancelled"


This PR does not change the closed-unmerged path. Please revert this hunk.

czpython · 2026-11-03T14:08:00Z

    assert [event.type for event in events] == [outcome]
    assert events[0].payload == {"repo": "acme/widget", "pr_number": 42}
    assert item.resolution == outcome
+    if merged and state == "parked":


Keep the answer patch, but please remove these checks. This test is about the activity record, and the webhook test already pins the answer call.

czpython · 2026-11-03T14:08:00Z

    )

-    await run.cancel(failure="pr merged while parked")
+    await run.cancel(failure="operator cancelled")


With the cancel branch gone, this PR has no reason to change this string. Please revert this file.

dserbano requested a review from czpython as a code owner November 2, 2026 19:37

dserbano force-pushed the issues/1011 branch 2 times, most recently from 3d8c153 to 154610f Compare November 2, 2026 21:35

czpython reviewed Nov 3, 2026

View reviewed changes

dserbano force-pushed the issues/1011 branch from 154610f to 05d3807 Compare November 3, 2026 13:53

dserbano mentioned this pull request Nov 3, 2026

Scope build GitHub reviewer credentials to evaluation #1073

Open

czpython reviewed Nov 3, 2026

View reviewed changes

Finish parked builds when GitHub PRs merge

2a37305

dserbano force-pushed the issues/1011 branch from 05d3807 to 2a37305 Compare November 3, 2026 14:33

czpython approved these changes Nov 3, 2026

View reviewed changes

czpython merged commit a429bd0 into main Nov 3, 2026
6 checks passed

czpython deleted the issues/1011 branch November 3, 2026 15:44



		@pytest.mark.parametrize("merge_timing", ["parked", "before_park", "after_review"])
		async def test_merged_build_finishes_successfully(runtime, monkeypatch, merge_timing):

		assert reparked == [True]


		async def test_merged_work_exits_review_without_more_work(monkeypatch):



		@pytest.mark.parametrize("resolution", ["merged", "closed", "cancelled"])
		async def test_resolved_build_resume_keeps_ticket_status(druks_db, monkeypatch, resolution):

Conversation

dserbano commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czpython left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dserbano commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czpython left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dserbano commented Nov 2, 2026 •

edited

Loading

dserbano commented Nov 3, 2026 •

edited

Loading