From 8c8275a7d61c10b8f0269e2e57845ad71b44c188 Mon Sep 17 00:00:00 2001 From: Dan Serbanoiu <32792904+dserbano@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:05:07 -0400 Subject: [PATCH] Scope build GitHub reviewer credentials to evaluation --- backend/druks/agents.py | 2 +- .../contrib/software_factory/workflows.py | 23 +++++-- backend/druks/workflows.py | 9 ++- backend/druks/workspaces.py | 7 +- .../software_factory/test_build_workspace.py | 6 +- backend/tests/software_factory/test_review.py | 30 +++++++- backend/tests/test_agents.py | 13 ++++ backend/tests/test_mcp_servers.py | 2 +- backend/tests/test_warm_host_rotation.py | 68 +++++++++++++++++-- docs/configuration.md | 9 +-- docs/writing-an-app.md | 14 ++-- 11 files changed, 153 insertions(+), 30 deletions(-) diff --git a/backend/druks/agents.py b/backend/druks/agents.py index 069f5ccca..e85dc0de4 100644 --- a/backend/druks/agents.py +++ b/backend/druks/agents.py @@ -353,7 +353,7 @@ async def _run( mcp_servers, mcp_secret_refs = (), [] if self.include_mcp: mcp_servers, mcp_secret_refs = await workspace_class.get_all_mcp_servers( - session, subject, workflow.account_id + session, subject, workflow.account_id, agent=self.id ) if lent_browser: server, secret = lent_browser diff --git a/backend/druks/contrib/software_factory/workflows.py b/backend/druks/contrib/software_factory/workflows.py index 787e9a545..8bbcc7a67 100644 --- a/backend/druks/contrib/software_factory/workflows.py +++ b/backend/druks/contrib/software_factory/workflows.py @@ -66,13 +66,16 @@ async def get_secrets(cls, subject: Any) -> list[SandboxSecret]: ] @classmethod - async def get_mcp_servers(cls, subject: Any) -> tuple[SandboxMcpServer, ...]: - # GitHub MCP acts as the review actor. The clone acts as the operator. - actor = await get_review_actor() + async def get_mcp_servers( + cls, subject: Any, *, agent: str = "" + ) -> tuple[SandboxMcpServer, ...]: + service = cls.github + if agent == SoftwareFactory.evaluate_implementation.id: + service = (await get_review_actor()).service github = SandboxMcpServer( name=GITHUB_MCP_NAME, url=GITHUB_MCP_URL, - secret_id=(await actor.service.get()).id, + secret_id=(await service.get()).id, resources=await get_project_repos(cls.get_repo(subject)), ) if (await SoftwareFactory.settings()).tracker == "druks": @@ -321,6 +324,7 @@ async def _implement_phase(self) -> None: await self._create_branch() while True: await self.implement() + await self._check_review_access() evaluation = await SoftwareFactory.evaluate_implementation() if evaluation.verdict == EvaluationVerdict.FAIL and ( self.journal.implementation_revision < self._settings.max_implementation_revisions @@ -385,6 +389,13 @@ async def implement(self) -> ImplementationOutput: raise FatalError(f"implementation needs clarification: {delivery.summary}") return delivery + @step + async def _check_review_access(self) -> None: + # Drukbox fetches the evaluator's MCP entry on the agent's first call, not at + # creation, so a missing reviewer installation would reach only the agent. + client = await (await get_review_actor()).service.get_app_client() + await client.get_installation_id((await self.subject).repo) + @step async def _create_branch(self) -> None: # Replay keeps the branch: GitHub holds it, and a second pass would create the @@ -507,7 +518,9 @@ async def get_secrets(cls, subject: Any) -> list[SandboxSecret]: ] @classmethod - async def get_mcp_servers(cls, subject: Any) -> tuple[SandboxMcpServer, ...]: + async def get_mcp_servers( + cls, subject: Any, *, agent: str = "" + ) -> tuple[SandboxMcpServer, ...]: actor = await get_review_actor() return ( SandboxMcpServer( diff --git a/backend/druks/workflows.py b/backend/druks/workflows.py index 7e227a855..91679edc8 100644 --- a/backend/druks/workflows.py +++ b/backend/druks/workflows.py @@ -1007,6 +1007,9 @@ async def _lease_host( # only the host-id matters across steps — held-across-steps never fights replay. if not self.steps_reuse_sandbox: return + secrets_hash = hashlib.sha256( + json.dumps([config.secrets_hash, sorted(ref.key for ref in refs)]).encode() + ).hexdigest() # A crashed process left its box behind. Its identity finds it again. if not self._host and refs: identity = await SandboxIdentity.lookup( @@ -1018,8 +1021,8 @@ async def _lease_host( ) if identity: self._host = await sandbox_client.reattach(host_id=identity.host_id) - self._host_secrets_hash = config.secrets_hash - if self._host and self._host_secrets_hash != config.secrets_hash: + self._host_secrets_hash = secrets_hash + if self._host and self._host_secrets_hash != secrets_hash: # Drukbox binds entries at creation. await self._reap_run() if not self._host: @@ -1048,7 +1051,7 @@ async def _lease_host( identity=identity, lease_seconds=WORKFLOW_HOST_LEASE_SECONDS, ) - self._host_secrets_hash = config.secrets_hash + self._host_secrets_hash = secrets_hash return self._host.id async def _reap_run(self) -> None: diff --git a/backend/druks/workspaces.py b/backend/druks/workspaces.py index 95589a148..2d7194d88 100644 --- a/backend/druks/workspaces.py +++ b/backend/druks/workspaces.py @@ -62,7 +62,9 @@ def get_env(self) -> dict[str, str]: return {} @classmethod - async def get_mcp_servers(cls, subject: Any) -> tuple[SandboxMcpServer, ...]: + async def get_mcp_servers( + cls, subject: Any, *, agent: str = "" + ) -> tuple[SandboxMcpServer, ...]: # Override to declare this workspace's servers and the vault row each # one issues through. Read before the box exists. Base: none. return () @@ -214,6 +216,7 @@ async def get_all_mcp_servers( account_id: str | None, *, skip_unauthenticated: bool = False, + agent: str = "", ) -> tuple[tuple[McpServer, ...], list[SecretRef]]: """The MCP servers a box of this workspace reaches, as the harness names them, and the secret refs for the box's entries, one per bearer and per @@ -222,7 +225,7 @@ async def get_all_mcp_servers( server that cannot authenticate fails here, before the box, unless each account keys or connects it for itself or ``skip_unauthenticated`` leaves it out.""" - workspace_servers = await cls.get_mcp_servers(subject) + workspace_servers = await cls.get_mcp_servers(subject, agent=agent) workspace_names = {server.name for server in workspace_servers} if len(workspace_names) != len(workspace_servers): # One config key per name in the emitted harness config — a dupe diff --git a/backend/tests/software_factory/test_build_workspace.py b/backend/tests/software_factory/test_build_workspace.py index 0b3ca2a64..368685a0f 100644 --- a/backend/tests/software_factory/test_build_workspace.py +++ b/backend/tests/software_factory/test_build_workspace.py @@ -69,7 +69,7 @@ async def base_run_agent(self: Any, **kwargs: Any) -> str: assert execs == [["mkdir", "-p", get_related_root("exedev")], ["run_agent"]] -async def test_build_workspace_declares_its_github_mcp_as_the_review_actor(druks_db): +async def test_build_workspace_defaults_to_operator_mcp(druks_db): operator = await connect_service( "github", identity={"app_id": "1", "slug": "druks-operator"}, secrets={"private_key": "pem"} ) @@ -84,6 +84,10 @@ async def test_build_workspace_declares_its_github_mcp_as_the_review_actor(druks assert github.url == GITHUB_MCP_URL assert github.bearer_token_env_var == get_bearer_token_env_var(GITHUB_MCP_NAME) + assert ref.key == ("mcp_github_token", operator.id, ("o/main",), "api.githubcopilot.com") + _, [ref] = await BuildWorkspace.get_all_mcp_servers( + druks_db, subject, None, agent=SoftwareFactory.evaluate_implementation.id + ) assert ref.key == ("mcp_github_token", reviewer.id, ("o/main",), "api.githubcopilot.com") [clone] = await BuildWorkspace.get_secrets(subject) assert clone == SandboxSecret( diff --git a/backend/tests/software_factory/test_review.py b/backend/tests/software_factory/test_review.py index 81672362d..cc0a2f2cf 100644 --- a/backend/tests/software_factory/test_review.py +++ b/backend/tests/software_factory/test_review.py @@ -16,9 +16,11 @@ from druks.contrib.software_factory.github import get_review_actor from druks.contrib.software_factory.models import Project, ProjectRepo from druks.contrib.software_factory.services import GithubReviewer -from druks.contrib.software_factory.workflows import PullRequestReview -from druks.core.apis.github import GITHUB_AUTHORITY +from druks.contrib.software_factory.workflows import Build, PullRequestReview +from druks.core.apis.exceptions import GitHubAppNotInstalledError +from druks.core.apis.github import GITHUB_AUTHORITY, GitHubAppClient from druks.core.services import Github +from druks.durable.engine import configure_engine from druks.prompts import render_prompt from druks.secrets.datastructures import Audience from druks.secrets.models import VaultSecret @@ -278,3 +280,27 @@ async def _start(cls, **kwargs): assert run_id == "run-review" [start] = started assert (start["account_id"], start["note"]) == (dev.id, "Focus on the upgrade path.") + + +async def test_build_evaluation_needs_the_reviewer_on_the_repo(druks_db, monkeypatch): + await _connect_operator() + await _connect_reviewer() + configure_engine(await druks_db.connection()) + + async def run_step(_options, fn): + return await fn() + + async def get_installation_id(self, repo): + raise GitHubAppNotInstalledError(repo, app=self._slug) + + monkeypatch.setattr("druks.workflows.DBOS.run_step_async", run_step) + monkeypatch.setattr(GitHubAppClient, "get_installation_id", get_installation_id) + build = Build() + build.input = Build._run_input_model() + build.subject = SimpleNamespace(repo="acme/app") + + try: + with pytest.raises(GitHubAppNotInstalledError, match="druks-reviewer has no access"): + await build._check_review_access() + finally: + configure_engine(None) diff --git a/backend/tests/test_agents.py b/backend/tests/test_agents.py index 1ab243f89..5ad813da3 100644 --- a/backend/tests/test_agents.py +++ b/backend/tests/test_agents.py @@ -1340,3 +1340,16 @@ async def test_event_requires_an_artifact(druks_db, tmp_path, monkeypatch, curre await DUMMY_AGENT._run(db_session(), workflow_id="wf-9") assert (await AgentCall.list_for_run(db_session(), "wf-9"))[0].status == "failed" + + +async def test_workspace_mcp_selection_receives_agent_id( + druks_db, tmp_path, monkeypatch, current_run +): + sandbox = _patch_runtime(monkeypatch, tmp_path, {"ok": True}) + _patch_ephemeral(monkeypatch, sandbox) + servers = AsyncMock(return_value=()) + monkeypatch.setattr(Workspace, "get_mcp_servers", servers) + + await DUMMY_AGENT._run(db_session(), workflow_id="wf-9") + + servers.assert_awaited_once_with(None, agent="dummy") diff --git a/backend/tests/test_mcp_servers.py b/backend/tests/test_mcp_servers.py index 53586a21b..89d94aae8 100644 --- a/backend/tests/test_mcp_servers.py +++ b/backend/tests/test_mcp_servers.py @@ -64,7 +64,7 @@ def _declaring(*servers: SandboxMcpServer) -> type[Workspace]: # through, as SoftwareFactory does. class _Declaring(Workspace): @classmethod - async def get_mcp_servers(cls, subject) -> tuple[SandboxMcpServer, ...]: + async def get_mcp_servers(cls, subject, *, agent="") -> tuple[SandboxMcpServer, ...]: return servers return _Declaring diff --git a/backend/tests/test_warm_host_rotation.py b/backend/tests/test_warm_host_rotation.py index b9c7fad4e..79a1abadb 100644 --- a/backend/tests/test_warm_host_rotation.py +++ b/backend/tests/test_warm_host_rotation.py @@ -4,11 +4,16 @@ import druks.workflows as sdk import pytest +from conftest import connect_provider, connect_service from drukbox_sdk import Secret from druks.accounts.models import Account from druks.db import db_session +from druks.harnesses.providers import AnthropicProvider from druks.sandbox.constants import WORKFLOW_HOST_LEASE_SECONDS +from druks.sandbox.models import SandboxIdentity, SecretRef +from druks.testing import seed_run from druks.workflows import Workflow +from druks_field_notes.workflows import Summarize def _config(secrets: dict[str, Secret], secrets_hash: str = "") -> SimpleNamespace: @@ -53,10 +58,11 @@ async def provision( ) -> _FakeSandbox: assert lease_seconds == WORKFLOW_HOST_LEASE_SECONDS assert template is None - assert identity is None self.provisions.append(idempotency_key) self.secrets.append(secrets) host_id = f"host-{len(self.provisions)}" + if identity: + await identity.bind(host_id) return _FakeSandbox(id=host_id, expires_at=datetime.now(UTC) + self.lease) async def release(self, *, host_id: str) -> None: @@ -178,12 +184,6 @@ async def test_no_warm_host_when_reuse_disabled(monkeypatch): async def test_a_replay_finds_the_warm_box_through_its_identity( monkeypatch: pytest.MonkeyPatch, druks_db ) -> None: - from conftest import connect_provider - from druks.harnesses.providers import AnthropicProvider - from druks.sandbox.models import SandboxIdentity, SecretRef - from druks.testing import seed_run - from druks_field_notes.workflows import Summarize - await seed_run(db_session(), kind=Summarize.kind, run_id="wf-1") subscription = await connect_provider( AnthropicProvider, {"claudeAiOauth": {"accessToken": "test-token"}} @@ -208,3 +208,57 @@ async def test_a_replay_finds_the_warm_box_through_its_identity( assert client.reattached == ["host-crashed"] assert client.provisions == [] + + +async def test_warm_box_switches_mcp_identity_and_switches_back(monkeypatch, druks_db): + await seed_run(db_session(), kind=Summarize.kind, run_id="wf-1") + operator = await connect_service( + "github", + identity={"app_id": "1", "slug": "operator"}, + secrets={"private_key": "operator-pem"}, + ) + reviewer = await connect_service( + "github_reviewer", + identity={"app_id": "2", "slug": "reviewer"}, + secrets={"private_key": "reviewer-pem"}, + ) + operator_refs = [ + SecretRef( + name="mcp_github_token", + secret_id=operator.id, + resources=["acme/app"], + host="api.githubcopilot.com", + ) + ] + reviewer_refs = [ + SecretRef( + name="mcp_github_token", + secret_id=reviewer.id, + resources=["acme/app"], + host="api.githubcopilot.com", + ) + ] + fake = _FakeSandboxClient() + monkeypatch.setattr(sdk, "sandbox_client", fake) + flow = _warm_workflow() + flow.account_id = (await Account.get_for_run(db_session(), None)).id + + assert await flow._lease_host(db_session(), _NONE, operator_refs) == "host-1" + assert await flow._lease_host(db_session(), _NONE, operator_refs) == "host-1" + assert await flow._lease_host(db_session(), _NONE, reviewer_refs) == "host-2" + resumed = _warm_workflow() + resumed.account_id = flow.account_id + assert await resumed._lease_host(db_session(), _NONE, reviewer_refs) == "host-2" + assert await resumed._lease_host(db_session(), _NONE, operator_refs) == "host-3" + + assert fake.released == ["host-1", "host-2"] + assert fake.reattached == ["host-2"] + assert len(set(fake.provisions)) == 3 + identity = await SandboxIdentity.lookup( + db_session(), + account_id=flow.account_id, + run_id="wf-1", + scoped_to="workflow", + secret_refs=operator_refs, + ) + assert identity.host_id == "host-3" diff --git a/docs/configuration.md b/docs/configuration.md index fbe51d163..9e6454f8b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -488,9 +488,9 @@ sandbox's repo, with the expiry GitHub gives it, and fetches a new one before it expires. The token reaches that repo and the project's other repos that the same installation covers, and no other repo. The `software_factory` build clones and pushes as the operator App. A review clones as the reviewer App when -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. +one is connected. Builds and reviews also get the GitHub MCP server for +`api.githubcopilot.com`. A review and a build's evaluation use it as the review +identity. The other build steps use it as the operator App. **To upgrade an existing installation**, paste the credentials one time on each active host. Open **Settings → Connections → Services**. Connect GitHub with the @@ -532,7 +532,8 @@ Connect a second App to let reviews approve pull requests and request changes. Open **Settings → Connections → Services → Github Reviewer** and select **Create App**. The flow is the same as for the operator App. Install the reviewer App on every repository that has the operator App, because builds also -post their reviews through it. +post their reviews through it. Without that installation, a build's evaluation +fails, and the error names the App and the repository. To register the reviewer App manually: diff --git a/docs/writing-an-app.md b/docs/writing-an-app.md index 30cc3dd00..1dd5e8e08 100644 --- a/docs/writing-an-app.md +++ b/docs/writing-an-app.md @@ -857,7 +857,7 @@ A vault row with one secret issues it under any name. For the secret of a service that does not depend on the subject, [list it on the agent](#give-an-agent-a-service-secret) instead. -Override `get_mcp_servers(subject)` to give the sandbox an MCP server with its +Override `get_mcp_servers(subject, *, agent="")` to give the sandbox an MCP server with its own vault row: ```python @@ -866,18 +866,24 @@ from druks.sandbox import SandboxMcpServer class BuildWorkspace(RepoWorkspace): @classmethod - async def get_mcp_servers(cls, subject) -> tuple[SandboxMcpServer, ...]: - actor = await get_review_actor() + async def get_mcp_servers(cls, subject, *, agent="") -> tuple[SandboxMcpServer, ...]: + service = Github + if agent == SoftwareFactory.evaluate_implementation.id: + service = (await get_review_actor()).service return ( SandboxMcpServer( name="github", url="https://api.githubcopilot.com/mcp/", - secret_id=(await actor.service.get()).id, + secret_id=(await service.get()).id, resources=(cls.get_repo(subject),), ), ) ``` +The `agent` keyword is the current agent ID, or an empty string outside an agent +call. Overrides must accept it. Use it to select a credential for a particular +agent. Warm sandboxes rotate when their secret references change. + The server names the vault row the issuer answers from and what the token covers: here a connected GitHub service and its repo. Druks binds the server's host and the variable `MCP_GITHUB_TOKEN` to the entry when it creates the