Skip to content

Review a build's pull request with the standalone review - #1073

Merged
czpython merged 1 commit into
mainfrom
issues/937
Nov 5, 2026
Merged

czpython merged 1 commit into
mainfrom
issues/937

Conversation

@dserbano

@dserbano dserbano commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

A build no longer reviews its own work. After implementation it dispatches PullRequestReview, the standalone review, and waits on it with self.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_changes goes back to implement, up to max_implementation_revisions. The next implement prompt carries the review's summary and findings under "Prior implementation review".
  • approve parks on ReviewWork as before. The pull request stays a draft until then.
  • review_pull_request holds the pull request to the ## Acceptance Criteria in its description. With none, it just reviews.
  • implement.md no longer forbids the verification profile: verification is the implementer's.
  • evaluate_implementation, EvaluationOutput, EvaluationVerdict, journal.evaluations, _github_review.md, and the review_code setting 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.completed subscriber answer the build, with ReviewReport.to_event gaining decision. to_event is recorded on the run but never published, so no subscriber sees it. The review's run lifecycle answers the build instead:

  • PullRequestReview.run returns its ReviewReport. The FINISHED event carries it, and a subscriber calls Build.answer(item, "review.completed", **result). The build validates the facts back into ReviewReport for the journal.
  • On FAILED and CANCELLED, a subscriber answers with failure= (Let an answer fail a wait #1101). It reaches only a build waiting on the review, whose wait raises WaitFailed with the review's reason.

Behaviour the issue left open

  • A comment decision parks the build like approve.
  • A build waits at most two hours for its review (REVIEW_TTL_SECONDS). Then it fails with wait on 'review.completed' timed out.
  • A failed or cancelled review fails the build, with the review's reason.
  • A review started by hand on the pull request a build is waiting on joins the build's live review. It does not start a second one.

Deploying

This changes Build's step order. A build that has already passed implementation when the new code starts fails on recovery with DBOSUnexpectedStepError. Drain those builds first, as docs/deployment.md describes, 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 with wait 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.

@dserbano
dserbano requested a review from czpython as a code owner November 3, 2026 02:16
@mintlify

mintlify Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

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

Project Status Preview Updated
druks 🟢 Ready View Preview Nov 5, 2026, 2:57 PM

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

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

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

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.

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

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.

get_secrets above reads cls.github. Use the same here.

Comment thread backend/druks/workflows.py Outdated
if not self.steps_reuse_sandbox:
return
secrets_hash = config.secrets_hash
if refs:

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

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 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(

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.

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

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.

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.

Comment thread docs/configuration.md Outdated
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

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.

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.

Comment thread docs/configuration.md Outdated
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

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

@dserbano

dserbano commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

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

  • get_mcp_servers reads cls.github, and the resources local is gone.
  • _lease_host always computes the combined hash. The if refs: branch is gone.
  • _github_review.md is back to main's text: an MCP error never changes, delays, or replaces the verdict.
  • The three review tests are gone. test_build_workspace_defaults_to_operator_mcp asserts the reviewer's row for agent=SoftwareFactory.evaluate_implementation.id.
  • The field_notes test is gone.
  • configuration.md uses your MCP wording. The reviewer-App sentence is back with the one added line, within the wrap width.

Not applied as asked: the pre-evaluation check

I removed the issue_token call from get_mcp_servers. Then I ran the missing-installation scenario against the current ghcr.io/czpython/drukbox:latest. The evaluation box was not stopped:

  • 15:26:47: POST /hosts returned 201. Drukbox created the evaluation box without fetching its entries.
  • 15:26:50: the first mcp_github_token fetch came from the agent's MCP client. Druks answered 422 with the reason (Answer 422 with the reason when a sandbox secret needs a person #964), but only the agent saw it.
  • The evaluator fell back to operator gh and posted a COMMENTED review as druks-app-dserbano[bot] on the test PR. The run parked with "Verdict: pass".

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, Build._check_review_access, run before evaluate_implementation. It is shaped around your objections:

  • It mints no token. It calls get_installation_id with the App's key.
  • get_mcp_servers declares servers and does nothing else.
  • It is a @step, so a replay reuses the checkpoint.
  • The prompt and the MCP-error policy are unchanged.

One test covers it: test_build_evaluation_needs_the_reviewer_on_the_repo.

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

  • Ruff lint and format pass. druks-house-rules check --base origin/main passes.
  • pytest backend/: 2512 passed, 1 failed. The failure is the known macOS test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds.
  • UI scenarios on the local image, using dserbano/druks-test-stub:

@czpython

czpython commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Superseded by the direction now in #937: the build dispatches the standalone review and waits on it with the wait from #1088. A fresh branch against #937 is fine.

@dserbano dserbano changed the title Scope build GitHub reviewer credentials to evaluation Review a build's pull request with the standalone review Nov 5, 2026
@dserbano

dserbano commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

#937: implementation and test report

This branch now follows the direction in #937. A build dispatches the standalone review and waits on it with the wait from #1088. Commit b628f9c3 replaces the earlier approach. Below: how each requirement is implemented, the one deviation, how the solution got here, the missing-installation case in depth, and the tests.

1. Requirements and how each is implemented

The requirements come from #937 and the inline review on the earlier version of this PR.

Requirement Implementation (b628f9c3)
The build's GitHub MCP signs as the operator, like the clone BuildWorkspace.get_mcp_servers uses secret_id=(await cls.github.get()).id. The test asserts the operator's secret ref.
After implementation, dispatch PullRequestReview and wait with #1088's self.wait(...); the PR stays a draft Build._review_work() dispatches the review from the workflow body, so a replay doesn't start a second one. It then calls self.wait("review.completed", label="Waiting for the review", ttl_seconds=REVIEW_TTL_SECONDS), which is 2 hours.
request_changes goes back to implement Build.run loops implement → _review_work while the decision is request_changes and fewer than max_implementation_revisions rounds have run. The review is recorded in the journal as a ReviewOutcome, and the next implement prompt shows it under "Prior implementation review".
approve parks on ReviewWork as today It falls through to the existing _work_gate().
One reviewer, holding the PR to ## Acceptance Criteria review_pull_request.md: a criterion the diff doesn't meet is a high-severity finding. With no criteria, it just reviews.
Verification is the implementer's implement.md drops the ban on the verification profile and adds "Verification is yours: before pushing, run the commands in the Verification profile above."
Remove evaluate_implementation, EvaluationOutput and their readers Removed: the agent, EvaluationOutput and its check types, EvaluationVerdict, journal.evaluations, _github_review.md, evaluate_implementation.md, the prompt-context review_code and review_mode, and the review_code setting, which only fed the evaluator. The templates now say "reviewer".
No agent keyword, no warm-box rotation, no reviewer-access check None added.
Earlier review: no token minted in get_mcp_servers; read cls.github; keep the MCP-error policy No minting anywhere, and cls.github is used. The evaluator prompt that held the MCP-error policy went with the evaluator, and no new error policy was added.
Earlier review: the docs say what fails, and the error names the App and repository configuration.md: "a build hands its pull request to a review that runs as the reviewer App. When that review fails, the build fails with the review's reason." Section 5 covers when that reason names the App and repository.

2. The one deviation, and why

The issue says a review.completed subscriber answers the build, and ReviewReport.to_event gains decision. That can't work: to_event is recorded on the run but never published, so no subscriber would ever see it.

The review's run lifecycle answers the build instead:

  • PullRequestReview.run returns {decision, review}.
  • @subscribe(WorkflowEvent.FINISHED, workflow=PullRequestReview) calls Build.answer(item, "review.completed", **result).
  • The FAILED and CANCELLED subscribers answer with the failure, so the build raises FatalError("the review failed: <reason>").
  • A review with no build waiting on it answers nothing.

3. Behaviour the issue left open, now specified and tested

  • A comment decision parks the build like an approval.
  • A build waits at most 2 hours for its review. Then it fails with wait on 'review.completed' timed out.
  • A failed or cancelled review fails the build, with the reason.
  • A review started by hand on the same PR joins the build's live review instead of starting a second one. This is existing Druks behaviour: one live run per subject.

4. Iterations to the current solution, and why

  1. First version of this PR. The build selected the MCP identity per agent with an agent keyword, so only evaluate_implementation acted as the reviewer. It also replaced the warm box when the secret refs changed, checked for a token in get_mcp_servers, and changed the prompt so it forbade an operator gh fallback. Review: the token check mints a token nothing uses, and the prompt change reverses the MCP-error policy.
  2. Second version (8c8275a7). Seven comments applied. The token check became Build._check_review_access, which mints no token. It stayed because a test showed Drukbox didn't fail the evaluation box at creation, and the evaluator fell back to operator gh and posted as the operator.
  3. A build's GitHub MCP server acts as the reviewer App for every step, not only the evaluation #937 rewritten. The build no longer reviews its own work. It dispatches PullRequestReview and waits, with no agent keyword, no rotation and no access check.
  4. b628f9c3. This implements A build's GitHub MCP server acts as the reviewer App for every step, not only the evaluation #937 as written, apart from the to_event route (section 2).
  5. A missing reviewer installation, tested against A build's GitHub MCP server acts as the reviewer App for every step, not only the evaluation #937 (DSE-30). The build failed only after the 30-minute agent timeout, with "claude timed out". Section 5 traces why.
  6. A Druks-side attempt, set aside (DSE-31). I tried a check that the review was actually posted, plus a prompt rule against retrying. The agent still never exited, because a leftover background task kept it alive, so the check never ran. The prompt rule is also a new error policy, which the earlier review asked to see in its own PR. The code is on issues/937-posted-review-check, outside this PR.
  7. The cause is in Drukbox. Drukbox fetches every secret at creation only for providers that push secret values (docker_sbx). Providers that swap secrets through the proxy (docker) fetch nothing until the first request. So the statement in the earlier review, that Drukbox fetches every entry at creation and answers 422 (Answer 422 with the reason when a sandbox secret needs a person #964), holds for push providers only.
  8. The Drukbox fix, narrowed. Fetching each secret at creation in proxy mode fixed it (DSE-32). But that version also failed a box on a temporary issuer error, which the proxy retries today. The narrowed version refuses only on MissingSecretError (a 422), and lets everything else through as today (DSE-33, plus an isolated test). It touches no Druks code.
  9. Two findings from the remaining tests. Reviewers can reverse earlier rounds, which a prompt follow-up fixes. A build that has already passed implementation at deploy fails on recovery, and Retry run recovers it.

5. Missing reviewer installation, in depth

Where the reason gets lost

  1. The review's box declares github_reviewer (git and gh) and mcp_github_token (GitHub MCP), both signed by the reviewer App.
  2. Push providers (docker_sbx, needs_value = True): at creation, put_secrets fetches every entry from Druks. Druks answers 422 MissingSecretError with "The GitHub App druks-review has no access to — install the app…". Drukbox marks the host failed and raises that reason (hosts/service.py). No agent runs.
  3. Proxy providers (docker, needs_value = False): the box gets placeholders only, and creation succeeds. The first fetch happens when the agent's gh call reaches the proxy, which then loses the reason in two places:
    • The exchange: RefreshableSecret.refresh catches the 422 as a generic IssuerError, logs it and schedules a retry. /authorize then answers 503 with Retry-After: 5.
    • The proxy: swap() turns any answer other than 200 or 403 into 503 exchange unavailable.
  4. In DSE-30, the agent saw only HTTP 503 exchange unavailable and read it as a GitHub outage. The repository is public, so it read the diff with an unauthenticated git fetch. It left a background loop retrying gh pr review --approve, and returned decision: approve with nothing posted. The loop kept the process alive, so that decision never reached the build. The agent was killed at 1800 s, and the review failed. The build then failed with "the review failed: claude timed out after 1800s".

Outcome by setup

Setup When it fails What the UI says PR
This PR + current Drukbox, proxy provider (docker) After about 32 min (agent timeout) the review failed: claude timed out after 1800s Draft, no review
This PR + current Drukbox, push provider (docker_sbx) At review box creation (expected from the code; not run, because docker_sbx doesn't run on macOS) the review failed: The GitHub App druks-review has no access to <repo> … Draft, no review
This PR + narrowed Drukbox fix, any provider At review box creation, about 1 min after implementation the review failed: The GitHub App druks-review has no access to dserbano/druks-test-stub (DSE-33) Draft, no review

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 isolation

It runs the patched put_secrets against a local issuer:

Issuer answer Current Drukbox Patched
Proxy: 200 Created Created
Proxy: 503 (temporary) Created Created (the proxy still retries at request time)
Proxy: unreachable Created Created
Proxy: 422 (needs a person) Created; the agent later gets a 503 Refused with the reason
Push: 200 / 422 Unchanged Unchanged

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 (b628f9c3)

Automated

  • ruff lint and format, and druks-house-rules check --base origin/main, pass.
  • pytest backend/: 2546 passed. The 1 failure is the known macOS-only test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds.
  • Frontend lint passes, and 661 frontend tests pass.

New tests cover:

  • the review hand-off and its journal entry;
  • a failed review raising FatalError;
  • the request_changes loop and its cap;
  • FINISHED, FAILED and CANCELLED answering the build, and no answer when no build is waiting;
  • the prior review rendered in the implement prompt;
  • the build's MCP signing as the operator.

Through the UI

These ran on a local install of this commit, against dserbano/druks-test-stub, with board tickets created and approved through the UI. Identities and states were read back from GitHub and the database.

# 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 ⚠️ Failed on recovery with 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 ⚠️ Failed after 32 min with "claude timed out" (section 5)

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

  1. Drukbox creates proxy-provider boxes without checking secrets. A refused secret only reaches the agent, as a 503. The narrowed fix above closes this.
  2. 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.
  3. 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.
  4. A replay-mismatch failure shows no reason in the UI.
  5. A Claude agent with a leftover background task never exits in --print mode. It only stops at the 30-minute limit.

@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 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):

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.

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"]}

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.

Return report.


{% if review.body %}
{{ review.body }}
{{ review.review }}

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.

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

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.

constants.py holds the package's constants.

Comment thread docs/configuration.md Outdated
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

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.

Lost its noun. "The build's server acts as the operator App. The review's acts as the review identity."

@dserbano

dserbano commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review of b628f9c: changes and tests

Thanks for the review. All six comments are applied in the commit on top of b628f9c3.

How each comment was addressed

Comment Change
ReviewReport already is ReviewOutcome ReviewOutcome is removed. The build validates the finished event's facts into ReviewReport (ReviewReport.model_validate(facts)) and records it in the journal. journal.reviews and revision_request read ReviewReport. The review string built from the artifact is gone.
Return report PullRequestReview.run returns the ReviewReport.
Render review.summary and its findings _contract.md renders the summary, then one line per finding: severity, summary, path:line, evidence. The decision is only in the round header, so the duplicated Decision: line is gone.
constants.py holds the package's constants REVIEW_TTL_SECONDS moved there.
Docs sentence lost its noun "The build's server acts as the operator App. The review's acts as the review identity." The reviewer-App paragraph now says the build is cancelled with the review's reason.
Two shapes on one topic The FAILED/CANCELLED subscriber calls Build.cancel(item, failure="the review stopped: <reason>"). The facts on review.completed are always a review, and the failure branch and FatalError in _review_work are gone. The build reads cancelled.

Question. Build.answer delivered only while the build waited on review.completed. Build.cancel stops the item's live build at any step. So a review someone starts by hand on the PR, if it fails while the build is implementing, now cancels the build. Fine as is, or should the cancel apply only while the build waits on the review?

Automated checks

  • ruff lint and druks-house-rules check --base origin/main pass. ruff format --check flags only durable/engine.py and test_agent_config.py. This commit doesn't touch either, and the base commit shows the same.
  • pytest backend/: 2545 passed, 1 failed. The failure is the known macOS-only test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds.
  • Updated tests:
    • The hand-off asserts a ReviewReport in the journal.
    • The implement-prompt test asserts the summary and a rendered finding.
    • The stopped-review subscriber asserts Build.cancel with the reason.
    • The finished-review subscriber passes the report's shape.
    • The test for a failed review raising FatalError is removed, because that path is gone.

UI tests on this commit

These ran on a local install, against dserbano/druks-test-stub, with board tickets.

# Scenario Ticket / PR Result
1–2 Approve, then merge DSE-45 / #58 ✅ druks-review approved, and the finished event carried the full report. The build parked at the gate with the PR ready. Merging finished the build, and the ticket went to Done.
3 Request changes (report with one finding, injected) DSE-46 / #59 ✅ The build went back to Implement. Round 2 was approved and the build parked.
9 Review cancelled while the build waits DSE-47 / #60 ✅ The build ended cancelled with "the review stopped: cancelled for the 937 UI test".
13 Standalone review, no build #57 ✅ Finished with the report in its event (comment, no findings). No build was affected.

The round-2 implement prompt from DSE-46's sandbox:

## Prior implementation review

### Round 1 — decision: request_changes

Add the missing period.

- **high** The README line is missing its trailing period. (`README.md`): Add a period to the line you appended.

Why only these were re-run

This commit changes three things:

  • what the review returns, and how the build reads it;
  • how a stopped review reaches the build;
  • how the prompt renders a prior review.

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:

  • Approve and merge: the real report through FINISHED → ReviewReport → gate.
  • Request changes: the journal entry and the new prompt rendering.
  • Cancel: Build.cancel replacing the failure answer.
  • Standalone review: run returning the report with no build waiting.

The earlier results on b628f9c3 still hold for everything else:

Scenario (earlier report) Why it wasn't re-run
comment decision (DSE-37) Same path as approve: anything except request_changes falls through to the gate.
Comment mode, no reviewer App (DSE-44) Prompt and identity are unchanged.
Request changes repeated, revision cap (DSE-36) Same loop as scenario 3. Only the journal type changed, and unit tests cover the cap.
Identities (all runs) get_mcp_servers is unchanged.
Wait limit (DSE-40) self.wait and the TTL value are unchanged. Only the constant moved.
Restart while the build waits (DSE-39) DBOS recovery and the step order are unchanged.
Manual review joins the live review (DSE-38) One live run per subject, which this change doesn't touch.
Builds in flight across the deploy (DSE-41, DSE-42) The step order is the same as b628f9c3.
Settings page Unchanged.
Missing reviewer installation (DSE-30, DSE-33) The failure now ends in Build.cancel, which scenario 9 covers. Drukbox's reason text is unchanged.

@czpython

czpython commented Nov 5, 2026

Copy link
Copy Markdown
Owner

Good catch, and it is a hole in my suggestion: cancel reaches the build at any step, a parked one included. The failure should travel through answer, which reaches only a build waiting on the review. #1101 adds that: Build.answer(item, "review.completed", failure=...) makes the build's wait raise WaitFailed with the reason, and the build fails with it. Once it lands, the stopped-review subscriber goes back to one answer call and _review_work stays as it is now.

@dserbano

dserbano commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on #1101: the failure goes through answer

675857f7 is one commit on main that includes #1101.

What changed

  • The stopped-review subscriber is back to one call. On FAILED and CANCELLED it runs Build.answer(item, "review.completed", failure=failure or "the review stopped"), which reaches only a build waiting on the review. That build's wait raises WaitFailed, and the build fails with the review's reason. A build at any other step, including one parked at the gate, is untouched.
  • _review_work is unchanged.
  • The test test_a_stopped_review_fails_the_build_with_its_failure asserts the answer call with failure=.
  • configuration.md and the description say the build fails with the review's reason.

A fix to the previous push. 1957769 was squashed against a newer main than this branch's base. As a result it also reverted 13 files of other work on main, including the bound_session change in druks/workflows.py. I rebuilt the branch on today's main from this PR's own diff, so the PR is back to its 25 files.

Checks on 675857f7

  • ruff lint and druks-house-rules check --base origin/main pass.
  • pytest backend/: 2574 passed. The 1 failure is the known macOS-only test_upload_dir_tar_survives_more_warnings_than_a_pipe_holds.
  • Frontend lint and build pass, and 661 frontend tests pass. The PR touches WorkItemPage.tsx.

UI tests on this commit

These ran on a local install against dserbano/druks-test-stub, with board tickets.

# 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:.

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

nice! 💯

@czpython
czpython merged commit 740780b into main Nov 5, 2026
7 checks passed
@czpython
czpython deleted the issues/937 branch November 5, 2026 15:31

This branch was successfully deployed

1 active deployment
staging - docs — 675857f7 Deployed Nov 5, 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.

A build's GitHub MCP server acts as the reviewer App for every step, not only the evaluation

2 participants