Repository navigation
Review a build's pull request with the standalone review - #1073
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
czpython
left a comment
There was a problem hiding this comment.
Selecting the MCP identity per agent with an agent keyword is the right shape. Replacing the warm box when its refs change keeps a missing reviewer installation from blocking the whole build. Comments are inline. The two main ones: the token check before evaluation repeats what provisioning already does, and the prompt change reverses the rule that an MCP error never changes the verdict.
A cost to know about: each switch between the operator and the reviewer replaces the warm VM. So each evaluation round provisions two more VMs and clones the repo two more times.
| if agent == SoftwareFactory.evaluate_implementation.id: | ||
| service = (await get_review_actor()).service | ||
| # Fail before the agent can bypass an unavailable reviewer with operator gh. | ||
| await service.issue_token(resources, name="mcp_github_token") |
There was a problem hiding this comment.
Drukbox fetches every entry when it creates the box, and answers 422 with the issuer's reason (#964). The SDK passes that reason through, and Druks does not retry it. So the evaluation box already fails before the agent starts, with the same message.
This call mints a token that nothing uses, on every evaluation, inside a method that only declares servers. Please remove it, and the resources local with it. test_missing_reviewer_installation_does_not_block_implementation and the issue_token mocks then go too.
| async def get_mcp_servers( | ||
| cls, subject: Any, *, agent: str = "" | ||
| ) -> tuple[SandboxMcpServer, ...]: | ||
| service = Github |
There was a problem hiding this comment.
get_secrets above reads cls.github. Use the same here.
| if not self.steps_reuse_sandbox: | ||
| return | ||
| secrets_hash = config.secrets_hash | ||
| if refs: |
There was a problem hiding this comment.
The hash is only compared in memory, so it can be computed every time. The if refs: branch adds nothing.
| A genuine github MCP error is the only acceptable reason to skip a step — never | ||
| your own choice — and it must never change, delay, or replace the JSON verdict | ||
| you return. | ||
| If GitHub MCP is unavailable or rejects the review, do not use `gh`, git, or |
There was a problem hiding this comment.
The text this replaces said an MCP error never changes, delays, or replaces the JSON verdict. With the new text, a short MCP outage makes the evaluation blocked and sends the build to a person. Issue 937 does not ask for that, and the evaluator also had operator gh before this PR. Please keep the old text, and propose this policy in its own PR if you want it.
| assert (start["account_id"], start["note"]) == (dev.id, "Focus on the upgrade path.") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
These three tests repeat test_build_workspace_defaults_to_operator_mcp, plus a test of the token check. One assertion in that test covers the change: call get_all_mcp_servers with agent=SoftwareFactory.evaluate_implementation.id and assert the reviewer's row. The six parametrized agents check the same == six times, and servers is unused.
| ) | ||
|
|
||
|
|
||
| async def test_workspace_can_select_mcp_for_an_agent(druks_db): |
There was a problem hiding this comment.
This defines a throwaway workspace inside the test, so it does not exercise the proof app. test_workspace_mcp_selection_receives_agent_id in test_agents.py already checks that the agent ID reaches get_mcp_servers. Please remove it.
| one is connected. Builds and reviews also get the GitHub MCP server for | ||
| `api.githubcopilot.com`. Build planning, implementation, and feedback use the | ||
| operator App. Only implementation evaluation uses the review identity for MCP. | ||
| Git and `gh` in a build continue to use the operator App. Druks replaces the |
There was a problem hiding this comment.
Shorter: "A review and a build's evaluation use it as the review identity. The other build steps use it as the operator App." writing-an-app.md already describes the warm-box replacement. "continue to use" describes the change, not the behavior.
| reviewer App on every repository that has the operator App, because builds also | ||
| post their reviews through it. | ||
| reviewer App on the repositories it will review. Other project repositories | ||
| are available only when that installation also covers them. A missing reviewer installation affects evaluation |
There was a problem hiding this comment.
The old sentence is still true: builds post their reviews through the reviewer App. Keep it, and add one line: without that installation, a build's evaluation fails, and the error names the App and the repository. The token-check and CLI sentences go if those changes go. This line also runs past the wrap width.
|
Thanks for the review. 2059583 addresses it. Seven comments are applied as asked. On the token check I kept a check in a different form, for the reason below. Applied as asked
Not applied as asked: the pre-evaluation check I removed the
So #964 makes the reason visible, but it doesn't fail the box at creation. Without a check, the docs line "a build's evaluation fails, and the error names the App and the repository" would be false. The check is now its own step,
One test covers it: If you'd rather accept the operator fallback, I can drop the step and the docs line instead. Cost note: acknowledged. Each switch between the operator and the reviewer replaces the warm VM, so each evaluation round adds two VMs and two clones. I left this as is. Verification on 2059583
|
#937: implementation and test reportThis branch now follows the direction in #937. A build dispatches the standalone review and waits on it with the wait from #1088. Commit 1. Requirements and how each is implementedThe requirements come from #937 and the inline review on the earlier version of this PR.
2. The one deviation, and whyThe issue says a The review's run lifecycle answers the build instead:
3. Behaviour the issue left open, now specified and tested
4. Iterations to the current solution, and why
5. Missing reviewer installation, in depthWhere the reason gets lost
Outcome by setup
In every setup the build fails, and nothing reaches the human gate unreviewed. The Drukbox fix only changes how fast it fails and what the message says. The Drukbox fix (narrowed), checked in isolationIt runs the patched
At creation it costs one fetch per secret: six secrets took about 0.9 s. Minted GitHub tokens are reused through the vault, so the request-time fetch doesn't mint again. I'll open it on Drukbox separately. 6. Tests of the current implementation (
|
| # | Scenario | Ticket / PR | Result |
|---|---|---|---|
| 1 | Approve | DSE-24 / #37, DSE-26, DSE-34 / #47 | ✅ druks-review approved, the PR was marked ready, the build parked at the gate |
| 2 | Merge after approval | DSE-24 / #37 | ✅ The build finished, and the ticket went to Done |
| 3 | Request changes | DSE-25 / #38 | ✅ Back to Implement with the review in the prompt |
| 4 | Request changes, then fixes, repeated | DSE-36 / #49 | ✅ The implementer applied every review |
| 5 | Revision cap | DSE-36 / #49 | ✅ Stopped after 5 rounds and parked at the gate |
| 6 | comment decision |
DSE-37 / #50 | ✅ Parked like an approval, with no extra round |
| 7 | Comment mode, no reviewer App | DSE-44 / #57 | ✅ The operator posted a COMMENT that opens with the verdict; the build parked |
| 8 | Identities | All runs | ✅ Every build sandbox's GitHub MCP is the operator, and every review sandbox's is druks-review |
| 9 | Review cancelled while the build waits | DSE-29 / #42 | ✅ The build failed at once with "the review failed: " |
| 10 | Wait limit (20 s in a test image) | DSE-40 / #53 | ✅ "wait on 'review.completed' timed out" |
| 11 | Restart while the build waits | DSE-39 / #52 | ✅ DBOS recovered the review, which approved once; the build parked |
| 12 | Review started by hand on the same PR while the build waits | DSE-38 / #51 | ✅ It joined the build's live review, so no second review ran |
| 13 | Standalone review, no build | PR #38 | ✅ Posted as druks-review; no build affected |
| 14 | A build waiting for plan approval when the new code arrives | DSE-42 / #55 | ✅ Finished normally on the new code |
| 15 | A build already past implementation when the new code arrives | DSE-41 / #54 | DBOSUnexpectedStepError. ✅ Retry run continued it through the new review to the gate. |
| 16 | The removed "Code review" setting | — | ✅ Gone from the settings page |
| 17 | Missing reviewer installation, current Drukbox, proxy provider | DSE-30 / #43 |
Comment mode (7) ran with get_review_actor() forced to its no-reviewer answer in a test image, so the real reviewer connection stayed in place. Tests 1 (DSE-34), 4–6, 10–12 and 14–15 ran with the narrowed Drukbox fix. It only changes the case where a secret needs a person, and every secret in those runs was valid.
7. Findings for follow-ups, not in this PR
- Drukbox creates proxy-provider boxes without checking secrets. A refused secret only reaches the agent, as a 503. The narrowed fix above closes this.
- Reviewers can reverse earlier rounds. The implementer dismisses the reviews it addressed, and the reviewer doesn't read earlier rounds, so a later review can undo an earlier one until the cap stops it (DSE-36). One prompt paragraph fixes it ("Read the pull request's earlier reviews first, including dismissed ones. Do not reverse what an earlier review asked for unless it breaks an acceptance criterion; when you do, say which and why."). In a rerun, the build converged in 3 rounds instead of 5 (DSE-43). It's on
issues/937-review-earlier-rounds, for its own PR. - A review that outlives its build can still post. A cancelled review approved 40 seconds after the cancel, and a build past its wait limit left an approved draft PR.
- A replay-mismatch failure shows no reason in the UI.
- A Claude agent with a leftover background task never exits in
--printmode. It only stops at the 30-minute limit.
czpython
left a comment
There was a problem hiding this comment.
The shape is right, and the switch to the finished run's result is correct: Artifact.record writes the event row and never publishes, while a returned model rides the finished event for exactly this. One change, two nits, one question.
| status: Literal["pass", "fail", "not_run"] | ||
| evidence: str | ||
|
|
||
| class ReviewOutcome(BaseModel): |
There was a problem hiding this comment.
ReviewReport already is this. The platform dumps a returned model into the finished event's result, so PullRequestReview.run can return the report and the build can validate the facts back into ReviewReport for the journal. That drops this class, the review string built from the artifact, and the duplicated Decision: line in the implement prompt.
| async def run(self, note: str = "") -> dict[str, str]: | ||
| # The finished event carries this, so a build waiting on the review reads it there. | ||
| report = await SoftwareFactory.review_pull_request() | ||
| return {"decision": report.decision, "review": report.to_artifact()["content"]} |
|
|
||
| {% if review.body %} | ||
| {{ review.body }} | ||
| {{ review.review }} |
There was a problem hiding this comment.
With the report in the journal this renders review.summary and its findings. The header already states the decision.
| logger = logging.getLogger(__name__) | ||
|
|
||
| # A review takes minutes. A build still waiting after this long was never answered. | ||
| REVIEW_TTL_SECONDS = 2 * 60 * 60 |
There was a problem hiding this comment.
constants.py holds the package's constants.
| one is connected. Builds and reviews | ||
| also get the GitHub MCP server for `api.githubcopilot.com`. It acts as the | ||
| review identity through a second entry. | ||
| also get the GitHub MCP server for `api.githubcopilot.com`. A build's acts as |
There was a problem hiding this comment.
Lost its noun. "The build's server acts as the operator App. The review's acts as the review identity."
Review of b628f9c: changes and testsThanks for the review. All six comments are applied in the commit on top of How each comment was addressed
Question. Automated checks
UI tests on this commitThese ran on a local install, against
The round-2 implement prompt from DSE-46's sandbox: Why only these were re-runThis commit changes three things:
It doesn't change the build's step order, the wait, the TTL, sandbox identities or the settings. Each re-run scenario covers one of the changed paths:
The earlier results on
|
|
Good catch, and it is a hole in my suggestion: |
Rebased on #1101: the failure goes through
|
| # | Scenario | Ticket / PR | Result |
|---|---|---|---|
| 1 | Review cancelled while the build waits | DSE-49 / #61 | ✅ The build failed with wait on 'review.completed' failed: cancelled for the 937 UI test, code wait_failed |
| 2 | Review started by hand on the PR and cancelled while the build is parked at the gate | DSE-50 / #62 | ✅ The review ended cancelled. The build stayed parked at review_work, with the ticket In Review and no new events. With Build.cancel it would have been cancelled. |
| 3 | Approve | DSE-50 / #62 | ✅ druks-review approved, the report reached the build, and it parked at the gate |
The install's GitHub App was still delivering to an expired tunnel, so GitHub's pull_request.opened events for #61 and #62 didn't arrive, and the work items had no PR number. I posted the two opened pull requests, as the API returns them, to the install's GitHub webhook endpoint, signed with the App's webhook secret. Then I started each review from the UI.
Why the other tests were not re-run
Since the last round, the change is the stopped-review path. The rest of the PR is the same diff, now on a newer main.
- Scenarios 1 and 2 cover both sides of that path: a build waiting on the review, and a build that isn't.
- Scenario 3 is the smoke test for the rebase, from review to gate on today's
main.
Earlier results still hold for the rest:
| Scenario (earlier reports) | Why it wasn't re-run |
|---|---|
| Request changes, with the findings in the next prompt (DSE-46) | _review_work, the journal, and the template are unchanged since that run |
| Repeated rounds and the revision cap (DSE-36) | Same loop; unit tests cover the cap |
comment decision, comment mode (DSE-37, DSE-44) |
Same path as approve; prompt and identity unchanged |
| Merge after approval (DSE-45) | The ReviewWork gate and the merge path are unchanged |
| Standalone review with no build (#57) | PullRequestReview.run is unchanged. A review with no build waiting still answers no one, and scenario 2 shows a stopped one leaves a build alone. |
| Wait limit, restart, manual review joining the live review (DSE-40, DSE-39, DSE-38) | self.wait, the TTL, DBOS recovery and one live run per subject are unchanged |
| Identities, settings page, builds in flight across the deploy | get_mcp_servers, the settings and the step order are unchanged |
| Missing reviewer installation (DSE-30, DSE-33) | It reaches the build through the same failure path as scenario 1. The message now starts with wait on 'review.completed' failed:. |
A build no longer reviews its own work. After implementation it dispatches
PullRequestReview, the standalone review, and waits on it withself.wait("review.completed", …)from #1088. The review runs on its own sandbox as the review actor. The build's GitHub MCP signs as the operator (cls.github), like its clone.request_changesgoes back to implement, up tomax_implementation_revisions. The next implement prompt carries the review's summary and findings under "Prior implementation review".approveparks onReviewWorkas before. The pull request stays a draft until then.review_pull_requestholds the pull request to the## Acceptance Criteriain its description. With none, it just reviews.implement.mdno longer forbids the verification profile: verification is the implementer's.evaluate_implementation,EvaluationOutput,EvaluationVerdict,journal.evaluations,_github_review.md, and thereview_codesetting that only fed the evaluator are gone. No agent keyword, no warm-box rotation, no reviewer-access check.One change from the issue
The issue has a
review.completedsubscriber answer the build, withReviewReport.to_eventgainingdecision.to_eventis recorded on the run but never published, so no subscriber sees it. The review's run lifecycle answers the build instead:PullRequestReview.runreturns itsReviewReport. TheFINISHEDevent carries it, and a subscriber callsBuild.answer(item, "review.completed", **result). The build validates the facts back intoReviewReportfor the journal.FAILEDandCANCELLED, a subscriber answers withfailure=(Let an answer fail a wait #1101). It reaches only a build waiting on the review, whose wait raisesWaitFailedwith the review's reason.Behaviour the issue left open
commentdecision parks the build likeapprove.REVIEW_TTL_SECONDS). Then it fails withwait on 'review.completed' timed out.Deploying
This changes
Build's step order. A build that has already passed implementation when the new code starts fails on recovery withDBOSUnexpectedStepError. Drain those builds first, asdocs/deployment.mddescribes, or press Retry run on each. Retry resumes at the failed step, which is now the review, and the build continues. A build that has not reached implementation continues unchanged.Missing reviewer installation
On Drukbox providers that push secret values (
docker_sbx), a review whose reviewer App is not installed on the repository fails at box creation with the issuer's reason (#964). The build then fails withwait on 'review.completed' failed: The GitHub App druks-review has no access to <repo> — install the app on the repo ….On providers that swap secrets through the proxy (
docker, the local install), Drukbox creates the box without fetching. The agent's first GitHub call then gets a bare 503, and the review ends at the agent timeout (30 minutes).Fixes #937.