From 0fba4eed012db49c1937ea9f0dab787a2a2543fb Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 21 Jun 2026 02:48:07 +0200 Subject: [PATCH] refactor(content): move orchestration markers off quick_context to typed jsonb --- .../041_structured_content_columns.py | 3 + roboco/api/routes/tasks.py | 4 +- roboco/foundation/policy/content/markers.py | 14 +- roboco/runtime/orchestrator.py | 8 +- .../services/gateway/choreographer/_impl.py | 10 +- roboco/services/gateway/choreographer/doc.py | 10 +- roboco/services/gateway/choreographer/qa.py | 10 +- roboco/services/self_heal_engine.py | 5 +- roboco/services/task.py | 222 +++++++----------- tests/integration/test_lifecycle_real_db.py | 2 +- .../test_task_service_background.py | 2 +- .../test_task_service_lifecycle_misc.py | 4 +- tests/integration/test_task_service_misc.py | 52 ++-- .../test_task_service_route_orchestration.py | 4 +- .../test_task_service_transitions.py | 8 +- tests/integration/test_tasks_routes.py | 4 +- .../unit/services/test_external_pr_ingest.py | 70 ++++-- tests/unit/services/test_pr_review_db.py | 12 +- tests/unit/services/test_required_cells.py | 51 ++-- .../services/test_self_heal_originate_db.py | 2 +- .../unit/services/test_supersede_umbrella.py | 89 +++---- 21 files changed, 270 insertions(+), 316 deletions(-) diff --git a/alembic/versions/041_structured_content_columns.py b/alembic/versions/041_structured_content_columns.py index a2efee78..1df876c7 100644 --- a/alembic/versions/041_structured_content_columns.py +++ b/alembic/versions/041_structured_content_columns.py @@ -44,6 +44,9 @@ _LINE_MARKERS: dict[str, tuple[str, bool]] = { "original_developer:": ("original_developer", False), "documenter:": ("documenter", False), "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). _TOKEN_MARKERS: dict[str, str] = { diff --git a/roboco/api/routes/tasks.py b/roboco/api/routes/tasks.py index 65e83698..dd6ccb5e 100644 --- a/roboco/api/routes/tasks.py +++ b/roboco/api/routes/tasks.py @@ -1361,7 +1361,7 @@ async def pass_qa( # QA cannot review their own tasks (prevent self-review) # 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: audit = get_audit_service() @@ -1442,7 +1442,7 @@ async def fail_qa( # QA cannot review their own tasks (prevent self-review) # 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: raise HTTPException( diff --git a/roboco/foundation/policy/content/markers.py b/roboco/foundation/policy/content/markers.py index 190d0ac8..b27e2ab0 100644 --- a/roboco/foundation/policy/content/markers.py +++ b/roboco/foundation/policy/content/markers.py @@ -33,20 +33,24 @@ DISMISSED = "dismissed" 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: - markers = dict(task.orchestration_markers or {}) + om = getattr(task, "orchestration_markers", None) + markers = dict(om) if isinstance(om, dict) else {} markers[key] = value task.orchestration_markers = markers def clear_marker(task: HasMarkers, key: str) -> None: - current = task.orchestration_markers or {} - if key not in current: + om = getattr(task, "orchestration_markers", None) + if not isinstance(om, dict) or key not in om: return - markers = dict(current) + markers = dict(om) del markers[key] task.orchestration_markers = markers or None diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index f63f47eb..bd8ad378 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -8157,8 +8157,6 @@ Never `commit`, never write code, never run `git`. PMs coordinate. - original_developer if pr_created=False (tracked in quick_context as "original_developer:") """ - from roboco.services.task import extract_original_developer - # Fetch both `awaiting_documentation` and `claimed` because the # doc's claim transitions status from awaiting_documentation → # 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: if self._is_task_handled_this_tick(task.get("id")): continue - await self._doc_dispatch_one(client, task, extract_original_developer) + await self._doc_dispatch_one(client, task) async def _auto_assign_doc( 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, client: httpx.AsyncClient, task: dict[str, Any], - extract_original_developer: Any, ) -> None: """Process a single task for `_dispatch_doc_work`.""" team = task.get("team") if team not in ["backend", "frontend", "ux_ui"]: return - quick_context = task.get("quick_context") or "" - dev_uuid = extract_original_developer(quick_context) + dev_uuid = (task.get("orchestration_markers") or {}).get("original_developer") status = task.get("status") # Only consider `claimed` tasks actually in the doc/PR parallel diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 86ab43b9..5731a9c6 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -21,6 +21,7 @@ import structlog from roboco.exceptions import MergeConflictError 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.claim_guards import ( 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 original developer cannot self-doc). """ - qc = getattr(task, "quick_context", None) or "" - 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 + return markers.get_original_developer(task) @dataclass(frozen=True) diff --git a/roboco/services/gateway/choreographer/doc.py b/roboco/services/gateway/choreographer/doc.py index f1fa26af..5759f1bd 100644 --- a/roboco/services/gateway/choreographer/doc.py +++ b/roboco/services/gateway/choreographer/doc.py @@ -40,6 +40,7 @@ from typing import TYPE_CHECKING, Any from roboco.config import settings from roboco.foundation.policy import lifecycle as spec_module from roboco.foundation.policy import tracing as _tr +from roboco.foundation.policy.content import markers from roboco.models.task import DocRef from roboco.services.gateway.envelope import Envelope 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`` and the mixin builds the Context. """ - qc = getattr(task, "quick_context", None) or "" - 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 + return markers.get_original_developer(task) class DocMixin(_Base): diff --git a/roboco/services/gateway/choreographer/qa.py b/roboco/services/gateway/choreographer/qa.py index b8d0042b..afdf1db7 100644 --- a/roboco/services/gateway/choreographer/qa.py +++ b/roboco/services/gateway/choreographer/qa.py @@ -42,6 +42,7 @@ from typing import TYPE_CHECKING, Any from roboco.config import settings from roboco.foundation.policy import lifecycle as spec_module 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.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`` and the mixins build the Context. """ - qc = getattr(task, "quick_context", None) or "" - 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 + return markers.get_original_developer(task) class QAMixin(_Base): diff --git a/roboco/services/self_heal_engine.py b/roboco/services/self_heal_engine.py index b776ea7f..14a01851 100644 --- a/roboco/services/self_heal_engine.py +++ b/roboco/services/self_heal_engine.py @@ -29,6 +29,7 @@ from typing import TYPE_CHECKING, cast from roboco.config import settings 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.services.base import BaseService from roboco.services.notification import NotificationService @@ -143,7 +144,7 @@ class SelfHealEngine(BaseService): open_tasks = await task_svc.list_open_self_heal_tasks() open_fps: set[str] = set() for existing in open_tasks: - fp = extract_self_heal_fingerprint(existing.quick_context) + fp = extract_self_heal_fingerprint(existing) if fp: open_fps.add(fp) open_count = len(open_tasks) @@ -196,7 +197,7 @@ class SelfHealEngine(BaseService): ) # Carry the fingerprint so a later cycle sees this regression already # 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() open_fps.add(obs.fingerprint) open_count += 1 diff --git a/roboco/services/task.py b/roboco/services/task.py index 6d64397c..9ac8c6c2 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -32,6 +32,8 @@ from roboco.enforcement import ( validate_task_transition, ) 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 ( AgentRole, AgentStatus, @@ -54,6 +56,7 @@ from roboco.services.base import ( UnauthorizedError, ValidationError, ) +from roboco.services.content_notes import apply_structured_note from roboco.utils.converters import require_uuid, to_python_uuid if TYPE_CHECKING: @@ -301,68 +304,41 @@ def _get_valid_claim_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. - - 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 + dev_id = markers.get_original_developer(task) + if not dev_id: return None + if len(dev_id) == _UUID_LENGTH and dev_id.count("-") == _UUID_HYPHEN_COUNT: + return dev_id return None -_REQUIRED_CELLS_PREFIX = "required_cells:" - - def _normalize_cell(value: object) -> str: """Normalize a team/cell token for comparison (e.g. backend, frontend, ux_ui).""" raw = str(getattr(value, "value", value)).strip().lower() return raw.replace("/", "_").replace("-", "_").replace(" ", "") -def extract_required_cells(quick_context: str | None) -> list[str]: - """Cells the brief explicitly named, from a ``required_cells:`` marker line. +def extract_required_cells(task: Any) -> list[str]: + """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 - collapse one into a neighbour — see commit 60de3499). The marker is a single - line, e.g. ``required_cells: backend, frontend, ux_ui``. Absent → no - constraint (the gate is inert). Returns normalized, de-duplicated cells in - marker order. + collapse one into a neighbour — see commit 60de3499). Stored in + ``orchestration_markers`` (migration 041). Absent → no constraint (the gate + is inert). Returns normalized, de-duplicated cells in marker order. """ - if not quick_context: - return [] - for raw in quick_context.splitlines(): - line = raw.strip() - if not line.lower().startswith(_REQUIRED_CELLS_PREFIX): - continue - 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 [] + seen: list[str] = [] + for tok in markers.get_required_cells(task): + cell = _normalize_cell(tok) + if cell and cell not in seen: + seen.append(cell) + return seen # 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. 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. - -def extract_self_heal_fingerprint(quick_context: str | None) -> str | None: - """The ``self_heal_fp=`` marker from quick_context, or None. - - 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. + The per-signal dedupe key carried on a self-heal task (in + ``orchestration_markers`` after migration 041), so the loop can tell a + regression already has an open fix task. """ - for token in (quick_context or "").split(): - if token.startswith(_SELF_HEAL_FP_PREFIX): - return token[len(_SELF_HEAL_FP_PREFIX) :] or None - return None + return markers.get_self_heal_fingerprint(task) -_SUPERSEDE_MARKER_PREFIX = "external_pr_supersede" +def supersede_marker_line(task: Any) -> str: + """The supersede marker value, or "" if none. - -def supersede_marker_line(quick_context: str | None) -> str: - """Return the supersede marker line from a (multi-writer) quick_context. - - 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`). + ``pr={n} review={uuid}`` plus a ``closed=1`` token once the contributor PR is + retired. Stored in ``orchestration_markers`` (migration 041); dedup and + close-state checks parse this value (``needle in ...`` / ``"closed=1" in + ....split()``) exactly as before. """ - for raw in (quick_context or "").splitlines(): - line = raw.strip() - if line.startswith(_SUPERSEDE_MARKER_PREFIX): - return line - return "" + return markers.get_external_pr_supersede(task) or "" class TaskService(BaseService): @@ -705,21 +666,20 @@ class TaskService(BaseService): open a fresh review for the change). """ result = await self.session.execute( - select(TaskTable.quick_context).where( + select(TaskTable.orchestration_markers).where( TaskTable.project_id == project_id, TaskTable.source.in_(PR_REVIEW_SOURCES), TaskTable.pr_number == pr_number, ) ) - contexts = result.scalars().all() - if not contexts: + marker_rows = result.scalars().all() + if not marker_rows: return False if not head_sha: return True - marker = f"external_pr_head={head_sha}" - for qc in contexts: - text = qc or "" - if marker in text or "external_pr_head=" not in text: + for om in marker_rows: + stored = (om or {}).get("external_pr_head") + if not stored or stored == head_sha: return True return False @@ -787,7 +747,7 @@ class TaskService(BaseService): # Record the reviewed head commit so a later push (new SHA) re-reviews, # while an unchanged PR is skipped (see external_review_task_exists). if head_sha: - task.quick_context = f"external_pr_head={head_sha}" + markers.set_external_pr_head(task, head_sha) await self.session.flush() return task @@ -842,7 +802,7 @@ class TaskService(BaseService): return [ t 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]: @@ -873,7 +833,7 @@ class TaskService(BaseService): return [ t 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: @@ -886,8 +846,8 @@ class TaskService(BaseService): task = await self.get(task_id) if task is None or getattr(task, "source", "") not in PR_REVIEW_SOURCES: return None - if "dismissed=1" not in (task.quick_context or "").split(): - task.quick_context = f"{task.quick_context or ''} dismissed=1".strip() + if not markers.is_dismissed(task): + markers.mark_dismissed(task) await self.session.flush() return task @@ -1010,8 +970,8 @@ class TaskService(BaseService): # rule), not the default branch. The marker links back to the review + # contributor PR for dedup and close-on-land (no parent link needed). umbrella.branch_name = branch_name - umbrella.quick_context = ( - f"external_pr_supersede pr={pr_number} review={review_task_id}" + markers.set_external_pr_supersede( + umbrella, f"pr={pr_number} review={review_task_id}" ) await self.session.flush() self.log.info( @@ -1040,7 +1000,7 @@ class TaskService(BaseService): ) needle = f"pr={pr_number} review=" 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 None @@ -1063,7 +1023,7 @@ class TaskService(BaseService): ) pending: list[TaskTable] = [] 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 if not await self._supersede_replacement_landed(cast("UUID", task.id)): continue @@ -1106,15 +1066,9 @@ class TaskService(BaseService): task = await self.get(task_id) if task is None: return - lines = (task.quick_context or "").splitlines() - for i, raw in enumerate(lines): - if raw.strip().startswith(_SUPERSEDE_MARKER_PREFIX): - 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) + current = markers.get_external_pr_supersede(task) or "" + if "closed=1" not in current.split(): + markers.set_external_pr_supersede(task, f"{current} closed=1".strip()) await self.session.flush() 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) if role not in ("qa", "documenter"): 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): return "cannot review your own work (self-review)" return None @@ -1689,14 +1643,13 @@ class TaskService(BaseService): role = agent.role.value if hasattr(agent.role, "value") else str(agent.role) if role not in ("qa", "documenter"): return - existing_context = task.quick_context or "" - if "original_developer:" in existing_context: + if markers.get_original_developer(task): return # 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 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]] = { TaskStatus.PENDING, @@ -2469,7 +2422,7 @@ class TaskService(BaseService): async def _index_qa_review_background( self, task_id: UUID, - quick_context: str | None, + original_developer: str | None, passed: bool, qa_notes: str, qa_agent_id: UUID | None, @@ -2480,7 +2433,7 @@ class TaskService(BaseService): try: optimal = await get_optimal_service() - original_dev = extract_original_developer(quick_context) + original_dev = original_developer await optimal.record_review( IndexReviewParams( @@ -3391,7 +3344,7 @@ class TaskService(BaseService): # record for self-review prevention (QA can't review own work). original_dev = str(task.assigned_to) if task.assigned_to else None 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 # `task.awaiting_qa` audit row is attributed to the dev who @@ -3481,7 +3434,7 @@ class TaskService(BaseService): bg_task = asyncio.create_task( self._index_qa_review_background( require_uuid(task.id), - task.quick_context, + extract_original_developer(task), True, notes or "Passed QA review", to_python_uuid(qa_agent_id), @@ -3533,7 +3486,7 @@ class TaskService(BaseService): qa_agent_id = task.assigned_to # 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: task.assigned_to = 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( self._index_qa_review_background( require_uuid(task.id), - task.quick_context, + extract_original_developer(task), False, notes, to_python_uuid(qa_agent_id), @@ -3665,27 +3618,29 @@ class TaskService(BaseService): @staticmethod 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: return - task.quick_context = _append_capped( - task.quick_context, f"doc_notes:{doc_notes}" - ) + if (task.notes_structured or {}).get("doc"): + return + try: + apply_structured_note(task, "doc", {"summary": doc_notes}) + except ContentValidationError: + return @staticmethod 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: return - existing_context = task.quick_context or "" - if "documenter:" in existing_context: + if markers.get_documenter(task): return - doc_context = f"documenter:{task.assigned_to}" - task.quick_context = ( - f"{existing_context}\n{doc_context}".strip() - if existing_context - else doc_context - ) + markers.set_documenter(task, task.assigned_to) async def _resolve_pm_for_review(self, task: TaskTable) -> UUID | None: """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_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 from roboco.enforcement.task_lifecycle import check_parallel_completion @@ -4620,7 +4564,7 @@ class TaskService(BaseService): task_id=str(task_id), ) else: - original_dev = extract_original_developer(task.quick_context) + original_dev = extract_original_developer(task) if original_dev: task.assigned_to = 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) if parent is None: return [] - required = extract_required_cells(parent.quick_context) + required = extract_required_cells(parent) if not required: return [] children = await self.get_subtasks(parent_task_id) @@ -5548,7 +5492,7 @@ class TaskService(BaseService): # QA / Documenter cannot claim what they themselves developed. 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: raise UnauthorizedError( action="claim", @@ -5670,7 +5614,7 @@ class TaskService(BaseService): 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: from roboco.services.audit import get_audit_service diff --git a/tests/integration/test_lifecycle_real_db.py b/tests/integration/test_lifecycle_real_db.py index d4a31d2d..a3b92c6c 100644 --- a/tests/integration/test_lifecycle_real_db.py +++ b/tests/integration/test_lifecycle_real_db.py @@ -548,7 +548,7 @@ async def test_qa_fail_path( # spec layer's slug-based self-review check is a separate code path # (``_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. - task.quick_context = f"original_developer:{dev_agent.id}" + task.orchestration_markers = {"original_developer": str(dev_agent.id)} await db_session.flush() task_service = TaskService(db_session) diff --git a/tests/integration/test_task_service_background.py b/tests/integration/test_task_service_background.py index 21ff10b0..a0de10fd 100644 --- a/tests/integration/test_task_service_background.py +++ b/tests/integration/test_task_service_background.py @@ -574,7 +574,7 @@ async def test_index_qa_review_calls_record_review( dev_id = uuid4() await svc._index_qa_review_background( uuid4(), - f"original_developer:{dev_id}", + str(dev_id), passed=True, qa_notes="LGTM", qa_agent_id=uuid4(), diff --git a/tests/integration/test_task_service_lifecycle_misc.py b/tests/integration/test_task_service_lifecycle_misc.py index c2fa2dac..9781b610 100644 --- a/tests/integration/test_task_service_lifecycle_misc.py +++ b/tests/integration/test_task_service_lifecycle_misc.py @@ -669,7 +669,7 @@ async def test_submit_for_qa_clears_assignment_and_records_dev( assert out is not None assert out.status == TaskStatus.AWAITING_QA 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"] task = await svc.create(_req(task_setup)) 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() fake_optimal = MagicMock() fake_optimal.record_review = AsyncMock() diff --git a/tests/integration/test_task_service_misc.py b/tests/integration/test_task_service_misc.py index 29afbbad..f8855330 100644 --- a/tests/integration/test_task_service_misc.py +++ b/tests/integration/test_task_service_misc.py @@ -25,6 +25,7 @@ Targets: from __future__ import annotations import asyncio +from types import SimpleNamespace from typing import TYPE_CHECKING, Any, cast from unittest.mock import AsyncMock, MagicMock 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.permissions import AgentContext from roboco.models.task import TaskCreateRequest +from roboco.foundation.policy.content import markers from roboco.models.work_session import WorkSessionStatus from roboco.services.base import ValidationError 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: - """Invalid UUID format returns None even when prefix matches.""" - out = extract_original_developer("original_developer:not-a-uuid") - assert out is None + """Invalid UUID format returns None even when the marker is present.""" + task = SimpleNamespace(orchestration_markers={"original_developer": "not-a-uuid"}) + assert extract_original_developer(task) is None def test_extract_original_developer_no_match() -> None: - out = extract_original_developer("some other context") - assert out is None + task = SimpleNamespace(orchestration_markers={"documenter": "x"}) + assert extract_original_developer(task) is None def test_extract_original_developer_empty() -> None: - assert extract_original_developer(None) is None - assert extract_original_developer("") is None + assert extract_original_developer(SimpleNamespace(orchestration_markers=None)) is None + assert extract_original_developer(SimpleNamespace(orchestration_markers={})) is None def test_extract_original_developer_valid() -> None: test_uuid = "12345678-1234-1234-1234-123456789012" - out = extract_original_developer(f"original_developer:{test_uuid}") - assert out == test_uuid + task = SimpleNamespace(orchestration_markers={"original_developer": 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"] aid = uuid4() task = MagicMock() - task.quick_context = f"original_developer:{aid}" + task.orchestration_markers = {"original_developer": str(aid)} agent = MagicMock(role=AgentRole.QA) out = svc._validate_not_self_review(task, agent, agent_id=aid) 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: svc = task_setup["svc"] task = MagicMock() - task.quick_context = "original_developer:already-set" + task.orchestration_markers = {"original_developer": "already-set"} task.assigned_to = uuid4() agent = MagicMock(role=AgentRole.QA, id=uuid4()) - # Should not change quick_context - before = task.quick_context + # An existing original_developer marker must not be overwritten. + before = dict(task.orchestration_markers) 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: @@ -731,10 +733,10 @@ def test_record_documenter_context_skips_when_already(task_setup: dict) -> None: svc = task_setup["svc"] task = MagicMock() task.assigned_to = uuid4() - task.quick_context = "documenter:something" - before = task.quick_context + task.orchestration_markers = {"documenter": "something"} + before = dict(task.orchestration_markers) 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: @@ -743,9 +745,10 @@ def test_record_documenter_context_appends(task_setup: dict) -> None: task = MagicMock() task.assigned_to = aid task.quick_context = "existing" + task.orchestration_markers = None svc._record_documenter_context(task) - assert "documenter:" in task.quick_context - assert "existing" in task.quick_context + assert markers.get_documenter(task) == str(aid) + assert task.quick_context == "existing" # human field untouched 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() task = MagicMock() task.assigned_to = aid - task.quick_context = None + task.orchestration_markers = None 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: @@ -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: - """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"] task = MagicMock() - task.quick_context = "" + task.orchestration_markers = None other_id = uuid4() task.assigned_to = other_id agent = MagicMock(role=AgentRole.QA, id=uuid4()) svc._set_original_developer_context(task, agent) - assert "original_developer:" in task.quick_context - assert str(other_id) in task.quick_context + assert markers.get_original_developer(task) == str(other_id) @pytest.mark.asyncio diff --git a/tests/integration/test_task_service_route_orchestration.py b/tests/integration/test_task_service_route_orchestration.py index 0b92b302..8dbccb2e 100644 --- a/tests/integration/test_task_service_route_orchestration.py +++ b/tests/integration/test_task_service_route_orchestration.py @@ -188,7 +188,7 @@ async def test_claim_task_for_agent_self_review_rejected( svc = task_setup["svc"] qa_id = uuid4() 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() agent_ctx = _ctx(qa_id, AgentRole.QA) perms = _Permissions(can_claim=True) @@ -423,7 +423,7 @@ async def test_docs_complete_for_task_self_documentation_blocked( svc = task_setup["svc"] doc_id = task_setup["agent_id"] 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() agent_ctx = _ctx(doc_id, AgentRole.DOCUMENTER) audit_mock = AsyncMock() diff --git a/tests/integration/test_task_service_transitions.py b/tests/integration/test_task_service_transitions.py index f3736679..4412a5af 100644 --- a/tests/integration/test_task_service_transitions.py +++ b/tests/integration/test_task_service_transitions.py @@ -527,7 +527,7 @@ async def test_fail_qa_reassigns_to_original_developer( dev_id = task_setup["agent_id"] task = await svc.create(_req(task_setup)) 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() failed = await svc.fail_qa(task.id, notes="missing tests") assert failed is not None @@ -597,7 +597,7 @@ async def test_ceo_reject_reassigns_to_original_dev( dev_id = task_setup["agent_id"] task = await svc.create(_req(task_setup)) 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() rejected = await svc.ceo_reject(task.id, reason="re-do auth flow") 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.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() 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.status = TaskStatus.AWAITING_QA 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() out = await svc.claim(task.id, qa_agent.id) assert out is None diff --git a/tests/integration/test_tasks_routes.py b/tests/integration/test_tasks_routes.py index c68b2aad..ad5d69ff 100644 --- a/tests/integration/test_tasks_routes.py +++ b/tests/integration/test_tasks_routes.py @@ -2050,7 +2050,7 @@ async def test_pass_qa_self_review_forbidden(qa_client: dict) -> None: task = _seed_task_qa( qa_client, 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() 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.""" task = _seed_task_qa( 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() response = await qa_client["client"].post( diff --git a/tests/unit/services/test_external_pr_ingest.py b/tests/unit/services/test_external_pr_ingest.py index 5c093059..c8dc238a 100644 --- a/tests/unit/services/test_external_pr_ingest.py +++ b/tests/unit/services/test_external_pr_ingest.py @@ -1,28 +1,50 @@ """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 -unchanged PR (same head) is skipped, new commits (a new head SHA) open a fresh -review, and legacy/unknown-SHA tasks are never re-reviewed (no spam). +``external_review_task_exists`` drives re-review off the PR's head SHA, stored as +the ``external_pr_head`` orchestration marker (migration 041); dismissal is the +``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 types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 import pytest +from roboco.foundation.policy.content import markers from roboco.services.task import TaskService -def _service(quick_contexts: list[str | None]) -> TaskService: - """A TaskService whose review-task query returns these quick_context values.""" +def _service(scalar_rows: list[object]) -> TaskService: + """A TaskService whose next query returns these scalar rows.""" res = MagicMock() - res.scalars.return_value.all.return_value = quick_contexts + res.scalars.return_value.all.return_value = scalar_rows session = MagicMock() session.execute = AsyncMock(return_value=res) + session.flush = AsyncMock() 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 async def test_no_task_yet_ingests() -> None: svc = _service([]) @@ -31,14 +53,14 @@ async def test_no_task_yet_ingests() -> None: @pytest.mark.asyncio 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 @pytest.mark.asyncio async def test_new_head_sha_rereviews() -> None: # 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 @@ -52,25 +74,26 @@ async def test_legacy_markerless_task_not_rereviewed() -> None: @pytest.mark.asyncio async def test_unknown_head_sha_does_not_spam() -> None: # 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 @pytest.mark.asyncio 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, "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 async def test_list_awaiting_decision_excludes_dismissed() -> None: - pending = MagicMock(quick_context="external_pr_head=abc") - dismissed = MagicMock(quick_context="external_pr_head=def dismissed=1") + pending = SimpleNamespace(orchestration_markers=_markers("abc")) + dismissed = SimpleNamespace(orchestration_markers=_markers("def", dismissed=True)) svc = _service([pending, dismissed]) out = await svc.list_external_pr_reviews_awaiting_decision() assert out == [pending] @@ -78,10 +101,8 @@ async def test_list_awaiting_decision_excludes_dismissed() -> None: @pytest.mark.asyncio async def test_list_external_pr_reviews_excludes_dismissed() -> None: - # The panel queue surfaces in-flight reviews too (the status filter lives in - # SQL); here we pin the post-query behavior: dismissed reviews drop out. - reviewing = MagicMock(quick_context="external_pr_head=abc") - dismissed = MagicMock(quick_context="external_pr_head=def dismissed=1") + reviewing = SimpleNamespace(orchestration_markers=_markers("abc")) + dismissed = SimpleNamespace(orchestration_markers=_markers("def", dismissed=True)) svc = _service([reviewing, dismissed]) out = await svc.list_external_pr_reviews() assert out == [reviewing] @@ -89,15 +110,14 @@ async def test_list_external_pr_reviews_excludes_dismissed() -> None: @pytest.mark.asyncio async def test_dismiss_marks_and_is_idempotent() -> None: - task = MagicMock(source="external_pr", quick_context="external_pr_head=abc") - session = MagicMock() - session.flush = AsyncMock() - svc = TaskService(session) + task = SimpleNamespace(source="external_pr", orchestration_markers=_markers("abc")) + svc = _service([]) _bind(svc, "get", AsyncMock(return_value=task)) 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 - 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 @@ -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 # --------------------------------------------------------------------------- diff --git a/tests/unit/services/test_pr_review_db.py b/tests/unit/services/test_pr_review_db.py index 65e45fea..8870e185 100644 --- a/tests/unit/services/test_pr_review_db.py +++ b/tests/unit/services/test_pr_review_db.py @@ -135,7 +135,7 @@ async def test_ingest_creates_review_task(db_session: AsyncSession) -> None: assert task.task_type == TaskType.CODE assert task.confirmed_by_human is False assert task.status == TaskStatus.PENDING - assert task.quick_context == "external_pr_head=abc123" + assert task.orchestration_markers == {"external_pr_head": "abc123"} @pytest.mark.asyncio @@ -187,7 +187,7 @@ async def test_ingest_new_head_rereviews(db_session: AsyncSession) -> None: await db_session.flush() 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() matching = [t for t in reviews if t.pr_number == EXTERNAL_PR] 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 fifty is not None assert UUID(str(five.id)) != UUID(str(fifty.id)) - assert "pr=5 review=" in (five.quick_context or "") - assert "pr=50 review=" in (fifty.quick_context or "") + assert "pr=5 review=" in (five.orchestration_markers or {}).get( + "external_pr_supersede", "" + ) + assert "pr=50 review=" in (fifty.orchestration_markers or {}).get( + "external_pr_supersede", "" + ) # --------------------------------------------------------------------------- diff --git a/tests/unit/services/test_required_cells.py b/tests/unit/services/test_required_cells.py index df6fd5c5..2e4091a0 100644 --- a/tests/unit/services/test_required_cells.py +++ b/tests/unit/services/test_required_cells.py @@ -1,36 +1,53 @@ """required_cells decomposition gate — marker parse + uncovered-cell coverage. 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 -gate is inert when no marker is present, so legacy decompositions never block. +(recorded as a ``required_cells`` orchestration marker on the parent). The gate +is inert when no marker is present, so legacy decompositions never block. """ from __future__ import annotations +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 import pytest 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: - assert extract_required_cells(None) == [] - assert extract_required_cells("original_developer: abc\ndoc_notes: y") == [] + assert extract_required_cells(_task()) == [] + assert ( + extract_required_cells( + SimpleNamespace(orchestration_markers={"original_developer": "abc"}) + ) + == [] + ) -def test_extract_required_cells_parses_and_normalizes() -> None: - qc = "original_developer: abc\nrequired_cells: Backend, Frontend , UX/UI" - assert extract_required_cells(qc) == ["backend", "frontend", "ux_ui"] +def test_extract_required_cells_normalizes() -> None: + assert extract_required_cells(_task(["Backend", "Frontend ", "UX/UI"])) == [ + "backend", + "frontend", + "ux_ui", + ] def test_extract_required_cells_dedups_in_order() -> None: - out = extract_required_cells("required_cells: backend, backend, frontend") - assert out == ["backend", "frontend"] + assert extract_required_cells(_task(["backend", "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.""" svc = TaskService(MagicMock()) - parent = MagicMock(quick_context=parent_qc) + parent = _task(required_cells) children = [MagicMock(team=t) for t in child_teams] object.__setattr__(svc, "get", AsyncMock(return_value=parent)) 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 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()) == [] @pytest.mark.asyncio async def test_uncovered_flags_the_dropped_cell() -> None: # 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"] @pytest.mark.asyncio 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()) == [] @pytest.mark.asyncio async def test_uncovered_normalizes_child_team_form() -> None: # 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()) == [] diff --git a/tests/unit/services/test_self_heal_originate_db.py b/tests/unit/services/test_self_heal_originate_db.py index f3def1af..d4b543da 100644 --- a/tests/unit/services/test_self_heal_originate_db.py +++ b/tests/unit/services/test_self_heal_originate_db.py @@ -156,7 +156,7 @@ async def test_originate_creates_pending_main_pm_assigned_task( assert task.team == Team.MAIN_PM assert task.source == "self_heal" 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 diff --git a/tests/unit/services/test_supersede_umbrella.py b/tests/unit/services/test_supersede_umbrella.py index a88fc8fa..3f3a57ba 100644 --- a/tests/unit/services/test_supersede_umbrella.py +++ b/tests/unit/services/test_supersede_umbrella.py @@ -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 -which — a landed supersede's contributor PR gets retired: - -- ``supersede_marker_line`` anchors marker/state checks to the marker line, so - 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. +The supersede marker (``pr={n} review={uuid}`` plus a ``closed=1`` token once +the contributor PR is retired) lives in ``orchestration_markers`` after +migration 041 — isolated from the human ``quick_context``, so CEO escalation / +approval notes can no longer be mistaken for it. """ from __future__ import annotations +from types import SimpleNamespace +from typing import Any from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 @@ -23,11 +17,10 @@ import pytest from roboco.models.base import TaskStatus 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: - """A session.execute return value whose .scalars().all() yields `rows`.""" res = MagicMock() res.scalars.return_value.all.return_value = rows return res @@ -44,25 +37,23 @@ def _bind(svc: TaskService, name: str, value: object) -> None: 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: - qc = f"{_MARKER}\nceo_approval_notes: shipped, looks good" - 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_returns_value() -> None: + assert supersede_marker_line(_task(_VALUE)) == _VALUE def test_marker_line_empty_when_absent() -> None: - assert supersede_marker_line("no marker here\nescalation_notes: x") == "" - assert supersede_marker_line(None) == "" + assert supersede_marker_line(_task()) == "" + assert supersede_marker_line(SimpleNamespace(orchestration_markers=None)) == "" # --------------------------------------------------------------------------- @@ -72,29 +63,25 @@ def test_marker_line_empty_when_absent() -> None: @pytest.mark.asyncio 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])) _bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True)) assert await svc.supersede_umbrellas_pending_close() == [] @pytest.mark.asyncio -async def test_pending_close_keeps_umbrella_with_closed_token_only_in_note() -> None: - # A CEO note containing the literal "closed=1" must NOT retire the PR. - umbrella = MagicMock( - id=uuid4(), quick_context=f"{_MARKER}\nceo_approval_notes: closed=1 elsewhere" - ) +async def test_pending_close_keeps_open_landed_umbrella() -> None: + umbrella = _task(_VALUE, id=uuid4()) svc = _service(_scalars_all([umbrella])) _bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True)) - out = await svc.supersede_umbrellas_pending_close() - assert out == [umbrella] + assert await svc.supersede_umbrellas_pending_close() == [umbrella] @pytest.mark.asyncio async def test_pending_close_requires_landed_replacement() -> None: # COMPLETED + no closed marker, but the replacement never landed (the code # 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])) _bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=False)) 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: child = MagicMock(id=uuid4(), status=TaskStatus.CANCELLED, pr_number=42) 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 @@ -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 -async def test_find_umbrella_matches_marker_not_note() -> None: - match = MagicMock(id=uuid4(), quick_context=f"{_MARKER}\nescalation_notes: x") - other = MagicMock( - 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", - ) +async def test_find_umbrella_matches_by_value() -> None: + match = _task(_VALUE, id=uuid4()) + other = _task("pr=9 review=z", id=uuid4()) # different PR svc = _service(_scalars_all([other, match])) found = await svc.find_supersede_umbrella(uuid4(), 5) 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 -async def test_mark_closed_appends_token_to_marker_line() -> None: - task = MagicMock(quick_context=f"{_MARKER}\nceo_approval_notes: shipped") +async def test_mark_closed_appends_token() -> None: + task = _task(_VALUE) svc = _service(_scalars_all([])) _bind(svc, "get", AsyncMock(return_value=task)) await svc.mark_supersede_pr_closed(uuid4()) - lines = task.quick_context.splitlines() - assert lines[0] == f"{_MARKER} closed=1" - assert lines[1] == "ceo_approval_notes: shipped" # note untouched + assert supersede_marker_line(task) == f"{_VALUE} closed=1" @pytest.mark.asyncio -async def test_mark_closed_is_idempotent_on_marker_line() -> None: - task = MagicMock(quick_context=f"{_MARKER} closed=1\nceo_approval_notes: x") +async def test_mark_closed_is_idempotent() -> None: + task = _task(f"{_VALUE} closed=1") svc = _service(_scalars_all([])) _bind(svc, "get", AsyncMock(return_value=task)) await svc.mark_supersede_pr_closed(uuid4()) - # No second closed=1 token appended. - assert task.quick_context.splitlines()[0] == f"{_MARKER} closed=1" + assert supersede_marker_line(task) == f"{_VALUE} closed=1"