diff --git a/panel/src/components/dashboard/__tests__/release-proposal-card.test.tsx b/panel/src/components/dashboard/__tests__/release-proposal-card.test.tsx index ddb7c9bc..58524ca7 100644 --- a/panel/src/components/dashboard/__tests__/release-proposal-card.test.tsx +++ b/panel/src/components/dashboard/__tests__/release-proposal-card.test.tsx @@ -124,3 +124,53 @@ describe("ReleaseProposalCard — query-failure surfacing (F082)", () => { ).toBeInTheDocument(); }); }); + +describe("ReleaseProposalCard — execute outcome surfacing (W8b)", () => { + it("renders the in-flight badge and disables actions while execute runs", () => { + // execute_in_flight is the UX for the Redis-mutex-protected background + // execute; the approve/reject buttons disable so the CEO can't double-click. + mockUseQuery.mockReturnValue({ + data: { ...buildProposal(), execute_in_flight: true }, + isLoading: false, + isError: false, + error: null, + refetch: vi.fn(), + }); + + render(withPageRefresh()); + + expect( + screen.getByText(/release execute running in the background/i), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /Reject with changes/i }), + ).toBeDisabled(); + // Approve stays present but disabled while the execute runs. + const approve = screen.getByRole("button", { name: /Approve & publish/i }); + expect(approve).toBeDisabled(); + }); + + it("renders the failure block and a Retry label from a persisted execute_status", () => { + // A failed ~40min execute left the proposal open with a persisted + // execute_status; the card surfaces the reason and flips Approve to Retry. + mockUseQuery.mockReturnValue({ + data: { + ...buildProposal(), + execute_status: "gate_failed", + execute_detail: "make quality failed", + }, + isLoading: false, + isError: false, + error: null, + refetch: vi.fn(), + }); + + render(withPageRefresh()); + + expect(screen.getByText(/last execute failed/i)).toBeInTheDocument(); + expect(screen.getByText(/make quality failed/i)).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /Retry approve & publish/i }), + ).toBeInTheDocument(); + }); +}); diff --git a/panel/src/components/dashboard/release-proposal-card.tsx b/panel/src/components/dashboard/release-proposal-card.tsx index 77297d05..c4823849 100644 --- a/panel/src/components/dashboard/release-proposal-card.tsx +++ b/panel/src/components/dashboard/release-proposal-card.tsx @@ -23,7 +23,7 @@ import { } from "@/components/ui/dialog"; import { Textarea } from "@/components/ui/textarea"; import { Label } from "@/components/ui/label"; -import { CheckCircle2, XCircle, Rocket, AlertTriangle } from "lucide-react"; +import { CheckCircle2, XCircle, Rocket, AlertTriangle, Loader2 } from "lucide-react"; import { toast } from "sonner"; import { usePageRefresh } from "@/hooks"; import { HelpTip } from "@/components/ui/help-tip"; @@ -101,7 +101,7 @@ export function ReleaseProposalCard({ className }: { className?: string }) { mutationFn: (changes: string) => releaseApi.reject(changes), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["release", "proposal"] }); - toast.success("Proposal sent back with required changes"); + toast.success("Proposal rejected — a fresh assessment runs next cycle"); closeDialog(); }, onError: (error) => { @@ -158,6 +158,12 @@ export function ReleaseProposalCard({ className }: { className?: string }) { const { report } = proposal; const pending = approveMutation.isPending || rejectMutation.isPending; + // The ~40min execute runs in the background; the Redis mutex already refuses + // a double-click server-side — execute_in_flight is the UX (disable approve, + // show a running badge). A persisted execute_status on a still-open proposal + // is a failure (a publish would have completed + hidden the card). + const executeInFlight = !!proposal.execute_in_flight; + const executeFailed = !!proposal.execute_status && !executeInFlight; return ( <> @@ -238,12 +244,40 @@ export function ReleaseProposalCard({ className }: { className?: string }) {

)} + {executeInFlight && ( +
+ + + Release execute running in the background (~40 min) — this card + updates when it finishes. + +
+ )} + + {executeFailed && ( +
+

+ + Last execute failed ({proposal.execute_status}) +

+ {proposal.execute_detail && ( +

+ {proposal.execute_detail} +

+ )} +

+ Fix the cause and approve again to retry. +

+
+ )} +
@@ -271,7 +306,7 @@ export function ReleaseProposalCard({ className }: { className?: string }) { {action === "approve" ? "This runs the fail-closed executor: write the bumps + CHANGELOG, run make quality, commit, wait for green CI, then publish. It aborts on a red gate or red CI." - : "Record what must change. The proposal stays open for revision; nothing is published."} + : "Record what must change. The proposal is cancelled and the release manager re-assesses next cycle; nothing is published."} diff --git a/panel/src/lib/api/release.ts b/panel/src/lib/api/release.ts index 3aa41f36..a963c5e5 100644 --- a/panel/src/lib/api/release.ts +++ b/panel/src/lib/api/release.ts @@ -28,6 +28,9 @@ export interface ReleaseProposal { title: string; status: string; required_changes?: string | null; + execute_status?: string | null; + execute_detail?: string | null; + execute_in_flight?: boolean; report: ReleaseReport; } diff --git a/roboco/api/routes/release.py b/roboco/api/routes/release.py index e3c3bb85..c3e93373 100644 --- a/roboco/api/routes/release.py +++ b/roboco/api/routes/release.py @@ -2,10 +2,12 @@ CEO-only. ``GET /proposal`` renders the held proposal + its readiness report; ``approve`` runs the fail-closed executor; ``reject`` records required changes and -keeps the proposal held. Nothing here publishes without the CEO's explicit POST. +cancels the proposal (freeing the one-open dedup for a fresh re-assessment). +Nothing here publishes without the CEO's explicit POST. """ from typing import TYPE_CHECKING, cast +from uuid import UUID from fastapi import APIRouter, HTTPException, status from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker @@ -23,11 +25,10 @@ from roboco.security import guard_deco from roboco.services.release_proposal import ( dispatch_approve, get_release_proposal_service, + is_approve_in_flight, ) if TYPE_CHECKING: - from uuid import UUID - from roboco.db.tables import TaskTable router = APIRouter() @@ -44,11 +45,15 @@ def _status_value(task: "TaskTable") -> str: def _to_response(task: "TaskTable") -> ReleaseProposalResponse: report = markers.get_release_report(task) or {} + outcome = markers.get_release_execute_outcome(task) return ReleaseProposalResponse( task_id=str(task.id), title=task.title, status=_status_value(task), required_changes=markers.get_release_required_changes(task), + execute_status=outcome[0] if outcome else None, + execute_detail=outcome[1] if outcome else None, + execute_in_flight=is_approve_in_flight(UUID(str(task.id))), report=ReleaseReportModel( proposed_version=report.get("proposed_version", ""), bump_kind=report.get("bump_kind", ""), @@ -134,7 +139,8 @@ async def approve_release_proposal( async def reject_release_proposal( data: ReleaseRejectRequest, db: DbSession, agent: CurrentAgentContext ) -> ReleaseProposalResponse: - """Reject the held proposal with required changes; it stays held for revision.""" + """Reject the held proposal with required changes; it is cancelled so the + release manager re-assesses and may originate a fresh proposal next cycle.""" _require_ceo(agent) svc = get_release_proposal_service(db) task = await svc.open_proposal() diff --git a/roboco/api/schemas/release.py b/roboco/api/schemas/release.py index 5b501249..648a3b26 100644 --- a/roboco/api/schemas/release.py +++ b/roboco/api/schemas/release.py @@ -32,6 +32,9 @@ class ReleaseProposalResponse(BaseModel): title: str status: str required_changes: str | None = None + execute_status: str | None = None + execute_detail: str | None = None + execute_in_flight: bool = False report: ReleaseReportModel diff --git a/roboco/foundation/policy/content/markers.py b/roboco/foundation/policy/content/markers.py index d0343af7..8ca0cf8e 100644 --- a/roboco/foundation/policy/content/markers.py +++ b/roboco/foundation/policy/content/markers.py @@ -34,6 +34,8 @@ ESCALATION = "escalation" APPROVE_AND_START_NOTES = "approve_and_start_notes" RELEASE_REPORT = "release_report" RELEASE_REQUIRED_CHANGES = "release_required_changes" +RELEASE_EXECUTE_STATUS = "release_execute_status" +RELEASE_EXECUTE_DETAIL = "release_execute_detail" X_DRAFT_BODY = "x_draft_body" X_RELEASE_VERSION = "x_release_version" X_MENTION_REF = "x_mention_ref" @@ -143,6 +145,24 @@ def set_release_required_changes(task: HasMarkers, text: str) -> None: set_marker(task, RELEASE_REQUIRED_CHANGES, text) +def get_release_execute_outcome(task: HasMarkers) -> tuple[str, str] | None: + """The last execute outcome ``(status, detail)`` — e.g. ``("gate_failed", + "...")`` — or None when the proposal has never been approved. Surfaced to + the CEO via ``GET /proposal`` so a failed ~40min execute isn't a silent + PENDING.""" + status = get_marker(task, RELEASE_EXECUTE_STATUS) + if not status: + return None + detail = get_marker(task, RELEASE_EXECUTE_DETAIL) + return str(status), str(detail) if detail else "" + + +def set_release_execute_outcome(task: HasMarkers, status: str, detail: str) -> None: + """Record the outcome of the latest approve execute on the proposal.""" + set_marker(task, RELEASE_EXECUTE_STATUS, status) + set_marker(task, RELEASE_EXECUTE_DETAIL, detail) + + # --- X (Twitter) held post/reply ------------------------------------------- # A held x_post / x_reply proposal (never dispatched — CEO approve/reject # only) carries its draft body, and a reply additionally carries the mention diff --git a/roboco/services/release_proposal.py b/roboco/services/release_proposal.py index b8db3009..7750323f 100644 --- a/roboco/services/release_proposal.py +++ b/roboco/services/release_proposal.py @@ -348,11 +348,18 @@ class ReleaseProposalService(BaseService): await asyncio.sleep(_RELEASE_LOCK_HEARTBEAT_SECONDS) async def reject(self, task_id: UUID, required_changes: str) -> TaskTable | None: - """Record the CEO's required changes; keep the proposal held for revision.""" + """Record the CEO's required changes and cancel the proposal. + + Cancelling (not holding) is what frees the one-open-proposal dedup — + ``list_open_release_proposals`` excludes CANCELLED, so the next + ``run_cycle`` re-assesses and may originate a fresh proposal. The + ``required_changes`` marker stays on the cancelled row for history. + Mirrors the video-post reject (``video_post_service.py``).""" task = await get_task_service(self.session).get(task_id) if task is None or task.source != RELEASE_MANAGER_SOURCE: return None markers.set_release_required_changes(task, required_changes) + task.status = TaskStatus.CANCELLED await self.session.flush() return task @@ -410,7 +417,13 @@ async def _run_approve_background( ) -> None: """Run ``approve`` in a background task with a fresh session (the request session closes when the 202 returns). Commits the outcome; a failure logs - and rolls back — the proposal stays open for the CEO to retry.""" + and rolls back — the proposal stays open for the CEO to retry. + + The execute outcome (status + detail) is persisted as a marker on the task + so a failed ~40min execute isn't a silent PENDING — ``GET /proposal`` + surfaces it. ``already_in_progress`` is transient (a concurrent click) and + is NOT persisted, so it can't clobber the real running execute's eventual + outcome.""" async with session_factory() as bg_db: try: result = await get_release_proposal_service(bg_db).approve(task_id) @@ -419,12 +432,32 @@ async def _run_approve_background( task_id, result.status if result is not None else "no_report", ) + if result is not None and result.status != "already_in_progress": + task = await get_task_service(bg_db).get(task_id) + if task is not None: + markers.set_release_execute_outcome( + task, result.status, result.detail + ) await bg_db.commit() - except Exception: + except Exception as exc: logger.exception( "release approve background task failed task_id=%s", task_id ) await bg_db.rollback() + # Re-fetch post-rollback and record the crash so the CEO sees a + # reason instead of a silent PENDING. + task = await get_task_service(bg_db).get(task_id) + if task is not None: + markers.set_release_execute_outcome(task, "error", str(exc)[:500]) + await bg_db.commit() + + +def is_approve_in_flight(task_id: UUID) -> bool: + """True iff a background release execute is currently running for this proposal. + + Single-process (one orchestrator) by construction; the durable cross-restart + signal is the execute-outcome marker, this is the live progress nicety.""" + return task_id in _INFLIGHT_APPROVES def dispatch_approve( diff --git a/tests/integration/test_release_routes.py b/tests/integration/test_release_routes.py index d61ad704..b7866706 100644 --- a/tests/integration/test_release_routes.py +++ b/tests/integration/test_release_routes.py @@ -2,6 +2,8 @@ from __future__ import annotations +import asyncio +import contextlib from http import HTTPStatus from typing import TYPE_CHECKING from unittest.mock import AsyncMock, patch @@ -9,23 +11,24 @@ from uuid import UUID, uuid4 import pytest import pytest_asyncio +import roboco.services.release_proposal as rp from fastapi import FastAPI from httpx import ASGITransport, AsyncClient from roboco.api.deps import get_agent_context, get_db from roboco.api.routes import release as release_route from roboco.api.routes.release import router as release_router from roboco.db.tables import AgentTable, ProjectTable, TaskTable +from roboco.foundation.policy.content import markers from roboco.models import AgentRole, AgentStatus, Team from roboco.models.base import TaskNature, TaskStatus, TaskType from roboco.models.permissions import AgentContext from roboco.services.release_executor import ReleaseResult from roboco.services.release_proposal import ReleaseProposalService from roboco.services.release_readiness import ReleaseReadinessReport, report_to_dict -from roboco.services.task import RELEASE_MANAGER_SOURCE +from roboco.services.task import RELEASE_MANAGER_SOURCE, TaskService from sqlalchemy import delete if TYPE_CHECKING: - import asyncio from collections.abc import AsyncIterator from typing import Any @@ -269,12 +272,101 @@ async def test_approve_gate_failure_keeps_proposal_open_async( await captured["task"] await db_session.refresh(task) assert task.status == TaskStatus.PENDING # still held for retry + # The failure reason is persisted as a marker + surfaced via GET /proposal + # so a failed ~40min execute isn't a silent PENDING. + outcome = markers.get_release_execute_outcome(task) + assert outcome is not None + assert outcome[0] == "gate_failed" + assert "make quality failed" in outcome[1] + poll = await ceo_client.get("/api/release/proposal") + assert poll.status_code == HTTPStatus.OK + body = poll.json() + assert body["execute_status"] == "gate_failed" + assert "make quality failed" in (body["execute_detail"] or "") @pytest.mark.asyncio -async def test_reject_records_changes_and_keeps_open( +async def test_approve_exception_records_error_marker( db_session: AsyncSession, ceo_client: AsyncClient ) -> None: + """An unexpected crash in the background execute (not a structured + ReleaseResult failure) is recorded as an ``error`` marker so the CEO sees a + reason instead of a silent PENDING.""" + task = await _seed_proposal(db_session) + fake_executor = AsyncMock() + fake_executor.execute = AsyncMock(side_effect=RuntimeError("boom")) + captured: dict[str, asyncio.Task[None]] = {} + real_dispatch = release_route.dispatch_approve + + def _capturing_dispatch(task_id: UUID, factory: Any) -> asyncio.Task[None]: + bg = real_dispatch(task_id, factory) + captured["task"] = bg + return bg + + with ( + patch( + "roboco.services.release_proposal.get_release_executor", + AsyncMock(return_value=fake_executor), + ), + patch( + "roboco.api.routes.release.dispatch_approve", + side_effect=_capturing_dispatch, + ), + patch.object( + ReleaseProposalService, "_acquire_release_lock", AsyncMock(return_value="t") + ), + patch.object( + ReleaseProposalService, + "_release_release_lock", + AsyncMock(return_value=None), + ), + patch.object( + ReleaseProposalService, + "_heartbeat_release_lock", + AsyncMock(return_value=True), + ), + ): + await ceo_client.post("/api/release/proposal/approve") + await captured["task"] + await db_session.refresh(task) + assert task.status == TaskStatus.PENDING # still held for retry + outcome = markers.get_release_execute_outcome(task) + assert outcome is not None + assert outcome[0] == "error" + assert "boom" in outcome[1] + + +@pytest.mark.asyncio +async def test_get_proposal_surfaces_in_flight( + db_session: AsyncSession, ceo_client: AsyncClient +) -> None: + """execute_in_flight is derived from the in-memory _INFLIGHT_APPROVES + registry — True while a background execute is registered.""" + await _seed_proposal(db_session) + resp = await ceo_client.get("/api/release/proposal") + tid = UUID(resp.json()["task_id"]) + # Register a real pending task under the proposal id, as dispatch_approve does. + sentinel = asyncio.create_task(asyncio.sleep(3600)) + rp._INFLIGHT_APPROVES[tid] = sentinel + try: + in_flight_resp = await ceo_client.get("/api/release/proposal") + assert in_flight_resp.json()["execute_in_flight"] is True + finally: + rp._INFLIGHT_APPROVES.pop(tid, None) + sentinel.cancel() + with contextlib.suppress(asyncio.CancelledError): + await sentinel + idle_resp = await ceo_client.get("/api/release/proposal") + assert idle_resp.json()["execute_in_flight"] is False + + +@pytest.mark.asyncio +async def test_reject_records_changes_and_cancels_frees_dedup( + db_session: AsyncSession, ceo_client: AsyncClient +) -> None: + """Reject cancels the proposal (not holds it) so the one-open-proposal dedup + frees and the release manager can re-assess next cycle. The required-changes + marker stays on the cancelled row for history.""" task = await _seed_proposal(db_session) resp = await ceo_client.post( "/api/release/proposal/reject", @@ -284,7 +376,10 @@ async def test_reject_records_changes_and_keeps_open( assert "Tighten the CHANGELOG" in (resp.json()["required_changes"] or "") refreshed = await db_session.get(TaskTable, task.id) assert refreshed is not None - assert refreshed.status == TaskStatus.PENDING # stays held for revision + assert refreshed.status == TaskStatus.CANCELLED # cancelled, not held + # The dedup no longer counts it as open — a fresh proposal can originate. + open_proposals = await TaskService(db_session).list_open_release_proposals() + assert task.id not in {t.id for t in open_proposals} @pytest.mark.asyncio