From ed828a719bbaec1865c292fb82b532b3c09c7226 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sat, 16 May 2026 10:14:45 +0200 Subject: [PATCH] =?UTF-8?q?feat(gateway):=20substantive-plan=20gate=20?= =?UTF-8?q?=E2=80=94=20approach=20>=3D150=20+=20real=20sub=5Ftask=20descri?= =?UTF-8?q?ptions=20(#171)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plans were vague because the gate accepted the bare minimum: PM approach >=20 chars and title-only sub_tasks. minimax wrote exactly the minimum. - IWillPlanRequest.approach min_length 20 -> 150 (kept in sync with _PM_APPROACH_MIN_LEN; the gate enforces it at the choreographer layer too so direct/MCP callers can't bypass the HTTP boundary). - New _thin_subtask_hint: every PM sub_task must have a title and a description >= _PM_SUBTASK_DESC_MIN_LEN (60) saying what the step does — each sub_task is both a delegate target AND a progress-checklist item, so a title alone is not a plan. - cell_pm/main_pm role prompts: explicit "the gate REJECTS thin plans" framing + concrete sub_task example + the new minimums. - Updated all affected test fixtures across the suite to use substantive approaches/descriptions; added thin-sub_task rejection coverage. Commit 1 of 3 for the plan/progress quality work (#171/#172/#173). --- agents/prompts/roles/cell_pm.md | 2 +- agents/prompts/roles/main_pm.md | 2 +- roboco/api/schemas/v2/flow.py | 5 +- .../services/gateway/choreographer/_impl.py | 67 +++++++-- tests/unit/api/routes/v2/test_flow_cell_pm.py | 15 +- tests/unit/api/routes/v2/test_flow_main_pm.py | 23 ++- .../v2/test_i_will_plan_rich_required.py | 26 ++-- .../test_choreographer_claim_guards.py | 28 +++- .../test_choreographer_impl_branches.py | 54 ++++++- .../gateway/test_choreographer_pm_extras.py | 137 +++++++++++++++--- tests/unit/gateway/test_claim_arg_order.py | 21 ++- .../test_i_will_plan_persists_rich_plan.py | 53 +++++-- .../test_i_will_plan_sub_tasks_gate.py | 47 +++++- .../gateway/test_work_session_auto_create.py | 17 ++- 14 files changed, 415 insertions(+), 82 deletions(-) diff --git a/agents/prompts/roles/cell_pm.md b/agents/prompts/roles/cell_pm.md index 773c6028..a20ed4ec 100644 --- a/agents/prompts/roles/cell_pm.md +++ b/agents/prompts/roles/cell_pm.md @@ -20,7 +20,7 @@ You merge what your developers submit (leaf PRs into your cell branch via `compl | Verb | What it does | Preconditions | |---|---|---| | `give_me_work()` | Returns your highest-priority task (your own pending PM task, or a subtask in `awaiting_pm_review` for you to merge). | None. | -| `i_will_plan(task_id, plan, approach?, technical_considerations?, risks?, open_questions?)` | Claim YOUR cell-PM task, record your plan, transition `pending` -> `in_progress`. Always call this before `delegate`. **Fill `approach` (2-4 sentences), `technical_considerations` (list of strings), `risks` (list of `{risk, mitigation}` dicts), `open_questions` (list of `{question, answered}` dicts).** Empty values produce an empty Plan tab — a regression. | Task assigned to you; task in `pending`/`needs_revision`. | +| `i_will_plan(task_id, plan, approach, sub_tasks, technical_considerations?, risks?, open_questions?)` | Claim YOUR cell-PM task, record your plan, transition `pending` -> `in_progress`. Always call this before `delegate`. **The gate REJECTS thin plans:** `approach` must be **≥150 chars** explaining HOW you decompose + route + sequence (not a one-liner); `sub_tasks` is a non-empty list of `{title, description}` where **every `description` is ≥60 chars saying what that step actually does** — each sub_task is both a `delegate` target AND a progress-checklist item, so it must be a real step. Also fill `technical_considerations`, `risks` (`{risk, mitigation}`), `open_questions` (`{question, answered}`). Example sub_task: `{"title": "Add timestamp comment to README", "description": "be-dev-1 edits README.md, prepends an HTML comment above the H1, leaving the rest of the file untouched"}`. Empty/thin values are rejected, not just an empty Plan tab. | Task assigned to you; task in `pending`/`needs_revision`. | | `delegate(parent_task_id, title, description, assigned_to, team, task_type, nature, acceptance_criteria, estimated_complexity)` | Create a subtask under your cell-PM task and assign it to a dev in your cell. `nature` ∈ `technical`/`non_technical`. `task_type` for devs must be `code`/`documentation`/`research`. Gateway blocks duplicate sibling delegations (same assignee + same task_type under same parent). | Parent claimed by you and `in_progress`; assignee is a dev slug in your cell. | | `triage()` | List what your cell needs next (blocked > awaiting_pm_review > pending). | None. | | `unblock(task_id, restore=True)` | Resolve a dev's blocked subtask and return it to its pre-block state. | Subtask is in your cell. | diff --git a/agents/prompts/roles/main_pm.md b/agents/prompts/roles/main_pm.md index 185cbc93..348833b1 100644 --- a/agents/prompts/roles/main_pm.md +++ b/agents/prompts/roles/main_pm.md @@ -20,7 +20,7 @@ You merge what your Cell PMs submit (cell PRs into your root branch via `complet | Verb | What it does | Preconditions | |---|---|---| | `give_me_work()` | Returns your highest-priority task (your root in `pending`, or a cell-PM task in `awaiting_pm_review` for you to merge). | None. | -| `i_will_plan(task_id, plan, approach?, technical_considerations?, risks?, open_questions?)` | Claim YOUR root task, record your cell-distribution plan, transition `pending` -> `in_progress`. Always call this before `delegate`. **Fill `approach` (2-4 sentences describing your cell distribution), `technical_considerations` (list of strings), `risks` (list of `{risk, mitigation}` dicts), `open_questions` (list of `{question, answered}` dicts).** Empty values produce an empty Plan tab — a regression. | Task assigned to you; task in `pending`/`needs_revision`. | +| `i_will_plan(task_id, plan, approach, sub_tasks, technical_considerations?, risks?, open_questions?)` | Claim YOUR root task, record your cell-distribution plan, transition `pending` -> `in_progress`. Always call this before `delegate`. **The gate REJECTS thin plans:** `approach` must be **≥150 chars** describing HOW you split work across cells + sequencing + dependencies (not a one-liner); `sub_tasks` is a non-empty list of `{title, description}` where **every `description` is ≥60 chars** stating what that cell slice delivers — each sub_task is both a `delegate` target AND a progress-checklist item. Also fill `technical_considerations`, `risks` (`{risk, mitigation}`), `open_questions` (`{question, answered}`). Empty/thin values are rejected, not just an empty Plan tab. | Task assigned to you; task in `pending`/`needs_revision`. | | `delegate(parent_task_id, title, description, assigned_to, team, task_type, nature, acceptance_criteria, estimated_complexity)` | Create a subtask under your root and assign it to a Cell PM (`be-pm`, `fe-pm`, `ux-pm`). One subtask per cell that needs work. **`task_type` must be `planning`** (Cell PMs decompose; they don't execute). `nature` ∈ `technical`/`non_technical`. Gateway blocks duplicate sibling delegations (same Cell PM + same task_type under same parent). | Parent claimed by you and `in_progress`; assignee is a Cell PM slug. | | `triage_all()` | List blockers and reviews across all cells. | None. | | `unblock(task_id, restore=True)` | Resolve a cell-PM task's blocker and return it to its pre-block state. | None. | diff --git a/roboco/api/schemas/v2/flow.py b/roboco/api/schemas/v2/flow.py index 68ef63f8..27f35df9 100644 --- a/roboco/api/schemas/v2/flow.py +++ b/roboco/api/schemas/v2/flow.py @@ -129,7 +129,10 @@ class IWillPlanRequest(BaseModel): # could not transition claimed → in_progress without filling this in the # pre-gateway flow. The Plan tab depends on it; smoke run 3 confirmed # the empty default lets agents through with thin plans. - approach: str = Field(..., min_length=20) + # min_length must match choreographer._impl._PM_APPROACH_MIN_LEN. Raised + # 20→150 (smoke-15): a 20-char approach was a one-liner; the approach + + # sub_tasks are also the progress checklist, so they must be substantive. + approach: str = Field(..., min_length=150) sub_tasks: list[dict[str, str]] = Field( default_factory=list, description="List of {title, description} — server assigns id + order", diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 44f3de0c..c2d31c8f 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -47,9 +47,40 @@ from roboco.services.gateway.remediation import ( logger = structlog.get_logger() -# Minimum character length enforced on rich_plan["approach"] by the PM sub-tasks -# gate. Must match the Pydantic min_length on IWillPlanRequest.approach. -_PM_APPROACH_MIN_LEN = 20 +# Minimum character length enforced on rich_plan["approach"] by the PM +# sub-tasks gate. Must match the Pydantic min_length on +# IWillPlanRequest.approach. Raised 20→150 (smoke-15): plans were vague +# because 20 chars is a one-liner; the approach + sub_tasks are also the +# progress checklist, so they must be substantive. +_PM_APPROACH_MIN_LEN = 150 + +# Each PM sub_task is a real work step (it becomes a delegate target AND a +# progress-checklist item). A title alone is not a plan — require a +# description that actually says what the step does. +_PM_SUBTASK_DESC_MIN_LEN = 60 + + +def _thin_subtask_hint(sub_tasks: list[Any]) -> str | None: + """Return a hint if any PM sub_task is title-only / thin (#171). + + Each sub_task is a delegate target AND a progress-checklist item, so + a title with no real description is not a plan. Returns None when + every sub_task carries a title and a substantive description. + """ + for i, st in enumerate(sub_tasks): + if not isinstance(st, dict): + return f"sub_task #{i + 1} must be an object {{title, description}}." + title = str(st.get("title", "")).strip() + desc = str(st.get("description") or "").strip() + if not title: + return f"sub_task #{i + 1} has no title." + if len(desc) < _PM_SUBTASK_DESC_MIN_LEN: + return ( + f"sub_task #{i + 1} ('{title[:40]}') description is too thin " + f"({len(desc)} chars) — need >= {_PM_SUBTASK_DESC_MIN_LEN} " + "characters describing what the step actually does." + ) + return None def _normalize_sub_task(st: dict[str, Any], order: int) -> dict[str, Any]: @@ -429,14 +460,18 @@ class Choreographer: task_id: UUID, briefing: dict[str, Any], ) -> Envelope | None: - """Wave A1 gate: PMs must supply approach (>= 20 chars) and sub_tasks. + """Wave A1 gate: PMs must supply a substantive approach + sub_tasks. Enforces both fields at the choreographer layer so direct service-layer callers (MCP server, test fixtures, orchestrator-internal Python) cannot persist a plan that bypassed the HTTP Pydantic boundary. - Returns a rejection Envelope when the caller is a PM role and either - field is absent/insufficient; returns None to signal the gate passed. + Smoke-15: a 20-char approach and title-only sub_tasks were "no + effort" plans. approach must be >= _PM_APPROACH_MIN_LEN and every + sub_task needs a real title + a description that says what the + step does (it is both a delegate target and a progress-checklist + item). Returns a rejection Envelope when the caller is a PM role + and any field is absent/thin; returns None when the gate passed. """ if role_str not in ("cell_pm", "main_pm"): return None @@ -446,16 +481,21 @@ class Choreographer: if len(str(approach_raw).strip()) < _PM_APPROACH_MIN_LEN: missing.append("approach") field_hints["approach"] = ( - "approach must be a non-empty string of at least 20 characters " - "describing how the PM will decompose and route this task." + f"approach must be a non-empty string of at least " + f"{_PM_APPROACH_MIN_LEN} characters describing HOW you will " + "decompose and route this task — not a one-liner." ) - if not (rich_plan and rich_plan.get("sub_tasks")): + sub_tasks = (rich_plan or {}).get("sub_tasks") or [] + if not sub_tasks: missing.append("sub_tasks") field_hints["sub_tasks"] = ( - "PMs must list at least one sub_task — a " - "non-empty list of {title, description}. " - "Each becomes a delegate target after i_will_plan." + "PMs must list at least one sub_task — a non-empty list of " + "{title, description}. Each becomes a delegate target AND a " + "progress-checklist item." ) + elif thin := _thin_subtask_hint(sub_tasks): + missing.append("sub_tasks") + field_hints["sub_tasks"] = thin if not missing: return None return await self._emit_rejection( @@ -465,7 +505,8 @@ class Choreographer: remediate=( "re-issue i_will_plan(task_id, plan, approach, " "sub_tasks=[{'title': '...', 'description': '...'}, ...]) " - "with approach >= 20 chars and a non-empty sub_tasks list." + f"with approach >= {_PM_APPROACH_MIN_LEN} chars and every " + f"sub_task description >= {_PM_SUBTASK_DESC_MIN_LEN} chars." ), context_briefing=briefing, ).with_introspection(task=task, role=role_str), diff --git a/tests/unit/api/routes/v2/test_flow_cell_pm.py b/tests/unit/api/routes/v2/test_flow_cell_pm.py index d3dd9947..08bf59aa 100644 --- a/tests/unit/api/routes/v2/test_flow_cell_pm.py +++ b/tests/unit/api/routes/v2/test_flow_cell_pm.py @@ -237,11 +237,20 @@ async def test_i_will_plan_dispatches_to_choreographer() -> None: "task_id": _TASK_ID, "plan": "break into 3 subtasks for backend", "approach": ( - "Decompose into backend API slice, QA verification pass, " - "and documentation update." + "Decompose into a backend API slice that be-dev-1 owns end " + "to end; QA reviews after the PR opens, documentation follows, " + "then be-pm completes and submits up. Single-cell — no " + "frontend or ux work; strict sequencing with no cross-cell " + "dependencies for this planning task." ), "sub_tasks": [ - {"title": "Backend API slice", "description": "Implement endpoint"} + { + "title": "Backend API slice", + "description": ( + "be-dev-1 implements the endpoint with tests, commits " + "with the task-id prefix, opens the leaf PR for QA." + ), + } ], }, headers=_HEADERS, diff --git a/tests/unit/api/routes/v2/test_flow_main_pm.py b/tests/unit/api/routes/v2/test_flow_main_pm.py index 7211d9ff..c69b58b8 100644 --- a/tests/unit/api/routes/v2/test_flow_main_pm.py +++ b/tests/unit/api/routes/v2/test_flow_main_pm.py @@ -208,12 +208,27 @@ async def test_i_will_plan_dispatches_to_choreographer() -> None: "task_id": _TASK_ID, "plan": "split into backend, frontend, ux cells", "approach": ( - "Three-cell decomposition: backend handles API, frontend " - "handles UI integration, ux-ui handles design." + "Three-cell decomposition: backend handles the API, frontend " + "handles UI integration, ux-ui handles design. Sequenced so " + "backend lands first, QA reviews each PR after it opens, " + "documentation follows, then complete and submit up. No " + "cross-cell dependencies beyond the stated ordering." ), "sub_tasks": [ - {"title": "Backend cell", "description": "API implementation"}, - {"title": "Frontend cell", "description": "UI integration"}, + { + "title": "Backend cell", + "description": ( + "be-dev-1 implements the API endpoint and migration " + "with tests, opens the leaf PR for QA." + ), + }, + { + "title": "Frontend cell", + "description": ( + "fe-dev-1 wires the panel to the new endpoint with " + "loading and error states, opens the leaf PR." + ), + }, ], }, headers=_HEADERS, diff --git a/tests/unit/api/routes/v2/test_i_will_plan_rich_required.py b/tests/unit/api/routes/v2/test_i_will_plan_rich_required.py index 138b864f..02379548 100644 --- a/tests/unit/api/routes/v2/test_i_will_plan_rich_required.py +++ b/tests/unit/api/routes/v2/test_i_will_plan_rich_required.py @@ -19,7 +19,19 @@ _AGENT_ID = "00000000-0000-0000-0004-000000000001" _HEADERS = {"X-Agent-ID": _AGENT_ID, "X-Agent-Role": "main_pm"} _HTTP_UNPROCESSABLE = 422 _HTTP_OK = 200 -_MIN_APPROACH_LEN = 20 +# #171: raised 20→150 — must stay in sync with IWillPlanRequest.approach +# min_length and choreographer._impl._PM_APPROACH_MIN_LEN. +_MIN_APPROACH_LEN = 150 +_GOOD_APPROACH = ( + "Single-cell decomposition for the git-workflow smoke test: be-pm owns " + "the backend slice end to end — claim, branch, delegate the README edit " + "to be-dev-1, sequence QA after the PR opens, then documentation, then " + "complete and submit_up. Frontend and UX cells are unaffected." +) +_GOOD_SUBTASK_DESC = ( + "be-dev-1 creates the feature branch, prepends the smoke-test HTML " + "comment above the README H1 leaving the rest untouched, commits, opens PR." +) def _make_envelope(error: str, missing: list[str] | None = None) -> MagicMock: @@ -76,10 +88,7 @@ def test_i_will_plan_rejects_empty_subtasks_for_pm() -> None: json={ "task_id": str(uuid4()), "plan": "Route to backend cell only", - "approach": ( - "Single-cell decomposition: backend cell handles the " - "smoke test end-to-end; frontend and ux unaffected." - ), + "approach": _GOOD_APPROACH, "sub_tasks": [], # empty — gateway rejects for PM }, ) @@ -101,11 +110,8 @@ def test_i_will_plan_schema_accepts_rich_plan() -> None: req = IWillPlanRequest( task_id=uuid4(), plan="Route to backend", - approach=( - "Single-cell decomposition for the smoke test: be-pm handles " - "git workflow validation end to end." - ), - sub_tasks=[{"title": "Backend slice", "description": "Branch + edit + PR"}], + approach=_GOOD_APPROACH, + sub_tasks=[{"title": "Backend slice", "description": _GOOD_SUBTASK_DESC}], risks=[], open_questions=[], ) diff --git a/tests/unit/gateway/test_choreographer_claim_guards.py b/tests/unit/gateway/test_choreographer_claim_guards.py index 7ed283d6..1430218b 100644 --- a/tests/unit/gateway/test_choreographer_claim_guards.py +++ b/tests/unit/gateway/test_choreographer_claim_guards.py @@ -397,11 +397,20 @@ async def test_cell_pm_can_plan_code_typed_parent_via_i_will_plan() -> None: plan="Decompose into 2 dev subtasks.", rich_plan={ "approach": ( - "Split code-typed parent into two developer-claimable subtasks: " - "one for API implementation, one for test coverage." + "Split the code-typed parent into two developer-claimable " + "subtasks: one for API implementation, one for test coverage. " + "Sequenced so the API lands first; QA reviews each PR after " + "it opens, documentation follows, then complete and submit " + "up. No cross-cell dependencies for this slice." ), "sub_tasks": [ - {"title": "API subtask", "description": "Implement endpoint"}, + { + "title": "API subtask", + "description": ( + "be-dev-1 implements the endpoint with tests, commits " + "with the task-id prefix, opens the leaf PR for QA." + ), + }, ], }, ) @@ -450,10 +459,19 @@ async def test_pm_can_plan_non_code_parent() -> None: rich_plan={ "approach": ( "Single-cell decomposition: backend handles the full scope; " - "no frontend or ux work required for this planning task." + "no frontend or ux work required for this planning task. " + "be-dev-1 owns the change end to end; QA reviews after the " + "PR opens, documentation follows, then be-pm completes and " + "submits up. Strict sequencing, no cross-cell dependencies." ), "sub_tasks": [ - {"title": "Backend planning slice", "description": "Scope and assign"} + { + "title": "Backend planning slice", + "description": ( + "scope the change, assign be-dev-1, who implements " + "with tests and opens the leaf PR for QA review." + ), + } ], }, ) diff --git a/tests/unit/gateway/test_choreographer_impl_branches.py b/tests/unit/gateway/test_choreographer_impl_branches.py index 1a87bfa1..3222e387 100644 --- a/tests/unit/gateway/test_choreographer_impl_branches.py +++ b/tests/unit/gateway/test_choreographer_impl_branches.py @@ -325,8 +325,22 @@ async def test_i_will_plan_pm_with_already_active_task_rejects() -> None: task_id, plan="x" * 30, rich_plan={ - "approach": "Decompose planning task into backend and frontend subtasks.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Decompose the planning task into backend and frontend " + "developer-claimable subtasks. Backend lands first, QA " + "reviews each PR after it opens, documentation follows, then " + "complete and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() @@ -427,8 +441,22 @@ async def test_i_will_plan_pending_claim_fails() -> None: task_id, plan="my plan that is long enough", rich_plan={ - "approach": "Decompose planning task into backend and frontend subtasks.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Decompose the planning task into backend and frontend " + "developer-claimable subtasks. Backend lands first, QA " + "reviews each PR after it opens, documentation follows, then " + "complete and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() @@ -1047,8 +1075,22 @@ async def test_i_will_plan_pending_claim_returns_none_emit_rejection() -> None: task_id, plan="my plan that is long enough", rich_plan={ - "approach": "Decompose planning task into backend and frontend subtasks.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Decompose the planning task into backend and frontend " + "developer-claimable subtasks. Backend lands first, QA " + "reviews each PR after it opens, documentation follows, then " + "complete and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() diff --git a/tests/unit/gateway/test_choreographer_pm_extras.py b/tests/unit/gateway/test_choreographer_pm_extras.py index 27a06a79..e371efbf 100644 --- a/tests/unit/gateway/test_choreographer_pm_extras.py +++ b/tests/unit/gateway/test_choreographer_pm_extras.py @@ -118,11 +118,26 @@ async def test_i_will_plan_claims_starts_and_sets_plan() -> None: rich_plan={ "approach": ( "Three-cell decomposition: backend, frontend, and ux each " - "own a vertical slice of the work." + "own a vertical slice of the work. Backend lands first, QA " + "reviews each PR after it opens, documentation follows, then " + "complete and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." ), "sub_tasks": [ - {"title": "Backend slice", "description": "API + DB"}, - {"title": "Frontend slice", "description": "UI integration"}, + { + "title": "Backend slice", + "description": ( + "be-dev-1 implements the API + DB migration with " + "tests and opens the leaf PR for QA review." + ), + }, + { + "title": "Frontend slice", + "description": ( + "fe-dev-1 wires the UI integration with loading and " + "error states and opens the leaf PR for QA." + ), + }, ], }, ) @@ -189,11 +204,26 @@ async def test_i_will_plan_blocks_when_journal_decision_at_claim_missing() -> No rich_plan={ "approach": ( "Three-cell decomposition: backend, frontend, and ux each " - "own a vertical slice of the work." + "own a vertical slice of the work. Backend lands first, QA " + "reviews each PR after it opens, documentation follows, then " + "complete and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." ), "sub_tasks": [ - {"title": "Backend slice", "description": "API + DB"}, - {"title": "Frontend slice", "description": "UI integration"}, + { + "title": "Backend slice", + "description": ( + "be-dev-1 implements the API + DB migration with " + "tests and opens the leaf PR for QA review." + ), + }, + { + "title": "Frontend slice", + "description": ( + "fe-dev-1 wires the UI integration with loading and " + "error states and opens the leaf PR for QA." + ), + }, ], }, ) @@ -239,8 +269,22 @@ async def test_i_will_plan_rejects_non_pending_state() -> None: task_id, plan="x", rich_plan={ - "approach": "Single-cell decomposition: backend handles all scope.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Single-cell decomposition: backend handles all scope. " + "be-dev-1 owns the change end to end — branch, implement, " + "test, open PR; QA reviews after the PR opens, documentation " + "follows, then be-pm completes and submits up. No cross-cell " + "dependencies for this planning task." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() @@ -300,12 +344,27 @@ async def test_i_will_plan_calls_claim_when_pre_assigned_and_pending() -> None: plan="distribute to be-pm and fe-pm", rich_plan={ "approach": ( - "Two-cell dispatch: be-pm owns backend vertical, " - "fe-pm owns frontend vertical." + "Two-cell dispatch: be-pm owns the backend vertical, fe-pm " + "owns the frontend vertical. Backend lands first, QA reviews " + "each PR after it opens, documentation follows, then complete " + "and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." ), "sub_tasks": [ - {"title": "Backend cell", "description": "Assign to be-pm"}, - {"title": "Frontend cell", "description": "Assign to fe-pm"}, + { + "title": "Backend cell", + "description": ( + "be-pm decomposes and delegates the backend vertical " + "to be-dev-1, who implements and opens the leaf PR." + ), + }, + { + "title": "Frontend cell", + "description": ( + "fe-pm decomposes and delegates the frontend vertical " + "to fe-dev-1, who implements and opens the leaf PR." + ), + }, ], }, ) @@ -363,8 +422,22 @@ async def test_i_will_plan_surfaces_start_failure_instead_of_faking_ok() -> None task_id, plan="x", rich_plan={ - "approach": "Single-cell decomposition: backend handles all scope.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Single-cell decomposition: backend handles all scope. " + "be-dev-1 owns the change end to end — branch, implement, " + "test, open PR; QA reviews after the PR opens, documentation " + "follows, then be-pm completes and submits up. No cross-cell " + "dependencies for this planning task." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() @@ -508,8 +581,22 @@ async def test_i_will_plan_still_rejects_in_progress_for_other_agent() -> None: task_id, plan="x", rich_plan={ - "approach": "Single-cell decomposition: backend handles all scope.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Single-cell decomposition: backend handles all scope. " + "be-dev-1 owns the change end to end — branch, implement, " + "test, open PR; QA reviews after the PR opens, documentation " + "follows, then be-pm completes and submits up. No cross-cell " + "dependencies for this planning task." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() @@ -534,8 +621,22 @@ async def test_i_will_plan_returns_tracing_gap_without_plan() -> None: task_id, plan="", rich_plan={ - "approach": "Single-cell decomposition: backend handles all scope.", - "sub_tasks": [{"title": "Slice A", "description": "backend API work"}], + "approach": ( + "Single-cell decomposition: backend handles all scope. " + "be-dev-1 owns the change end to end — branch, implement, " + "test, open PR; QA reviews after the PR opens, documentation " + "follows, then be-pm completes and submits up. No cross-cell " + "dependencies for this planning task." + ), + "sub_tasks": [ + { + "title": "Slice A", + "description": ( + "be-dev-1 implements the backend API change with " + "tests and opens the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() diff --git a/tests/unit/gateway/test_claim_arg_order.py b/tests/unit/gateway/test_claim_arg_order.py index d0167d48..ead06111 100644 --- a/tests/unit/gateway/test_claim_arg_order.py +++ b/tests/unit/gateway/test_claim_arg_order.py @@ -275,11 +275,26 @@ async def test_i_will_plan_calls_claim_and_start_with_task_id_first() -> None: rich_plan={ "approach": ( "Three-cell decomposition: backend, frontend, and ux each " - "own a vertical slice of the work." + "own a vertical slice of the work. Backend lands first, QA " + "reviews each PR after it opens, documentation follows, then " + "complete and submit up. Strict sequencing with no cross-cell " + "dependencies beyond the stated ordering." ), "sub_tasks": [ - {"title": "Backend slice", "description": "API + DB"}, - {"title": "Frontend slice", "description": "UI integration"}, + { + "title": "Backend slice", + "description": ( + "be-dev-1 implements the API + DB migration with " + "tests and opens the leaf PR for QA review." + ), + }, + { + "title": "Frontend slice", + "description": ( + "fe-dev-1 wires the UI integration with loading and " + "error states and opens the leaf PR for QA." + ), + }, ], }, ) diff --git a/tests/unit/gateway/test_i_will_plan_persists_rich_plan.py b/tests/unit/gateway/test_i_will_plan_persists_rich_plan.py index e8272f97..8dc0c72f 100644 --- a/tests/unit/gateway/test_i_will_plan_persists_rich_plan.py +++ b/tests/unit/gateway/test_i_will_plan_persists_rich_plan.py @@ -133,11 +133,28 @@ async def test_i_will_plan_persists_panel_shaped_plan() -> None: c = Choreographer(deps) sub_tasks_in = [ - {"title": "Backend slice", "description": "API + DB schema"}, - {"title": "UX slice", "description": "Panel changes"}, + { + "title": "Backend slice", + "description": ( + "be-dev-1 implements the API endpoint and the DB schema " + "migration, with tests, behind the existing service layer." + ), + }, + { + "title": "UX slice", + "description": ( + "fe-dev-1 wires the panel view to the new endpoint and " + "renders the result list with loading + error states." + ), + }, ] rich = { - "approach": "Three-slice decomposition: api, db, ui — each as a subtask.", + "approach": ( + "Three-slice decomposition for the feature: backend builds the " + "API + DB migration first, UX consumes it after the endpoint is " + "merged, QA reviews each PR, docs follow. Strict sequencing; the " + "UX slice depends on the backend slice landing first." + ), "sub_tasks": sub_tasks_in, "risks": [{"risk": "schema migration may block", "mitigation": "rehearse"}], } @@ -170,12 +187,11 @@ async def test_i_will_plan_persists_panel_shaped_plan() -> None: @pytest.mark.asyncio async def test_i_will_plan_with_thin_rich_plan_persists_string() -> None: - """PM passing rich_plan with empty rich fields — set_plan receives the str. + """PM passing a valid rich_plan — set_plan receives the dict shape. - _resolve_effective_plan only switches to the dict shape when at least one - rich-plan field is populated. If a PM somehow bypasses _pm_sub_tasks_gate - (e.g. via the re-entry path) with an empty rich_plan, the raw string - must still pass through. TaskService.set_plan wraps it as {"text": str}. + Regression coverage for _resolve_effective_plan: with rich-plan fields + populated the dict shape (not the raw string) must reach set_plan, and + the raw paragraph is preserved under `text`. """ pm_id = uuid4() task_id = uuid4() @@ -183,15 +199,28 @@ async def test_i_will_plan_with_thin_rich_plan_persists_string() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - # Single sub_task satisfies the gate; we test the str-vs-dict branch of - # _resolve_effective_plan by checking what shape reaches set_plan. + # A valid rich_plan satisfies the gate; we test the str-vs-dict branch + # of _resolve_effective_plan by checking what shape reaches set_plan. env = await c.i_will_plan( pm_id, task_id, plan="bare plan paragraph", rich_plan={ - "approach": "PM-approved approach text that is long enough to pass.", - "sub_tasks": [{"title": "Slice", "description": "the work"}], + "approach": ( + "Single-slice decomposition: be-dev-1 owns the change end " + "to end — branch, implement, test, open PR; QA reviews after " + "the PR opens; docs follow; then be-pm completes and submits " + "up. No cross-cell dependencies for this task." + ), + "sub_tasks": [ + { + "title": "Slice", + "description": ( + "be-dev-1 implements the change with tests and opens " + "the leaf PR for QA review." + ), + } + ], }, ) body = env.as_dict() diff --git a/tests/unit/gateway/test_i_will_plan_sub_tasks_gate.py b/tests/unit/gateway/test_i_will_plan_sub_tasks_gate.py index e7f5b68c..195bee25 100644 --- a/tests/unit/gateway/test_i_will_plan_sub_tasks_gate.py +++ b/tests/unit/gateway/test_i_will_plan_sub_tasks_gate.py @@ -20,7 +20,18 @@ from uuid import uuid4 import pytest from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps -_MIN_APPROACH_LEN = 20 +_MIN_APPROACH_LEN = 150 # #171: raised 20→150 (must match _PM_APPROACH_MIN_LEN) +_GOOD_APPROACH = ( + "Three-cell decomposition for the smoke test: backend owns the README " + "edit + PR, QA reviews after the PR opens, documentation follows, then " + "be-pm completes and submits up. Sequenced strictly; no cross-cell deps. " + "Frontend and UX cells are explicitly out of scope for this task." +) +_GOOD_SUBTASK_DESC = ( + "be-dev-1 branches, prepends the smoke-test HTML comment above the " + "README H1 leaving the rest untouched, commits with the task-id prefix, " + "and opens the leaf PR for QA." +) # --------------------------------------------------------------------------- @@ -203,8 +214,10 @@ async def test_pm_with_filled_sub_tasks_passes_gate() -> None: task_id, plan="decompose work", rich_plan={ - "approach": "Three-cell decomposition: backend, frontend, ux.", - "sub_tasks": [{"title": "Backend slice", "description": "API + DB"}], + "approach": _GOOD_APPROACH, + "sub_tasks": [ + {"title": "Backend slice", "description": _GOOD_SUBTASK_DESC} + ], }, ) body = env.as_dict() @@ -213,6 +226,34 @@ async def test_pm_with_filled_sub_tasks_passes_gate() -> None: assert body.get("error") != "incomplete_input", body +@pytest.mark.asyncio +async def test_pm_with_thin_subtask_description_gets_incomplete_input() -> None: + """#171: a good approach but a title-only / thin sub_task description is + rejected — sub_tasks must be real steps (delegate target + progress item).""" + pm_id = uuid4() + task_id = uuid4() + task_svc = _pm_task_svc(task_id, role="cell_pm") + deps = _make_deps(task=task_svc) + c = Choreographer(deps) + + env = await c.i_will_plan( + pm_id, + task_id, + plan="decompose work", + rich_plan={ + "approach": _GOOD_APPROACH, + "sub_tasks": [{"title": "Backend slice", "description": "API + DB"}], + }, + ) + body = env.as_dict() + assert body["error"] == "incomplete_input", body + assert "sub_tasks" in (body.get("missing") or []), body + assert ( + "thin" in str(body.get("field_hints", {})).lower() + or "thin" in str(body.get("remediate", "")).lower() + ), body + + # --------------------------------------------------------------------------- # Test 4: Developer with empty sub_tasks → gate does NOT fire # --------------------------------------------------------------------------- diff --git a/tests/unit/gateway/test_work_session_auto_create.py b/tests/unit/gateway/test_work_session_auto_create.py index 60464d44..5ce718f6 100644 --- a/tests/unit/gateway/test_work_session_auto_create.py +++ b/tests/unit/gateway/test_work_session_auto_create.py @@ -218,8 +218,21 @@ async def test_i_will_plan_calls_ensure_work_session() -> None: c = Choreographer(deps) rich_plan = { - "approach": "plan text with enough detail to pass the 20 char gate", - "sub_tasks": [{"title": "t1", "description": "desc1"}], + "approach": ( + "Single-cell decomposition: be-dev-1 owns the change end to end " + "— branch, implement, test, open PR; QA reviews after the PR " + "opens, documentation follows, then be-pm completes and submits " + "up. No cross-cell dependencies for this planning task." + ), + "sub_tasks": [ + { + "title": "t1", + "description": ( + "be-dev-1 implements the change with tests and opens the " + "leaf PR for QA review." + ), + } + ], } env = await c.i_will_plan( pm_agent_id, task_id, plan="plan text", rich_plan=rich_plan