diff --git a/roboco/api/schemas/tasks.py b/roboco/api/schemas/tasks.py index 932b166e..2ac6c2a5 100644 --- a/roboco/api/schemas/tasks.py +++ b/roboco/api/schemas/tasks.py @@ -532,7 +532,11 @@ def convert_plan(plan_data: dict | None) -> TaskPlanResponse | None: def _coerce_risk(r: object) -> dict[str, str]: if not isinstance(r, dict): - return {"description": str(r) if r else "", "mitigation": "", "severity": "medium"} + return { + "description": str(r) if r else "", + "mitigation": "", + "severity": "medium", + } sev = r.get("severity") return { "description": str(r.get("description") or r.get("risk") or ""), diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index ff0d2dc7..02e326c7 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1696,15 +1696,54 @@ class Choreographer: "cancelled) before this transition; wait for the closure " "dispatcher to bring you back when ready." ), + "journal:during_work>=1": ( + "no journal:decision / :learning / :struggle entry exists " + "for this task yet. Pre-gateway parity: developers write " + "at least one work-progress journal entry before submit. " + f"Call note(scope='decision'|'learning'|'struggle', " + f"task_id='{tid}', text='') " + "with a substantive entry, then retry i_am_done. " + "NOTE: scope='reflect' does NOT count for this requirement — " + "reflect is the post-work summary; during_work demands " + "an entry written while the work was happening." + ), + "journal:struggle": ( + f"call note(scope='struggle', task_id='{tid}', " + "text='') with the blocker details, " + "then retry." + ), + "commits>=1": ( + "no commits linked to this task yet. Use commit(message=...) " + "to record your changes, then retry." + ), + "pr_open": ( + "no PR has been opened for this task. Call open_pr() to " + "push the branch + open the PR, then retry." + ), + "self_verified": ( + "task has not been self-verified. i_am_done normally runs " + "submit_verification automatically; if you see this gap, " + "retry i_am_done after the previous call returned." + ), } return simple_hints.get(missing_key) async def _build_tracing_gap( self, agent_id: UUID, task_id: UUID, missing: list[str] ) -> Envelope: - """Translate missing requirement keys into agent-facing hints.""" + """Translate missing requirement keys into agent-facing hints. + + Task #159: multi-missing remediate uses a numbered list so the + agent sees each requirement as a distinct step instead of a + single semicolon-joined sentence the model parses as one + instruction. Each missing key with no hint in + ``_hint_for_missing_key`` still surfaces as a literal + ``missing[]`` entry (defense-in-depth: the agent gets at least + the key name even if no hint is registered). + """ hints: list[str] = [] unaddressed: list[str] = [] + unhinted: list[str] = [] for m in missing: if m.startswith("acceptance_criterion:"): unaddressed.append(m.split(":", 1)[1]) @@ -1712,6 +1751,8 @@ class Choreographer: hint = self._hint_for_missing_key(m, task_id) if hint is not None: hints.append(hint) + else: + unhinted.append(m) if unaddressed: hints.append( hint_for_unaddressed_acceptance_criteria( @@ -1719,9 +1760,25 @@ class Choreographer: task_id=str(task_id), ) ) + # Fallback hints for missing keys without a registered hint — + # the agent at least sees the literal token instead of nothing. + for token in unhinted: + hints.append( + f"requirement {token!r} not satisfied — see lifecycle docs " + f"or escalate via i_am_blocked if you do not know how to " + f"satisfy this." + ) + if len(hints) <= 1: + remediate = hints[0] if hints else "" + else: + numbered = "\n".join(f"{i + 1}. {h}" for i, h in enumerate(hints)) + remediate = ( + f"Multiple requirements missing — address ALL of the " + f"following before retrying:\n{numbered}" + ) return Envelope.tracing_gap( missing=missing, - remediate=" ; ".join(hints), + remediate=remediate, context_briefing=await self._briefing_for(agent_id, task_id), ) @@ -2563,14 +2620,38 @@ class Choreographer: context_briefing={}, ) + @staticmethod + def _is_cross_team_planning(new_type: str, new_team: str, sib_team: str) -> bool: + """Task #157: planning subtasks on different teams are NOT + over-decomposition — main_pm fans planning out to per-cell PMs. + Both teams must be non-empty so an empty-team escape hatch can't + bypass the cap defensively. + """ + return ( + new_type == "planning" + and bool(new_team) + and bool(sib_team) + and new_team != sib_team + ) + @classmethod def _sibling_dup_envelope( - cls, sibling: Any, new_type: str, new_assignee: str + cls, sibling: Any, new_type: str, new_team: str, new_assignee: str ) -> Envelope | None: - """Apply Rule-1 then Rule-2 against one non-terminal sibling.""" + """Apply Rule-1 then Rule-2 against one non-terminal sibling. + + ``code`` / ``documentation`` stay capped regardless of team — + a single repo on one branch shouldn't have two simultaneous code + subtasks. ``planning`` allows cross-team fanout (see + :meth:`_is_cross_team_planning`). + """ sib_type = str(getattr(sibling, "task_type", "")) + sib_team = str(getattr(sibling, "team", "") or "") sib_assignee = str(getattr(sibling, "assigned_to", "") or "") - if new_type in cls._SPINE_TASK_TYPES and sib_type == new_type: + same_spine_type = new_type in cls._SPINE_TASK_TYPES and sib_type == new_type + if same_spine_type and not cls._is_cross_team_planning( + new_type, new_team, sib_team + ): return cls._spine_type_dup_envelope(new_type, sibling, sib_assignee) if sib_assignee and sib_assignee == new_assignee and sib_type == new_type: return cls._same_assignee_dup_envelope(new_type, new_assignee, sibling) @@ -2588,6 +2669,9 @@ class Choreographer: 1. **Same-type concurrency cap**: a parent may have AT MOST one non-terminal subtask of types ``code`` / ``planning`` / ``documentation`` at any given time, regardless of assignee. + Task #157 exception: ``planning`` subtasks on different + teams are allowed in parallel — that's main_pm's legitimate + cross-cell fanout. 2. **Same-assignee same-type** (fallback): a PM never delegates two ``research``/``design``/``administrative`` subtasks to the same agent under the same parent. @@ -2597,11 +2681,14 @@ class Choreographer: """ siblings = await self.task.get_subtasks(parent_task_id) new_type = str(inputs.task_type or "") + new_team = str(inputs.team or "") new_assignee = str(inputs.assigned_to or "") for sibling in siblings: if str(getattr(sibling, "status", "")) in self._TERMINAL_STATUSES: continue - envelope = self._sibling_dup_envelope(sibling, new_type, new_assignee) + envelope = self._sibling_dup_envelope( + sibling, new_type, new_team, new_assignee + ) if envelope is not None: return envelope return None diff --git a/tests/unit/gateway/test_spine_cap_cross_team.py b/tests/unit/gateway/test_spine_cap_cross_team.py new file mode 100644 index 00000000..a2081819 --- /dev/null +++ b/tests/unit/gateway/test_spine_cap_cross_team.py @@ -0,0 +1,128 @@ +"""Task #157: spine-cap allows cross-team planning fanout. + +Pre-fix: + main_pm delegates a planning subtask to be-pm (backend cell). When + it then tries to delegate a second planning subtask to fe-pm + (frontend cell), the spine-cap rejects because there's already a + non-terminal task_type='planning' under the parent. Pre-gateway + allowed this parallel cross-cell pattern; the gateway over-applied + the over-decomposition cap. + +Fix: + `_sibling_dup_envelope` skips the spine-cap when: + - new task_type == "planning" + - new team != sibling team (both non-empty) + Other combinations stay capped: + - same-team planning: still over-decomposition (real bug) + - code / documentation regardless of team: a single repo on one + branch shouldn't have two simultaneous code subtasks +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from roboco.services.gateway.choreographer._impl import Choreographer + + +def _sibling(*, task_type: str, team: str, status: str = "pending") -> MagicMock: + sib = MagicMock() + sib.id = "11111111-aaaa-bbbb-cccc-dddddddddddd" + sib.status = status + sib.task_type = task_type + sib.team = team + sib.assigned_to = "some-pm" + return sib + + +def test_cross_team_planning_fanout_is_allowed() -> None: + """main-pm: planning→be-pm (backend) exists; planning→fe-pm (frontend) must pass.""" + sib = _sibling(task_type="planning", team="backend") + env = Choreographer._sibling_dup_envelope( + sibling=sib, + new_type="planning", + new_team="frontend", + new_assignee="fe-pm", + ) + assert env is None, ( + "Cross-team planning fanout (backend↔frontend) must NOT be rejected. " + f"Got envelope: {env}" + ) + + +def test_cross_team_planning_third_cell_also_allowed() -> None: + """Third cell (ux_ui) is also a valid cross-team planning fanout target.""" + sib = _sibling(task_type="planning", team="backend") + env = Choreographer._sibling_dup_envelope( + sibling=sib, + new_type="planning", + new_team="ux_ui", + new_assignee="ux-pm", + ) + assert env is None, env + + +def test_same_team_planning_still_rejected() -> None: + """Two planning subtasks on the SAME team is the real over-decomp pattern — + must still be blocked by the spine-cap.""" + sib = _sibling(task_type="planning", team="backend") + env = Choreographer._sibling_dup_envelope( + sibling=sib, + new_type="planning", + new_team="backend", + new_assignee="be-pm", + ) + assert env is not None + body = env.as_dict() + assert body["error"] == "invalid_state", body + + +def test_cross_team_code_still_rejected() -> None: + """Code subtasks stay capped regardless of team — only one code task per + parent at a time. (Cross-team code under one parent is meaningless; + each cell's code work lives under its own cell-PM planning task.)""" + sib = _sibling(task_type="code", team="backend") + env = Choreographer._sibling_dup_envelope( + sibling=sib, + new_type="code", + new_team="frontend", + new_assignee="fe-dev-1", + ) + assert env is not None + body = env.as_dict() + assert body["error"] == "invalid_state", body + + +def test_cross_team_documentation_still_rejected() -> None: + """Documentation subtasks stay capped regardless of team — single doc + pass per parent.""" + sib = _sibling(task_type="documentation", team="backend") + env = Choreographer._sibling_dup_envelope( + sibling=sib, + new_type="documentation", + new_team="frontend", + new_assignee="fe-doc", + ) + assert env is not None + + +def test_planning_with_missing_team_still_rejected() -> None: + """If either side has no team attribute (defensive), fall back to the + strict cap. Don't let an empty-team escape hatch sneak past.""" + sib_no_team = _sibling(task_type="planning", team="") + env = Choreographer._sibling_dup_envelope( + sibling=sib_no_team, + new_type="planning", + new_team="backend", + new_assignee="be-pm", + ) + assert env is not None, "Empty team on sibling must NOT bypass the cap" + + sib = _sibling(task_type="planning", team="backend") + env2 = Choreographer._sibling_dup_envelope( + sibling=sib, + new_type="planning", + new_team="", + new_assignee="be-pm", + ) + assert env2 is not None, "Empty team on new task must NOT bypass the cap" diff --git a/tests/unit/gateway/test_tracing_gap_hints.py b/tests/unit/gateway/test_tracing_gap_hints.py new file mode 100644 index 00000000..ce28551c --- /dev/null +++ b/tests/unit/gateway/test_tracing_gap_hints.py @@ -0,0 +1,178 @@ +"""Task #159: tracing-gap remediate covers every missing requirement. + +Pre-fix: + `journal:during_work>=1` (and a handful of other tokens) had no entry + in `_hint_for_missing_key`'s `simple_hints` dict. When the + requirement was missing, the agent saw the token in `missing[]` but + the `remediate` string contained NO instruction for how to satisfy + it. The agent fixed the items with hints, retried, hit the same + rejection again on the unhinted token, and looped. + +Fix: + Register hints for the previously-unhinted tokens + (`journal:during_work>=1`, `journal:struggle`, `commits>=1`, + `pr_open`, `self_verified`). Multi-hint remediate switches to a + numbered list so the model sees each requirement as a distinct + instruction instead of a semicolon-blob. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock +from uuid import uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + +_MIN_HINT_LEN = 10 + + +def _build_choreographer() -> Choreographer: + deps = ChoreographerDeps( + task=AsyncMock(), + work_session=AsyncMock(), + git=AsyncMock(), + a2a=AsyncMock(), + journal=AsyncMock(), + audit=AsyncMock(), + evidence_repo=AsyncMock(), + messaging=AsyncMock(), + ) + repo = deps.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 = [] + return Choreographer(deps) + + +# --------------------------------------------------------------------------- +# Hint registration — every previously-unhinted token now has a hint +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "token", + [ + "journal:during_work>=1", + "journal:struggle", + "commits>=1", + "pr_open", + "self_verified", + ], +) +def test_hint_registered_for_previously_unhinted_token(token: str) -> None: + """Every requirement token must have a non-empty hint so the agent + knows how to satisfy it.""" + hint = Choreographer._hint_for_missing_key(token, uuid4()) + assert hint is not None, ( + f"requirement token {token!r} has no hint; agent will see the " + f"token in `missing[]` but no actionable instruction" + ) + assert len(hint) > _MIN_HINT_LEN, ( + f"hint for {token!r} is suspiciously short: {hint!r}" + ) + + +def test_during_work_hint_warns_reflect_doesnt_count() -> None: + """The during_work hint must explicitly say reflect doesn't satisfy + it — that's the exact confusion smoke-10's be-dev-1 hit.""" + hint = Choreographer._hint_for_missing_key("journal:during_work>=1", uuid4()) + assert hint is not None + lower = hint.lower() + assert "reflect" in lower and ( + "does not count" in lower + or "doesn't count" in lower + or "do not count" in lower + or "not count" in lower + ), f"during_work hint must warn that scope='reflect' doesn't satisfy this: {hint!r}" + + +# --------------------------------------------------------------------------- +# Multi-hint remediate — numbered list, not semicolon-joined +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_single_missing_remediate_is_plain_string() -> None: + """One missing item: remediate is the hint itself, no numbering.""" + c = _build_choreographer() + env = await c._build_tracing_gap(uuid4(), uuid4(), ["journal:reflect"]) + body = env.as_dict() + assert body["error"] == "tracing_gap" + assert body["missing"] == ["journal:reflect"] + assert "Multiple requirements missing" not in (body["remediate"] or "") + assert "1." not in (body["remediate"] or "") + + +@pytest.mark.asyncio +async def test_multi_missing_remediate_is_numbered_list() -> None: + """Multiple missing items: remediate must be a numbered list so the + agent treats each as a distinct step.""" + c = _build_choreographer() + env = await c._build_tracing_gap( + uuid4(), + uuid4(), + ["journal:reflect", "journal:during_work>=1", "commits>=1"], + ) + body = env.as_dict() + remediate = body["remediate"] or "" + assert "Multiple requirements missing" in remediate, remediate + assert "1." in remediate + assert "2." in remediate + assert "3." in remediate + # All three hints must be substring-present (proves each was emitted). + assert "reflect" in remediate.lower() + assert "during" in remediate.lower() or "decision" in remediate.lower() + assert "commit" in remediate.lower() + + +@pytest.mark.asyncio +async def test_unhinted_token_still_appears_in_remediate() -> None: + """If a future requirement is added without a hint (defense), the + agent at least sees the literal token in the remediate — not silently + dropped.""" + c = _build_choreographer() + env = await c._build_tracing_gap( + uuid4(), + uuid4(), + ["unknown_future_requirement"], + ) + body = env.as_dict() + remediate = body["remediate"] or "" + assert "unknown_future_requirement" in remediate, ( + f"unhinted token must surface in remediate as fallback. Got: {remediate!r}" + ) + + +@pytest.mark.asyncio +async def test_acceptance_criteria_grouped_into_one_hint() -> None: + """Acceptance criteria entries collapse into a single hint regardless + of how many criteria are missing (so 5 unaddressed criteria don't + produce 5 separate numbered items).""" + c = _build_choreographer() + task_id = uuid4() + env = await c._build_tracing_gap( + uuid4(), + task_id, + [ + "acceptance_criterion:Branch named correctly", + "acceptance_criterion:Commit prefix present", + "acceptance_criterion:PR opened", + ], + ) + body = env.as_dict() + remediate = body["remediate"] or "" + # All three criterion names should appear in the SAME hint section + # (collapsed into one hint, not three separate numbered items). + assert "Branch named correctly" in remediate + assert "Commit prefix present" in remediate + assert "PR opened" in remediate + # Single-hint case → no "Multiple requirements" preamble. + assert "Multiple requirements missing" not in remediate