Files
roboco/tests/unit/gateway/test_choreographer_submit_qa_gates.py
T
ff35a646fa Chore: reduce analytics complexity (#100)
* refactor(analytics): reduce cyclomatic complexity in usage/pricing/rollup

Collapse the three near-identical get_by_* aggregation methods in
UsageService into a shared _aggregate_by helper parameterized by group
column and key name, and centralize token null-coalescing in a
_row_tokens helper. Extract the per-row upsert in _sweep_daily_rollup
into _upsert_rollup_row, and the pricing-table lookup into
_lookup_prices. All blocks now rank <= B and both modules rank A, so the
xenon gate passes; behavior is unchanged and existing tests stay green.

* feat(billing): make token pricing provider-aware

Distinguish three cases when a model has no per-token rate: a non-Anthropic
model (local Ollama, or an Ollama Cloud ":cloud" model billed by flat
subscription / GPU-time) legitimately has no per-token cost and returns 0.0
silently; an unpriced Anthropic ("claude"-named) model also returns 0.0 but
logs a warning, since that is real spend being undercounted and catches new or
renamed Claude models missing from the table. Folds the old ollama/ prefix
special-case into the general non-Anthropic path so there is one code path,
and replaces the blanket 'no pricing data' warning that fired even for
self-hosted models.

* fix(tasks): preserve ownership when force-unclaiming to pending

The stale-claim reaper and the dependency-blocked release both routed through
_force_unclaim_to_pending, which nulled assigned_to and left the task in a
pending state owned by nobody — no dispatcher re-spawns an ownerless pending
task, so it went dormant. The dispatcher-side claimed_by fallback only masked
half the cases.

Capture the owner before releasing the claim and keep both assigned_to and
claimed_by pointed at it (mirroring the unblock restore), releasing only the
live claim (active_claimant_id + heartbeat) and the WorkSession. The same agent
now resumes the task once it re-dispatches. Updates the reaper test that
asserted the old orphaning behavior and adds owner-preservation coverage for
both the reaper and dependency-release paths.

* fix(tasks): unblock restores the owner into both ownership fields

Audit follow-up to the force-unclaim ownership fix. unblock() only restored
assigned_to from blocker_raised_by, which block() stashes solely from
assigned_to. A task claimed via give_me_work (claimed_by set, assigned_to null)
therefore unblocked into a split-owner state — assigned_to null but claimed_by
set — that both the dev dispatcher and the PM pool-router race to pick up. It
also left claimed_by pointing at the resolver PM after an escalation.

Resolve the owner as blocker_raised_by or assigned_to or claimed_by and write
it to both fields, matching the force-unclaim and reassign convention so the
original worker resumes cleanly. Adds coverage for the give_me_work-claim case
and asserts owner restoration on the existing in_progress-resume test.

* test(orchestrator): cover dev owner resolution and the claimed_by fallback

_resolve_dev_owner_uuid had no coverage. Add the status-dependent precedence
(claimed/blocked prefer the live claimant; other statuses prefer the
PM-assigned owner) and the half-reap fallback where a pending task with
assigned_to nulled still resolves its owner from claimed_by instead of going
dormant.

* fix(tasks): wire the pre-block snapshot so unblock(restore=True) works

The restore=True path on a PM unblock was a no-op: pre_block_state /
pre_block_assignee (migration 006) were read by unblock_with_restore but never
written, so it always fell through to legacy unblock() and the restore flag did
nothing.

Snapshot the resting status + owner at every block entry (dependency block,
soft block, escalation) before mutating, capturing only the first block in a
chain so a re-block doesn't overwrite the original state. Escalation snapshots
the outgoing owner, not the escalation target, so restore returns the original
worker. The restore path applies the same branchless guard legacy unblock()
relies on — a snapshotted in_progress with no branch diverts to pending instead
of looping the dispatcher — and is extracted into _apply_pre_block_restore to
keep complexity under the gate. Adds coverage for snapshot capture, restore,
the branchless divert, and escalation owner restoration.

* test(tasks): update orphan-reconciler and dependency-release tests for owner preservation

Both the startup orphan reconciler and the dependency-blocked claim release
route through unclaim_for_reaper / _force_unclaim_to_pending, which now preserve
the owner instead of nulling assigned_to. Update the two tests that asserted the
old orphaning behavior to assert the owner is kept (so the same agent resumes)
while the live claim is released.

* chore(tests): scrub internal work-item labels from test names, docstrings, comments

Rename four test files that carried audit work-item IDs in their filenames
(test_p0_7_branch_atomicity, test_p2_8_orphan_reconciler,
test_p2_9_autogen_prompt_layer, test_p2_7_attempt_id) to describe what they
test, and strip the matching P-/D-/S- cluster labels from docstrings, comments,
and assertion messages across the test suite and two orchestrator comments.
These are internal references with no meaning in the codebase; behavior is
unchanged.

* style: reformat assertion line shortened by the internal-ref scrub

* build: waive unreachable torch CVE-2025-3000 in pip-audit gate

torch is a transitive CPU-pinned dep (piragi / sentence-transformers) never
loaded at runtime — the stack uses Ollama over HTTP for all embeddings/LLM, so
the vulnerable torch.jit.script path is unreachable. CVE-2025-3000 is MEDIUM,
local-only, with no published fix. Documented --ignore-vuln waiver; revisit when
a fixed torch ships.

* fix(orchestrator): route unplaceable pending tasks to main-pm instead of dropping them

_get_routing_target returned None when a 'dev'-classified task had no cell
agent (no team, or a non-cell team like fullstack/system) or when the routing
classification was unrecognized. _route_unassigned_pm_task logged 'no routing
target found' and returned, leaving the task ownerless and pending — and no
dispatcher re-spawns an unrouted pending task, so it went dormant for 10+ min
until the stuck-task detector caught it.

Fall back to main-pm (the same default cell_pm routing and escalation already
use) so the task is always owned and triaged, never stranded. Logs the fallback
so unplaceable tasks stay visible. Adds a test asserting no (routing, team)
combination ever resolves to None.

* fix(panel): make intake chat markdown inherit the bubble's text color

MarkdownBody is shared by the assistant (text-foreground) and user
(text-primary-foreground) bubbles. [&_*]:!text-inherit only colored the prose
div's descendants, so the prose div itself kept the prose typography body color
(gray) and children inherited that — unreadable on the muted assistant bubble.
Add !text-inherit on the prose div itself so it inherits the bubble's color
too; descendants then inherit the correct foreground. Fixes both bubbles without
hardcoding a color.

* fix(prompter): keep a board-reviewed product on the board team so Approve & Start shows

A product coordination root confirmed via 'Board review & Start' is assigned to
a board reviewer (product-owner) for review, but create_task_from_draft set
team=main_pm for every product unconditionally. The CEO's Approve & Start gate
keys on team=board, so the button never appeared — and because the owner stayed
a board agent while the team said main_pm, the dispatcher routed it to the board
path (nothing left to do after review) and the task stranded at pending, with
the board agent fruitlessly trying to escalate it up.

Route a product by its assignee: a board reviewer keeps it team=board (so the
gate appears and approve_and_start later hands it to Main PM), while a main-pm
assignee — the 'Approve & Start' straight-through path — is team=main_pm. Adds
_assignee_is_board mirroring TaskService's board-role check, and a test.

---------

Co-authored-by: Renn F <rennf93@users.noreply.github.com>
2026-06-11 04:36:17 +02:00

350 lines
13 KiB
Python

"""Gate Set E: submit-qa field-level gates in Choreographer.i_am_done.
Pre-gateway location: roboco/api/routes/tasks.py:903-940 (route layer).
The four field-level gates returned 400 errors when the dev tried to
submit for QA without:
- NOT_SELF_VERIFIED: task.self_verified must be true.
- NO_COMMITS: task.commits must be non-empty.
- NO_PR: task.pr_number must be set.
- NO_PROGRESS: task.progress_updates must have at least one entry.
The gateway's i_am_done previously called _run_catch_up which silently
auto-ran the full chain. That hid the missing-commits failure mode
(catch-up tried to push nothing, opened an empty PR, etc.).
Now i_am_done is strict and tells the dev exactly which prerequisite
is missing. A separate i_am_done_with_catchup verb retains the smart-
catch-up behavior for the explicit-opt-in case.
"""
from __future__ import annotations
from datetime import UTC, datetime
from typing import Any
from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4
import pytest
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
def _make_deps(**overrides: Any) -> ChoreographerDeps:
base: dict[str, Any] = {
"task": AsyncMock(),
"work_session": AsyncMock(),
"git": AsyncMock(),
"a2a": AsyncMock(),
"journal": AsyncMock(),
"audit": AsyncMock(),
"evidence_repo": AsyncMock(),
}
base.update(overrides)
# VerbRunner uses task.session.begin_nested() as a savepoint context
# manager. Keep `session` itself an AsyncMock so other awaited methods
# (e.g. flush) still work, and override begin_nested with a sync
# MagicMock that returns the async-context-manager protocol.
task = base["task"]
task.session.begin_nested = MagicMock(
return_value=MagicMock(
__aenter__=AsyncMock(return_value=None),
__aexit__=AsyncMock(return_value=False),
)
)
repo = base["evidence_repo"]
for method in (
"list_unread_a2a",
"list_unread_mentions",
"list_pending_notifications",
"task_metadata_gaps",
"recent_team_activity",
"blockers_in_lane",
"journal_highlights_for_task",
):
getattr(repo, method).return_value = []
# C8: default-fresh journal:decision so PM-decision gate passes.
# Tests that exercise the gate boundary stub their own value.
# The check matches MagicMock and AsyncMock (the two default sentinel
# types pytest's unittest.mock leaves on un-stubbed return_values).
_ldef = base["journal"].latest_decision_at.return_value
if type(_ldef).__name__ in ("MagicMock", "AsyncMock"):
base["journal"].latest_decision_at.return_value = datetime.now(UTC)
return ChoreographerDeps(**base)
def _ready_task(task_id: Any, agent_id: Any) -> MagicMock:
"""Build a task that satisfies tracing AND field-level gates."""
return MagicMock(
id=task_id,
status="in_progress",
assigned_to=agent_id,
plan={"x": 1},
branch_name="feature/backend/abc--def",
work_session_id=uuid4(),
self_verified=True,
pr_number=8,
pr_url="https://x/pr/8",
team="backend",
progress_updates=[{"message": "did x"}],
acceptance_criteria=["AC1"],
acceptance_criteria_status=[
{"criterion": "AC1", "referencing_artifact_id": "c1"}
],
commits=[{"sha": "abc"}],
documents=[],
dev_notes="",
)
# ---------------------------------------------------------------------------
# self_verified is no longer a gate
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_done_auto_runs_submit_verification_when_in_progress() -> None:
"""Strict i_am_done auto-runs submit_verification (in_progress→verifying)
so the dev doesn't need a separate verb. The previous NOT_SELF_VERIFIED
gate required submit_for_verification which wasn't on any manifest.
The pre-flight tracing gate filters SELF_VERIFIED (it is set by the
auto-run submit_verification action and re-asserted by the spec's
own preconditions), so an unverified in_progress task can still
enter i_am_done.
"""
agent_id = uuid4()
task_id = uuid4()
t = _ready_task(task_id, agent_id)
t.self_verified = False
t.status = "in_progress"
after_verify = MagicMock(
**{**t.__dict__, "self_verified": True, "status": "verifying"}
)
after_submit = MagicMock(**{**after_verify.__dict__, "status": "awaiting_qa"})
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
task_svc.submit_verification.return_value = after_verify
task_svc.submit_qa.return_value = after_submit
task_svc.qa_agent_for_team.return_value = MagicMock(
id=uuid4(), skills=[{"id": "code_review"}]
)
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
# JOURNAL_DURING_WORK_AT_LEAST_ONE: ≥1 decision/learning/struggle.
journal_svc.has_decision_for_task.return_value = True
journal_svc.latest_decision_at.return_value = datetime.now(UTC)
journal_svc.has_learning_for_task.return_value = False
journal_svc.has_struggle_for_task.return_value = False
work_svc = AsyncMock()
work_svc.files_changed.return_value = ["foo.py"]
deps = _make_deps(task=task_svc, journal=journal_svc, work_session=work_svc)
c = Choreographer(deps)
env = await c.i_am_done(agent_id, task_id, "done")
body = env.as_dict()
assert body["error"] is None
task_svc.submit_verification.assert_awaited_once()
task_svc.submit_qa.assert_awaited_once()
# ---------------------------------------------------------------------------
# E.2 NO_COMMITS
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_done_blocks_when_no_commits() -> None:
"""Spec's PRECONDITION_COMMITS rejects with the canonical
`commits>=1` missing token before any state mutation."""
agent_id = uuid4()
task_id = uuid4()
t = _ready_task(task_id, agent_id)
t.commits = []
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
# JOURNAL_DURING_WORK_AT_LEAST_ONE: ≥1 decision/learning/struggle.
journal_svc.has_decision_for_task.return_value = True
journal_svc.latest_decision_at.return_value = datetime.now(UTC)
journal_svc.has_learning_for_task.return_value = False
journal_svc.has_struggle_for_task.return_value = False
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
env = await c.i_am_done(agent_id, task_id, "done")
body = env.as_dict()
assert body["error"] == "tracing_gap"
# Spec emits "commits>=1" via PRECONDITION_COMMITS.
assert "commits>=1" in body["missing"] or "NO_COMMITS" in body["missing"]
task_svc.submit_qa.assert_not_awaited()
# ---------------------------------------------------------------------------
# E.3 NO_PR
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_done_blocks_when_no_pr() -> None:
"""Defense-in-depth field gate fires NO_PR after the spec gate accepts.
The spec doesn't yet model PR-existence; the field-gate helper still
enforces it post-spec.
"""
agent_id = uuid4()
task_id = uuid4()
t = _ready_task(task_id, agent_id)
t.pr_number = None
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
# JOURNAL_DURING_WORK_AT_LEAST_ONE: ≥1 decision/learning/struggle.
journal_svc.has_decision_for_task.return_value = True
journal_svc.latest_decision_at.return_value = datetime.now(UTC)
journal_svc.has_learning_for_task.return_value = False
journal_svc.has_struggle_for_task.return_value = False
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
env = await c.i_am_done(agent_id, task_id, "done")
body = env.as_dict()
assert body["error"] == "tracing_gap"
# foundation.policy.tracing emits "pr_open" via PR_OPEN; the legacy
# _check_submit_qa_field_gates path emitted "NO_PR" but tracing now
# short-circuits before that field gate runs.
assert (
"pr_open" in body["missing"]
or "NO_PR" in body["missing"]
or "pr_number" in body["missing"]
)
task_svc.submit_qa.assert_not_awaited()
# ---------------------------------------------------------------------------
# E.4 NO_PROGRESS
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_done_blocks_when_no_progress() -> None:
agent_id = uuid4()
task_id = uuid4()
t = _ready_task(task_id, agent_id)
t.progress_updates = []
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
# JOURNAL_DURING_WORK_AT_LEAST_ONE: ≥1 decision/learning/struggle.
journal_svc.has_decision_for_task.return_value = True
journal_svc.latest_decision_at.return_value = datetime.now(UTC)
journal_svc.has_learning_for_task.return_value = False
journal_svc.has_struggle_for_task.return_value = False
deps = _make_deps(task=task_svc, journal=journal_svc)
c = Choreographer(deps)
env = await c.i_am_done(agent_id, task_id, "done")
body = env.as_dict()
assert body["error"] == "tracing_gap"
# progress>=1 is the existing tracing_gate Requirement key.
assert "progress>=1" in body["missing"] or "NO_PROGRESS" in body["missing"]
task_svc.submit_qa.assert_not_awaited()
# ---------------------------------------------------------------------------
# E.5 happy path: all gates pass → submit_qa runs (NO catch-up)
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_done_proceeds_when_all_gates_pass() -> None:
agent_id = uuid4()
task_id = uuid4()
t = _ready_task(task_id, agent_id)
# Pre-verifying state (caller already ran submit_for_verification or
# task is already in `verifying`). i_am_done skips the auto-verify
# step and goes straight to submit_qa.
t.status = "verifying"
t.self_verified = True
after_submit = MagicMock(
**{**t.__dict__, "status": "awaiting_qa"},
)
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
task_svc.submit_qa.return_value = after_submit
task_svc.qa_agent_for_team.return_value = MagicMock(
id=uuid4(), skills=[{"id": "code_review"}]
)
journal_svc = AsyncMock()
journal_svc.has_reflect_for_task.return_value = True
# JOURNAL_DURING_WORK_AT_LEAST_ONE: ≥1 decision/learning/struggle.
journal_svc.has_decision_for_task.return_value = True
journal_svc.latest_decision_at.return_value = datetime.now(UTC)
journal_svc.has_learning_for_task.return_value = False
journal_svc.has_struggle_for_task.return_value = False
work_svc = AsyncMock()
work_svc.files_changed.return_value = ["foo.py"]
deps = _make_deps(task=task_svc, journal=journal_svc, work_session=work_svc)
c = Choreographer(deps)
env = await c.i_am_done(agent_id, task_id, "all done")
body = env.as_dict()
assert body["error"] is None
assert body["status"] == "awaiting_qa"
task_svc.submit_qa.assert_awaited_once()
# Already-verifying status: recovery path runs only submit_qa, never
# the composed submit_verification action.
task_svc.submit_verification.assert_not_awaited()
# ---------------------------------------------------------------------------
# Removed: i_am_done_with_catchup verb deleted.
# Its functionality is now split between submit_for_qa (push + PR) and
# i_am_done (auto-run submit_verification then submit_qa).
# ---------------------------------------------------------------------------
@pytest.mark.asyncio
async def test_i_am_done_blocks_unauthorized() -> None:
"""Spec's PRECONDITION_OWNERSHIP rejects with tracing_gap when the
caller does not own the task.
Pre-spec migration the verb returned a separate not_authorized
envelope from an inline ownership check; the spec now drives this
decision via PRECONDITION_OWNERSHIP, which surfaces as tracing_gap
with the `owns_task` missing token.
"""
agent_id = uuid4()
other_id = uuid4()
task_id = uuid4()
t = _ready_task(task_id, other_id)
task_svc = AsyncMock()
task_svc.get.return_value = t
task_svc.agent_for.return_value = MagicMock(
id=agent_id, role="developer", team="backend", slug=None
)
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
env = await c.i_am_done(agent_id, task_id, "done")
body = env.as_dict()
assert body["error"] == "tracing_gap"
assert "owns_task" in body["missing"]