mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(notification): ack notifications can join the caller's transaction (#603)
The release engine's bell notification for a just-originated proposal inserted through a fresh session while the proposal task sat uncommitted in the engine's own transaction — the related_task_id FK rejected the row and the ping was silently lost (caught live in the postgres log; the DB-free Telegram DM still went out). send_ack_notification now accepts db_session, forwarded to _create_notification so the insert joins the caller's transaction, and the release engine passes its session. The other five callers pass no task_id or reference committed tasks. Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -50,6 +50,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
|
|||||||
- **`/auth/login` no longer 422s (#580).** FastAPI had demoted the db dependency to a query parameter on the login route.
|
- **`/auth/login` no longer 422s (#580).** FastAPI had demoted the db dependency to a query parameter on the login route.
|
||||||
- **Task cancellation closes the task's own PR, and the bulk branch sweep spares live dependents (#593).** Every cancel path now best-effort-closes the recorded open PR for the task and its cascaded descendants; the stale-branch sweep excludes branches still recorded by a non-terminal task or serving as a live child's resolved parent branch.
|
- **Task cancellation closes the task's own PR, and the bulk branch sweep spares live dependents (#593).** Every cancel path now best-effort-closes the recorded open PR for the task and its cascaded descendants; the stale-branch sweep excludes branches still recorded by a non-terminal task or serving as a live child's resolved parent branch.
|
||||||
- **Release/readiness hardening basket.** The root PR base resolves the project's env ladder instead of literal master; version detection accepts manifest variants and never crashes the sweep; unconfigured social platforms are skipped instead of becoming pending-forever failures (#545); the readiness sweep's CI wait polls the prod rung; and check-runs dedupe per name so a cancelled duplicate can't mask a green run.
|
- **Release/readiness hardening basket.** The root PR base resolves the project's env ladder instead of literal master; version detection accepts manifest variants and never crashes the sweep; unconfigured social platforms are skipped instead of becoming pending-forever failures (#545); the readiness sweep's CI wait polls the prod rung; and check-runs dedupe per name so a cancelled duplicate can't mask a green run.
|
||||||
|
- **The release-proposal bell notification survives its own transaction.** `send_ack_notification` inserted the notification row through a fresh session while the proposal task sat uncommitted in the origination engine's transaction, so the `related_task_id` FK rejected it and the panel-bell ping was silently lost (the Telegram DM, which is DB-free, still went out). The service now accepts the caller's session so the insert joins the same transaction, and the release engine passes it.
|
||||||
- **Panel/ops hardening basket.** Session links target the owning task (the `/work-sessions/<id>` route never existed); PM review turns are restart-safe; dialog triggers behind tooltips fire again and dotted composition ids render; `git pull` on a target branch became a hard sync to origin; the `git_provider` migration renumbered to 076 restoring a single Alembic head; CI-watch fixed its own regression on roboco-api (#563); agent commits no longer carry model self-attribution; and cockpit CI assertions became delta-based so the one-process run's row leaks can't flake them (#577, #578).
|
- **Panel/ops hardening basket.** Session links target the owning task (the `/work-sessions/<id>` route never existed); PM review turns are restart-safe; dialog triggers behind tooltips fire again and dotted composition ids render; `git pull` on a target branch became a hard sync to origin; the `git_provider` migration renumbered to 076 restoring a single Alembic head; CI-watch fixed its own regression on roboco-api (#563); agent commits no longer carry model self-attribution; and cockpit CI assertions became delta-based so the one-process run's row leaks can't flake them (#577, #578).
|
||||||
|
|
||||||
## [0.25.0] - 2026-07-16
|
## [0.25.0] - 2026-07-16
|
||||||
|
|||||||
@@ -611,6 +611,7 @@ class NotificationService:
|
|||||||
body: str,
|
body: str,
|
||||||
priority: NotificationPriority = NotificationPriority.NORMAL,
|
priority: NotificationPriority = NotificationPriority.NORMAL,
|
||||||
task_id: UUID | str | None = None,
|
task_id: UUID | str | None = None,
|
||||||
|
db_session: AsyncSession | None = None,
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Send a free-form ack-required notification (PM/Board only).
|
"""Send a free-form ack-required notification (PM/Board only).
|
||||||
|
|
||||||
@@ -625,6 +626,11 @@ class NotificationService:
|
|||||||
"""
|
"""
|
||||||
subject = body.split("\n", 1)[0][:200] or "Notification"
|
subject = body.split("\n", 1)[0][:200] or "Notification"
|
||||||
related_task_id = str(task_id) if task_id is not None else None
|
related_task_id = str(task_id) if task_id is not None else None
|
||||||
|
# A caller notifying about a row its OWN open transaction just
|
||||||
|
# created must pass db_session so the insert joins that transaction
|
||||||
|
# — a fresh session can't see the uncommitted task and the
|
||||||
|
# related_task_id FK rejects the row (the 0.26.0 release-proposal
|
||||||
|
# bell notification was lost exactly this way).
|
||||||
await self._create_notification(
|
await self._create_notification(
|
||||||
CreateNotificationParams(
|
CreateNotificationParams(
|
||||||
notification_type=NotificationType.ALERT,
|
notification_type=NotificationType.ALERT,
|
||||||
@@ -634,7 +640,8 @@ class NotificationService:
|
|||||||
subject=subject,
|
subject=subject,
|
||||||
body=body,
|
body=body,
|
||||||
related_task_id=related_task_id,
|
related_task_id=related_task_id,
|
||||||
)
|
),
|
||||||
|
db_session=db_session,
|
||||||
)
|
)
|
||||||
|
|
||||||
async def send_broadcast_notification(
|
async def send_broadcast_notification(
|
||||||
|
|||||||
@@ -201,7 +201,11 @@ class ReleaseManagerEngine(BaseService):
|
|||||||
)
|
)
|
||||||
try:
|
try:
|
||||||
await NotificationService().send_ack_notification(
|
await NotificationService().send_ack_notification(
|
||||||
from_agent="system", to_agent="ceo", body=body, task_id=str(task.id)
|
from_agent="system",
|
||||||
|
to_agent="ceo",
|
||||||
|
body=body,
|
||||||
|
task_id=str(task.id),
|
||||||
|
db_session=self.session,
|
||||||
)
|
)
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
self.log.warning("release CEO notify failed (best-effort)", error=str(exc))
|
self.log.warning("release CEO notify failed (best-effort)", error=str(exc))
|
||||||
|
|||||||
@@ -0,0 +1,89 @@
|
|||||||
|
"""send_ack_notification must be able to join the caller's transaction.
|
||||||
|
|
||||||
|
The origination engines notify about a task their OWN open transaction just
|
||||||
|
created; a fresh session cannot see that uncommitted row, so the
|
||||||
|
notifications.related_task_id FK rejects the insert and the bell
|
||||||
|
notification is silently lost (the 0.26.0 release-proposal case, caught in
|
||||||
|
the live postgres log). Real-Postgres coverage for both directions.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from typing import TYPE_CHECKING, cast
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.db.tables import AgentTable, NotificationTable, TaskTable
|
||||||
|
from roboco.models.base import (
|
||||||
|
AgentRole,
|
||||||
|
AgentStatus,
|
||||||
|
Complexity,
|
||||||
|
TaskNature,
|
||||||
|
TaskStatus,
|
||||||
|
TaskType,
|
||||||
|
Team,
|
||||||
|
)
|
||||||
|
from roboco.services.notification import NotificationService
|
||||||
|
from sqlalchemy import select
|
||||||
|
|
||||||
|
if TYPE_CHECKING:
|
||||||
|
from uuid import UUID
|
||||||
|
|
||||||
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
|
|
||||||
|
async def _seed(db_session: AsyncSession) -> tuple[AgentTable, TaskTable]:
|
||||||
|
unique = uuid4().hex[:6]
|
||||||
|
agent = AgentTable(
|
||||||
|
id=uuid4(),
|
||||||
|
name=f"ceo-{unique}",
|
||||||
|
slug=f"ceo-{unique}",
|
||||||
|
role=AgentRole.CEO,
|
||||||
|
team=None,
|
||||||
|
status=AgentStatus.ACTIVE,
|
||||||
|
model_config={},
|
||||||
|
system_prompt="x",
|
||||||
|
capabilities=[],
|
||||||
|
permissions={},
|
||||||
|
metrics={},
|
||||||
|
)
|
||||||
|
db_session.add(agent)
|
||||||
|
await db_session.flush()
|
||||||
|
task = TaskTable(
|
||||||
|
id=uuid4(),
|
||||||
|
title="Release proposal: v9.9.9",
|
||||||
|
description="d",
|
||||||
|
acceptance_criteria=["ac"],
|
||||||
|
task_type=TaskType.ADMINISTRATIVE,
|
||||||
|
nature=TaskNature.NON_TECHNICAL,
|
||||||
|
status=TaskStatus.PENDING,
|
||||||
|
team=Team.BOARD,
|
||||||
|
created_by=agent.id,
|
||||||
|
estimated_complexity=Complexity.LOW,
|
||||||
|
)
|
||||||
|
db_session.add(task)
|
||||||
|
# Flushed but NOT committed — exactly the engine's state at notify time.
|
||||||
|
await db_session.flush()
|
||||||
|
return agent, task
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ack_notification_joins_the_callers_transaction(
|
||||||
|
db_session: AsyncSession,
|
||||||
|
) -> None:
|
||||||
|
agent, task = await _seed(db_session)
|
||||||
|
await NotificationService().send_ack_notification(
|
||||||
|
from_agent=agent.slug,
|
||||||
|
to_agent=agent.slug,
|
||||||
|
body="v9.9.9 ready",
|
||||||
|
task_id=cast("UUID", task.id),
|
||||||
|
db_session=db_session,
|
||||||
|
)
|
||||||
|
row = (
|
||||||
|
await db_session.execute(
|
||||||
|
select(NotificationTable).where(
|
||||||
|
NotificationTable.related_task_id == task.id
|
||||||
|
)
|
||||||
|
)
|
||||||
|
).scalar_one()
|
||||||
|
assert row.subject == "v9.9.9 ready"
|
||||||
Reference in New Issue
Block a user