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 build waits for CI on each pushed head before the review, and a PR that is not ready stays a draft.

  • The implement prompt asks for the lint and typecheck commands, and the tests narrowed to the changed files, before every push, with each result in checks.
  • After each delivery that moves the head, the build waits on ci.updated. GitHub's check_run and status webhooks publish it, and the wait also ends every minute. Each wake reads the check suites and the commit statuses on the head.
  • Workflow.wait takes release_sandbox. The CI wait passes it, so no sandbox runs while the build waits, and the next agent call boots a new one.
  • CI has finished when every check suite that has check runs is completed and no commit status is pending. The build reads suites, not check runs: Actions creates the check run of a needs: job only when the jobs it needs finish, but the suite stays open until the workflow run completes.
  • A suite with no check runs counts as running for three minutes after the push, then is ignored. With nothing registered by then, the repo has no CI, and the round is green. An app that leaves such a suite on every push makes each CI wait last at least three minutes.
  • Green means success, neutral, or skipped on every suite, and success on every status.
  • Red CI sends the round back to implement before its review, within max_implementation_revisions. The implementer reads the failures itself with gh and fixes what its diff caused. When no red check comes from its diff, it pushes nothing and reports each one in checks with the reason. A delivery that leaves the head unchanged skips the CI wait and goes to the review.
  • CI still running after an hour goes to the review unfinished.
  • At the work park, the PR leaves draft only when the round's review of a head with green CI requested no changes. Otherwise it stays a draft, and the park's label says that CI is red or did not finish.
  • A person's approval with on_approval: merge clears the draft before the merge, because GitHub merges no draft.

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 10, 2026, 5:33 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.

@dserbano

Copy link
Copy Markdown
Collaborator Author

The new direction: how each point was done, and the live tests

c3b29036 reworks the change to the shape in your review, and is rebased on current main. The build only waits for CI. The implementer reads the failures.

Each point of the review

Your point What changed
The implementer pushes and its call ends. The build waits on ci.updated, woken by a webhook or the 60-second timer Build._wait_for_ci loops _read_ci and Workflow.wait("ci.updated", ttl_seconds=CI_POLL_SECONDS, release_sandbox=True).
Release the VM during the CI wait Workflow.wait takes release_sandbox: bool = False. With True, it calls _reap_run(), as a gate park does. The wait section of docs/writing-an-app.md says so.
The build reads only whether CI on the head has finished and whether it is green _read_ci reads list_check_suites (checks.async_list_suites_for_ref) and list_commit_statuses, and CiReport.judge returns green, red, running (past the hour), or None while CI can still change.
Remove Check, CheckFailure, the per-check rules, get_job_log, list_check_runs, CI_LOG_LINES, and the default-branch read All removed. CiReport has only head_sha and conclusion.
Finished: every suite with check runs is completed, and no status is pending CiReport.judge.
A suite with no check runs counts as running for 3 minutes, then is ignored CiReport.judge drops a suite with latest_check_runs_count == 0 once CI_REGISTER_SECONDS have passed.
Green: every suite is success, neutral, or skipped, and every status is success CiReport.judge.
On red, a one-line revision request CiReport.revision_request is your sentence: "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."
Drop the red-on-main rule. The implementer decides, and pushes nothing when a red check is not from its diff The rule is gone. The implement prompt allows that round: push nothing, report each red check in checks with the reason, and report the branch's current head as base_sha, head_sha, and commit_sha.
A delivery whose head_sha equals the previous delivery's skips the CI wait and goes to review, and the park label says CI is red _implement_phase compares the delivery with the one before it. The park label reads "CI on <sha> is red", or "did not finish within 60 minutes".
Drop pre_push_tests, keep the prompt change, remove the key from docs/configuration.md The key, the template branch, and the docs paragraph are gone. Before every push, the prompt asks for lint, typecheck, and the tests narrowed to the changed files.
Narrow _approved_work. Clear the draft before a merge only on a person's approval, and leave none to DRU-614 _approved_work is as on main. The person's approve branch of _work_gate clears the draft first when on_approval: merge.
Keep the webhooks, ci.updated and its subscriber, the timer, _now and _read_ci as steps, the limits, the revision cap, is_ready_for_review, and the park label Unchanged. The label's text changed only so that it can say "red" or "did not finish", as the diagram asks.
Tests: drop the Check.get_log and red-on-default tests. Add the suite read, with a needs: wave and a suite with no runs, and the unchanged head Dropped. test_build_ci.py covers a workflow between needs: waves, a suite with no runs, a pending status, and red and green. test_build_plan_phase.py covers the unchanged head, the label, and the person's approval before a merge. test_durable_sdk.py covers release_sandbox.

Automated checks

  • ruff check, ruff format --check, and druks-house-rules check --base origin/main pass.
  • pytest backend/ passed 2653 tests. The one failure is the known macOS-only test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds.

Live tests

Each test ran a real build on a local install of this commit against dserbano/druks-test-stub, which has real GitHub Actions. I filed each board ticket and approved each plan in the dashboard. A person's GitHub review gave each approval and change request.

The test repository's CI has these parts:

  • a wave job that needs: two others, so its check run appears in a second wave;
  • external/lint and external/compliance, which are commit statuses posted from a workflow;
  • length, which fails a note longer than two lines;
  • slow, which runs 65 minutes for one test's note;
  • an e2e job that fails while main carries a marker file.

The repository also has three apps (Netlify, Supabase, and the operator App) that leave a queued suite with no check runs on every commit.

# Case PR Result
1 Suites with no runs count as running for 3 minutes, then are ignored #94 ✅ Actions completed green 39 seconds after the wait began. The build kept reading None until the 3 minutes had passed, then recorded green.
2 The VM is released during the CI wait #94 ✅ The box identity was revoked 1 second after the wait began. No sandbox ran during the wait.
3 A restart during the CI wait #94 ✅ I restarted the web container 30 seconds into the wait. The run recovered with no replay error.
4 Webhooks wake the wait #95, #99 ✅ Reads came seconds after each delivery, not only every 60 seconds.
5 A pending commit status keeps the wait open #99 ✅ I held a status pending by hand for 6 minutes. Actions completed, and the 3 minutes passed, but the build read running until I posted success. A webhook then woke it, and it recorded the verdict 1 second later.
6 Red, fixed, green, and the PR leaves draft #99 ✅ external/compliance failed. The implementer read the status, fixed the note, and pushed. CI on the new head was green, the review approved, and the PR left draft.
7 The implementer reads red CI with gh and fixes what its diff caused #95 ✅ It ran gh pr checks and gh run view, fixed length and the title, and reported e2e in checks as caused by the marker on main.
8 CI red only from main, so the implementer pushes nothing #95 ✅ The next round returned the same head_sha with each red check and its reason in checks. The build skipped the CI wait and went to review.
9 The park after red CI #95 ✅ The review approved, but the PR stayed a draft. The label was "Review implementation on GitHub: PR #95. CI on 005e6ac is red", and the dashboard showed the same text under "Needs you".
10 A person approves a red PR #95 ✅ The run resumed, cleared the draft, and finished.
11 A failing commit status alone is red #96 ✅ Every check run was green, but external/compliance failed. The verdict was red.
12 Red with no revision left #96 ✅ With max implementation revisions at 1, red went to the review, not back to implement. The PR parked as a draft.
13 A person approves with on_approval: merge #96 ✅ _clear_draft ran, then declare_merge_intent, and GitHub merged the PR.
14 A repository with no CI #97 ✅ The PR changed only docs/, which the workflow ignores, so only the empty suites appeared. The verdict was green after 3 minutes, and the PR left draft.
15 A person's change request after the park #94 ✅ Triage turned it into instructions, the implementer pushed, and a new CI wait on the new head read green before the review. An earlier change request that said only "Test" got a triage question, and the run parked again.
16 CI still running after an hour #98 ✅ slow still ran when the wait reached 60 minutes. The build went to review, the label was "CI on b40c36b did not finish within 60 minutes", and the PR stayed a draft.

Not run live: the review agent requesting changes after green CI. Every live review approved, and a unit test covers that loop.

Test 1 (PR #94) ran while the install's public tunnel was down. GitHub's pr.opened delivery never arrived, so I replayed it through the app's own subscriber. The review's answer was dropped, so I sent it to the run. Everything after the tunnel came back ran with no help.

@dserbano
dserbano requested a review from czpython November 10, 2026 16:39

@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.

The rework does everything the last review asked, and the live runs cover the cases that worried me: the needs: wave, a status held pending, the unchanged head, and the merge after a person's approval. One behavior change is left, and four small fixes. I updated the PR description to this version myself.

  1. A comment review also takes the PR out of draft. The review returns approve, comment, or request_changes, and comment means findings with none blocking. _implement_phase already lets comment go on to the park, but is_ready_for_review asks for approve. So a build with green CI and a review that only leaves notes parks with a draft PR, which every park on main cleared. Use the loop's rule in journal.py: the last review after green CI is not request_changes. The ticket said "approving review", so this one is on the ticket. Update the property's docstring, its test, and the draft sentence in the PR description.

  2. Rename the running verdict to unfinished. CiReport.judge returns None while CI runs, and running once the build stops waiting for it, so the word has two meanings. The CI_TTL_SECONDS comment already says "unfinished", and the label says "did not finish".

  3. Name the CiReport variable ci_report. It is checks in _implement_phase, _work_gate, and both journal properties, and report in _wait_for_ci. checks is also ImplementationOutput.checks, the field the new prompt and the revision request tell the implementer to fill.

  4. Docs.

    • docs/writing-an-app.md: drop "such as one on the checks of a commit". Core docs give no app's use case: "A wait that can last long can pass release_sandbox=True."
    • docs/configuration.md: there is no new key now, so two sentences are enough: "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."
  5. Test helpers in test_build_plan_phase.py. _implementing_build and _parked_build return a tuple of the flow and a list, but their annotations say -> list and -> list[bool]. _parked_build has one caller, so inline it.

One thing to know, with no change asked: because the operator App leaves a suite with no check runs on every push, each CI wait lasts at least three minutes, as your live test 1 showed. That rule came from my review. It stays for now, and the PR description says so.

@dserbano

Copy link
Copy Markdown
Collaborator Author

The second review: each point and the tests

89dbb81d makes every change from your second review, and is rebased on current main.

Each point

Your point What changed
A comment review takes the PR out of draft. Use the loop's rule in journal.py, and update the docstring, its test, and the draft sentence of the description is_ready_for_review is true when the last review after green CI is not request_changes. Its docstring reads "The round's review of a head with green CI requested no changes." The journal test covers approve and comment, which are ready, and request_changes, unfinished CI, and a review recorded before CI, which are not. The park test adds a comment review on green CI, and the PR leaves draft. The on_wait comment says "requested changes", not "did not approve". In the description, the draft sentence now ends "…the round's review of a head with green CI requested no changes."
Rename the running verdict to unfinished CiReport.conclusion is Literal["green", "red", "unfinished"]. The docstring, judge, the park label's check, and the tests use it.
Name the CiReport variable ci_report It is ci_report in _implement_phase, _wait_for_ci, _work_gate, is_ready_for_review, and revision_request.
docs/writing-an-app.md: no app use case Your sentence: "A wait that can last long can pass release_sandbox=True."
docs/configuration.md: two sentences Your two sentences replace the paragraph.
Test helpers in test_build_plan_phase.py _implementing_build is annotated -> tuple[Build, list[str]]. _parked_build is inlined into its one caller.

Automated checks

  • ruff check, ruff format --check, and druks-house-rules check --base origin/main pass.
  • pytest backend/ passed 2665 tests. Three tests fail, and each also fails on main on my Mac:
    • test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds, which is macOS-only;
    • the two test_druks_sandbox_script tests, where the script exits with 127 locally.

Live tests on this commit

Each test ran a real build on a local install of 89dbb81d against dserbano/druks-test-stub. I filed each board ticket and approved each plan in the dashboard.

# Case PR Result
1 A comment review after green CI #102 ✅ CI read green. For the review agent to comment, the test repository's review.md asked for one non-blocking finding on each note. The review returned decision: comment, and GitHub shows it as COMMENTED. The build went to the park, and the PR left draft. Afterwards I removed the rule.
2 Red, fixed, green, and the PR leaves draft #100 ✅ external/compliance failed on 107c1fc, and the round went back to implement with no review. The implementer read the status and pushed c24daf5. CI on it was green, the review approved, the park label was plain, and the PR left draft. This test runs the renamed ci_report and the new readiness rule.

The live tests from my last comment still cover everything else. This change only renames those paths.

A question: a network blip fails the build

One live run of case 1 failed before its review, with no change made. I dropped and reconnected my Wi-Fi during the test. A _read_ci call to GitHub then hit [Errno -3] Temporary failure in name resolution, and the build failed with that error. The next run passed.

The cause was my network. But the CI wait reads GitHub about once a minute for up to an hour, so it is more exposed to a brief network error than a single step is. _read_ci is a step with no retries, so one failed read fails the build.

@step already takes retries=, so @step(retries=3) on _read_ci would ride out such an error. No step uses it yet. Do you want that here, or keep a failed read failing the build?

@dserbano
dserbano requested a review from czpython November 10, 2026 17:56

This branch was successfully deployed

1 active deployment
staging - docs — 89dbb81d Deployed Nov 10, 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