diff --git a/services/hackbot-api/app/routers/runs.py b/services/hackbot-api/app/routers/runs.py index 661b2d859f..1167dd5a39 100644 --- a/services/hackbot-api/app/routers/runs.py +++ b/services/hackbot-api/app/routers/runs.py @@ -5,6 +5,7 @@ from typing import Annotated from fastapi import APIRouter, Depends, Header, HTTPException, Query, status +from hackbot_runtime.actions.phabricator import PATCH_ACTION_TYPES from pydantic import BeforeValidator from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -19,6 +20,7 @@ from app.jobs import ExecutionStatus from app.schemas import ( AgentDescriptor, + ArtifactRef, RunActionDoc, RunDoc, RunRef, @@ -30,6 +32,8 @@ router = APIRouter(dependencies=[Depends(require_api_key)]) +_PATCH_ARTIFACT = "changes/changes.patch" + def _normalize_identity(email: str | None) -> str | None: if not email: @@ -269,9 +273,29 @@ async def finalize_run(db: AsyncSession, run: Run) -> None: run.finalized_at = datetime.now(timezone.utc) await db.commit() + + if _has_unsubmitted_patch(summary, artifacts): + log.error( + "Agent run produced code changes without submitting a patch " + "(run_id=%s, agent=%s)", + run.run_id, + run.agent, + ) await pubsub.publish_run_completed(str(run.run_id), run.agent, run.status) +def _has_unsubmitted_patch( + summary: RunSummary | None, + artifacts: list[ArtifactRef], +) -> bool: + """Whether a run produced source changes without a patch action.""" + has_patch_artifact = any(artifact.name == _PATCH_ARTIFACT for artifact in artifacts) + has_patch_action = summary is not None and any( + action["type"] in PATCH_ACTION_TYPES for action in summary.actions + ) + return has_patch_artifact and not has_patch_action + + def _terminal_status( exec_status: ExecutionStatus, summary: RunSummary | None ) -> tuple[RunStatus, str | None]: diff --git a/services/hackbot-api/tests/test_finalize_run.py b/services/hackbot-api/tests/test_finalize_run.py index 03afef1ccb..cc0149b1ec 100644 --- a/services/hackbot-api/tests/test_finalize_run.py +++ b/services/hackbot-api/tests/test_finalize_run.py @@ -12,6 +12,7 @@ import pytest from app import gcs, jobs, pubsub from app.jobs import ExecutionStatus +from app.routers import runs as runs_module from app.routers.runs import finalize_run from app.schemas import ArtifactRef, RunStatus, RunSummary @@ -95,6 +96,35 @@ async def test_finalizes_succeeded_run(monkeypatch, _no_publish): assert _no_publish == [(str(run.run_id), run.agent, RunStatus.succeeded.value)] +@pytest.mark.parametrize( + ("actions", "artifacts", "expected"), + [ + ([], ["changes/changes.patch"], True), + ( + ["phabricator.submit_patch"], + ["changes/changes.patch"], + False, + ), + ( + ["phabricator.update_patch"], + ["changes/changes.patch"], + False, + ), + ([], [], False), + (None, ["changes/changes.patch"], True), + ], +) +def test_has_unsubmitted_patch(actions, artifacts, expected): + summary = ( + None + if actions is None + else RunSummary(status="ok", actions=[{"type": action} for action in actions]) + ) + artifact_refs = [ArtifactRef(name=artifact, size=10) for artifact in artifacts] + + assert runs_module._has_unsubmitted_patch(summary, artifact_refs) is expected + + async def test_finalizes_as_failed_when_summary_missing(monkeypatch): run = _FakeRun() db = _FakeDB()