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-08-03 07:23:24 +02:00 · 2026-06-11 04:36:17 +02:00
parent b3057628b0
commit ff35a646fa
26 changed files with 814 additions and 353 deletions
@@ -1,4 +1,4 @@
-"""P2-9: the prompt composer injects the autogen verb table.
+"""The prompt composer injects the autogen verb table.

 `compose_prompt` reads `agents/prompts/_generated/<role>.md` and
 includes it as a composition layer (between role and team). This pins
@@ -137,8 +137,7 @@ def _reload_mcp_module(monkeypatch: pytest.MonkeyPatch, dotted: str) -> ModuleTy
    importlib at the top-level keeps PLC0415 happy.

    Also writes a stub manifest file and points the MCP server at it,
-    since both servers now refuse to register any tools without one
-    (audit P0-5 / D-12).
+    since both servers now refuse to register any tools without one.
    """
    monkeypatch.setenv("ROBOCO_AGENT_ID", "00000000-0000-0000-0000-000000000001")
    monkeypatch.setenv("ROBOCO_AGENT_ROLE", "developer")
@@ -12,7 +12,7 @@ Covers:
 from __future__ import annotations

 import pytest
-from roboco.billing.pricing import calculate_cost
+from roboco.billing.pricing import _is_anthropic_model, calculate_cost

 # ---------------------------------------------------------------------------
 # Named constants (ruff PLR2004: magic values in comparisons must be named).
@@ -297,3 +297,35 @@ class TestSubstringMatchPriority:
        )
        assert lower_cost == upper_cost
        assert lower_cost > _ZERO_COST
+
+
+# ---------------------------------------------------------------------------
+# Provider awareness — non-Anthropic models have no per-token cost
+# ---------------------------------------------------------------------------
+
+
+class TestProviderAwareness:
+    """Non-Anthropic models (local Ollama / Ollama Cloud) cost 0.0 per token."""
+
+    def test_ollama_prefixed_model_returns_zero(self) -> None:
+        """Self-hosted Ollama models (``ollama/`` prefix) have no API cost."""
+        cost = calculate_cost("ollama/llama3", tokens_input=_M, tokens_output=_M)
+        assert cost == _ZERO_COST
+
+    def test_ollama_cloud_model_returns_zero(self) -> None:
+        """Ollama Cloud (``:cloud`` tag) is subscription-billed, not per token."""
+        cost = calculate_cost("glm-5:cloud", tokens_input=_M, tokens_output=_M)
+        assert cost == _ZERO_COST
+
+    def test_bare_local_model_returns_zero(self) -> None:
+        """A bare local embedding model has no per-token cost."""
+        cost = calculate_cost("qwen3-embedding:0.6b", tokens_input=_M, tokens_output=0)
+        assert cost == _ZERO_COST
+
+    def test_is_anthropic_model_true_for_claude_names(self) -> None:
+        for name in ("claude-opus-4-6", "claude-fable-5", "opus", "sonnet", "haiku"):
+            assert _is_anthropic_model(name) is True, name
+
+    def test_is_anthropic_model_false_for_non_claude_names(self) -> None:
+        for name in ("ollama/llama3", "glm-5:cloud", "qwen3-embedding", "gpt-4o"):
+            assert _is_anthropic_model(name) is False, name
@@ -391,7 +391,7 @@ async def test_i_will_work_on_blocks_when_journal_note_at_claim_missing() -> Non
    task_svc.start.assert_awaited_once_with(task_id, agent_id)


-# test_i_am_done_with_catchup_full_chain removed (audit P2-5/D-16):
+# test_i_am_done_with_catchup_full_chain removed:
 # i_am_done_with_catchup verb deleted. submit_for_qa now does push + PR
 # explicitly; i_am_done auto-runs submit_verification + submit_qa.

@@ -97,7 +97,7 @@ def _ready_task(task_id: Any, agent_id: Any) -> MagicMock:


 # ---------------------------------------------------------------------------
-# E.1 self_verified is no longer a gate (audit P1-3/D-08)
+# self_verified is no longer a gate
 # ---------------------------------------------------------------------------


@@ -315,7 +315,7 @@ async def test_i_am_done_proceeds_when_all_gates_pass() -> None:


 # ---------------------------------------------------------------------------
-# E.6 — Removed: i_am_done_with_catchup verb deleted (audit P2-5/D-16).
+# 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).
 # ---------------------------------------------------------------------------
@@ -1,4 +1,4 @@
-"""P2-7: every gateway.rejected audit row carries an attempt_id.
+"""Every gateway.rejected audit row carries an attempt_id.

 The attempt_id (uuid4 per rejection) lets post-mortem queries group
 all attempts on a task within a window, even when multiple calls share
@@ -73,8 +73,8 @@ async def test_rejection_includes_attempt_id() -> None:
    audit_svc.log_event.assert_awaited()
    args = audit_svc.log_event.await_args
    details = args.kwargs["details"]
-    assert "attempt_id" in details, "P2-7: audit row must include attempt_id"
-    assert _is_uuid(details["attempt_id"]), "P2-7: attempt_id must be a UUID string"
+    assert "attempt_id" in details, "audit row must include attempt_id"
+    assert _is_uuid(details["attempt_id"]), "attempt_id must be a UUID string"


@pytest.mark.asyncio
@@ -94,9 +94,7 @@ async def test_distinct_rejections_emit_distinct_attempt_ids() -> None:
    expected_distinct_ids = 2
    calls = audit_svc.log_event.await_args_list
    ids = {call.kwargs["details"]["attempt_id"] for call in calls}
-    assert len(ids) == expected_distinct_ids, (
-        "P2-7: each rejection emits its own attempt_id"
-    )
+    assert len(ids) == expected_distinct_ids, "each rejection emits its own attempt_id"


@pytest.mark.asyncio
@@ -10,7 +10,7 @@ from unittest.mock import MagicMock, patch
 import pytest

 # Same pattern as test_flow_server: do_server now refuses to start without
-# a manifest (audit P0-5 / D-12). The test fixture writes a stub manifest
+# a manifest. The test fixture writes a stub manifest
 # with the full do-tool superset; production manifests are role-scoped.
 _DO_TEST_MANIFEST = {
    "agent_id": "00000000-0000-0000-0000-000000000001",
@@ -1,4 +1,4 @@
-"""P0-6 / D-13: MCP _post() surfaces envelope body on 4xx.
+"""MCP _post() surfaces envelope body on 4xx.

 The pre-fix path called ``response.raise_for_status()`` then ``.json()``,
 which discarded the body on any 4xx — agents saw a Python
@@ -0,0 +1,79 @@
+"""Owner resolution for dev dispatch — _resolve_dev_owner_uuid.
+
+A stale-claim reap (or a half-applied ownership write) can leave a task
+``pending`` with ``assigned_to`` nulled but ``claimed_by`` still set. The dev
+dispatcher must still resolve an owner from ``claimed_by`` so the task is
+re-spawned instead of going dormant. For ``claimed``/``blocked`` the live
+claimant (``claimed_by``) wins; for every other status ``assigned_to`` is the
+PM-assigned owner and wins, falling back to ``claimed_by``.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from roboco.runtime.orchestrator import AgentOrchestrator
+
+_ASSIGNED = "11111111-1111-1111-1111-111111111111"
+_CLAIMED = "22222222-2222-2222-2222-222222222222"
+
+
+def _resolve(status: str, *, assigned: str | None, claimed: str | None) -> str | None:
+    task: dict[str, Any] = {
+        "status": status,
+        "assigned_to": assigned,
+        "claimed_by": claimed,
+    }
+    return AgentOrchestrator._resolve_dev_owner_uuid(task)
+
+
+# ---------------------------------------------------------------------------
+# pending — assigned_to preferred, claimed_by is the fallback (Bug 3 / 06b0802f)
+# ---------------------------------------------------------------------------
+
+
+def test_pending_prefers_assigned_to() -> None:
+    assert _resolve("pending", assigned=_ASSIGNED, claimed=_CLAIMED) == _ASSIGNED
+
+
+def test_pending_falls_back_to_claimed_by_when_unassigned() -> None:
+    # The half-reap case: assigned_to nulled, claimed_by survives.
+    assert _resolve("pending", assigned=None, claimed=_CLAIMED) == _CLAIMED
+
+
+def test_pending_with_no_owner_returns_none() -> None:
+    assert _resolve("pending", assigned=None, claimed=None) is None
+
+
+# ---------------------------------------------------------------------------
+# claimed / blocked — the live claimant wins, assigned_to is the fallback
+# ---------------------------------------------------------------------------
+
+
+def test_claimed_prefers_claimed_by() -> None:
+    assert _resolve("claimed", assigned=_ASSIGNED, claimed=_CLAIMED) == _CLAIMED
+
+
+def test_blocked_prefers_claimed_by() -> None:
+    assert _resolve("blocked", assigned=_ASSIGNED, claimed=_CLAIMED) == _CLAIMED
+
+
+def test_blocked_falls_back_to_assigned_to() -> None:
+    assert _resolve("blocked", assigned=_ASSIGNED, claimed=None) == _ASSIGNED
+
+
+# ---------------------------------------------------------------------------
+# other statuses — assigned_to preferred, claimed_by fallback
+# ---------------------------------------------------------------------------
+
+
+def test_in_progress_prefers_assigned_to() -> None:
+    assert _resolve("in_progress", assigned=_ASSIGNED, claimed=_CLAIMED) == _ASSIGNED
+
+
+def test_in_progress_falls_back_to_claimed_by() -> None:
+    assert _resolve("in_progress", assigned=None, claimed=_CLAIMED) == _CLAIMED
+
+
+def test_needs_revision_prefers_assigned_to() -> None:
+    assert _resolve("needs_revision", assigned=_ASSIGNED, claimed=_CLAIMED) == _ASSIGNED
@@ -0,0 +1,78 @@
+"""Routing-target resolution never strands an unassigned pending task.
+
+`_get_routing_target` must always resolve to *some* agent slug — returning
+None leaves an ownerless pending task dormant, because no dispatcher re-spawns
+an unrouted task. Tasks that can't be placed on a cell (no team, or a non-cell
+team like ``fullstack`` / ``system``) and any unrecognized routing fall back to
+main-pm, which triages them.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from roboco.runtime.orchestrator import AgentOrchestrator
+
+
+def _orch() -> AgentOrchestrator:
+    orch = object.__new__(AgentOrchestrator)
+    orch._instances = {}
+    return orch
+
+
+def _resolve(routing: str, team: str | None) -> str | None:
+    task: dict[str, Any] = {"id": "t1", "team": team}
+    return _orch()._get_routing_target(routing, task)
+
+
+# ---------------------------------------------------------------------------
+# Happy paths still resolve to the right agent
+# ---------------------------------------------------------------------------
+
+
+def test_dev_on_cell_team_selects_cell_agent() -> None:
+    assert _resolve("dev", "backend") == "be-dev-1"
+
+
+def test_board_routes_to_product_owner() -> None:
+    assert _resolve("board", None) == "product-owner"
+
+
+def test_main_pm_routes_to_main_pm() -> None:
+    assert _resolve("main_pm", None) == "main-pm"
+
+
+def test_cell_pm_on_team_routes_to_cell_pm() -> None:
+    assert _resolve("cell_pm", "frontend") == "fe-pm"
+
+
+def test_cell_pm_without_team_falls_back_to_main_pm() -> None:
+    assert _resolve("cell_pm", None) == "main-pm"
+
+
+# ---------------------------------------------------------------------------
+# Fallbacks — never None (no dormancy)
+# ---------------------------------------------------------------------------
+
+
+def test_dev_without_team_falls_back_to_main_pm() -> None:
+    assert _resolve("dev", None) == "main-pm"
+
+
+def test_dev_on_non_cell_team_falls_back_to_main_pm() -> None:
+    # fullstack / system are valid Team values with no cell agent pool.
+    assert _resolve("dev", "fullstack") == "main-pm"
+    assert _resolve("dev", "system") == "main-pm"
+
+
+def test_unknown_routing_falls_back_to_main_pm() -> None:
+    assert _resolve("frobnicate", "backend") == "main-pm"
+
+
+def test_no_routing_ever_returns_none() -> None:
+    """Every (routing, team) combination resolves to some agent — never None."""
+    routings = ["board", "main_pm", "marketing", "cell_pm", "dev", "bogus"]
+    teams: list[str | None] = [None, "backend", "fullstack", "system", "marketing"]
+    for routing in routings:
+        for team in teams:
+            assert _resolve(routing, team) is not None, (routing, team)
@@ -434,6 +434,44 @@ async def test_create_session_db(db_session: Any) -> None:
    assert session.agent_id == agent_id


+@pytest.mark.asyncio
+async def test_assignee_is_board_distinguishes_roles(db_session: Any) -> None:
+    """Drives product team routing: a board reviewer keeps the root on the board.
+
+    A product confirmed via "Board review & Start" is assigned to a board
+    reviewer and must stay team=board so the CEO's Approve & Start gate appears;
+    one assigned to main-pm (or a cell dev) is not a board task.
+    """
+    service = get_prompter_service(db=db_session)
+
+    def _agent(role: AgentRole) -> AgentTable:
+        return AgentTable(
+            id=uuid4(),
+            name="A",
+            slug=f"a-{uuid4().hex[:8]}",
+            role=role,
+            team=None,
+            status=AgentStatus.ACTIVE,
+            model_config={},
+            system_prompt="x",
+            capabilities=[],
+            permissions={},
+            metrics={},
+        )
+
+    po = _agent(AgentRole.PRODUCT_OWNER)
+    hom = _agent(AgentRole.HEAD_MARKETING)
+    dev = _agent(AgentRole.DEVELOPER)
+    db_session.add_all([po, hom, dev])
+    await db_session.flush()
+
+    assert await service._assignee_is_board(po.id) is True
+    assert await service._assignee_is_board(hom.id) is True
+    assert await service._assignee_is_board(dev.id) is False
+    # Unknown id is not a board agent — defensive, must not raise.
+    assert await service._assignee_is_board(uuid4()) is False
+
+
@pytest.mark.asyncio
 async def test_get_session_not_found(db_session: Any) -> None:
    """_get_session raises NotFoundError for unknown session ID."""