[w8b] Fix release-proposal flow: reject frees dedup, surface execute outcome (#525)

Reject cancelled the proposal's status but never moved it out of the
held-proposal set, so the one-open-proposal dedup blocked the release
manager from ever re-assessing — a rejected proposal deadlocked the cycle.
reject() now sets CANCELLED (mirroring video_post_service), which
list_open_release_proposals already excludes, so a fresh proposal can
originate next cycle.

A failed ~40min background execute (gate red, CI red, or an unexpected
crash) left the proposal silently PENDING with no signal to the CEO.
_run_approve_background now writes a release_execute_outcome marker
(status + detail) on every terminal outcome, and an 'error' marker on an
unhandled exception. GET /proposal surfaces execute_status / execute_detail
/ execute_in_flight (derived from the in-memory _INFLIGHT_APPROVES registry)
so the panel can show a running badge, a failure block with the reason, and
a Retry-approve label instead of a silent wait.

Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
2026-07-15 06:10:31 +02:00
committed by GitHub
co-authored by Renn F
parent bb3b4b0c6d
commit be553ee9dd
8 changed files with 260 additions and 15 deletions
@@ -124,3 +124,53 @@ describe("ReleaseProposalCard — query-failure surfacing (F082)", () => {
).toBeInTheDocument(); ).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(<ReleaseProposalCard />));
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(<ReleaseProposalCard />));
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();
});
});
@@ -23,7 +23,7 @@ import {
} from "@/components/ui/dialog"; } from "@/components/ui/dialog";
import { Textarea } from "@/components/ui/textarea"; import { Textarea } from "@/components/ui/textarea";
import { Label } from "@/components/ui/label"; 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 { toast } from "sonner";
import { usePageRefresh } from "@/hooks"; import { usePageRefresh } from "@/hooks";
import { HelpTip } from "@/components/ui/help-tip"; import { HelpTip } from "@/components/ui/help-tip";
@@ -101,7 +101,7 @@ export function ReleaseProposalCard({ className }: { className?: string }) {
mutationFn: (changes: string) => releaseApi.reject(changes), mutationFn: (changes: string) => releaseApi.reject(changes),
onSuccess: () => { onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["release", "proposal"] }); queryClient.invalidateQueries({ queryKey: ["release", "proposal"] });
toast.success("Proposal sent back with required changes"); toast.success("Proposal rejected — a fresh assessment runs next cycle");
closeDialog(); closeDialog();
}, },
onError: (error) => { onError: (error) => {
@@ -158,6 +158,12 @@ export function ReleaseProposalCard({ className }: { className?: string }) {
const { report } = proposal; const { report } = proposal;
const pending = approveMutation.isPending || rejectMutation.isPending; 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 ( return (
<> <>
@@ -238,12 +244,40 @@ export function ReleaseProposalCard({ className }: { className?: string }) {
</p> </p>
)} )}
{executeInFlight && (
<div className="flex items-center gap-2 rounded-md border border-blue-500/40 bg-blue-500/10 p-3">
<Loader2 className="h-4 w-4 animate-spin text-blue-500" />
<span className="text-sm text-blue-600 dark:text-blue-400">
Release execute running in the background (~40 min) this card
updates when it finishes.
</span>
</div>
)}
{executeFailed && (
<div className="rounded-md border border-red-500/40 bg-red-500/10 p-3">
<p className="flex items-center gap-1.5 text-sm font-medium text-red-600">
<XCircle className="h-4 w-4" />
Last execute failed ({proposal.execute_status})
</p>
{proposal.execute_detail && (
<p className="mt-1 text-sm text-muted-foreground">
{proposal.execute_detail}
</p>
)}
<p className="mt-1 text-xs text-muted-foreground">
Fix the cause and approve again to retry.
</p>
</div>
)}
<div className="flex flex-col-reverse gap-2 pt-1 sm:flex-row sm:items-center sm:justify-end"> <div className="flex flex-col-reverse gap-2 pt-1 sm:flex-row sm:items-center sm:justify-end">
<Button <Button
variant="outline" variant="outline"
size="sm" size="sm"
className="text-destructive hover:text-destructive" className="text-destructive hover:text-destructive"
onClick={() => setAction("reject")} onClick={() => setAction("reject")}
disabled={executeInFlight}
> >
<XCircle className="mr-1 h-4 w-4" /> <XCircle className="mr-1 h-4 w-4" />
Reject with changes Reject with changes
@@ -252,9 +286,10 @@ export function ReleaseProposalCard({ className }: { className?: string }) {
size="sm" size="sm"
className="bg-green-600 hover:bg-green-700" className="bg-green-600 hover:bg-green-700"
onClick={() => setAction("approve")} onClick={() => setAction("approve")}
disabled={executeInFlight}
> >
<CheckCircle2 className="mr-1 h-4 w-4" /> <CheckCircle2 className="mr-1 h-4 w-4" />
Approve &amp; publish {executeFailed ? "Retry approve & publish" : "Approve & publish"}
</Button> </Button>
</div> </div>
</CardContent> </CardContent>
@@ -271,7 +306,7 @@ export function ReleaseProposalCard({ className }: { className?: string }) {
<DialogDescription> <DialogDescription>
{action === "approve" {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." ? "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."}
</DialogDescription> </DialogDescription>
</DialogHeader> </DialogHeader>
+3
View File
@@ -28,6 +28,9 @@ export interface ReleaseProposal {
title: string; title: string;
status: string; status: string;
required_changes?: string | null; required_changes?: string | null;
execute_status?: string | null;
execute_detail?: string | null;
execute_in_flight?: boolean;
report: ReleaseReport; report: ReleaseReport;
} }
+10 -4
View File
@@ -2,10 +2,12 @@
CEO-only. ``GET /proposal`` renders the held proposal + its readiness report; CEO-only. ``GET /proposal`` renders the held proposal + its readiness report;
``approve`` runs the fail-closed executor; ``reject`` records required changes and ``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 typing import TYPE_CHECKING, cast
from uuid import UUID
from fastapi import APIRouter, HTTPException, status from fastapi import APIRouter, HTTPException, status
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker
@@ -23,11 +25,10 @@ from roboco.security import guard_deco
from roboco.services.release_proposal import ( from roboco.services.release_proposal import (
dispatch_approve, dispatch_approve,
get_release_proposal_service, get_release_proposal_service,
is_approve_in_flight,
) )
if TYPE_CHECKING: if TYPE_CHECKING:
from uuid import UUID
from roboco.db.tables import TaskTable from roboco.db.tables import TaskTable
router = APIRouter() router = APIRouter()
@@ -44,11 +45,15 @@ def _status_value(task: "TaskTable") -> str:
def _to_response(task: "TaskTable") -> ReleaseProposalResponse: def _to_response(task: "TaskTable") -> ReleaseProposalResponse:
report = markers.get_release_report(task) or {} report = markers.get_release_report(task) or {}
outcome = markers.get_release_execute_outcome(task)
return ReleaseProposalResponse( return ReleaseProposalResponse(
task_id=str(task.id), task_id=str(task.id),
title=task.title, title=task.title,
status=_status_value(task), status=_status_value(task),
required_changes=markers.get_release_required_changes(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( report=ReleaseReportModel(
proposed_version=report.get("proposed_version", ""), proposed_version=report.get("proposed_version", ""),
bump_kind=report.get("bump_kind", ""), bump_kind=report.get("bump_kind", ""),
@@ -134,7 +139,8 @@ async def approve_release_proposal(
async def reject_release_proposal( async def reject_release_proposal(
data: ReleaseRejectRequest, db: DbSession, agent: CurrentAgentContext data: ReleaseRejectRequest, db: DbSession, agent: CurrentAgentContext
) -> ReleaseProposalResponse: ) -> 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) _require_ceo(agent)
svc = get_release_proposal_service(db) svc = get_release_proposal_service(db)
task = await svc.open_proposal() task = await svc.open_proposal()
+3
View File
@@ -32,6 +32,9 @@ class ReleaseProposalResponse(BaseModel):
title: str title: str
status: str status: str
required_changes: str | None = None required_changes: str | None = None
execute_status: str | None = None
execute_detail: str | None = None
execute_in_flight: bool = False
report: ReleaseReportModel report: ReleaseReportModel
@@ -34,6 +34,8 @@ ESCALATION = "escalation"
APPROVE_AND_START_NOTES = "approve_and_start_notes" APPROVE_AND_START_NOTES = "approve_and_start_notes"
RELEASE_REPORT = "release_report" RELEASE_REPORT = "release_report"
RELEASE_REQUIRED_CHANGES = "release_required_changes" 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_DRAFT_BODY = "x_draft_body"
X_RELEASE_VERSION = "x_release_version" X_RELEASE_VERSION = "x_release_version"
X_MENTION_REF = "x_mention_ref" 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) 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 ------------------------------------------- # --- X (Twitter) held post/reply -------------------------------------------
# A held x_post / x_reply proposal (never dispatched — CEO approve/reject # A held x_post / x_reply proposal (never dispatched — CEO approve/reject
# only) carries its draft body, and a reply additionally carries the mention # only) carries its draft body, and a reply additionally carries the mention
+36 -3
View File
@@ -348,11 +348,18 @@ class ReleaseProposalService(BaseService):
await asyncio.sleep(_RELEASE_LOCK_HEARTBEAT_SECONDS) await asyncio.sleep(_RELEASE_LOCK_HEARTBEAT_SECONDS)
async def reject(self, task_id: UUID, required_changes: str) -> TaskTable | None: 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) task = await get_task_service(self.session).get(task_id)
if task is None or task.source != RELEASE_MANAGER_SOURCE: if task is None or task.source != RELEASE_MANAGER_SOURCE:
return None return None
markers.set_release_required_changes(task, required_changes) markers.set_release_required_changes(task, required_changes)
task.status = TaskStatus.CANCELLED
await self.session.flush() await self.session.flush()
return task return task
@@ -410,7 +417,13 @@ async def _run_approve_background(
) -> None: ) -> None:
"""Run ``approve`` in a background task with a fresh session (the request """Run ``approve`` in a background task with a fresh session (the request
session closes when the 202 returns). Commits the outcome; a failure logs 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: async with session_factory() as bg_db:
try: try:
result = await get_release_proposal_service(bg_db).approve(task_id) result = await get_release_proposal_service(bg_db).approve(task_id)
@@ -419,12 +432,32 @@ async def _run_approve_background(
task_id, task_id,
result.status if result is not None else "no_report", 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() await bg_db.commit()
except Exception: except Exception as exc:
logger.exception( logger.exception(
"release approve background task failed task_id=%s", task_id "release approve background task failed task_id=%s", task_id
) )
await bg_db.rollback() 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( def dispatch_approve(
+99 -4
View File
@@ -2,6 +2,8 @@
from __future__ import annotations from __future__ import annotations
import asyncio
import contextlib
from http import HTTPStatus from http import HTTPStatus
from typing import TYPE_CHECKING from typing import TYPE_CHECKING
from unittest.mock import AsyncMock, patch from unittest.mock import AsyncMock, patch
@@ -9,23 +11,24 @@ from uuid import UUID, uuid4
import pytest import pytest
import pytest_asyncio import pytest_asyncio
import roboco.services.release_proposal as rp
from fastapi import FastAPI from fastapi import FastAPI
from httpx import ASGITransport, AsyncClient from httpx import ASGITransport, AsyncClient
from roboco.api.deps import get_agent_context, get_db from roboco.api.deps import get_agent_context, get_db
from roboco.api.routes import release as release_route from roboco.api.routes import release as release_route
from roboco.api.routes.release import router as release_router from roboco.api.routes.release import router as release_router
from roboco.db.tables import AgentTable, ProjectTable, TaskTable 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 import AgentRole, AgentStatus, Team
from roboco.models.base import TaskNature, TaskStatus, TaskType from roboco.models.base import TaskNature, TaskStatus, TaskType
from roboco.models.permissions import AgentContext from roboco.models.permissions import AgentContext
from roboco.services.release_executor import ReleaseResult from roboco.services.release_executor import ReleaseResult
from roboco.services.release_proposal import ReleaseProposalService from roboco.services.release_proposal import ReleaseProposalService
from roboco.services.release_readiness import ReleaseReadinessReport, report_to_dict 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 from sqlalchemy import delete
if TYPE_CHECKING: if TYPE_CHECKING:
import asyncio
from collections.abc import AsyncIterator from collections.abc import AsyncIterator
from typing import Any from typing import Any
@@ -269,12 +272,101 @@ async def test_approve_gate_failure_keeps_proposal_open_async(
await captured["task"] await captured["task"]
await db_session.refresh(task) await db_session.refresh(task)
assert task.status == TaskStatus.PENDING # still held for retry 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 @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 db_session: AsyncSession, ceo_client: AsyncClient
) -> None: ) -> 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) task = await _seed_proposal(db_session)
resp = await ceo_client.post( resp = await ceo_client.post(
"/api/release/proposal/reject", "/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 "") assert "Tighten the CHANGELOG" in (resp.json()["required_changes"] or "")
refreshed = await db_session.get(TaskTable, task.id) refreshed = await db_session.get(TaskTable, task.id)
assert refreshed is not None 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 @pytest.mark.asyncio