Skip to content

DRU-1044 -- Wait for CI before the review, and keep a PR that is not ready in draft - #1114

Open
dserbano wants to merge 1 commit into
mainfrom
issues/1044
Open

dserbano wants to merge 1 commit into
mainfrom
issues/1044

Conversation

@dserbano

@dserbano dserbano commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

A build reviewed and offered a pull request whose CI had not finished or was red, and ReviewWork.on_wait took the PR out of draft whatever the round's outcome. Now the implementer runs the repo's tests before every push, and the build waits for CI before the review.

  • config.yml takes a top-level pre_push_tests. The implement prompt asks for those tests before every push, or, without the key, for the profile's test commands narrowed to the changed files, and for each result in checks. The key is not under verification, because a pinned verification replaces every command the profiler found.
  • After each delivery, the build waits until every check run and commit status on the head has finished. GitHub's check_run and status webhooks publish ci.updated, which wakes the wait. Each wake, and a poll every minute, reads the checks from GitHub, so a check that finished before the wait began is not missed.
  • Green means success, neutral, or skipped on every check. A red check that is also red on the default branch blocks the round instead. With no check registered three minutes after the push, the repo has no CI, and the round is green. CI still running after an hour blocks the round, and the work park's label gives the reason.
  • A red round goes back to implement before its review, within max_implementation_revisions. The revision request names each failing check and carries the last 200 lines of its log: the Actions job log, or the check's own output for any other check.
  • At the work park, the PR leaves draft only when the round's review approved a head with every check green. A blocked or capped round parks with the PR still a draft.

A person's approval at the park takes the PR out of draft, whatever CI says, because GitHub does not merge a draft and on_approval: merge would otherwise park again.

The CI wait adds steps after implement, so a build that is past its first delivery when this deploys fails on replay and needs a new start.

@mintlify

mintlify Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
druks 🟢 Ready View Preview Nov 6, 2026, 7:50 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

@dserbano

dserbano commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

DRU-1044: overview, decisions, tests, and open questions

All results are from fb6640c4, which is this change rebased on current main (after #1104 and #1108).

Overview

The problem. A build could hand a person code that nobody had tested:

  • The implementer was never told to run tests before pushing.
  • After a push, the build did not wait for CI. It went straight to the review.
  • When the build parked for a person, ReviewWork.on_wait always took the PR out of draft, even when the round's review requested changes or CI was red.

So a PR could look ready to merge while its CI was failing.

The solution. The build now checks the work before anyone is asked to look at it:

  1. Before every push, the implementer runs the repo's lint and typecheck commands, plus tests. A repo can name the tests in config.yml with pre_push_tests. Without that key, the implementer runs the repo's test commands narrowed to the files it changed. It reports each result.
  2. After every push, the build waits until every check run and commit status on that commit has finished, then judges the result:
    • Green: every check passed, was neutral, or was skipped. The round goes on to the review.
    • Red: a check failed on this PR. The round goes back to the implementer with the end of the failing job's log, and no review runs, within the usual revision limit.
    • Blocked: every failing check also fails on the default branch, so the diff cannot fix it, or CI was still running after an hour. The round goes on to the review, and the park says why CI is blocked.
    • No CI: no check appears within three minutes of the push. The round counts as green.
  3. At the park, the PR leaves draft only when the review approved a commit on which every check is green. A blocked round, or one that used all its revisions, parks with the PR still a draft.

The approach. The build learns about CI the way it learns about the review. GitHub's check_run and status webhooks publish ci.updated for the branch, and that wakes the build's Workflow.wait, the mechanism from DRU-1047. A gate would be wrong here: a gate tells a person the run needs them, and nobody needs to act while CI runs. The webhook only says "look again". The build always reads the checks from GitHub itself. It also reads them once a minute, so a check that finished before the wait began, or a delivery that never arrived, is not missed. The verdict is written to the run's journal as a CiReport. The journal then answers two questions from that record: what the next implement round must fix, and whether the PR is ready to leave draft.

What changed, and why

Change Why this way
pre_push_tests is a top-level key of RepoPolicy. The implement prompt lists those tests, or asks for the profile's test commands narrowed to the changed files. The issue's first decision: a pinned verification replaces every command the profiler found, so a key inside it would remove them.
GitHubClient.list_check_runs, list_commit_statuses, and get_job_log These are plain reads in the API client. The operator App already has the permissions (DRU-938).
on_check_run_completed and on_status publish ci.updated with the branches the commit heads. The subscriber ci_update_wakes_the_build answers the build on each branch with no facts. One shape on the topic: the fact only says "look again", and the build reads the checks itself. A check on a fork's commit or a tag has no head branch, so it wakes no one.
Check is one check run or commit status. It is built with from_check_run / from_status, and reads its own log with get_log: the Actions job log, or the check's own output for any other app. The data and the rule that reads it are on one class.
CiReport.judge holds the green, red, and blocked rule. It is a plain method with no I/O. The rule lives on the class that owns the verdict. The tests call it directly, without DBOS or a __wrapped__ step.
Build._wait_for_ci: a _now step, then a loop of the _read_ci step and Workflow.wait("ci.updated", ttl_seconds=60) _read_ci is a step, so a replay reuses each reading. _now records the start time once, so the three-minute and one-hour limits survive a restart. A WaitTimeout only means "read again".
The implement loop: red with revisions left goes back to implement before the review. Red with no revision left, blocked, and green go to the review. "The build does not dispatch the review of a round until every check on the head commit has finished." A capped or blocked round still gets a review, so the person at the park sees one.
BuildJournal.revision_request returns red CI's failures until a review supersedes them. is_ready_for_review is true only for an approving review recorded after green CI. Both are derived from the journal. Nothing new is stored.
ReviewWork.on_wait clears the draft only when the round is ready. The park label adds the reason when CI is blocked. "A blocked or capped build parks with the PR still a draft."
_approved_work clears the draft before it merges or ends. A person's approval should end the build, and GitHub merges no draft. See question 1.
Constants (CI_REGISTER_SECONDS, CI_TTL_SECONDS, CI_POLL_SECONDS, CI_LOG_LINES) in constants.py, and a paragraph in docs/configuration.md The docs state behavior only: the key, the wait, and when the PR leaves draft.

Two things to know:

  • Builds in flight at deploy. The CI wait adds steps after implement. A build that is past its first push when this deploys fails on replay and needs a new start. I added no compatibility code, because strict backward compatibility is not required yet.
  • Which commit the wait reads. The wait reads CI on the commit the implementer delivered (head_sha). That is the commit the implementer's next round can fix.

Tests

Automated, on fb6640c4:

  • ruff check, ruff format --check on the changed files, and druks-house-rules check --base origin/main pass. GitHub CI passes on all seven checks.

  • pytest backend/: 2614 passed. The only failure is the known macOS-only test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds.

  • test_build_ci.py tests CiReport.judge:

    • no CI before and after three minutes;
    • running CI before and after one hour;
    • neutral and skipped checks;
    • a check red only on the PR, with its log;
    • checks also red on the default branch.

    It also tests Check.get_log: the end of an Actions log, and another app's output.

  • test_build_journal.py: red CI is the revision request until a review supersedes it, and only an approving review after green CI is ready.

  • test_build_plan_phase.py:

    • red CI goes back to implement before the review, and goes to review when no revision is left;
    • the park clears the draft only when the round is ready;
    • a blocked park's label gives the reason.
  • test_webhooks_github.py and test_lane_reactions.py: check_run and status publish ci.updated with the branches, and the subscriber wakes only the build of that branch.

  • test_build_prompts.py: the prompt renders pre_push_tests when they are set, and the narrowed test commands when they are not.

Live, and why. The change sits between GitHub and a durable run: webhooks, a wait that has to survive restarts, and CI that finishes on its own schedule. No unit test spans that, so each case ran a real build on a local install against dserbano/druks-test-stub, which has real GitHub Actions. I filed each board ticket and approved each plan in the dashboard UI.

The test repository's CI has these jobs:

  • notes fails when a note does not end with Checked..
  • e2e fails while main carries a marker file.
  • external-lint posts the commit status external/lint, pending first, and fails it when a note has no # title line.

Its pull_request trigger ignores docs/**.

# Why Ticket, PR Result
1 The main path: red, then green DSE-68, PR #78 ✅ notes failed on c9b49b8, and the round went back to implement without a review. The implementer fixed it from the log and pushed 13522c3. CI was green, the review approved, and the PR left draft.
2 The webhook wakes the wait, not only the poll DSE-66, PR #76 ✅ The last check finished at 14:08:25, and the wait woke at 14:08:26.
3 Red on main too is blocked, not red DSE-69, PR #79 ✅ There was one implement and no CI round trip. The park label said CI is blocked and named e2e. The review approved, but the PR stayed a draft.
4 A repository with no CI DSE-70, PR #80, which changes only docs/ ✅ No check registered. The build continued as green after three minutes, and the PR left draft after the approval.
5 pre_push_tests DSE-71, PR #81 ✅ The implementer ran sh .ci/check-notes.sh before its first push and reported it in checks as pass. The first push was green. The same ticket without the key (test 1) went red on its first push.
6 Red with no revision left DSE-72, PR #82, with Max implementation revisions set to 1 in the UI ✅ The round went to review, not back to implement. The review requested changes, and the PR stayed a draft.
7 Durability: a restart during the wait DSE-73, PR #83. I restarted the web container about 70 seconds into the wait. ✅ The run recovered with no replay error and continued as green after three minutes.
8 A person's change request after the park, and which instructions win PR #83 ✅ Triage ran, and the new note went red on 8595021. The next round received the CI failure, not the older triage instructions, and pushed 7cc2e64. CI was green, and the PR left draft.
9 A commit status, not a check run (outside CI services) DSE-75, PR #84 ✅ The build waited on the pending external/lint status while every check run was green. The status webhook woke it when the status failed. The implementer fixed the note from the status description, and the PR left draft after green.
10 After the rebase on #1113, which changes how a step commits DSE-77, PR #86 ✅ Red on 4202df5 went back to implement. On e96b99c everything was green, the review approved, and the PR left draft.
11 After the rebase on #1104, which changes Build.dispatch, where every board build starts DSE-78, PR #87 ✅ Red on 2fba230 went back to implement. On 2bb74c8 everything was green, the review approved, and the PR left draft. No replay or step error was logged.

DSE-66 stopped with needs_clarification. Its plan pinned the note to exactly one line, which the CI rule contradicts. The implementer reported the contradiction instead of guessing. The ticket's wording caused it, not this change.

Not run live, and why:

Pending Questions

  1. The draft and the none gate. _approved_work now clears the draft before it merges, as well as before it ends. A person's approval runs through it, which is the intended change: GitHub merges no draft. But implementation_approval: none also calls _approved_work.

    • With on_approval: none, nothing changes: main already clears the draft there.
    • With on_approval: merge, main asks GitHub to merge a PR that is still a draft. Now the build clears the draft first and asks GitHub to merge, even when CI is not green. Auto-merge still waits for the repository's required checks.

    The issue gives the none gate to DRU-614. Should I narrow the change so that only a person's approval clears the draft before a merge, which keeps the none gate as it is on main? Or should I keep it and add a sentence to the description?

@czpython czpython left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on the direction, not on the work. This follows DRU-1044 closely, and the live runs are thorough. The changes come from rethinking the ticket.

The new shape: the implementer pushes and its call ends. The build waits on ci.updated with the VM released, wakes on a webhook or the 60-second timer, and reads only whether CI on the head has finished and whether it is green. When CI is red, the implementer reads the failures itself.

  implement              lint, typecheck, tests for its diff, commit, push
      |
      v
  head changed? -- no -----------------------> review      it pushed nothing
      | yes
      v
  read CI <-----------------+
      |                     |
      +-- running --> wait -+    a webhook or the 60 s timer wakes it;
      |                          the VM is released, and no agent runs
      +-- green -----------------------------> review
      +-- nothing registered after 3 min ----> review
      +-- still running after 1 h -----------> review      the park label says so
      +-- red -+-- revisions left -----------> implement   "CI on <sha> is red"
               +-- none left ----------------> review

  review
      |
      +-- request changes, revisions left --> implement
      +-- approve, or none left ------------> park: the PR leaves draft only if
                                                    CI is green and the review after it approved
  1. The implementer reads CI; the build only waits. The build reads two facts from GitHub: whether CI on the head has finished, so it knows when to wake the implementer, and whether it is green, for the draft rule. Remove Check, CheckFailure, the per-check rules in CiReport.judge, get_job_log, list_check_runs, CI_LOG_LINES, and the default-branch read. On red, the revision request is one line: "CI on <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." gh in the box acts as the operator App, which can read Actions logs. The box's GitHub MCP server loads only its default toolsets, which leave out actions.

  2. Read "finished" from check suites, not check runs. Actions creates the check run of a needs: job only when the jobs it needs finish. On astral-sh/uv run 42286852249, 67 jobs appeared in 9 waves over about 8 minutes. The webhook that wakes the read is an upstream job's completion, so "every check run finished" can be true between waves. Actions opens one check suite per workflow run, and the suite stays open until the run completes.

    • Finished: every suite with check runs is completed, and no commit status is pending.
    • A suite with no check runs counts as running for 3 minutes after the push, then is ignored. Some apps leave a queued suite with no runs for good. The claude app has one on this PR's head.
    • Green: every suite is success, neutral, or skipped, and every status is success.
    • githubkit has checks.async_list_suites_for_ref.
  3. Drop the red-on-main rule. It matches check names, and on this repo it fails both ways. The test workflows run only on pull_request, so main never has their checks. On main, backend is the release-image build, not the tests. Instead, the implementer decides. When a red check is not from its diff, it pushes nothing and reports the check in checks with the reason. In _implement_phase, a delivery whose head_sha equals the previous delivery's skips the CI wait and goes to review. The PR stays a draft, and the park label says CI is red. The implement prompt must allow that round with no push.

  4. Release the VM during the CI wait. Add release_sandbox: bool = False to Workflow.wait, and pass True from the CI wait. It calls _reap_run(), as a gate park does. release revokes the box identity before it deletes the VM, so the next implement call boots a new VM, and RepoWorkspace re-clones before every call anyway. Update the wait section of docs/writing-an-app.md.

  5. Drop pre_push_tests. A fixed list is the same for every diff, so in a large repo it is either too slow or it tests the wrong area. Keep the prompt change: before every push, run lint, typecheck, and the tests narrowed to the files you changed. Remove the key from docs/configuration.md.

  6. Narrow _approved_work (your question 1). Clear the draft before a merge only on a person's approval. Leave implementation_approval: none to DRU-614. As it is now, with none and on_approval: merge, merge_when_ready squash-merges a capped red round on a repo without required checks.

Keep as they are: the check_run and status webhooks, ci.updated and its subscriber, the 60-second timer, _now and _read_ci as steps, the 3-minute and 1-hour limits, the revision cap, is_ready_for_review, and the park label.

Tests: drop the Check.get_log and red-on-default tests. Add one for the suite read, with a needs: wave and a suite with no runs, and one for the unchanged head.

This branch was successfully deployed

1 active deployment
staging - docs — fb6640c4 Deployed Nov 6, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants