refactor(content): move orchestration markers off quick_context to typed jsonb

This commit is contained in:
Renn F
2026-06-21 02:48:07 +02:00
parent 7e74112fcc
commit 0fba4eed01
21 changed files with 270 additions and 316 deletions
@@ -44,6 +44,9 @@ _LINE_MARKERS: dict[str, tuple[str, bool]] = {
"original_developer:": ("original_developer", False), "original_developer:": ("original_developer", False),
"documenter:": ("documenter", False), "documenter:": ("documenter", False),
"required_cells:": ("required_cells", True), "required_cells:": ("required_cells", True),
# pr_author was a write-only marker (no reader); park legacy values out of
# the human quick_context rather than leave them as panel soup.
"pr_author:": ("pr_author", False),
} }
# Whitespace-token ``key=value`` markers (may be space-appended onto a line). # Whitespace-token ``key=value`` markers (may be space-appended onto a line).
_TOKEN_MARKERS: dict[str, str] = { _TOKEN_MARKERS: dict[str, str] = {
+2 -2
View File
@@ -1361,7 +1361,7 @@ async def pass_qa(
# QA cannot review their own tasks (prevent self-review) # QA cannot review their own tasks (prevent self-review)
# Check against original developer stored in quick_context, not current assigned_to # Check against original developer stored in quick_context, not current assigned_to
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev and str(agent.agent_id) == original_dev: if original_dev and str(agent.agent_id) == original_dev:
audit = get_audit_service() audit = get_audit_service()
@@ -1442,7 +1442,7 @@ async def fail_qa(
# QA cannot review their own tasks (prevent self-review) # QA cannot review their own tasks (prevent self-review)
# Check against original developer stored in quick_context, not current assigned_to # Check against original developer stored in quick_context, not current assigned_to
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev and str(agent.agent_id) == original_dev: if original_dev and str(agent.agent_id) == original_dev:
raise HTTPException( raise HTTPException(
+9 -5
View File
@@ -33,20 +33,24 @@ DISMISSED = "dismissed"
def get_marker(task: HasMarkers, key: str, default: Any = None) -> Any: def get_marker(task: HasMarkers, key: str, default: Any = None) -> Any:
return (task.orchestration_markers or {}).get(key, default) om = getattr(task, "orchestration_markers", None)
if not isinstance(om, dict):
return default
return om.get(key, default)
def set_marker(task: HasMarkers, key: str, value: Any) -> None: def set_marker(task: HasMarkers, key: str, value: Any) -> None:
markers = dict(task.orchestration_markers or {}) om = getattr(task, "orchestration_markers", None)
markers = dict(om) if isinstance(om, dict) else {}
markers[key] = value markers[key] = value
task.orchestration_markers = markers task.orchestration_markers = markers
def clear_marker(task: HasMarkers, key: str) -> None: def clear_marker(task: HasMarkers, key: str) -> None:
current = task.orchestration_markers or {} om = getattr(task, "orchestration_markers", None)
if key not in current: if not isinstance(om, dict) or key not in om:
return return
markers = dict(current) markers = dict(om)
del markers[key] del markers[key]
task.orchestration_markers = markers or None task.orchestration_markers = markers or None
+2 -6
View File
@@ -8157,8 +8157,6 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
- original_developer if pr_created=False (tracked in - original_developer if pr_created=False (tracked in
quick_context as "original_developer:<uuid>") quick_context as "original_developer:<uuid>")
""" """
from roboco.services.task import extract_original_developer
# Fetch both `awaiting_documentation` and `claimed` because the # Fetch both `awaiting_documentation` and `claimed` because the
# doc's claim transitions status from awaiting_documentation → # doc's claim transitions status from awaiting_documentation →
# claimed. Without including `claimed` we'd miss tasks where doc # claimed. Without including `claimed` we'd miss tasks where doc
@@ -8170,7 +8168,7 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
for task in tasks: for task in tasks:
if self._is_task_handled_this_tick(task.get("id")): if self._is_task_handled_this_tick(task.get("id")):
continue continue
await self._doc_dispatch_one(client, task, extract_original_developer) await self._doc_dispatch_one(client, task)
async def _auto_assign_doc( async def _auto_assign_doc(
self, client: httpx.AsyncClient, task: dict[str, Any], team: str self, client: httpx.AsyncClient, task: dict[str, Any], team: str
@@ -8201,15 +8199,13 @@ Never `commit`, never write code, never run `git`. PMs coordinate.
self, self,
client: httpx.AsyncClient, client: httpx.AsyncClient,
task: dict[str, Any], task: dict[str, Any],
extract_original_developer: Any,
) -> None: ) -> None:
"""Process a single task for `_dispatch_doc_work`.""" """Process a single task for `_dispatch_doc_work`."""
team = task.get("team") team = task.get("team")
if team not in ["backend", "frontend", "ux_ui"]: if team not in ["backend", "frontend", "ux_ui"]:
return return
quick_context = task.get("quick_context") or "" dev_uuid = (task.get("orchestration_markers") or {}).get("original_developer")
dev_uuid = extract_original_developer(quick_context)
status = task.get("status") status = task.get("status")
# Only consider `claimed` tasks actually in the doc/PR parallel # Only consider `claimed` tasks actually in the doc/PR parallel
@@ -21,6 +21,7 @@ import structlog
from roboco.exceptions import MergeConflictError from roboco.exceptions import MergeConflictError
from roboco.foundation.policy import lifecycle as spec_module from roboco.foundation.policy import lifecycle as spec_module
from roboco.foundation.policy.content import markers
from roboco.services.gateway.choreographer._verb_runner import VerbRunner from roboco.services.gateway.choreographer._verb_runner import VerbRunner
from roboco.services.gateway.claim_guards import ( from roboco.services.gateway.claim_guards import (
already_active_guard, already_active_guard,
@@ -185,14 +186,7 @@ def _extract_original_developer(task: Any) -> str | None:
spec's self-review precondition (a documenter who is also the spec's self-review precondition (a documenter who is also the
original developer cannot self-doc). original developer cannot self-doc).
""" """
qc = getattr(task, "quick_context", None) or "" return markers.get_original_developer(task)
marker = "original_developer:"
if marker not in qc:
return None
tail = qc.split(marker, 1)[1].strip()
if not tail:
return None
return tail.split()[0] or None
@dataclass(frozen=True) @dataclass(frozen=True)
+2 -8
View File
@@ -40,6 +40,7 @@ from typing import TYPE_CHECKING, Any
from roboco.config import settings from roboco.config import settings
from roboco.foundation.policy import lifecycle as spec_module from roboco.foundation.policy import lifecycle as spec_module
from roboco.foundation.policy import tracing as _tr from roboco.foundation.policy import tracing as _tr
from roboco.foundation.policy.content import markers
from roboco.models.task import DocRef from roboco.models.task import DocRef
from roboco.services.gateway.envelope import Envelope from roboco.services.gateway.envelope import Envelope
from roboco.services.gateway.evidence_builder import build_evidence_for_task from roboco.services.gateway.evidence_builder import build_evidence_for_task
@@ -86,14 +87,7 @@ def _extract_original_developer(task: Any) -> str | None:
the spec's self-review block reads ``ctx.original_developer_slug`` the spec's self-review block reads ``ctx.original_developer_slug``
and the mixin builds the Context. and the mixin builds the Context.
""" """
qc = getattr(task, "quick_context", None) or "" return markers.get_original_developer(task)
marker = "original_developer:"
if marker not in qc:
return None
tail = qc.split(marker, 1)[1].strip()
if not tail:
return None
return tail.split()[0] or None
class DocMixin(_Base): class DocMixin(_Base):
+2 -8
View File
@@ -42,6 +42,7 @@ from typing import TYPE_CHECKING, Any
from roboco.config import settings from roboco.config import settings
from roboco.foundation.policy import lifecycle as spec_module from roboco.foundation.policy import lifecycle as spec_module
from roboco.foundation.policy import tracing as _tr from roboco.foundation.policy import tracing as _tr
from roboco.foundation.policy.content import markers
from roboco.services.gateway.envelope import Envelope from roboco.services.gateway.envelope import Envelope
from roboco.services.gateway.evidence_builder import build_evidence_for_task from roboco.services.gateway.evidence_builder import build_evidence_for_task
@@ -78,14 +79,7 @@ def _extract_original_developer(task: Any) -> str | None:
the spec's self-review block reads ``ctx.original_developer_slug`` the spec's self-review block reads ``ctx.original_developer_slug``
and the mixins build the Context. and the mixins build the Context.
""" """
qc = getattr(task, "quick_context", None) or "" return markers.get_original_developer(task)
marker = "original_developer:"
if marker not in qc:
return None
tail = qc.split(marker, 1)[1].strip()
if not tail:
return None
return tail.split()[0] or None
class QAMixin(_Base): class QAMixin(_Base):
+3 -2
View File
@@ -29,6 +29,7 @@ from typing import TYPE_CHECKING, cast
from roboco.config import settings from roboco.config import settings
from roboco.foundation import identity as _foundation from roboco.foundation import identity as _foundation
from roboco.foundation.policy.content import markers
from roboco.models.base import Complexity, TaskNature, TaskStatus, TaskType, Team from roboco.models.base import Complexity, TaskNature, TaskStatus, TaskType, Team
from roboco.services.base import BaseService from roboco.services.base import BaseService
from roboco.services.notification import NotificationService from roboco.services.notification import NotificationService
@@ -143,7 +144,7 @@ class SelfHealEngine(BaseService):
open_tasks = await task_svc.list_open_self_heal_tasks() open_tasks = await task_svc.list_open_self_heal_tasks()
open_fps: set[str] = set() open_fps: set[str] = set()
for existing in open_tasks: for existing in open_tasks:
fp = extract_self_heal_fingerprint(existing.quick_context) fp = extract_self_heal_fingerprint(existing)
if fp: if fp:
open_fps.add(fp) open_fps.add(fp)
open_count = len(open_tasks) open_count = len(open_tasks)
@@ -196,7 +197,7 @@ class SelfHealEngine(BaseService):
) )
# Carry the fingerprint so a later cycle sees this regression already # Carry the fingerprint so a later cycle sees this regression already
# has an open fix task (parsed by extract_self_heal_fingerprint). # has an open fix task (parsed by extract_self_heal_fingerprint).
task.quick_context = f"self_heal_fp={obs.fingerprint}" markers.set_self_heal_fingerprint(task, obs.fingerprint)
await self.session.flush() await self.session.flush()
open_fps.add(obs.fingerprint) open_fps.add(obs.fingerprint)
open_count += 1 open_count += 1
+83 -139
View File
@@ -32,6 +32,8 @@ from roboco.enforcement import (
validate_task_transition, validate_task_transition,
) )
from roboco.events import Event, EventType, get_event_bus from roboco.events import Event, EventType, get_event_bus
from roboco.foundation.policy.content import markers
from roboco.foundation.policy.content.validators import ContentValidationError
from roboco.models.base import ( from roboco.models.base import (
AgentRole, AgentRole,
AgentStatus, AgentStatus,
@@ -54,6 +56,7 @@ from roboco.services.base import (
UnauthorizedError, UnauthorizedError,
ValidationError, ValidationError,
) )
from roboco.services.content_notes import apply_structured_note
from roboco.utils.converters import require_uuid, to_python_uuid from roboco.utils.converters import require_uuid, to_python_uuid
if TYPE_CHECKING: if TYPE_CHECKING:
@@ -301,68 +304,41 @@ def _get_valid_claim_statuses(
return statuses return statuses
def extract_original_developer(quick_context: str | None) -> str | None: def extract_original_developer(task: Any) -> str | None:
"""The original-developer UUID from a task's orchestration markers.
Stored as the ``original_developer`` marker migration 041 moved it out of
the human ``quick_context`` blob into ``orchestration_markers``. Returns the
value only when it is a well-formed UUID; anything else reads as absent.
""" """
Safely extract original developer ID from quick_context. dev_id = markers.get_original_developer(task)
if not dev_id:
The quick_context stores original developer as the "original_developer:
{uuid}" entry on its own line. Other entries (doc_notes, documenter,
etc.) may be appended on subsequent lines, so scan line-by-line rather
than assuming the field is the first and only token.
Args:
quick_context: The task's quick_context field value
Returns:
UUID string of original developer, or None if not found/invalid
"""
if not quick_context:
return None
prefix = "original_developer:"
for raw in quick_context.splitlines():
line = raw.strip()
if not line.startswith(prefix):
continue
dev_id = line[len(prefix) :].strip()
if len(dev_id) == _UUID_LENGTH and dev_id.count("-") == _UUID_HYPHEN_COUNT:
return dev_id
return None return None
if len(dev_id) == _UUID_LENGTH and dev_id.count("-") == _UUID_HYPHEN_COUNT:
return dev_id
return None return None
_REQUIRED_CELLS_PREFIX = "required_cells:"
def _normalize_cell(value: object) -> str: def _normalize_cell(value: object) -> str:
"""Normalize a team/cell token for comparison (e.g. backend, frontend, ux_ui).""" """Normalize a team/cell token for comparison (e.g. backend, frontend, ux_ui)."""
raw = str(getattr(value, "value", value)).strip().lower() raw = str(getattr(value, "value", value)).strip().lower()
return raw.replace("/", "_").replace("-", "_").replace(" ", "") return raw.replace("/", "_").replace("-", "_").replace(" ", "")
def extract_required_cells(quick_context: str | None) -> list[str]: def extract_required_cells(task: Any) -> list[str]:
"""Cells the brief explicitly named, from a ``required_cells:`` marker line. """Cells the brief explicitly named, from the ``required_cells`` marker.
The Main PM must create a subtask for each named cell (it may not silently The Main PM must create a subtask for each named cell (it may not silently
collapse one into a neighbour see commit 60de3499). The marker is a single collapse one into a neighbour see commit 60de3499). Stored in
line, e.g. ``required_cells: backend, frontend, ux_ui``. Absent no ``orchestration_markers`` (migration 041). Absent no constraint (the gate
constraint (the gate is inert). Returns normalized, de-duplicated cells in is inert). Returns normalized, de-duplicated cells in marker order.
marker order.
""" """
if not quick_context: seen: list[str] = []
return [] for tok in markers.get_required_cells(task):
for raw in quick_context.splitlines(): cell = _normalize_cell(tok)
line = raw.strip() if cell and cell not in seen:
if not line.lower().startswith(_REQUIRED_CELLS_PREFIX): seen.append(cell)
continue return seen
body = line[len(_REQUIRED_CELLS_PREFIX) :]
seen: list[str] = []
for tok in body.split(","):
cell = _normalize_cell(tok)
if cell and cell not in seen:
seen.append(cell)
return seen
return []
# Review-task sources. The inbound-PR reviewer handles both external/fork PRs # Review-task sources. The inbound-PR reviewer handles both external/fork PRs
@@ -377,40 +353,25 @@ PR_REVIEW_SOURCES = ("external_pr", "internal_pr")
# Approve-&-Starts it; the loop itself never starts/approves/merges it. # Approve-&-Starts it; the loop itself never starts/approves/merges it.
SELF_HEAL_SOURCE = "self_heal" SELF_HEAL_SOURCE = "self_heal"
_SELF_HEAL_FP_PREFIX = "self_heal_fp=" def extract_self_heal_fingerprint(task: Any) -> str | None:
"""The self-heal dedupe fingerprint from a task's markers, or None.
The per-signal dedupe key carried on a self-heal task (in
def extract_self_heal_fingerprint(quick_context: str | None) -> str | None: ``orchestration_markers`` after migration 041), so the loop can tell a
"""The ``self_heal_fp=<fp>`` marker from quick_context, or None. regression already has an open fix task.
The per-signal dedupe key carried on a self-heal task, so the loop can tell
a regression already has an open fix task without a schema change.
""" """
for token in (quick_context or "").split(): return markers.get_self_heal_fingerprint(task)
if token.startswith(_SELF_HEAL_FP_PREFIX):
return token[len(_SELF_HEAL_FP_PREFIX) :] or None
return None
_SUPERSEDE_MARKER_PREFIX = "external_pr_supersede" def supersede_marker_line(task: Any) -> str:
"""The supersede marker value, or "" if none.
``pr={n} review={uuid}`` plus a ``closed=1`` token once the contributor PR is
def supersede_marker_line(quick_context: str | None) -> str: retired. Stored in ``orchestration_markers`` (migration 041); dedup and
"""Return the supersede marker line from a (multi-writer) quick_context. close-state checks parse this value (``needle in ...`` / ``"closed=1" in
....split()``) exactly as before.
The supersede marker (``external_pr_supersede pr={n} review={uuid}`` plus a
``closed=1`` token once the contributor PR is retired) is always written on
its own line, while ``escalate_to_ceo`` / ``ceo_approve`` append free-form
CEO notes on later lines. Dedup and close-state checks therefore parse THIS
line rather than substring-scanning the whole field a CEO note that
happened to contain ``closed=1`` or ``pr=N review=`` must not be mistaken
for the marker (mirrors :func:`extract_original_developer`).
""" """
for raw in (quick_context or "").splitlines(): return markers.get_external_pr_supersede(task) or ""
line = raw.strip()
if line.startswith(_SUPERSEDE_MARKER_PREFIX):
return line
return ""
class TaskService(BaseService): class TaskService(BaseService):
@@ -705,21 +666,20 @@ class TaskService(BaseService):
open a fresh review for the change). open a fresh review for the change).
""" """
result = await self.session.execute( result = await self.session.execute(
select(TaskTable.quick_context).where( select(TaskTable.orchestration_markers).where(
TaskTable.project_id == project_id, TaskTable.project_id == project_id,
TaskTable.source.in_(PR_REVIEW_SOURCES), TaskTable.source.in_(PR_REVIEW_SOURCES),
TaskTable.pr_number == pr_number, TaskTable.pr_number == pr_number,
) )
) )
contexts = result.scalars().all() marker_rows = result.scalars().all()
if not contexts: if not marker_rows:
return False return False
if not head_sha: if not head_sha:
return True return True
marker = f"external_pr_head={head_sha}" for om in marker_rows:
for qc in contexts: stored = (om or {}).get("external_pr_head")
text = qc or "" if not stored or stored == head_sha:
if marker in text or "external_pr_head=" not in text:
return True return True
return False return False
@@ -787,7 +747,7 @@ class TaskService(BaseService):
# Record the reviewed head commit so a later push (new SHA) re-reviews, # Record the reviewed head commit so a later push (new SHA) re-reviews,
# while an unchanged PR is skipped (see external_review_task_exists). # while an unchanged PR is skipped (see external_review_task_exists).
if head_sha: if head_sha:
task.quick_context = f"external_pr_head={head_sha}" markers.set_external_pr_head(task, head_sha)
await self.session.flush() await self.session.flush()
return task return task
@@ -842,7 +802,7 @@ class TaskService(BaseService):
return [ return [
t t
for t in result.scalars().all() for t in result.scalars().all()
if "dismissed=1" not in (t.quick_context or "").split() if not markers.is_dismissed(t)
] ]
async def list_external_pr_reviews(self) -> list[TaskTable]: async def list_external_pr_reviews(self) -> list[TaskTable]:
@@ -873,7 +833,7 @@ class TaskService(BaseService):
return [ return [
t t
for t in result.scalars().all() for t in result.scalars().all()
if "dismissed=1" not in (t.quick_context or "").split() if not markers.is_dismissed(t)
] ]
async def dismiss_external_pr_review(self, task_id: UUID) -> TaskTable | None: async def dismiss_external_pr_review(self, task_id: UUID) -> TaskTable | None:
@@ -886,8 +846,8 @@ class TaskService(BaseService):
task = await self.get(task_id) task = await self.get(task_id)
if task is None or getattr(task, "source", "") not in PR_REVIEW_SOURCES: if task is None or getattr(task, "source", "") not in PR_REVIEW_SOURCES:
return None return None
if "dismissed=1" not in (task.quick_context or "").split(): if not markers.is_dismissed(task):
task.quick_context = f"{task.quick_context or ''} dismissed=1".strip() markers.mark_dismissed(task)
await self.session.flush() await self.session.flush()
return task return task
@@ -1010,8 +970,8 @@ class TaskService(BaseService):
# rule), not the default branch. The marker links back to the review + # rule), not the default branch. The marker links back to the review +
# contributor PR for dedup and close-on-land (no parent link needed). # contributor PR for dedup and close-on-land (no parent link needed).
umbrella.branch_name = branch_name umbrella.branch_name = branch_name
umbrella.quick_context = ( markers.set_external_pr_supersede(
f"external_pr_supersede pr={pr_number} review={review_task_id}" umbrella, f"pr={pr_number} review={review_task_id}"
) )
await self.session.flush() await self.session.flush()
self.log.info( self.log.info(
@@ -1040,7 +1000,7 @@ class TaskService(BaseService):
) )
needle = f"pr={pr_number} review=" needle = f"pr={pr_number} review="
for task in result.scalars().all(): for task in result.scalars().all():
if needle in supersede_marker_line(task.quick_context): if needle in supersede_marker_line(task):
return task return task
return None return None
@@ -1063,7 +1023,7 @@ class TaskService(BaseService):
) )
pending: list[TaskTable] = [] pending: list[TaskTable] = []
for task in result.scalars().all(): for task in result.scalars().all():
if "closed=1" in supersede_marker_line(task.quick_context).split(): if "closed=1" in supersede_marker_line(task).split():
continue continue
if not await self._supersede_replacement_landed(cast("UUID", task.id)): if not await self._supersede_replacement_landed(cast("UUID", task.id)):
continue continue
@@ -1106,15 +1066,9 @@ class TaskService(BaseService):
task = await self.get(task_id) task = await self.get(task_id)
if task is None: if task is None:
return return
lines = (task.quick_context or "").splitlines() current = markers.get_external_pr_supersede(task) or ""
for i, raw in enumerate(lines): if "closed=1" not in current.split():
if raw.strip().startswith(_SUPERSEDE_MARKER_PREFIX): markers.set_external_pr_supersede(task, f"{current} closed=1".strip())
if "closed=1" not in raw.split():
lines[i] = f"{raw} closed=1"
break
else:
lines.append(f"{_SUPERSEDE_MARKER_PREFIX} closed=1")
task.quick_context = "\n".join(lines)
await self.session.flush() await self.session.flush()
async def _inherit_parent_session( async def _inherit_parent_session(
@@ -1670,7 +1624,7 @@ class TaskService(BaseService):
role = agent.role.value if hasattr(agent.role, "value") else str(agent.role) role = agent.role.value if hasattr(agent.role, "value") else str(agent.role)
if role not in ("qa", "documenter"): if role not in ("qa", "documenter"):
return None return None
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev and original_dev == str(agent_id): if original_dev and original_dev == str(agent_id):
return "cannot review your own work (self-review)" return "cannot review your own work (self-review)"
return None return None
@@ -1689,14 +1643,13 @@ class TaskService(BaseService):
role = agent.role.value if hasattr(agent.role, "value") else str(agent.role) role = agent.role.value if hasattr(agent.role, "value") else str(agent.role)
if role not in ("qa", "documenter"): if role not in ("qa", "documenter"):
return return
existing_context = task.quick_context or "" if markers.get_original_developer(task):
if "original_developer:" in existing_context:
return return
# Only set original_developer if it's a DIFFERENT agent than the one claiming # Only set original_developer if it's a DIFFERENT agent than the one claiming
# This prevents blocking QA/Documenter when PM assigns directly to them # This prevents blocking QA/Documenter when PM assigns directly to them
if task.assigned_to and str(task.assigned_to) != str(agent.id): if task.assigned_to and str(task.assigned_to) != str(agent.id):
task.quick_context = f"original_developer:{task.assigned_to}" markers.set_original_developer(task, task.assigned_to)
_CLAIMABLE_STATUSES: ClassVar[set[TaskStatus]] = { _CLAIMABLE_STATUSES: ClassVar[set[TaskStatus]] = {
TaskStatus.PENDING, TaskStatus.PENDING,
@@ -2469,7 +2422,7 @@ class TaskService(BaseService):
async def _index_qa_review_background( async def _index_qa_review_background(
self, self,
task_id: UUID, task_id: UUID,
quick_context: str | None, original_developer: str | None,
passed: bool, passed: bool,
qa_notes: str, qa_notes: str,
qa_agent_id: UUID | None, qa_agent_id: UUID | None,
@@ -2480,7 +2433,7 @@ class TaskService(BaseService):
try: try:
optimal = await get_optimal_service() optimal = await get_optimal_service()
original_dev = extract_original_developer(quick_context) original_dev = original_developer
await optimal.record_review( await optimal.record_review(
IndexReviewParams( IndexReviewParams(
@@ -3391,7 +3344,7 @@ class TaskService(BaseService):
# record for self-review prevention (QA can't review own work). # record for self-review prevention (QA can't review own work).
original_dev = str(task.assigned_to) if task.assigned_to else None original_dev = str(task.assigned_to) if task.assigned_to else None
if original_dev: if original_dev:
task.quick_context = f"original_developer:{original_dev}" markers.set_original_developer(task, original_dev)
# Capture the developer's UUID BEFORE clearing claimed_by so the # Capture the developer's UUID BEFORE clearing claimed_by so the
# `task.awaiting_qa` audit row is attributed to the dev who # `task.awaiting_qa` audit row is attributed to the dev who
@@ -3481,7 +3434,7 @@ class TaskService(BaseService):
bg_task = asyncio.create_task( bg_task = asyncio.create_task(
self._index_qa_review_background( self._index_qa_review_background(
require_uuid(task.id), require_uuid(task.id),
task.quick_context, extract_original_developer(task),
True, True,
notes or "Passed QA review", notes or "Passed QA review",
to_python_uuid(qa_agent_id), to_python_uuid(qa_agent_id),
@@ -3533,7 +3486,7 @@ class TaskService(BaseService):
qa_agent_id = task.assigned_to qa_agent_id = task.assigned_to
# Reassign to original developer so they can work on revisions # Reassign to original developer so they can work on revisions
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev: if original_dev:
task.assigned_to = cast("Any", UUID(original_dev)) task.assigned_to = cast("Any", UUID(original_dev))
task.claimed_by = cast("Any", UUID(original_dev)) task.claimed_by = cast("Any", UUID(original_dev))
@@ -3557,7 +3510,7 @@ class TaskService(BaseService):
review_task = asyncio.create_task( review_task = asyncio.create_task(
self._index_qa_review_background( self._index_qa_review_background(
require_uuid(task.id), require_uuid(task.id),
task.quick_context, extract_original_developer(task),
False, False,
notes, notes,
to_python_uuid(qa_agent_id), to_python_uuid(qa_agent_id),
@@ -3665,27 +3618,29 @@ class TaskService(BaseService):
@staticmethod @staticmethod
def _record_doc_notes(task: TaskTable, doc_notes: str | None) -> None: def _record_doc_notes(task: TaskTable, doc_notes: str | None) -> None:
"""Append doc_notes into quick_context if supplied.""" """Capture the documenter's note as a structured DocNote (best-effort).
Routes through the content chokepoint so it lands in the ``doc_notes``
mirror + ``notes_structured``. Skips if a richer DocNote already exists
(the gateway ``note`` tool) or the text is too trivial to validate.
"""
if not doc_notes: if not doc_notes:
return return
task.quick_context = _append_capped( if (task.notes_structured or {}).get("doc"):
task.quick_context, f"doc_notes:{doc_notes}" return
) try:
apply_structured_note(task, "doc", {"summary": doc_notes})
except ContentValidationError:
return
@staticmethod @staticmethod
def _record_documenter_context(task: TaskTable) -> None: def _record_documenter_context(task: TaskTable) -> None:
"""Stamp documenter id into quick_context if missing.""" """Stamp the documenter id into orchestration markers if missing."""
if not task.assigned_to: if not task.assigned_to:
return return
existing_context = task.quick_context or "" if markers.get_documenter(task):
if "documenter:" in existing_context:
return return
doc_context = f"documenter:{task.assigned_to}" markers.set_documenter(task, task.assigned_to)
task.quick_context = (
f"{existing_context}\n{doc_context}".strip()
if existing_context
else doc_context
)
async def _resolve_pm_for_review(self, task: TaskTable) -> UUID | None: async def _resolve_pm_for_review(self, task: TaskTable) -> UUID | None:
"""Walk up the parent chain to find the PM who owns this work. """Walk up the parent chain to find the PM who owns this work.
@@ -3801,17 +3756,6 @@ class TaskService(BaseService):
task.pr_number = pr_number task.pr_number = pr_number
task.pr_url = pr_url task.pr_url = pr_url
# Store developer who created PR in quick_context
if task.assigned_to:
existing_context = task.quick_context or ""
if "pr_author:" not in existing_context:
pr_context = f"pr_author:{task.assigned_to}"
task.quick_context = (
f"{existing_context}\n{pr_context}".strip()
if existing_context
else pr_context
)
# Check if BOTH docs_complete AND pr_created are now true # Check if BOTH docs_complete AND pr_created are now true
from roboco.enforcement.task_lifecycle import check_parallel_completion from roboco.enforcement.task_lifecycle import check_parallel_completion
@@ -4620,7 +4564,7 @@ class TaskService(BaseService):
task_id=str(task_id), task_id=str(task_id),
) )
else: else:
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev: if original_dev:
task.assigned_to = cast("Any", UUID(original_dev)) task.assigned_to = cast("Any", UUID(original_dev))
task.claimed_by = cast("Any", UUID(original_dev)) task.claimed_by = cast("Any", UUID(original_dev))
@@ -5335,7 +5279,7 @@ class TaskService(BaseService):
parent = await self.get(parent_task_id) parent = await self.get(parent_task_id)
if parent is None: if parent is None:
return [] return []
required = extract_required_cells(parent.quick_context) required = extract_required_cells(parent)
if not required: if not required:
return [] return []
children = await self.get_subtasks(parent_task_id) children = await self.get_subtasks(parent_task_id)
@@ -5548,7 +5492,7 @@ class TaskService(BaseService):
# QA / Documenter cannot claim what they themselves developed. # QA / Documenter cannot claim what they themselves developed.
if agent.role in (AgentRole.QA, AgentRole.DOCUMENTER): if agent.role in (AgentRole.QA, AgentRole.DOCUMENTER):
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev and str(agent.agent_id) == original_dev: if original_dev and str(agent.agent_id) == original_dev:
raise UnauthorizedError( raise UnauthorizedError(
action="claim", action="claim",
@@ -5670,7 +5614,7 @@ class TaskService(BaseService):
reason="Only documenters can mark documentation as complete", reason="Only documenters can mark documentation as complete",
) )
original_dev = extract_original_developer(task.quick_context) original_dev = extract_original_developer(task)
if original_dev and str(agent.agent_id) == original_dev: if original_dev and str(agent.agent_id) == original_dev:
from roboco.services.audit import get_audit_service from roboco.services.audit import get_audit_service
+1 -1
View File
@@ -548,7 +548,7 @@ async def test_qa_fail_path(
# spec layer's slug-based self-review check is a separate code path # spec layer's slug-based self-review check is a separate code path
# (``_extract_original_developer`` in qa.py) which only fires when # (``_extract_original_developer`` in qa.py) which only fires when
# an actor's slug equals this value, so a UUID here doesn't trip it. # an actor's slug equals this value, so a UUID here doesn't trip it.
task.quick_context = f"original_developer:{dev_agent.id}" task.orchestration_markers = {"original_developer": str(dev_agent.id)}
await db_session.flush() await db_session.flush()
task_service = TaskService(db_session) task_service = TaskService(db_session)
@@ -574,7 +574,7 @@ async def test_index_qa_review_calls_record_review(
dev_id = uuid4() dev_id = uuid4()
await svc._index_qa_review_background( await svc._index_qa_review_background(
uuid4(), uuid4(),
f"original_developer:{dev_id}", str(dev_id),
passed=True, passed=True,
qa_notes="LGTM", qa_notes="LGTM",
qa_agent_id=uuid4(), qa_agent_id=uuid4(),
@@ -669,7 +669,7 @@ async def test_submit_for_qa_clears_assignment_and_records_dev(
assert out is not None assert out is not None
assert out.status == TaskStatus.AWAITING_QA assert out.status == TaskStatus.AWAITING_QA
assert out.assigned_to is None assert out.assigned_to is None
assert "original_developer:" in (out.quick_context or "") assert (out.orchestration_markers or {}).get("original_developer")
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@@ -687,7 +687,7 @@ async def test_fail_qa_with_indexing_runs(
dev_id = task_setup["agent_id"] dev_id = task_setup["agent_id"]
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.status = TaskStatus.AWAITING_QA task.status = TaskStatus.AWAITING_QA
task.quick_context = f"original_developer:{dev_id}" task.orchestration_markers = {"original_developer": str(dev_id)}
await db_session.flush() await db_session.flush()
fake_optimal = MagicMock() fake_optimal = MagicMock()
fake_optimal.record_review = AsyncMock() fake_optimal.record_review = AsyncMock()
+27 -25
View File
@@ -25,6 +25,7 @@ Targets:
from __future__ import annotations from __future__ import annotations
import asyncio import asyncio
from types import SimpleNamespace
from typing import TYPE_CHECKING, Any, cast from typing import TYPE_CHECKING, Any, cast
from unittest.mock import AsyncMock, MagicMock from unittest.mock import AsyncMock, MagicMock
from uuid import UUID, uuid4 from uuid import UUID, uuid4
@@ -37,6 +38,7 @@ from roboco.models import AgentRole, AgentStatus, Team
from roboco.models.base import Complexity, TaskNature, TaskStatus, TaskType from roboco.models.base import Complexity, TaskNature, TaskStatus, TaskType
from roboco.models.permissions import AgentContext from roboco.models.permissions import AgentContext
from roboco.models.task import TaskCreateRequest from roboco.models.task import TaskCreateRequest
from roboco.foundation.policy.content import markers
from roboco.models.work_session import WorkSessionStatus from roboco.models.work_session import WorkSessionStatus
from roboco.services.base import ValidationError from roboco.services.base import ValidationError
from roboco.services.task import ( from roboco.services.task import (
@@ -146,25 +148,25 @@ def test_default_claim_statuses_for_none() -> None:
def test_extract_original_developer_invalid_format() -> None: def test_extract_original_developer_invalid_format() -> None:
"""Invalid UUID format returns None even when prefix matches.""" """Invalid UUID format returns None even when the marker is present."""
out = extract_original_developer("original_developer:not-a-uuid") task = SimpleNamespace(orchestration_markers={"original_developer": "not-a-uuid"})
assert out is None assert extract_original_developer(task) is None
def test_extract_original_developer_no_match() -> None: def test_extract_original_developer_no_match() -> None:
out = extract_original_developer("some other context") task = SimpleNamespace(orchestration_markers={"documenter": "x"})
assert out is None assert extract_original_developer(task) is None
def test_extract_original_developer_empty() -> None: def test_extract_original_developer_empty() -> None:
assert extract_original_developer(None) is None assert extract_original_developer(SimpleNamespace(orchestration_markers=None)) is None
assert extract_original_developer("") is None assert extract_original_developer(SimpleNamespace(orchestration_markers={})) is None
def test_extract_original_developer_valid() -> None: def test_extract_original_developer_valid() -> None:
test_uuid = "12345678-1234-1234-1234-123456789012" test_uuid = "12345678-1234-1234-1234-123456789012"
out = extract_original_developer(f"original_developer:{test_uuid}") task = SimpleNamespace(orchestration_markers={"original_developer": test_uuid})
assert out == test_uuid assert extract_original_developer(task) == test_uuid
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@@ -535,7 +537,7 @@ def test_validate_not_self_review_qa_self(task_setup: dict) -> None:
svc = task_setup["svc"] svc = task_setup["svc"]
aid = uuid4() aid = uuid4()
task = MagicMock() task = MagicMock()
task.quick_context = f"original_developer:{aid}" task.orchestration_markers = {"original_developer": str(aid)}
agent = MagicMock(role=AgentRole.QA) agent = MagicMock(role=AgentRole.QA)
out = svc._validate_not_self_review(task, agent, agent_id=aid) out = svc._validate_not_self_review(task, agent, agent_id=aid)
assert "self-review" in (out or "") assert "self-review" in (out or "")
@@ -544,13 +546,13 @@ def test_validate_not_self_review_qa_self(task_setup: dict) -> None:
def test_set_original_developer_skips_when_already_set(task_setup: dict) -> None: def test_set_original_developer_skips_when_already_set(task_setup: dict) -> None:
svc = task_setup["svc"] svc = task_setup["svc"]
task = MagicMock() task = MagicMock()
task.quick_context = "original_developer:already-set" task.orchestration_markers = {"original_developer": "already-set"}
task.assigned_to = uuid4() task.assigned_to = uuid4()
agent = MagicMock(role=AgentRole.QA, id=uuid4()) agent = MagicMock(role=AgentRole.QA, id=uuid4())
# Should not change quick_context # An existing original_developer marker must not be overwritten.
before = task.quick_context before = dict(task.orchestration_markers)
svc._set_original_developer_context(task, agent) svc._set_original_developer_context(task, agent)
assert task.quick_context == before assert task.orchestration_markers == before
def test_set_original_developer_skips_when_no_role(task_setup: dict) -> None: def test_set_original_developer_skips_when_no_role(task_setup: dict) -> None:
@@ -731,10 +733,10 @@ def test_record_documenter_context_skips_when_already(task_setup: dict) -> None:
svc = task_setup["svc"] svc = task_setup["svc"]
task = MagicMock() task = MagicMock()
task.assigned_to = uuid4() task.assigned_to = uuid4()
task.quick_context = "documenter:something" task.orchestration_markers = {"documenter": "something"}
before = task.quick_context before = dict(task.orchestration_markers)
svc._record_documenter_context(task) svc._record_documenter_context(task)
assert task.quick_context == before assert task.orchestration_markers == before
def test_record_documenter_context_appends(task_setup: dict) -> None: def test_record_documenter_context_appends(task_setup: dict) -> None:
@@ -743,9 +745,10 @@ def test_record_documenter_context_appends(task_setup: dict) -> None:
task = MagicMock() task = MagicMock()
task.assigned_to = aid task.assigned_to = aid
task.quick_context = "existing" task.quick_context = "existing"
task.orchestration_markers = None
svc._record_documenter_context(task) svc._record_documenter_context(task)
assert "documenter:" in task.quick_context assert markers.get_documenter(task) == str(aid)
assert "existing" in task.quick_context assert task.quick_context == "existing" # human field untouched
def test_record_documenter_context_first_entry(task_setup: dict) -> None: def test_record_documenter_context_first_entry(task_setup: dict) -> None:
@@ -753,9 +756,9 @@ def test_record_documenter_context_first_entry(task_setup: dict) -> None:
aid = uuid4() aid = uuid4()
task = MagicMock() task = MagicMock()
task.assigned_to = aid task.assigned_to = aid
task.quick_context = None task.orchestration_markers = None
svc._record_documenter_context(task) svc._record_documenter_context(task)
assert "documenter:" in task.quick_context assert markers.get_documenter(task) == str(aid)
def test_record_completion_notes_skips_empty(task_setup: dict) -> None: def test_record_completion_notes_skips_empty(task_setup: dict) -> None:
@@ -1158,16 +1161,15 @@ def test_validate_not_self_review_qa_with_different_dev(task_setup: dict) -> Non
def test_set_original_developer_records_when_different(task_setup: dict) -> None: def test_set_original_developer_records_when_different(task_setup: dict) -> None:
"""Cover line 889: sets quick_context when assigned_to != agent.id.""" """Sets the original_developer marker when assigned_to != agent.id."""
svc = task_setup["svc"] svc = task_setup["svc"]
task = MagicMock() task = MagicMock()
task.quick_context = "" task.orchestration_markers = None
other_id = uuid4() other_id = uuid4()
task.assigned_to = other_id task.assigned_to = other_id
agent = MagicMock(role=AgentRole.QA, id=uuid4()) agent = MagicMock(role=AgentRole.QA, id=uuid4())
svc._set_original_developer_context(task, agent) svc._set_original_developer_context(task, agent)
assert "original_developer:" in task.quick_context assert markers.get_original_developer(task) == str(other_id)
assert str(other_id) in task.quick_context
@pytest.mark.asyncio @pytest.mark.asyncio
@@ -188,7 +188,7 @@ async def test_claim_task_for_agent_self_review_rejected(
svc = task_setup["svc"] svc = task_setup["svc"]
qa_id = uuid4() qa_id = uuid4()
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.quick_context = f"original_developer:{qa_id}" task.orchestration_markers = {"original_developer": str(qa_id)}
await db_session.flush() await db_session.flush()
agent_ctx = _ctx(qa_id, AgentRole.QA) agent_ctx = _ctx(qa_id, AgentRole.QA)
perms = _Permissions(can_claim=True) perms = _Permissions(can_claim=True)
@@ -423,7 +423,7 @@ async def test_docs_complete_for_task_self_documentation_blocked(
svc = task_setup["svc"] svc = task_setup["svc"]
doc_id = task_setup["agent_id"] doc_id = task_setup["agent_id"]
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.quick_context = f"original_developer:{doc_id}" task.orchestration_markers = {"original_developer": str(doc_id)}
await db_session.flush() await db_session.flush()
agent_ctx = _ctx(doc_id, AgentRole.DOCUMENTER) agent_ctx = _ctx(doc_id, AgentRole.DOCUMENTER)
audit_mock = AsyncMock() audit_mock = AsyncMock()
@@ -527,7 +527,7 @@ async def test_fail_qa_reassigns_to_original_developer(
dev_id = task_setup["agent_id"] dev_id = task_setup["agent_id"]
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.status = TaskStatus.AWAITING_QA task.status = TaskStatus.AWAITING_QA
task.quick_context = f"original_developer:{dev_id}" task.orchestration_markers = {"original_developer": str(dev_id)}
await db_session.flush() await db_session.flush()
failed = await svc.fail_qa(task.id, notes="missing tests") failed = await svc.fail_qa(task.id, notes="missing tests")
assert failed is not None assert failed is not None
@@ -597,7 +597,7 @@ async def test_ceo_reject_reassigns_to_original_dev(
dev_id = task_setup["agent_id"] dev_id = task_setup["agent_id"]
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.status = TaskStatus.AWAITING_CEO_APPROVAL task.status = TaskStatus.AWAITING_CEO_APPROVAL
task.quick_context = f"original_developer:{dev_id}" task.orchestration_markers = {"original_developer": str(dev_id)}
await db_session.flush() await db_session.flush()
rejected = await svc.ceo_reject(task.id, reason="re-do auth flow") rejected = await svc.ceo_reject(task.id, reason="re-do auth flow")
assert rejected is not None assert rejected is not None
@@ -695,7 +695,7 @@ async def test_ceo_reject_writes_handoff_journal(
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.status = TaskStatus.AWAITING_CEO_APPROVAL task.status = TaskStatus.AWAITING_CEO_APPROVAL
task.quick_context = f"original_developer:{task_setup['agent_id']}" task.orchestration_markers = {"original_developer": str(task_setup["agent_id"])}
await db_session.flush() await db_session.flush()
reason = "AC9/AC10 totals must include cache tokens" reason = "AC9/AC10 totals must include cache tokens"
@@ -919,7 +919,7 @@ async def test_claim_rejects_self_review_for_qa(
task = await svc.create(_req(task_setup)) task = await svc.create(_req(task_setup))
task.status = TaskStatus.AWAITING_QA task.status = TaskStatus.AWAITING_QA
task.branch_name = "feature/backend/x" task.branch_name = "feature/backend/x"
task.quick_context = f"original_developer:{qa_agent.id}" task.orchestration_markers = {"original_developer": str(qa_agent.id)}
await db_session.flush() await db_session.flush()
out = await svc.claim(task.id, qa_agent.id) out = await svc.claim(task.id, qa_agent.id)
assert out is None assert out is None
+2 -2
View File
@@ -2050,7 +2050,7 @@ async def test_pass_qa_self_review_forbidden(qa_client: dict) -> None:
task = _seed_task_qa( task = _seed_task_qa(
qa_client, qa_client,
pr_number=42, pr_number=42,
quick_context=f"original_developer:{qa_client['agent'].id}", orchestration_markers={"original_developer": str(qa_client["agent"].id)},
) )
await qa_client["db"].flush() await qa_client["db"].flush()
response = await qa_client["client"].post( response = await qa_client["client"].post(
@@ -2151,7 +2151,7 @@ async def test_fail_qa_self_review_forbidden(qa_client: dict) -> None:
"""QA cannot fail-QA on a task where they were the dev.""" """QA cannot fail-QA on a task where they were the dev."""
task = _seed_task_qa( task = _seed_task_qa(
qa_client, qa_client,
quick_context=f"original_developer:{qa_client['agent'].id}", orchestration_markers={"original_developer": str(qa_client["agent"].id)},
) )
await qa_client["db"].flush() await qa_client["db"].flush()
response = await qa_client["client"].post( response = await qa_client["client"].post(
+45 -25
View File
@@ -1,28 +1,50 @@
"""External-PR review dedup — review once per (project, PR, head commit). """External-PR review dedup — review once per (project, PR, head commit).
``external_review_task_exists`` drives re-review off the PR's head SHA: an ``external_review_task_exists`` drives re-review off the PR's head SHA, stored as
unchanged PR (same head) is skipped, new commits (a new head SHA) open a fresh the ``external_pr_head`` orchestration marker (migration 041); dismissal is the
review, and legacy/unknown-SHA tasks are never re-reviewed (no spam). ``dismissed`` marker. An unchanged PR (same head) is skipped, new commits open a
fresh review, and legacy/unknown-SHA tasks are never re-reviewed.
""" """
from __future__ import annotations from __future__ import annotations
from types import SimpleNamespace
from unittest.mock import AsyncMock, MagicMock from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4 from uuid import uuid4
import pytest import pytest
from roboco.foundation.policy.content import markers
from roboco.services.task import TaskService from roboco.services.task import TaskService
def _service(quick_contexts: list[str | None]) -> TaskService: def _service(scalar_rows: list[object]) -> TaskService:
"""A TaskService whose review-task query returns these quick_context values.""" """A TaskService whose next query returns these scalar rows."""
res = MagicMock() res = MagicMock()
res.scalars.return_value.all.return_value = quick_contexts res.scalars.return_value.all.return_value = scalar_rows
session = MagicMock() session = MagicMock()
session.execute = AsyncMock(return_value=res) session.execute = AsyncMock(return_value=res)
session.flush = AsyncMock()
return TaskService(session) return TaskService(session)
def _markers(head: str | None = None, dismissed: bool = False) -> dict:
om: dict = {}
if head is not None:
om["external_pr_head"] = head
if dismissed:
om["dismissed"] = True
return om
def _bind(svc: TaskService, name: str, value: object) -> None:
object.__setattr__(svc, name, value)
# ---------------------------------------------------------------------------
# external_review_task_exists — scalars are orchestration_markers dicts
# ---------------------------------------------------------------------------
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_no_task_yet_ingests() -> None: async def test_no_task_yet_ingests() -> None:
svc = _service([]) svc = _service([])
@@ -31,14 +53,14 @@ async def test_no_task_yet_ingests() -> None:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_same_head_sha_skips() -> None: async def test_same_head_sha_skips() -> None:
svc = _service(["external_pr_head=abc"]) svc = _service([_markers("abc")])
assert await svc.external_review_task_exists(uuid4(), 170, "abc") is True assert await svc.external_review_task_exists(uuid4(), 170, "abc") is True
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_new_head_sha_rereviews() -> None: async def test_new_head_sha_rereviews() -> None:
# PR got new commits since the last review → open a fresh review. # PR got new commits since the last review → open a fresh review.
svc = _service(["external_pr_head=abc"]) svc = _service([_markers("abc")])
assert await svc.external_review_task_exists(uuid4(), 170, "def") is False assert await svc.external_review_task_exists(uuid4(), 170, "def") is False
@@ -52,25 +74,26 @@ async def test_legacy_markerless_task_not_rereviewed() -> None:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_unknown_head_sha_does_not_spam() -> None: async def test_unknown_head_sha_does_not_spam() -> None:
# Can't detect change (no SHA from GitHub) → treat as reviewed. # Can't detect change (no SHA from GitHub) → treat as reviewed.
svc = _service(["external_pr_head=abc"]) svc = _service([_markers("abc")])
assert await svc.external_review_task_exists(uuid4(), 170, None) is True assert await svc.external_review_task_exists(uuid4(), 170, None) is True
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_multiple_old_shas_still_rereviews_new() -> None: async def test_multiple_old_shas_still_rereviews_new() -> None:
svc = _service(["external_pr_head=abc", "external_pr_head=def"]) svc = _service([_markers("abc"), _markers("def")])
assert await svc.external_review_task_exists(uuid4(), 170, "ghi") is False assert await svc.external_review_task_exists(uuid4(), 170, "ghi") is False
assert await svc.external_review_task_exists(uuid4(), 170, "def") is True assert await svc.external_review_task_exists(uuid4(), 170, "def") is True
def _bind(svc: TaskService, name: str, value: object) -> None: # ---------------------------------------------------------------------------
object.__setattr__(svc, name, value) # list queues — post-query dismissed filter (scalars are task objects)
# ---------------------------------------------------------------------------
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_list_awaiting_decision_excludes_dismissed() -> None: async def test_list_awaiting_decision_excludes_dismissed() -> None:
pending = MagicMock(quick_context="external_pr_head=abc") pending = SimpleNamespace(orchestration_markers=_markers("abc"))
dismissed = MagicMock(quick_context="external_pr_head=def dismissed=1") dismissed = SimpleNamespace(orchestration_markers=_markers("def", dismissed=True))
svc = _service([pending, dismissed]) svc = _service([pending, dismissed])
out = await svc.list_external_pr_reviews_awaiting_decision() out = await svc.list_external_pr_reviews_awaiting_decision()
assert out == [pending] assert out == [pending]
@@ -78,10 +101,8 @@ async def test_list_awaiting_decision_excludes_dismissed() -> None:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_list_external_pr_reviews_excludes_dismissed() -> None: async def test_list_external_pr_reviews_excludes_dismissed() -> None:
# The panel queue surfaces in-flight reviews too (the status filter lives in reviewing = SimpleNamespace(orchestration_markers=_markers("abc"))
# SQL); here we pin the post-query behavior: dismissed reviews drop out. dismissed = SimpleNamespace(orchestration_markers=_markers("def", dismissed=True))
reviewing = MagicMock(quick_context="external_pr_head=abc")
dismissed = MagicMock(quick_context="external_pr_head=def dismissed=1")
svc = _service([reviewing, dismissed]) svc = _service([reviewing, dismissed])
out = await svc.list_external_pr_reviews() out = await svc.list_external_pr_reviews()
assert out == [reviewing] assert out == [reviewing]
@@ -89,15 +110,14 @@ async def test_list_external_pr_reviews_excludes_dismissed() -> None:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_dismiss_marks_and_is_idempotent() -> None: async def test_dismiss_marks_and_is_idempotent() -> None:
task = MagicMock(source="external_pr", quick_context="external_pr_head=abc") task = SimpleNamespace(source="external_pr", orchestration_markers=_markers("abc"))
session = MagicMock() svc = _service([])
session.flush = AsyncMock()
svc = TaskService(session)
_bind(svc, "get", AsyncMock(return_value=task)) _bind(svc, "get", AsyncMock(return_value=task))
await svc.dismiss_external_pr_review(uuid4()) await svc.dismiss_external_pr_review(uuid4())
assert "dismissed=1" in task.quick_context.split() assert markers.is_dismissed(task) is True
await svc.dismiss_external_pr_review(uuid4()) # idempotent await svc.dismiss_external_pr_review(uuid4()) # idempotent
assert task.quick_context.split().count("dismissed=1") == 1 assert markers.is_dismissed(task) is True
assert task.orchestration_markers["dismissed"] is True
@pytest.mark.asyncio @pytest.mark.asyncio
@@ -111,7 +131,7 @@ async def test_dismiss_rejects_non_external_pr() -> None:
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# active_task_owns_branch — the internal-PR "is this a lifecycle PR?" check (#3) # active_task_owns_branch — the internal-PR "is this a lifecycle PR?" check
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
+8 -4
View File
@@ -135,7 +135,7 @@ async def test_ingest_creates_review_task(db_session: AsyncSession) -> None:
assert task.task_type == TaskType.CODE assert task.task_type == TaskType.CODE
assert task.confirmed_by_human is False assert task.confirmed_by_human is False
assert task.status == TaskStatus.PENDING assert task.status == TaskStatus.PENDING
assert task.quick_context == "external_pr_head=abc123" assert task.orchestration_markers == {"external_pr_head": "abc123"}
@pytest.mark.asyncio @pytest.mark.asyncio
@@ -187,7 +187,7 @@ async def test_ingest_new_head_rereviews(db_session: AsyncSession) -> None:
await db_session.flush() await db_session.flush()
assert rereview is not None assert rereview is not None
assert rereview.quick_context == "external_pr_head=def456" assert rereview.orchestration_markers == {"external_pr_head": "def456"}
reviews = await svc.list_external_pr_reviews() reviews = await svc.list_external_pr_reviews()
matching = [t for t in reviews if t.pr_number == EXTERNAL_PR] matching = [t for t in reviews if t.pr_number == EXTERNAL_PR]
assert len(matching) == REVIEWS_AFTER_REREVIEW assert len(matching) == REVIEWS_AFTER_REREVIEW
@@ -376,8 +376,12 @@ async def test_find_supersede_umbrella_no_prefix_false_match(
assert five is not None assert five is not None
assert fifty is not None assert fifty is not None
assert UUID(str(five.id)) != UUID(str(fifty.id)) assert UUID(str(five.id)) != UUID(str(fifty.id))
assert "pr=5 review=" in (five.quick_context or "") assert "pr=5 review=" in (five.orchestration_markers or {}).get(
assert "pr=50 review=" in (fifty.quick_context or "") "external_pr_supersede", ""
)
assert "pr=50 review=" in (fifty.orchestration_markers or {}).get(
"external_pr_supersede", ""
)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
+35 -16
View File
@@ -1,36 +1,53 @@
"""required_cells decomposition gate — marker parse + uncovered-cell coverage. """required_cells decomposition gate — marker parse + uncovered-cell coverage.
The Main PM must create a subtask for each cell the brief explicitly names The Main PM must create a subtask for each cell the brief explicitly names
(recorded as a ``required_cells:`` marker on the parent's quick_context). The (recorded as a ``required_cells`` orchestration marker on the parent). The gate
gate is inert when no marker is present, so legacy decompositions never block. is inert when no marker is present, so legacy decompositions never block.
""" """
from __future__ import annotations from __future__ import annotations
from types import SimpleNamespace
from unittest.mock import AsyncMock, MagicMock from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4 from uuid import uuid4
import pytest import pytest
from roboco.services.task import TaskService, extract_required_cells from roboco.services.task import TaskService, extract_required_cells
def _task(required_cells: list[str] | None = None) -> SimpleNamespace:
om = {"required_cells": required_cells} if required_cells is not None else None
return SimpleNamespace(orchestration_markers=om)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# extract_required_cells (marker parser) # extract_required_cells (marker reader)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
def test_extract_required_cells_absent_is_empty() -> None: def test_extract_required_cells_absent_is_empty() -> None:
assert extract_required_cells(None) == [] assert extract_required_cells(_task()) == []
assert extract_required_cells("original_developer: abc\ndoc_notes: y") == [] assert (
extract_required_cells(
SimpleNamespace(orchestration_markers={"original_developer": "abc"})
)
== []
)
def test_extract_required_cells_parses_and_normalizes() -> None: def test_extract_required_cells_normalizes() -> None:
qc = "original_developer: abc\nrequired_cells: Backend, Frontend , UX/UI" assert extract_required_cells(_task(["Backend", "Frontend ", "UX/UI"])) == [
assert extract_required_cells(qc) == ["backend", "frontend", "ux_ui"] "backend",
"frontend",
"ux_ui",
]
def test_extract_required_cells_dedups_in_order() -> None: def test_extract_required_cells_dedups_in_order() -> None:
out = extract_required_cells("required_cells: backend, backend, frontend") assert extract_required_cells(_task(["backend", "backend", "frontend"])) == [
assert out == ["backend", "frontend"] "backend",
"frontend",
]
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@@ -38,10 +55,12 @@ def test_extract_required_cells_dedups_in_order() -> None:
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
def _service(parent_qc: str | None, child_teams: list[str | None]) -> TaskService: def _service(
required_cells: list[str] | None, child_teams: list[str | None]
) -> TaskService:
"""A TaskService whose get()/get_subtasks() return a parent + these children.""" """A TaskService whose get()/get_subtasks() return a parent + these children."""
svc = TaskService(MagicMock()) svc = TaskService(MagicMock())
parent = MagicMock(quick_context=parent_qc) parent = _task(required_cells)
children = [MagicMock(team=t) for t in child_teams] children = [MagicMock(team=t) for t in child_teams]
object.__setattr__(svc, "get", AsyncMock(return_value=parent)) object.__setattr__(svc, "get", AsyncMock(return_value=parent))
object.__setattr__(svc, "get_subtasks", AsyncMock(return_value=children)) object.__setattr__(svc, "get_subtasks", AsyncMock(return_value=children))
@@ -50,25 +69,25 @@ def _service(parent_qc: str | None, child_teams: list[str | None]) -> TaskServic
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_uncovered_inert_without_marker() -> None: async def test_uncovered_inert_without_marker() -> None:
svc = _service("doc_notes: x", ["backend"]) svc = _service(None, ["backend"])
assert await svc.uncovered_required_cells(uuid4()) == [] assert await svc.uncovered_required_cells(uuid4()) == []
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_uncovered_flags_the_dropped_cell() -> None: async def test_uncovered_flags_the_dropped_cell() -> None:
# Brief named backend+frontend+ux_ui; only backend+frontend got subtasks. # Brief named backend+frontend+ux_ui; only backend+frontend got subtasks.
svc = _service("required_cells: backend, frontend, ux_ui", ["backend", "frontend"]) svc = _service(["backend", "frontend", "ux_ui"], ["backend", "frontend"])
assert await svc.uncovered_required_cells(uuid4()) == ["ux_ui"] assert await svc.uncovered_required_cells(uuid4()) == ["ux_ui"]
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_uncovered_empty_when_all_named_cells_covered() -> None: async def test_uncovered_empty_when_all_named_cells_covered() -> None:
svc = _service("required_cells: backend, frontend", ["frontend", "backend"]) svc = _service(["backend", "frontend"], ["frontend", "backend"])
assert await svc.uncovered_required_cells(uuid4()) == [] assert await svc.uncovered_required_cells(uuid4()) == []
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_uncovered_normalizes_child_team_form() -> None: async def test_uncovered_normalizes_child_team_form() -> None:
# Marker uses underscore, child team uses the slash form — they match. # Marker uses underscore, child team uses the slash form — they match.
svc = _service("required_cells: ux_ui", ["UX/UI"]) svc = _service(["ux_ui"], ["UX/UI"])
assert await svc.uncovered_required_cells(uuid4()) == [] assert await svc.uncovered_required_cells(uuid4()) == []
@@ -156,7 +156,7 @@ async def test_originate_creates_pending_main_pm_assigned_task(
assert task.team == Team.MAIN_PM assert task.team == Team.MAIN_PM
assert task.source == "self_heal" assert task.source == "self_heal"
assert task.acceptance_criteria # non-empty (AC-guardrail) assert task.acceptance_criteria # non-empty (AC-guardrail)
assert "self_heal_fp=" in (task.quick_context or "") assert (task.orchestration_markers or {}).get("self_heal_fp")
@pytest.mark.asyncio @pytest.mark.asyncio
+34 -55
View File
@@ -1,21 +1,15 @@
"""Supersede umbrella close-on-land guards. """Supersede umbrella close-on-land guards (orchestration-marker storage).
Covers the parts of the external-PR supersede flow that decide whether and The supersede marker (``pr={n} review={uuid}`` plus a ``closed=1`` token once
which a landed supersede's contributor PR gets retired: the contributor PR is retired) lives in ``orchestration_markers`` after
migration 041 isolated from the human ``quick_context``, so CEO escalation /
- ``supersede_marker_line`` anchors marker/state checks to the marker line, so approval notes can no longer be mistaken for it.
free-form CEO escalation/approval notes appended to the same multi-writer
``quick_context`` can't be mistaken for the marker.
- ``supersede_umbrellas_pending_close`` only returns umbrellas whose
replacement work actually landed (a non-cancelled descendant carrying a PR),
not every COMPLETED umbrella the CEO can force-complete over a cancelled
code subtask.
- ``mark_supersede_pr_closed`` writes the ``closed=1`` idempotency token onto
the marker line, surviving appended notes.
""" """
from __future__ import annotations from __future__ import annotations
from types import SimpleNamespace
from typing import Any
from unittest.mock import AsyncMock, MagicMock from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4 from uuid import uuid4
@@ -23,11 +17,10 @@ import pytest
from roboco.models.base import TaskStatus from roboco.models.base import TaskStatus
from roboco.services.task import TaskService, supersede_marker_line from roboco.services.task import TaskService, supersede_marker_line
_MARKER = "external_pr_supersede pr=5 review=abc" _VALUE = "pr=5 review=abc"
def _scalars_all(rows: list[object]) -> MagicMock: def _scalars_all(rows: list[object]) -> MagicMock:
"""A session.execute return value whose .scalars().all() yields `rows`."""
res = MagicMock() res = MagicMock()
res.scalars.return_value.all.return_value = rows res.scalars.return_value.all.return_value = rows
return res return res
@@ -44,25 +37,23 @@ def _bind(svc: TaskService, name: str, value: object) -> None:
object.__setattr__(svc, name, value) object.__setattr__(svc, name, value)
def _task(supersede: str | None = None, **kw: Any) -> SimpleNamespace:
om = {"external_pr_supersede": supersede} if supersede is not None else None
return SimpleNamespace(orchestration_markers=om, **kw)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# supersede_marker_line — line anchoring # supersede_marker_line — reads the marker value
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
def test_marker_line_returns_marker_ignoring_appended_notes() -> None: def test_marker_line_returns_value() -> None:
qc = f"{_MARKER}\nceo_approval_notes: shipped, looks good" assert supersede_marker_line(_task(_VALUE)) == _VALUE
assert supersede_marker_line(qc) == _MARKER
def test_marker_line_not_fooled_by_closed_token_in_note() -> None:
qc = f"{_MARKER}\nceo_approval_notes: marked closed=1 in jira"
# The marker line itself carries no closed=1, so the PR is NOT yet closed.
assert "closed=1" not in supersede_marker_line(qc).split()
def test_marker_line_empty_when_absent() -> None: def test_marker_line_empty_when_absent() -> None:
assert supersede_marker_line("no marker here\nescalation_notes: x") == "" assert supersede_marker_line(_task()) == ""
assert supersede_marker_line(None) == "" assert supersede_marker_line(SimpleNamespace(orchestration_markers=None)) == ""
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@@ -72,29 +63,25 @@ def test_marker_line_empty_when_absent() -> None:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_pending_close_excludes_umbrella_with_closed_marker() -> None: async def test_pending_close_excludes_umbrella_with_closed_marker() -> None:
umbrella = MagicMock(id=uuid4(), quick_context=f"{_MARKER} closed=1") umbrella = _task(f"{_VALUE} closed=1", id=uuid4())
svc = _service(_scalars_all([umbrella])) svc = _service(_scalars_all([umbrella]))
_bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True)) _bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True))
assert await svc.supersede_umbrellas_pending_close() == [] assert await svc.supersede_umbrellas_pending_close() == []
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_pending_close_keeps_umbrella_with_closed_token_only_in_note() -> None: async def test_pending_close_keeps_open_landed_umbrella() -> None:
# A CEO note containing the literal "closed=1" must NOT retire the PR. umbrella = _task(_VALUE, id=uuid4())
umbrella = MagicMock(
id=uuid4(), quick_context=f"{_MARKER}\nceo_approval_notes: closed=1 elsewhere"
)
svc = _service(_scalars_all([umbrella])) svc = _service(_scalars_all([umbrella]))
_bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True)) _bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True))
out = await svc.supersede_umbrellas_pending_close() assert await svc.supersede_umbrellas_pending_close() == [umbrella]
assert out == [umbrella]
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_pending_close_requires_landed_replacement() -> None: async def test_pending_close_requires_landed_replacement() -> None:
# COMPLETED + no closed marker, but the replacement never landed (the code # COMPLETED + no closed marker, but the replacement never landed (the code
# subtask was cancelled) — close-on-land must skip it. # subtask was cancelled) — close-on-land must skip it.
umbrella = MagicMock(id=uuid4(), quick_context=_MARKER) umbrella = _task(_VALUE, id=uuid4())
svc = _service(_scalars_all([umbrella])) svc = _service(_scalars_all([umbrella]))
_bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=False)) _bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=False))
assert await svc.supersede_umbrellas_pending_close() == [] assert await svc.supersede_umbrellas_pending_close() == []
@@ -116,7 +103,6 @@ async def test_replacement_landed_true_for_completed_descendant_with_pr() -> Non
async def test_replacement_landed_false_when_descendant_cancelled() -> None: async def test_replacement_landed_false_when_descendant_cancelled() -> None:
child = MagicMock(id=uuid4(), status=TaskStatus.CANCELLED, pr_number=42) child = MagicMock(id=uuid4(), status=TaskStatus.CANCELLED, pr_number=42)
svc = _service(_scalars_all([child])) svc = _service(_scalars_all([child]))
# The `seen` guard terminates the walk even though the mock re-returns child.
assert await svc._supersede_replacement_landed(uuid4()) is False assert await svc._supersede_replacement_landed(uuid4()) is False
@@ -128,44 +114,37 @@ async def test_replacement_landed_false_when_completed_without_pr() -> None:
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# find_supersede_umbrella — marker-line dedup # find_supersede_umbrella — value dedup (pr=N review= prefix match)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_find_umbrella_matches_marker_not_note() -> None: async def test_find_umbrella_matches_by_value() -> None:
match = MagicMock(id=uuid4(), quick_context=f"{_MARKER}\nescalation_notes: x") match = _task(_VALUE, id=uuid4())
other = MagicMock( other = _task("pr=9 review=z", id=uuid4()) # different PR
id=uuid4(),
# marker for a different PR, but a note mentions "pr=5 review=" text
quick_context="external_pr_supersede pr=9 review=z\nnote: see pr=5 review= ok",
)
svc = _service(_scalars_all([other, match])) svc = _service(_scalars_all([other, match]))
found = await svc.find_supersede_umbrella(uuid4(), 5) found = await svc.find_supersede_umbrella(uuid4(), 5)
assert found is match assert found is match
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# mark_supersede_pr_closed — token written on the marker line # mark_supersede_pr_closed — token appended to the marker value
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_mark_closed_appends_token_to_marker_line() -> None: async def test_mark_closed_appends_token() -> None:
task = MagicMock(quick_context=f"{_MARKER}\nceo_approval_notes: shipped") task = _task(_VALUE)
svc = _service(_scalars_all([])) svc = _service(_scalars_all([]))
_bind(svc, "get", AsyncMock(return_value=task)) _bind(svc, "get", AsyncMock(return_value=task))
await svc.mark_supersede_pr_closed(uuid4()) await svc.mark_supersede_pr_closed(uuid4())
lines = task.quick_context.splitlines() assert supersede_marker_line(task) == f"{_VALUE} closed=1"
assert lines[0] == f"{_MARKER} closed=1"
assert lines[1] == "ceo_approval_notes: shipped" # note untouched
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_mark_closed_is_idempotent_on_marker_line() -> None: async def test_mark_closed_is_idempotent() -> None:
task = MagicMock(quick_context=f"{_MARKER} closed=1\nceo_approval_notes: x") task = _task(f"{_VALUE} closed=1")
svc = _service(_scalars_all([])) svc = _service(_scalars_all([]))
_bind(svc, "get", AsyncMock(return_value=task)) _bind(svc, "get", AsyncMock(return_value=task))
await svc.mark_supersede_pr_closed(uuid4()) await svc.mark_supersede_pr_closed(uuid4())
# No second closed=1 token appended. assert supersede_marker_line(task) == f"{_VALUE} closed=1"
assert task.quick_context.splitlines()[0] == f"{_MARKER} closed=1"