Repository navigation
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
czpython
left a comment
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
|
Build planning, implementation, and feedback now use the operator App for GitHub MCP. Only implementation evaluation selects the reviewer. Previously, a configured reviewer was selected when provisioning the shared build sandbox, so all steps inherited that identity and missing reviewer repository access prevented the build from starting.
The agent invocation passes its identity into workspace MCP selection. Warm sandboxes rotate credentials when secret references change, including after restart. Evaluation checks that the selected review identity can obtain a repository token before starting the evaluator. Missing installation fails with the App and repository named in the error. The review prompt forbids switching to operator CLI credentials when GitHub MCP fails. Without a configured reviewer, evaluation retains operator comment mode. Configuration and workspace-author documentation describe the behavior.
Automated coverage: 437 focused backend tests passed, covering the external proof app, identity selection, sandbox rotation/recovery, and missing reviewer access. All 29 review tests passed again after the final error-propagation adjustment. Ruff and local house rules passed. An earlier full local run encountered the known macOS tar-warning failure; the obsolete reviewer-default assertion was corrected and passed on rerun. GitHub CI on
46d89c67cdc2abfa466629dadd6e9f5158f7ac39passed all six checks: backend, backend (Python 3.14), backend (Postgres 18), frontend, house rules, and Mintlify deployment.Both manual scenarios below were run through the local Druks interface on 2 November 2026, using the deployed
druks-local:issue-937-testedimage anddserbano/druks-test-stub. Each ticket requested one exact sentence appended to README.md, preserving existing content and changing no other files. Each was created from Board with status Ready for Agent; its generated plan was read and approved through the UI.Negative scenario — reviewer lacks repository access: PASS (expected failure).
The reviewer installation was restricted to
dserbano/bunny_rag, excluding the test repository. Ticket DSE-9 successfully completed planning and implementation and opened test PR #6 asdruks-app-dserbano[bot]. Evaluation then stopped before starting its agent with:Druks displayed “FAILED · ESCALATED TO YOU” with successful planning and implementation in the timeline. The PR remained draft with no reviews and no fallback review comment. This demonstrates that missing reviewer installation no longer blocks provisioning, planning, or implementation, and the failure occurs at evaluation with a clear explanation. Run
01a19f34-d195-7e72-824b-b56185a602ac; displayed duration 2m 20s, cost $0.97.Positive scenario — reviewer access restored: PASS.
The operator restored the reviewer installation to all repositories, including the test repository. A separate ticket, DSE-10, preserved the failed run as evidence. Planning and implementation succeeded, creating test PR #7 as
druks-app-dserbano[bot]. Evaluation used the GitHub MCP review tool and posted an approval asdruks-review[bot]. Druks displayed “Verdict: pass” with all four acceptance criteria satisfied and no open findings. The operator then marked the PR ready for review and requested human review. Run01a19f3d-afd7-7f03-a609-1be07676bcfc; displayed duration 2m 50s, cost $1.45.At the time of the reviewer-identity test, both test PRs were unmerged and the positive run was parked at the human review gate. On 3 November, test PR #7 was subsequently merged during validation of #1070; DSE-10 finished and its ticket became Done after the failed GitHub webhook delivery was replayed locally (the public tunnel returned HTTP 502). That later merge test is separate from the reviewer-identity evidence above. GitHub records the bot approval but says it does not count as an applicable review by a reviewer with write access; the identity-switch test does not claim branch-protection approval requirements are satisfied. These are documentation-only UI scenarios, not a test of every harness or repository configuration. The earlier test PR #5 ran the old image and is excluded from acceptance evidence.
Screenshots are hosted on the separate
evidence/issue-937-ui-testsbranch so they do not add binary files or commits to this implementation PR.Fixes #937.
Rebased onto main at
66fca704. Conflict resolution preserves renewable workflow leases from main and credential-change rotation from this PR. Post-rebase validation: 206 focused tests passed across agent, MCP, reviewer, warm-host, proof-app, lease renewal, workflow execution, sandbox identity, and sandbox lifecycle coverage. Ruff lint, formatting, whitespace, and house-rules checks passed. One SQLAlchemy nested-transaction warning was emitted by a workflow-execution test.