mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
* feat(lifecycle): revision findings ledger — structured QA/PR/PM/CEO failure feedback, persisted and delivered down the chain Every bounce used to survive only as flattened prose: rounds overwrote each other in notes_structured, request_changes persisted nothing, two raw dev_notes appends were silently destroyed by the next handoff note, and the dev prompt pointed at fields (qa_notes via evidence(), pm_notes) the API never delivered. Agents re-interpreted and re-discovered every failure before they could start fixing it. - task_review_findings (migration 071, append-only): file/line/severity/ criterion(AC-id-validated)/expected/actual/fix/evidence per finding, with origin (qa|pr_gate|pm|ceo), round, and an open->addressed->verified lifecycle (waived reserved); new tasks.pm_notes + PmReviewContent give request_changes a structured home - producers: fail_review/pr_fail/request_changes take findings=[...] (prose issues shimmed+merged for one release, deprecation-logged); ceo_reject validates its reason (no 500), lands an origin=ceo finding, and bumps round+audit on branchless coordination roots; guardrails at the verb chokepoint (nudge >5, hard reject >10, field caps, traversal-safe file); the dev_notes data-loss appends are removed; new task.request_changes + task.ceo_reject audit events close rework attribution - delivery: qa_notes/pr_reviewer_notes/pm_notes carry the deterministic [F-id8] rendering; claim briefings, evidence(), the REVISION_REQUIRED spawn prompt, PM triage bounced-blocks, and A2A bodies deliver open findings; round-N+1 QA and gate reviewers get the full prior ledger; panel Findings tab + bounced-xN chip; metrics pm_rejects/ceo_rejects + findings counts; vault task notes render a Findings section (fail-open) - resolution closes for every origin: i_am_done and submit_up/submit_root take resolved_findings gated by FINDINGS_ADDRESSED (owner-gated so a stale non-owner PM can never mutate the ledger); pass_review/pr_pass/ complete verify-stamp same-transaction; ceo_approve stamps best-effort - 24 real-DB integration tests drive the full loop through the real choreographer; full suite 12856 green * docs: revision findings ledger sweep — CLAUDE.md, map, RAG corpus - CLAUDE.md: new ledger section + corrected request_changes row - docs/map/review-findings.md (new subsystem map) + surgical updates to task-service/pr-gate-review/metrics-observability/vault/panel maps - docs/rag: producers' findings contract across qa/pr-reviewer/developer/ cell-pm/main-pm/ceo role docs (the PM docs were missing request_changes entirely), verb references, and a new architecture/review-findings.md disambiguating ledger findings from convention findings * test(e2e): resubmit resolves the pr_fail finding per the ledger contract The scripted pr_fail revision loop resubmitted submit_up without resolved_findings — correctly rejected now that FINDINGS_ADDRESSED gates the PM resubmit verbs (green locally, red only in CI since the e2e suite skips without ROBOCO_E2E_SMOKE=1). The scripted PM now reads the open ledger row pr_fail persisted (new open_finding_ids arc helper) and resolves it on resubmit, asserting the open set drains — exercising the coordinator half of the new contract end to end. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
379 lines
14 KiB
Python
379 lines
14 KiB
Python
"""Dispatch routing for blocked tasks (#17) and agentless claims (#19).
|
|
|
|
#17: a blocked task reassigned to Main PM must dispatch THAT assignee to
|
|
unblock it, not the ex-assignee cell PM — the pre-unblock note is assignee-only
|
|
and the ex-assignee got not_authorized, livelocking the respawn.
|
|
|
|
#19: a task left claimed/in_progress with an assignee but no running container
|
|
is invisibly stuck (only PENDING tasks get fresh dispatch). The orchestrator
|
|
must (re)spawn the assignee after a short grace window, or release the claim to
|
|
pending when the assignee is unknown.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from datetime import UTC, datetime, timedelta
|
|
from typing import Any
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
from uuid import uuid4
|
|
|
|
import pytest
|
|
from roboco.models.runtime import AgentInstance
|
|
from roboco.runtime.orchestrator import AgentOrchestrator, AgentState
|
|
from roboco.seeds.initial_data import AGENT_UUIDS
|
|
|
|
|
|
def _orch() -> AgentOrchestrator:
|
|
orch = object.__new__(AgentOrchestrator)
|
|
orch._instances = {}
|
|
return orch
|
|
|
|
|
|
def _active_instance(agent_id: str) -> AgentInstance:
|
|
return AgentInstance(agent_id=agent_id, state=AgentState.ACTIVE)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# _blocker_resolver_slug (#17)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_blocked_task_assigned_to_main_pm_dispatches_main_pm() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"team": "backend",
|
|
"assigned_to": AGENT_UUIDS["main-pm"],
|
|
}
|
|
# The current assignee (Main PM) holds unblock authority — dispatch THEM,
|
|
# not the ex-assignee cell PM (be-pm), which would loop on not_authorized.
|
|
assert orch._blocker_resolver_slug(task) == "main-pm"
|
|
|
|
|
|
def test_blocked_task_assigned_to_board_is_not_dispatched() -> None:
|
|
# A board/advisory role (product-owner / head-marketing) has NO unblock
|
|
# verb — dispatching it to resolve a blocker is a futile catch-22 (it can
|
|
# only notify/triage, so it spam-notifies the CEO and respawns forever).
|
|
# The resolver must be None so the blocker dispatch SKIPS it; the task is
|
|
# mis-owned and must be re-routed / surfaced to the CEO out-of-band.
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"team": "backend",
|
|
"assigned_to": AGENT_UUIDS["product-owner"],
|
|
}
|
|
assert orch._blocker_resolver_slug(task) is None
|
|
|
|
|
|
def test_blocked_task_assigned_to_head_marketing_is_not_dispatched() -> None:
|
|
# Same catch-22 guard for the other board role.
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"team": "backend",
|
|
"assigned_to": AGENT_UUIDS["head-marketing"],
|
|
}
|
|
assert orch._blocker_resolver_slug(task) is None
|
|
|
|
|
|
def test_blocked_task_held_by_dev_falls_back_to_cell_pm() -> None:
|
|
orch = _orch()
|
|
# A dev raised i_am_blocked and still holds the task → cell PM resolves.
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"team": "backend",
|
|
"assigned_to": AGENT_UUIDS["be-dev-1"],
|
|
}
|
|
assert orch._blocker_resolver_slug(task) == "be-pm"
|
|
|
|
|
|
def test_blocked_task_unassigned_falls_back_to_cell_pm() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {"id": "t1", "team": "frontend", "assigned_to": None}
|
|
assert orch._blocker_resolver_slug(task) == "fe-pm"
|
|
|
|
|
|
def test_blocked_task_non_cell_team_unassigned_is_unroutable() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {"id": "t1", "team": "board", "assigned_to": None}
|
|
assert orch._blocker_resolver_slug(task) is None
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# _claimed_task_needs_agent — claimed-but-no-agent detection
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_STALE = (datetime.now(UTC) - timedelta(minutes=30)).isoformat()
|
|
|
|
|
|
def test_claimed_task_with_no_agent_past_grace_returns_assignee() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "claimed",
|
|
"assigned_to": AGENT_UUIDS["be-dev-1"],
|
|
"updated_at": _STALE,
|
|
}
|
|
assert orch._claimed_task_needs_agent(task) == "be-dev-1"
|
|
|
|
|
|
def test_claimed_task_with_active_agent_is_healthy() -> None:
|
|
orch = _orch()
|
|
orch._instances["be-dev-1"] = _active_instance("be-dev-1")
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "claimed",
|
|
"assigned_to": AGENT_UUIDS["be-dev-1"],
|
|
"updated_at": _STALE,
|
|
}
|
|
assert orch._claimed_task_needs_agent(task) is None
|
|
|
|
|
|
def test_claimed_task_within_grace_window_is_skipped() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "claimed",
|
|
"assigned_to": AGENT_UUIDS["be-dev-1"],
|
|
# Compute "fresh" at test time, not module load: the grace check uses
|
|
# wall-clock now(), so a module-level constant ages out of the window
|
|
# during a long full-suite run and flakes this assertion.
|
|
"updated_at": datetime.now(UTC).isoformat(),
|
|
}
|
|
# Fresh claim — spawn may still be in flight; do not churn.
|
|
assert orch._claimed_task_needs_agent(task) is None
|
|
|
|
|
|
def test_claimed_task_without_assignee_is_skipped() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "claimed",
|
|
"assigned_to": None,
|
|
"claimed_by": None,
|
|
"updated_at": _STALE,
|
|
}
|
|
assert orch._claimed_task_needs_agent(task) is None
|
|
|
|
|
|
def test_hitl_blocked_claimed_task_is_skipped() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "blocked",
|
|
"blocker_resolver_type": "human",
|
|
"assigned_to": AGENT_UUIDS["be-dev-1"],
|
|
"updated_at": _STALE,
|
|
}
|
|
assert orch._claimed_task_needs_agent(task) is None
|
|
|
|
|
|
def test_claimed_task_assigned_to_ceo_is_not_respawned() -> None:
|
|
# A claimed/in_progress task whose assignee is the CEO (or any human-only
|
|
# role) has no container to respawn — the CEO is the human operator. The
|
|
# resolver must return None so the dispatcher neither spawns a CEO
|
|
# container NOR releases a human-owned task to pending. Defense-in-depth
|
|
# for the spawn_agent human-role chokepoint (2026-06-27 CEO-spawn incident).
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "in_progress",
|
|
"assigned_to": AGENT_UUIDS["ceo"],
|
|
"updated_at": _STALE,
|
|
}
|
|
assert orch._claimed_task_needs_agent(task) is None
|
|
|
|
|
|
def test_in_progress_task_with_no_agent_returns_assignee() -> None:
|
|
orch = _orch()
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "in_progress",
|
|
"assigned_to": AGENT_UUIDS["fe-dev-2"],
|
|
"updated_at": _STALE,
|
|
}
|
|
assert orch._claimed_task_needs_agent(task) == "fe-dev-2"
|
|
|
|
|
|
def test_claimed_task_with_unknown_assignee_returns_slug_for_release() -> None:
|
|
# A claimed/in_progress task whose assignee is a stale/unknown UUID (no
|
|
# seeded agent) must reach the release-to-pending path: the human-only guard
|
|
# returns None for unknown slugs, so the slug falls through and is released.
|
|
orch = _orch()
|
|
unknown_uuid = str(uuid4())
|
|
task: dict[str, Any] = {
|
|
"id": "t1",
|
|
"status": "claimed",
|
|
"assigned_to": unknown_uuid,
|
|
"updated_at": _STALE,
|
|
}
|
|
# Returns the (unknown) slug, NOT None — the release path is reachable.
|
|
assert orch._claimed_task_needs_agent(task) == unknown_uuid
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# _get_prompt_for_agent — role-appropriate respawn prompt (#19)
|
|
# ---------------------------------------------------------------------------
|
|
#
|
|
# A respawn must hand each role the prompt it can act on. The bug: the PM/board
|
|
# branch fell through to the developer prompt, telling a PM/board agent to write
|
|
# code and call verbs it does not own.
|
|
|
|
|
|
def _task(**over: Any) -> dict[str, Any]:
|
|
base: dict[str, Any] = {
|
|
"id": "t1",
|
|
"title": "T",
|
|
"status": "in_progress",
|
|
"team": "backend",
|
|
}
|
|
base.update(over)
|
|
return base
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
("agent_slug", "marker"),
|
|
[
|
|
("be-dev-1", "development task"),
|
|
("be-qa", "ready for QA review"),
|
|
("be-doc", "ready for documentation"),
|
|
("be-pm", "PM for backend team"),
|
|
("main-pm", "MAIN PM at RoboCo"),
|
|
("product-owner", "You are on the Board"),
|
|
("auditor", "AUDIT"),
|
|
],
|
|
)
|
|
@pytest.mark.asyncio
|
|
async def test_get_prompt_for_agent_routes_by_role(
|
|
agent_slug: str, marker: str
|
|
) -> None:
|
|
orch = _orch()
|
|
prompt = await orch._get_prompt_for_agent(agent_slug, _task())
|
|
assert marker in prompt
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_get_prompt_for_pm_is_not_the_dev_prompt() -> None:
|
|
# Regression for #19: a respawned PM must NOT receive the developer prompt.
|
|
orch = _orch()
|
|
pm_prompt = await orch._get_prompt_for_agent("be-pm", _task())
|
|
assert "development task" not in pm_prompt
|
|
assert "You do NOT code" in pm_prompt
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_get_prompt_for_board_is_not_the_dev_prompt() -> None:
|
|
orch = _orch()
|
|
board_prompt = await orch._get_prompt_for_agent("product-owner", _task())
|
|
assert "development task" not in board_prompt
|
|
assert "do NOT build, code" in board_prompt
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_head_marketing_prompt_is_marketing_on_marketing_team() -> None:
|
|
orch = _orch()
|
|
prompt = await orch._get_prompt_for_agent("head-marketing", _task(team="marketing"))
|
|
assert "marketing task" in prompt
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_head_marketing_prompt_is_board_off_marketing_team() -> None:
|
|
orch = _orch()
|
|
prompt = await orch._get_prompt_for_agent("head-marketing", _task(team="backend"))
|
|
assert "You are on the Board" in prompt
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# _dispatch_claimed_without_agent — one-spawn-per-tick throttle (#19)
|
|
# ---------------------------------------------------------------------------
|
|
#
|
|
# `monkeypatch.setattr` is used to stub instance methods because direct
|
|
# attribute assignment (`orch.spawn_agent = ...`) trips mypy's method-assign
|
|
# check; the fixture is the type-safe, suppression-free way to do it.
|
|
|
|
|
|
def _stub_git_context(orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch) -> None:
|
|
monkeypatch.setattr(orch, "_task_git_context", lambda _task: None)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_dispatch_claimed_without_agent_spawns_at_most_one_per_tick(
|
|
monkeypatch: pytest.MonkeyPatch,
|
|
) -> None:
|
|
orch = _orch()
|
|
orch._tick_handled_tasks = set()
|
|
stale_tasks = [
|
|
{"id": f"t{i}", "status": "claimed", "assigned_to": AGENT_UUIDS["be-dev-1"]}
|
|
for i in range(3)
|
|
]
|
|
monkeypatch.setattr(orch, "_fetch_tasks", AsyncMock(return_value=stale_tasks))
|
|
monkeypatch.setattr(orch, "_claimed_task_needs_agent", lambda _task: "be-dev-1")
|
|
_stub_git_context(orch, monkeypatch)
|
|
spawn = AsyncMock()
|
|
monkeypatch.setattr(orch, "spawn_agent", spawn)
|
|
|
|
await orch._dispatch_claimed_without_agent(client=MagicMock())
|
|
|
|
# Three agentless claims, but only ONE container spawned this tick.
|
|
spawn.assert_awaited_once()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_dispatch_claimed_without_agent_releases_unknown_without_spending_budget(
|
|
monkeypatch: pytest.MonkeyPatch,
|
|
) -> None:
|
|
# The release-to-pending path spawns nothing and must NOT consume the
|
|
# per-tick spawn budget — it keeps draining stale unknown claims, then
|
|
# spawns the first task with a known assignee.
|
|
orch = _orch()
|
|
orch._tick_handled_tasks = set()
|
|
tasks = [
|
|
{"id": "u1", "status": "claimed", "assigned_to": "ghost-uuid"},
|
|
{"id": "u2", "status": "claimed", "assigned_to": "ghost-uuid"},
|
|
{"id": "k1", "status": "claimed", "assigned_to": AGENT_UUIDS["be-dev-1"]},
|
|
]
|
|
monkeypatch.setattr(orch, "_fetch_tasks", AsyncMock(return_value=tasks))
|
|
|
|
def _needs(task: dict[str, Any]) -> str:
|
|
return orch._resolve_agent_slug(str(task["assigned_to"]))
|
|
|
|
monkeypatch.setattr(orch, "_claimed_task_needs_agent", _needs)
|
|
_stub_git_context(orch, monkeypatch)
|
|
release = AsyncMock()
|
|
monkeypatch.setattr(orch, "_release_claim_to_pending", release)
|
|
spawn = AsyncMock()
|
|
monkeypatch.setattr(orch, "spawn_agent", spawn)
|
|
|
|
await orch._dispatch_claimed_without_agent(client=MagicMock())
|
|
|
|
expected_releases = 2 # both ghost claims released
|
|
assert release.await_count == expected_releases
|
|
spawn.assert_awaited_once() # then one known assignee respawned
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_handle_dev_existing_owner_skips_blocked() -> None:
|
|
"""A blocked task's owner is not respawned — it has no legal move from
|
|
blocked, so respawning it only churns; it waits for unblock or release."""
|
|
orch = _orch()
|
|
respawn_mock = AsyncMock()
|
|
with (
|
|
patch.object(orch, "_respawn_dev_if_inactive", new=respawn_mock),
|
|
patch.object(orch, "_is_agent_active", new=MagicMock(return_value=False)),
|
|
):
|
|
await orch._handle_dev_existing_owner({"id": "t1"}, "blocked", "be-dev-1")
|
|
respawn_mock.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_handle_dev_existing_owner_respawns_in_progress() -> None:
|
|
"""An in_progress task whose owner is inactive is still respawned."""
|
|
orch = _orch()
|
|
respawn_mock = AsyncMock()
|
|
with (
|
|
patch.object(orch, "_respawn_dev_if_inactive", new=respawn_mock),
|
|
patch.object(orch, "_is_agent_active", new=MagicMock(return_value=False)),
|
|
):
|
|
await orch._handle_dev_existing_owner({"id": "t1"}, "in_progress", "be-dev-1")
|
|
respawn_mock.assert_awaited_once()
|