mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): covers_parent_criteria hint that teaches the shape; CEO pause/resume (#686)
* fix(gateway): teach the delegate remediate + PM prompt the covers_parent_criteria shape; allow CEO through the plain pause route - A child draft rejected for missing covers_parent_criteria now gets a copy-pasteable corrected skeleton with the parent's real criteria inlined, and the PM delegation guidance shows the field as part of every child draft — a PM no longer loops on a rejection that named the field but never showed the shape. - The plain pause route now authorizes the CEO tier like its sibling lifecycle routes; agent-side pause restrictions are unchanged. * fix(gateway): delegate-coverage hint heals and degrades on legacy parents - The coverage-reject path self-heals a criteria-bearing parent whose ids are empty or out of length before rendering the hint, so the skeleton always shows real references; the renderer itself also falls back to quoted criterion texts for any criterion without an id instead of emitting a placeholder or truncating the listing. - The remediate names both legal reference forms (id or exact text) again. - Route comments state the pause/resume check as deliberately CEO-only instead of claiming a precedent whose role set is wider. * test(gateway): real TaskTable rows in the remediation hint round-trips mypy over tests/ rejects a SimpleNamespace where unknown_ac_refs takes a TaskTable; instantiating the ORM row directly needs no session and types cleanly. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -134,7 +134,7 @@ Decomposition is where scope silently disappears. The failure mode: your cell-PM
|
||||
|
||||
**The rule: before you `i_am_idle()` after delegating, account for EVERY acceptance criterion on your cell-PM task.** Walk the list. For each criterion, name the subtask whose `acceptance_criteria` cover it. Four legal outcomes per criterion — and only four:
|
||||
|
||||
1. **Covered now** — a subtask you just delegated has an `acceptance_criteria` entry that satisfies it. Make the mapping **machine-explicit**: pass `covers_parent_criteria=[<criterion ids>]` on that `delegate` so the gateway records which of YOUR criteria the child owns. The criterion ids are in your briefing under `parent_ac_coverage` (each `{id, text, claimed, verified}`); the ones still without a home are listed in `unclaimed_parent_acs`. Phrase the child's criteria so a reader can also trace each back by eye.
|
||||
1. **Covered now** — a subtask you just delegated has an `acceptance_criteria` entry that satisfies it. Make the mapping **machine-explicit**: pass `covers_parent_criteria=[<criterion ids>]` on that `delegate` so the gateway records which of YOUR criteria the child owns. The criterion ids are in your briefing under `parent_ac_coverage` (each `{id, text, claimed, verified}`); the ones still without a home are listed in `unclaimed_parent_acs`. Phrase the child's criteria so a reader can also trace each back by eye. `covers_parent_criteria` is not an optional extra — it is part of the SAME `delegate` call as `title`/`assigned_to`/`acceptance_criteria`, e.g. `delegate(parent_task_id="<your-task>", title="Add rate-limit middleware", description="...", assigned_to="be-dev-1", team="backend", task_type="code", nature="technical", estimated_complexity="medium", acceptance_criteria=["429 returned past the configured limit"], covers_parent_criteria=["<id from your parent_ac_coverage>"], intends_to_touch=["roboco/api/middleware/*"])`. Whenever your cell-PM task has any acceptance criteria at all, `delegate` **rejects the call outright** when `covers_parent_criteria` is missing or names an id/text that isn't one of your task's own — the rejection's `remediate` echoes your real criteria ids inline so you copy the right one straight in instead of re-deriving it.
|
||||
2. **Covered later, in sequence** — it belongs to a follow-on subtask that runs after the current one. Delegate that follow-on **now too**, placed later in the same dev's queue (a dev can hold a queue), so the criterion is claimed immediately and simply builds in turn. Record the sequencing in your `decision` note ("criterion 7 → be-dev-1's second queue item, after the first lands") so the order is intentional and visible.
|
||||
3. **Out of scope for your cell** — it genuinely belongs to another cell or the Main PM aggregate. Say so in the `decision` note. Do not silently drop it.
|
||||
4. **Cell-owned** — only YOUR own machinery satisfies it (never a dev's), the same principle one level up applies here too: declare it root-owned on your own task, `declare_coverage(task_id=<your own cell-PM task>, criteria=[<ids>])`. Never put it in a dev's `acceptance_criteria` — a dev can't act outside their own branch/PR.
|
||||
@@ -143,7 +143,7 @@ A criterion that fits none of the three is dropped scope — you under-decompose
|
||||
|
||||
This is the same discipline the `submit_up` checklist enforces at the end — pulled to the front, where a gap costs one extra `delegate` instead of a full cell revision loop.
|
||||
|
||||
**The gateway now backs this up.** Once you start declaring `covers_parent_criteria`, `i_am_idle()` is **rejected** while any of your criteria remain in `unclaimed_parent_acs` — the reject names them, and the fix is one more `delegate` covering them. Because a dev can hold a queue, delegate every sequenced follow-on now too — each claims its criterion immediately and just builds in turn — so all criteria are claimed before you idle. Check `parent_ac_coverage` in the response after each `delegate`: when `unclaimed_parent_acs` is empty, your decomposition covers the task and you may idle. (Mapping coverage is opt-in by design — if you never pass `covers_parent_criteria`, the gate stays silent — but declaring it is the expected practice and the only way the cell self-checks for dropped scope.)
|
||||
**The gateway now backs this up.** Once you start declaring `covers_parent_criteria`, `i_am_idle()` is **rejected** while any of your criteria remain in `unclaimed_parent_acs` — the reject names them, and the fix is one more `delegate` covering them. Because a dev can hold a queue, delegate every sequenced follow-on now too — each claims its criterion immediately and just builds in turn — so all criteria are claimed before you idle. Check `parent_ac_coverage` in the response after each `delegate`: when `unclaimed_parent_acs` is empty, your decomposition covers the task and you may idle. (This `i_am_idle` self-check is the ONLY opt-in part — it only starts enforcing once some child has declared `covers_parent_criteria` at all. `delegate` itself is never opt-in: it rejects the call outright, every time, whenever your cell-PM task carries acceptance criteria and this field is missing — that check runs before any child exists, so don't wait for the `i_am_idle` gate to start declaring coverage.)
|
||||
|
||||
### Collision surface — declare it on every `code` subtask so siblings sequence (READ THIS BEFORE DELEGATING)
|
||||
|
||||
|
||||
@@ -138,7 +138,7 @@ Keep it to goal + constraints + the unit breakdown; the `acceptance_criteria` ab
|
||||
|
||||
**Forward intake's observed facts verbatim; re-articulate only the solution.** This is the most important rule at your seat and the single biggest source of revision churn when you get it wrong. The WHAT — the file:line targets the intake analysis named, the code examples it quoted, the exact enums/components/APIs/signatures to reuse, the constraints and gotchas it surfaced — is the PO/HoM intake's analysis, already done. Carry it into the cell subtask's `description` **word-for-word, not paraphrased into a thinner restatement**. The HOW — the solution shape, the decomposition, the layout — is what you and the Cell PM own; re-articulate that freely. "Do not prescribe the solution" scopes ONLY to the solution; it does **not** license you to flatten the intake's technical detail into a vague goal on the way down. A dev who receives "improve the intake flow" instead of "`PrompterService.confirm_live_batch` at `roboco/services/prompter.py:412` drops the `project_ids` scope on a redraft re-confirm — thread `BatchConfirmRequest.task_id` through `update_live_batch` and re-run `_validate_batch_scope`" has to rebuild the intake's analysis from scratch, usually gets it wrong, and burns a revision cycle you could have prevented by forwarding the line you already had. Mine your `evidence(root_id)` response and the upstream PO/HoM handoff for that detail — at the root, the intake analysis lives in the root's own `description` and the PO/HoM journal handoff (a root has no parent, so its `parent_context` is empty); `parent_context` carries the upstream chain once you've delegated, on the cell-PM subtasks and the dev leaves below them. Pass the detail straight through to every cell subtask. If the intake genuinely gave no technical detail (only a goal), say so in the `decision` note rather than inventing vague targets, and `dm('product-owner', ...)` to get it filled before you delegate.
|
||||
|
||||
**Map your root's criteria to the cell subtask that owns them.** Your briefing carries `parent_ac_coverage` (each root criterion as `{id, text, claimed, verified}`) and `unclaimed_parent_acs` (the ids with no cell subtask yet). When you `delegate` a slice to a cell, pass `covers_parent_criteria=[<root criterion ids>]` naming which root criteria that cell now owns — every root criterion must be claimed by some cell before you idle. Once you start declaring coverage, the gateway **rejects `i_am_idle()`** while `unclaimed_parent_acs` is non-empty, naming the gap; the fix is one more `delegate` to the cell that should own it. (Opt-in: if you never pass `covers_parent_criteria` the gate stays silent, but declaring it is how a dropped cross-cell criterion gets caught here instead of at the CEO.)
|
||||
**Map your root's criteria to the cell subtask that owns them.** Your briefing carries `parent_ac_coverage` (each root criterion as `{id, text, claimed, verified}`) and `unclaimed_parent_acs` (the ids with no cell subtask yet). When you `delegate` a slice to a cell, pass `covers_parent_criteria=[<root criterion ids>]` naming which root criteria that cell now owns — every root criterion must be claimed by some cell before you idle. `covers_parent_criteria` rides the SAME `delegate` call as `assigned_to`/`team`/`task_type`, e.g. `delegate(parent_task_id="<your-root>", title="Backend: rate limiting", description="...", assigned_to="be-pm", team="backend", task_type="planning", nature="technical", estimated_complexity="medium", acceptance_criteria=["..."], covers_parent_criteria=["<id from your parent_ac_coverage>"])` — it is not a field you add later. Whenever your root has any acceptance criteria at all, `delegate` **rejects the call outright** when `covers_parent_criteria` is missing or names an id/text that isn't one of your root's own; the rejection's `remediate` echoes your real criteria ids inline so you copy the right one straight in. Separately, once you start declaring coverage, the gateway **rejects `i_am_idle()`** while `unclaimed_parent_acs` is non-empty, naming the gap; the fix is one more `delegate` to the cell that should own it. (That `i_am_idle` self-check is the only opt-in part of this — it activates once some cell has declared coverage at all; `delegate`'s own rejection above is never opt-in.)
|
||||
|
||||
**Some root criteria are yours alone — never delegate them.** A criterion satisfiable only by your own machinery (e.g. "a PR is opened from `feature/main_pm/...`", "contributor PR #N is closed and linked") cannot be honored by any cell — a cell can't operate in your branch namespace or close a PR it doesn't own. Do NOT push it into a cell's `acceptance_criteria` or `covers_parent_criteria`; declare it root-owned instead: `declare_coverage(task_id=<your own root>, criteria=[<ids>])`. `parent_ac_coverage` then shows `claimed_by: "root"` for it, and it counts as claimed+satisfied for `i_am_idle` and the roll-up gate — no cell involved.
|
||||
6. `i_am_idle()` -> wait. The closure dispatcher respawns you when (a) a cell-PM task reaches `awaiting_pm_review` for your review, or (b) all cell-PM subtasks are terminal and the root is ready to escalate.
|
||||
|
||||
@@ -1629,11 +1629,16 @@ async def pause_task(
|
||||
status_code=status.HTTP_404_NOT_FOUND, detail="Task not found"
|
||||
)
|
||||
|
||||
# Only assigned agent can pause their task
|
||||
if task.assigned_to != agent.agent_id:
|
||||
# Only the assigned agent or the CEO can pause a task. The lifecycle
|
||||
# spec's in_progress->paused transition carries no role restriction of
|
||||
# its own (enforced upstream by the gateway's flow verbs, which never
|
||||
# expose pause to agents at all) — this route is the sole gate, and the
|
||||
# CEO carve-out here is deliberately narrower than unblock/block's
|
||||
# (assignee-or-{CELL_PM, MAIN_PM, CEO}): pause has no PM-role carve-out.
|
||||
if task.assigned_to != agent.agent_id and agent.role != AgentRole.CEO:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail="Only the assigned agent can pause this task",
|
||||
detail="Only the assigned agent or the CEO can pause this task",
|
||||
)
|
||||
|
||||
task = await service.pause(task_id, agent.role)
|
||||
@@ -1661,11 +1666,13 @@ async def resume_task(
|
||||
status_code=status.HTTP_404_NOT_FOUND, detail="Task not found"
|
||||
)
|
||||
|
||||
# Only assigned agent can resume their task
|
||||
if task.assigned_to != agent.agent_id:
|
||||
# Only the assigned agent or the CEO can resume a task — same carve-out
|
||||
# as pause above, so a CEO who paused a task through the front door can
|
||||
# also resume it through the front door.
|
||||
if task.assigned_to != agent.agent_id and agent.role != AgentRole.CEO:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail="Only the assigned agent can resume this task",
|
||||
detail="Only the assigned agent or the CEO can resume this task",
|
||||
)
|
||||
|
||||
task = await service.resume(task_id, agent.role)
|
||||
|
||||
@@ -51,6 +51,7 @@ from roboco.services.gateway.evidence_builder import (
|
||||
from roboco.services.gateway.merge_chain import resolve_parent_branch
|
||||
from roboco.services.gateway.remediation import (
|
||||
hint_for_evidence_not_inspected,
|
||||
hint_for_missing_ac_coverage,
|
||||
hint_for_missing_doc_files,
|
||||
hint_for_missing_journal_decision,
|
||||
hint_for_missing_journal_learning,
|
||||
@@ -1010,6 +1011,7 @@ class Choreographer:
|
||||
"unclaimed_parent_acs": [
|
||||
c["id"] for c in coverage if not c["claimed"]
|
||||
],
|
||||
"delegate_hint": self._COVERS_PARENT_CRITERIA_HINT,
|
||||
}
|
||||
return briefing
|
||||
|
||||
@@ -5433,9 +5435,9 @@ class Choreographer:
|
||||
)
|
||||
if guard is not None:
|
||||
return guard
|
||||
return self._delegate_ac_coverage_guard(parent, inputs)
|
||||
return await self._delegate_ac_coverage_guard(parent, inputs)
|
||||
|
||||
def _delegate_ac_coverage_guard(
|
||||
async def _delegate_ac_coverage_guard(
|
||||
self, parent: Any, inputs: DelegateInputs
|
||||
) -> Envelope | None:
|
||||
"""Reject a child that doesn't map to the parent's own criteria.
|
||||
@@ -5452,6 +5454,12 @@ class Choreographer:
|
||||
coverage in one call: a wave may deliberately leave criteria for a
|
||||
later delegate (see the success envelope's ``parent_ac_coverage``
|
||||
evidence for that signal).
|
||||
|
||||
On reject, self-heals a legacy/drifted parent's
|
||||
``acceptance_criteria_ids`` in place before rendering the hint (same
|
||||
touchpoint ``uncovered_parent_acceptance_criteria`` et al. reach via
|
||||
``_parent_ac_ref_sets``) — otherwise a criteria-bearing parent with
|
||||
empty ids renders a ``'<id>'`` placeholder the PM can't act on.
|
||||
"""
|
||||
ac_texts = parent.acceptance_criteria or []
|
||||
if not ac_texts:
|
||||
@@ -5460,7 +5468,7 @@ class Choreographer:
|
||||
bad = self.task.unknown_ac_refs(parent, refs) if refs else []
|
||||
if refs and not bad:
|
||||
return None
|
||||
listing = "; ".join(ac_texts)
|
||||
await self.task.self_heal_ac_ids(parent)
|
||||
if not refs:
|
||||
message = (
|
||||
f"'{inputs.title}' declares no covers_parent_criteria, but the "
|
||||
@@ -5473,10 +5481,10 @@ class Choreographer:
|
||||
)
|
||||
return Envelope.invalid_state(
|
||||
message=message,
|
||||
remediate=(
|
||||
"Map this subtask to the parent criteria it advances via "
|
||||
"covers_parent_criteria (by id or exact text), or fix the "
|
||||
f"parent's criteria first. Parent criteria: {listing}"
|
||||
remediate=hint_for_missing_ac_coverage(
|
||||
ids=parent.acceptance_criteria_ids or [],
|
||||
texts=ac_texts,
|
||||
title=inputs.title,
|
||||
),
|
||||
context_briefing={},
|
||||
)
|
||||
@@ -5485,6 +5493,19 @@ class Choreographer:
|
||||
# Soft warn at 8, hard block at 13. Cap enforced by ``_subtask_cap_guard``.
|
||||
_SUBTASK_HARD_CAP: int = 12
|
||||
|
||||
# Proactive nudge surfaced alongside ``parent_ac_coverage`` in the
|
||||
# planning briefing and the delegate success envelope, ahead of any
|
||||
# rejection — delegate() enforces covers_parent_criteria on every call
|
||||
# once the parent has acceptance criteria, it is NOT deferred to
|
||||
# i_am_idle's separate, opt-in unclaimed-parent-acs self-check (a PM
|
||||
# reading only that gate's docs can otherwise assume the mapping is
|
||||
# optional and loop on the delegate-time rejection for hours).
|
||||
_COVERS_PARENT_CRITERIA_HINT: ClassVar[str] = (
|
||||
"every delegate() call under this parent must pass "
|
||||
"covers_parent_criteria=[<one or more ids from parent_ac_coverage>] "
|
||||
"— it is enforced right now, on this call, not deferred to i_am_idle."
|
||||
)
|
||||
|
||||
async def _delegate_extra_guards(
|
||||
self,
|
||||
pm_agent_id: UUID,
|
||||
@@ -6232,6 +6253,7 @@ class Choreographer:
|
||||
**briefing,
|
||||
"parent_ac_coverage": coverage,
|
||||
"unclaimed_parent_acs": [c["id"] for c in coverage if not c["claimed"]],
|
||||
"delegate_hint": self._COVERS_PARENT_CRITERIA_HINT,
|
||||
}
|
||||
return Envelope.ok(
|
||||
status="created",
|
||||
|
||||
@@ -33,6 +33,36 @@ def hint_for_unaddressed_acceptance_criteria(
|
||||
)
|
||||
|
||||
|
||||
def hint_for_missing_ac_coverage(
|
||||
*, ids: list[str], texts: list[str], title: str
|
||||
) -> str:
|
||||
"""`ids`/`texts`: the parent's own acceptance-criteria ids and texts, in
|
||||
declaration order — kept separate rather than pre-paired, so a criterion
|
||||
with no id at its index (a legacy/drifted parent the caller didn't heal)
|
||||
still gets a real reference: its own quoted text, the other legal
|
||||
``covers_parent_criteria`` form. Never renders an ``'<id>'`` placeholder
|
||||
when a real reference exists, and never drops a criterion past a
|
||||
shorter `ids` list.
|
||||
|
||||
Shows the exact corrected call shape with a real inlined reference so a
|
||||
PM fixing a rejected delegate can copy it verbatim instead of
|
||||
re-deriving the field's syntax or retyping criterion text (fragile —
|
||||
exact-text matching breaks on any punctuation drift).
|
||||
"""
|
||||
refs = [ids[i] if i < len(ids) else text for i, text in enumerate(texts)]
|
||||
example_ref = refs[0] if refs else "<id>"
|
||||
mapping = "; ".join(
|
||||
f"{ref!r}" if ref == text else f'{ref}="{text}"'
|
||||
for ref, text in zip(refs, texts, strict=True)
|
||||
)
|
||||
return (
|
||||
f"delegate(title={title!r}, ..., covers_parent_criteria=[{example_ref!r}]) "
|
||||
"— list the id(s) or exact text(s) of the parent criteria this child "
|
||||
"actually advances (one or more, not necessarily all of them). "
|
||||
f"Parent criteria (ref=text): {mapping}."
|
||||
)
|
||||
|
||||
|
||||
def hint_for_open_findings(*, finding_ids: list[str], task_id: str) -> str:
|
||||
ids = ", ".join(finding_ids)
|
||||
return (
|
||||
|
||||
@@ -9694,14 +9694,15 @@ class TaskService(BaseService):
|
||||
statuses = result.scalars().all()
|
||||
return all(s in terminal for s in statuses)
|
||||
|
||||
async def _self_heal_ac_ids(self, parent: TaskTable) -> None:
|
||||
async def self_heal_ac_ids(self, parent: TaskTable) -> None:
|
||||
"""Re-stamp ``acceptance_criteria_ids`` in place when it's empty or out
|
||||
of length with ``acceptance_criteria`` -- a legacy row from before every
|
||||
AC rewrite reconciled ids (``TaskService.update``), or any other drift.
|
||||
No-op when already 1:1. Reconciling against the row's own current
|
||||
criteria means any id a child already references by matching TEXT
|
||||
survives; the parent-coverage gate is live again instead of skipped
|
||||
forever (``_parent_ac_ref_sets``).
|
||||
forever (``_parent_ac_ref_sets``). Public: also called directly by the
|
||||
delegate-coverage guard so its rejection hint always has real ids.
|
||||
"""
|
||||
if len(parent.acceptance_criteria_ids or []) == len(parent.acceptance_criteria):
|
||||
return
|
||||
@@ -9737,12 +9738,12 @@ class TaskService(BaseService):
|
||||
A parent whose ``acceptance_criteria_ids`` is empty or out of length
|
||||
with ``acceptance_criteria`` (a legacy row from before every AC
|
||||
rewrite reconciled ids — or any other drift) self-heals via
|
||||
``_self_heal_ac_ids`` rather than silently disabling coverage.
|
||||
``self_heal_ac_ids`` rather than silently disabling coverage.
|
||||
"""
|
||||
parent = await self.get(task_id)
|
||||
if not parent or not parent.acceptance_criteria:
|
||||
return None
|
||||
await self._self_heal_ac_ids(parent)
|
||||
await self.self_heal_ac_ids(parent)
|
||||
result = await self.session.execute(
|
||||
select(TaskTable.status, TaskTable.parent_ac_refs).where(
|
||||
TaskTable.parent_task_id == task_id
|
||||
|
||||
@@ -2163,6 +2163,37 @@ async def test_resume_task_success(task_client: dict) -> None:
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pause_task_ceo_success(task_client: dict) -> None:
|
||||
"""The CEO can pause a task assigned to someone else through the plain
|
||||
pause route (a non-assignee, non-CEO caller still gets 403 —
|
||||
``test_pause_task_forbidden`` covers that unchanged)."""
|
||||
other = await _seed_agent(task_client)
|
||||
task = _seed_task(task_client, status=TaskStatus.IN_PROGRESS, assigned_to=other.id)
|
||||
await task_client["db"].flush()
|
||||
_as_ceo(task_client)
|
||||
response = await task_client["client"].post(
|
||||
f"/api/tasks/{task.id}/pause", headers=_HDR
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
assert response.json()["status"] == "paused"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resume_task_ceo_success(task_client: dict) -> None:
|
||||
"""The CEO can resume a task assigned to someone else through the plain
|
||||
resume route — same carve-out as pause above."""
|
||||
other = await _seed_agent(task_client)
|
||||
task = _seed_task(task_client, status=TaskStatus.PAUSED, assigned_to=other.id)
|
||||
await task_client["db"].flush()
|
||||
_as_ceo(task_client)
|
||||
response = await task_client["client"].post(
|
||||
f"/api/tasks/{task.id}/resume", headers=_HDR
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
assert response.json()["status"] != "paused"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_verify_task_success(task_client: dict) -> None:
|
||||
task = _seed_task(
|
||||
|
||||
@@ -12,16 +12,31 @@ before any subtask is created.
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import UTC, datetime
|
||||
from typing import Any
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.db.tables import AgentTable, ProjectTable, TaskTable
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
AgentStatus,
|
||||
Complexity,
|
||||
TaskNature,
|
||||
TaskStatus,
|
||||
TaskType,
|
||||
Team,
|
||||
)
|
||||
from roboco.services.gateway.choreographer import (
|
||||
Choreographer,
|
||||
ChoreographerDeps,
|
||||
DelegateInputs,
|
||||
)
|
||||
from roboco.services.task import get_task_service
|
||||
from sqlalchemy import select
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
|
||||
def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
@@ -211,3 +226,82 @@ async def test_delegate_wave_leaving_acs_uncovered_still_succeeds() -> None:
|
||||
assert coverage["covered"] == ["Criterion A"]
|
||||
assert coverage["uncovered"] == ["Criterion B", "Criterion C"]
|
||||
task_svc.create_subtask.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ac_coverage_guard_heals_empty_ids_parent_in_place(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""A criteria-bearing parent whose ``acceptance_criteria_ids`` is empty
|
||||
(a legacy row from before every AC rewrite reconciled ids) is
|
||||
self-healed to 1:1 by the reject path itself, against a real DB row —
|
||||
not just papered over in the rendered hint. Regression coverage for the
|
||||
adversarial finding on commit d259476b: the pre-fix guard rendered a
|
||||
literal ``'<id>'`` placeholder and an empty criteria listing on exactly
|
||||
this row shape, re-rejecting a PM who copy-pasted it verbatim."""
|
||||
agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name="PM",
|
||||
slug=f"pm-{uuid4().hex[:8]}",
|
||||
role=AgentRole.CELL_PM,
|
||||
team=Team.BACKEND,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="pm",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(agent)
|
||||
await db_session.flush()
|
||||
project = ProjectTable(
|
||||
id=uuid4(),
|
||||
name="P",
|
||||
slug=f"p-{uuid4().hex[:6]}",
|
||||
git_url="https://example.com/r.git",
|
||||
assigned_cell=Team.BACKEND,
|
||||
created_by=agent.id,
|
||||
)
|
||||
db_session.add(project)
|
||||
await db_session.flush()
|
||||
tid = uuid4()
|
||||
db_session.add(
|
||||
TaskTable(
|
||||
id=tid,
|
||||
title="parent",
|
||||
description="d",
|
||||
acceptance_criteria=["Criterion A", "Criterion B"],
|
||||
acceptance_criteria_ids=[],
|
||||
status=TaskStatus.IN_PROGRESS,
|
||||
priority=2,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.LOW,
|
||||
team=Team.BACKEND,
|
||||
confirmed_by_human=True,
|
||||
project_id=project.id,
|
||||
created_by=agent.id,
|
||||
branch_name="feature/x",
|
||||
assigned_to=agent.id,
|
||||
)
|
||||
)
|
||||
await db_session.flush()
|
||||
task_svc = get_task_service(db_session)
|
||||
parent = await task_svc.get(tid)
|
||||
assert parent is not None
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c._delegate_ac_coverage_guard(parent, _inputs(title="Orphan slice"))
|
||||
|
||||
assert env is not None
|
||||
body = env.as_dict()
|
||||
assert "'<id>'" not in body["remediate"]
|
||||
assert "Criterion A" in body["remediate"]
|
||||
assert "Criterion B" in body["remediate"]
|
||||
|
||||
row = (
|
||||
await db_session.execute(select(TaskTable).where(TaskTable.id == tid))
|
||||
).scalar_one()
|
||||
assert len(row.acceptance_criteria_ids) == len(row.acceptance_criteria)
|
||||
assert len(set(row.acceptance_criteria_ids)) == len(row.acceptance_criteria_ids)
|
||||
|
||||
@@ -2,11 +2,14 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from roboco.db.tables import TaskTable
|
||||
from roboco.services.gateway.remediation import (
|
||||
hint_for_missing_ac_coverage,
|
||||
hint_for_missing_progress,
|
||||
hint_for_missing_reflect,
|
||||
hint_for_unaddressed_acceptance_criteria,
|
||||
)
|
||||
from roboco.services.task import TaskService
|
||||
|
||||
|
||||
def test_missing_progress_hint() -> None:
|
||||
@@ -27,3 +30,61 @@ def test_unaddressed_criteria_hint() -> None:
|
||||
assert "criterion 1" in h
|
||||
assert "criterion 3" in h
|
||||
assert "t-1" in h
|
||||
|
||||
|
||||
def test_missing_ac_coverage_hint_shows_call_shape_and_real_ids() -> None:
|
||||
h = hint_for_missing_ac_coverage(
|
||||
ids=["id-a", "id-b"],
|
||||
texts=["Criterion A", "Criterion B"],
|
||||
title="Orphan slice",
|
||||
)
|
||||
assert "delegate(title='Orphan slice'" in h
|
||||
assert "covers_parent_criteria=['id-a']" in h
|
||||
assert 'id-a="Criterion A"' in h
|
||||
assert 'id-b="Criterion B"' in h
|
||||
|
||||
|
||||
def test_missing_ac_coverage_hint_names_both_legal_reference_forms() -> None:
|
||||
"""The remediate names both legal ``covers_parent_criteria`` forms —
|
||||
a criterion's id or its exact text — not just id."""
|
||||
h = hint_for_missing_ac_coverage(ids=["id-a"], texts=["Criterion A"], title="X")
|
||||
assert "id(s) or exact text(s)" in h
|
||||
|
||||
|
||||
def test_missing_ac_coverage_hint_handles_no_criteria() -> None:
|
||||
h = hint_for_missing_ac_coverage(ids=[], texts=[], title="X")
|
||||
assert "<id>" in h
|
||||
|
||||
|
||||
def test_missing_ac_coverage_hint_empty_ids_uses_quoted_text() -> None:
|
||||
"""A legacy/unhealed parent whose ids are empty must still get a real,
|
||||
copy-pasteable reference for every criterion — never a `'<id>'`
|
||||
placeholder, and never a truncated listing."""
|
||||
texts = ["Criterion A", "Criterion B", "Criterion C"]
|
||||
h = hint_for_missing_ac_coverage(ids=[], texts=texts, title="Orphan slice")
|
||||
|
||||
assert "'<id>'" not in h
|
||||
for text in texts:
|
||||
assert text in h
|
||||
|
||||
parent = TaskTable(acceptance_criteria_ids=[], acceptance_criteria=texts)
|
||||
for text in texts:
|
||||
assert TaskService.unknown_ac_refs(parent, [text]) == []
|
||||
|
||||
|
||||
def test_missing_ac_coverage_hint_drifted_ids_shorter_than_criteria() -> None:
|
||||
"""1 id against 3 criteria: every criterion is listed (id for the
|
||||
first, quoted text for the rest) — zip's `strict=False` used to drop
|
||||
the tail criteria silently."""
|
||||
texts = ["Criterion A", "Criterion B", "Criterion C"]
|
||||
h = hint_for_missing_ac_coverage(ids=["id-a"], texts=texts, title="Orphan slice")
|
||||
|
||||
assert "'<id>'" not in h
|
||||
assert 'id-a="Criterion A"' in h
|
||||
for text in texts[1:]:
|
||||
assert text in h
|
||||
|
||||
parent = TaskTable(acceptance_criteria_ids=["id-a"], acceptance_criteria=texts)
|
||||
assert TaskService.unknown_ac_refs(parent, ["id-a"]) == []
|
||||
for text in texts[1:]:
|
||||
assert TaskService.unknown_ac_refs(parent, [text]) == []
|
||||
|
||||
Reference in New Issue
Block a user