diff --git a/roboco/api/routes/v2/flow_dev.py b/roboco/api/routes/v2/flow_dev.py index 0dde197f..c496f03d 100644 --- a/roboco/api/routes/v2/flow_dev.py +++ b/roboco/api/routes/v2/flow_dev.py @@ -49,7 +49,13 @@ async def i_will_work_on( choreographer: _ChoreographerDep, ) -> dict: env = await choreographer.i_will_work_on( - x_agent_id, body.task_id, body.plan, steps=body.steps + x_agent_id, + body.task_id, + body.plan, + steps=body.steps, + technical_considerations=body.technical_considerations, + risks=body.risks, + open_questions=body.open_questions, ) return envelope_to_response(env, request) diff --git a/roboco/api/schemas/v2/flow.py b/roboco/api/schemas/v2/flow.py index 501faa93..cbb9183f 100644 --- a/roboco/api/schemas/v2/flow.py +++ b/roboco/api/schemas/v2/flow.py @@ -22,6 +22,14 @@ class IWillWorkOnRequest(BaseModel): default_factory=list, description="Ordered execution steps — list of {title, description}", ) + # Full parity with IWillPlanRequest so a dev leaf's Plan tab renders the + # same rich structure PMs author. Defaults stay permissive (NOT min_length) + # so re-entry/recovery calls that omit them still pass route validation; + # depth + presence are enforced on FRESH dev claims by + # choreographer._dev_plan_gate. The dev's `plan` doubles as the approach. + technical_considerations: list[str] = Field(default_factory=list) + risks: list[dict[str, str]] = Field(default_factory=list) + open_questions: list[dict[str, str | bool]] = Field(default_factory=list) class OpenPrRequest(BaseModel): diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index e0b0395e..7a86119e 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -214,23 +214,42 @@ def i_will_work_on( task_id: str, plan: str | None = None, steps: list[dict[str, str]] | None = None, + technical_considerations: list[str] | None = None, + risks: list[dict[str, str]] | None = None, + open_questions: list[dict[str, str | bool]] | None = None, ) -> dict[str, Any]: """Claim/start/recover a task. Works for pending, claimed, needs_revision. + On a FRESH claim a developer authors the SAME rich plan a PM does, so the + task's Plan tab is fully populated for audit/tracing — the gateway's + ``_dev_plan_gate`` rejects a thin one. Re-entry / recovery claims + (already-claimed, in_progress, needs_revision) do NOT re-supply any of + this; the gateway short-circuits before the gate. + Args: task_id: UUID of the task you are claiming. - plan: One-paragraph narrative of how you'll execute the task. + plan: 2-4 sentences (>= 150 chars) describing HOW you will implement + this. Doubles as the plan's "Approach". steps: Ordered execution checklist — list of - ``{"title": "...", "description": "..."}``. Required on a - FRESH claim: the gateway's ``_dev_steps_gate`` rejects an - empty or thin list, and the same list is reused as the - progress checklist (#173 — completing a step advances %). - Re-entry / recovery claims (already-claimed or - needs_revision) do not need steps re-supplied. + ``{"title": "...", "description": "..."}`` with every description + substantive. Becomes the plan's sub-tasks AND the progress + checklist (#173 — completing a step advances %). + technical_considerations: Bullet list (strings) of architectural / + library / approach notes. + risks: List of ``{"risk": "...", "mitigation": "..."}`` entries. + open_questions: Optional list of ``{"question": "...", + "answered": false}`` entries. """ return _post( _role_path("i_will_work_on"), - {"task_id": task_id, "plan": plan, "steps": steps or []}, + { + "task_id": task_id, + "plan": plan, + "steps": steps or [], + "technical_considerations": technical_considerations or [], + "risks": risks or [], + "open_questions": open_questions or [], + }, ) diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index f15c1040..64e82699 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -964,6 +964,9 @@ class Choreographer: task_id: UUID, plan: str | None = None, steps: list[dict[str, Any]] | None = None, + technical_considerations: list[str] | None = None, + risks: list[dict[str, Any]] | None = None, + open_questions: list[dict[str, Any]] | None = None, ) -> Envelope: """Claim a task and start work on it. @@ -1006,15 +1009,21 @@ class Choreographer: task_id=task_id, verb="i_will_work_on", ) - # #172: layer the step checklist onto the narrative plan via the - # same panel-shaped path PMs use, so task.plan.sub_tasks is - # populated (panel render + #173 progress). No steps → unchanged - # string behaviour. - effective_plan: str | dict[str, Any] | None = plan - if steps: - effective_plan = self._resolve_effective_plan( - plan or "", {"sub_tasks": steps} - ) + # #172/full-parity: a dev authors the same rich plan a PM does. The + # dev's `plan` doubles as the Approach; `steps` become sub_tasks. Built + # via the panel-shaped path so the Plan tab renders identically and + # feeds #173 progress. With no rich fields (re-entry/recovery) this + # falls through to unchanged string behaviour. + rich_plan = { + "approach": plan or "", + "sub_tasks": steps or [], + "technical_considerations": technical_considerations or [], + "risks": risks or [], + "open_questions": open_questions or [], + } + effective_plan: str | dict[str, Any] | None = self._resolve_effective_plan( + plan or "", rich_plan + ) spec_ctx = spec_module.Context( plan=effective_plan, actor_id=agent_id, @@ -1035,7 +1044,16 @@ class Choreographer: ): return reentry return await self._fresh_dev_claim( - ctx, role, spec_ctx, agent, steps, role_str, t, agent_id, task_id, briefing + ctx, + role, + spec_ctx, + agent, + rich_plan, + role_str, + t, + agent_id, + task_id, + briefing, ) async def _dev_reentry( @@ -1080,23 +1098,23 @@ class Choreographer: role: Any, spec_ctx: Any, agent: Any, - steps: list[dict[str, Any]] | None, + rich_plan: dict[str, Any], role_str: str, t: Any, agent_id: UUID, task_id: UUID, briefing: dict[str, Any], ) -> Envelope: - """Fresh (non-re-entry) i_will_work_on tail: spec gate → dev-steps + """Fresh (non-re-entry) i_will_work_on tail: spec gate → dev-plan gate → claim/plan/start → post-claim journal gate. Extracted so - i_will_work_on stays within the return-count budget; the dev-steps + i_will_work_on stays within the return-count budget; the dev-plan gate mirrors _pm_sub_tasks_gate's placement (after the spec gate). """ if rejection := await self._claim_plan_start_gate(ctx, role, spec_ctx): return rejection - if rejection := await self._dev_steps_gate( + if rejection := await self._dev_plan_gate( role_str=role_str, - steps=steps, + rich_plan=rich_plan, task=t, agent_id=agent_id, task_id=task_id, @@ -1108,66 +1126,80 @@ class Choreographer: "i_will_work_on", agent_id, task_id, envelope ) - async def _dev_steps_gate( + @staticmethod + def _dev_plan_field_gaps(rich_plan: dict[str, Any]) -> dict[str, str]: + """Collect missing/thin rich-plan fields for a fresh dev claim. + + Full parity with PMs: approach (the dev's `plan`, >= min chars), + substantive sub_tasks (the `steps` checklist), technical_considerations + and risks. open_questions stay optional. Returns {field: hint}. + """ + gaps: dict[str, str] = {} + approach = str(rich_plan.get("approach") or "").strip() + if len(approach) < _PM_APPROACH_MIN_LEN: + gaps["plan"] = ( + f"plan must be >= {_PM_APPROACH_MIN_LEN} chars describing HOW " + "you will implement this (it is the plan's Approach)." + ) + steps = rich_plan.get("sub_tasks") or [] + if not steps: + gaps["steps"] = ( + "a non-empty execution checklist — list of {title, " + "description}; each step is also a progress-checklist item." + ) + elif thin := _thin_subtask_hint(steps): + gaps["steps"] = thin + if not rich_plan.get("technical_considerations"): + gaps["technical_considerations"] = ( + "list >= 1 architectural / library / approach note (strings)." + ) + if not rich_plan.get("risks"): + gaps["risks"] = ( + "list >= 1 {risk, mitigation} entry — what could go wrong and " + "how you'll handle it." + ) + return gaps + + async def _dev_plan_gate( self, *, role_str: str, - steps: list[dict[str, Any]] | None, + rich_plan: dict[str, Any], task: Any, agent_id: UUID, task_id: UUID, briefing: dict[str, Any], ) -> Envelope | None: - """#172: a developer's fresh claim must carry a substantive step - checklist — it is the execution plan AND the progress checklist - (#173). Non-developer callers and re-entry are unaffected (the - re-entry/recovery paths return before this is reached). Returns a - rejection Envelope when steps are absent/thin; None when the gate - passed. + """A developer's FRESH claim must author the same rich plan a PM does, + so the task's Plan tab is fully populated for audit/tracing. Enforces + approach + steps + technical_considerations + risks (open_questions + optional). Non-developer callers and re-entry are unaffected — the + re-entry/recovery paths return before this is reached. Returns a + rejection Envelope when the plan is thin; None when it passes. """ if role_str != "developer": return None - if not steps: - return await self._emit_rejection( - Envelope.incomplete_input( - missing=["steps"], - field_hints={ - "steps": ( - "developers must plan their work as a step " - "checklist — a non-empty list of " - "{title, description}. Each step is both your " - "execution plan and a progress-checklist item." - ) - }, - remediate=( - "re-issue i_will_work_on(task_id, plan, " - "steps=[{'title': '...', 'description': '...'}, ...]) " - f"with every description >= {_PM_SUBTASK_DESC_MIN_LEN} " - "chars saying what that step does." - ), - context_briefing=briefing, - ).with_introspection(task=task, role=role_str), - agent_id=agent_id, - task_id=task_id, - verb="i_will_work_on", - ) - if thin := _thin_subtask_hint(steps): - return await self._emit_rejection( - Envelope.incomplete_input( - missing=["steps"], - field_hints={"steps": thin}, - remediate=( - "re-issue i_will_work_on with every step description " - f">= {_PM_SUBTASK_DESC_MIN_LEN} chars describing what " - "that step actually does." - ), - context_briefing=briefing, - ).with_introspection(task=task, role=role_str), - agent_id=agent_id, - task_id=task_id, - verb="i_will_work_on", - ) - return None + gaps = self._dev_plan_field_gaps(rich_plan) + if not gaps: + return None + return await self._emit_rejection( + Envelope.incomplete_input( + missing=sorted(gaps), + field_hints=gaps, + remediate=( + "re-issue i_will_work_on(task_id, plan='= " + f"{_PM_APPROACH_MIN_LEN} chars>', " + "steps=[{'title': '...', 'description': '...'}, ...], " + "technical_considerations=['...'], " + "risks=[{'risk': '...', 'mitigation': '...'}]) — the same " + "rich plan a PM authors, so your task's Plan tab is filled." + ), + context_briefing=briefing, + ).with_introspection(task=task, role=role_str), + agent_id=agent_id, + task_id=task_id, + verb="i_will_work_on", + ) @staticmethod def _with_briefing(env: Envelope, briefing: dict[str, Any]) -> Envelope: diff --git a/roboco/services/task.py b/roboco/services/task.py index 1090187d..90c1f969 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -4832,11 +4832,22 @@ class TaskService(BaseService): async def set_plan( self, task_id: UUID, plan: str | dict[str, Any] ) -> TaskTable | None: - """Write the task's plan field. Strings are wrapped as {'text': plan}.""" + """Write the task's plan field. Strings are wrapped as {'text': plan}. + + Never DOWNGRADE a rich plan to a contentless one: a recovery/re-entry + claim that omits the rich fields would otherwise clobber the Approach + + sub_tasks an earlier fresh claim already authored — this was why a + flaked-then-recovered dev leaf showed an empty Plan tab. If the incoming + plan has no approach but the stored one does, keep the stored plan. + """ task = await self.get(task_id) if not task: return None - task.plan = plan if isinstance(plan, dict) else {"text": plan} + new_plan = plan if isinstance(plan, dict) else {"text": plan} + existing = task.plan if isinstance(task.plan, dict) else {} + if existing.get("approach") and not new_plan.get("approach"): + return task + task.plan = new_plan await self.session.flush() return task diff --git a/tests/unit/gateway/test_choreographer_claim_guards.py b/tests/unit/gateway/test_choreographer_claim_guards.py index c4f7e5d6..a905e0f0 100644 --- a/tests/unit/gateway/test_choreographer_claim_guards.py +++ b/tests/unit/gateway/test_choreographer_claim_guards.py @@ -33,6 +33,21 @@ _STEPS = [ ), } ] +# Full parity: a fresh dev claim authors the same rich plan a PM does. +# These satisfy _dev_plan_gate (plan/approach >= 150 chars, +# technical_considerations, risks). +_GOOD_PLAN = ( + "Append the timestamp HTML comment to the very bottom of README.md without " + "touching any other line, then commit it on the task branch and open a PR. " + "Verify the diff is a single-line addition before submitting for QA." +) +_GOOD_TC = ["Use a trailing newline so the comment sits on its own line."] +_GOOD_RISKS = [ + { + "risk": "An accidental reformat of README.md balloons the diff.", + "mitigation": "Append only; assert the diff touches one line pre-commit.", + } +] def _make_deps(**overrides: Any) -> ChoreographerDeps: @@ -189,7 +204,14 @@ async def test_i_will_work_on_allows_when_earlier_sibling_terminal() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, target_id, steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + target_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) assert env.error is None task_svc.claim.assert_awaited_once_with(target_id, agent_id) @@ -219,7 +241,14 @@ async def test_root_task_no_sequence_check() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, target_id, steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + target_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) assert env.error is None # Sequence check should not have queried siblings on a root task task_svc.get_subtasks.assert_not_awaited() diff --git a/tests/unit/gateway/test_choreographer_dev.py b/tests/unit/gateway/test_choreographer_dev.py index 88089488..125b7061 100644 --- a/tests/unit/gateway/test_choreographer_dev.py +++ b/tests/unit/gateway/test_choreographer_dev.py @@ -19,6 +19,21 @@ _STEPS = [ ), } ] +# Full parity: a fresh dev claim authors the same rich plan a PM does. +# These satisfy _dev_plan_gate (plan/approach >= 150 chars, +# technical_considerations, risks). +_GOOD_PLAN = ( + "Append the timestamp HTML comment to the very bottom of README.md without " + "touching any other line, then commit it on the task branch and open a PR. " + "Verify the diff is a single-line addition before submitting for QA." +) +_GOOD_TC = ["Use a trailing newline so the comment sits on its own line."] +_GOOD_RISKS = [ + { + "risk": "An accidental reformat of README.md balloons the diff.", + "mitigation": "Append only; assert the diff touches one line pre-commit.", + } +] def _make_deps(**overrides: AsyncMock) -> ChoreographerDeps: @@ -149,7 +164,14 @@ async def test_i_will_work_on_pending_with_plan() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) assert env.error is None assert env.status == "in_progress" task_svc.claim.assert_awaited_once_with(task_id, agent_id) @@ -238,7 +260,14 @@ async def test_i_will_work_on_needs_revision_re_starts() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) assert env.status == "in_progress" task_svc.start.assert_awaited_once_with(task_id, agent_id) @@ -343,7 +372,14 @@ async def test_i_will_work_on_blocks_when_journal_note_at_claim_missing() -> Non deps = _make_deps(task=task_svc, journal=journal_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] == "tracing_gap" assert "journal:note_at_claim" in body["missing"] diff --git a/tests/unit/gateway/test_choreographer_impl_branches.py b/tests/unit/gateway/test_choreographer_impl_branches.py index 57869c29..95e614a5 100644 --- a/tests/unit/gateway/test_choreographer_impl_branches.py +++ b/tests/unit/gateway/test_choreographer_impl_branches.py @@ -30,6 +30,21 @@ _STEPS = [ ), } ] +# Full parity: a fresh dev claim authors the same rich plan a PM does. +# These satisfy _dev_plan_gate (plan/approach >= 150 chars, +# technical_considerations, risks). +_GOOD_PLAN = ( + "Append the timestamp HTML comment to the very bottom of README.md without " + "touching any other line, then commit it on the task branch and open a PR. " + "Verify the diff is a single-line addition before submitting for QA." +) +_GOOD_TC = ["Use a trailing newline so the comment sits on its own line."] +_GOOD_RISKS = [ + { + "risk": "An accidental reformat of README.md balloons the diff.", + "mitigation": "Append only; assert the diff touches one line pre-commit.", + } +] def _wire_dev_task_svc( @@ -152,7 +167,14 @@ async def test_i_will_work_on_pending_claim_raises_returns_invalid_state() -> No task_svc.claim.side_effect = RuntimeError("workspace down") deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="plan", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] == "invalid_state" assert "verb runner failed" in body["message"] @@ -168,7 +190,14 @@ async def test_i_will_work_on_pending_claim_returns_none_invalid_state() -> None task_svc.claim.return_value = None deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="plan", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] == "invalid_state" @@ -214,7 +243,14 @@ async def test_i_will_work_on_start_returns_none_invalid_state() -> None: task_svc.start.return_value = None # start fails deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="ok plan", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] == "invalid_state" assert "start failed" in body["message"] @@ -237,7 +273,14 @@ async def test_needs_revision_branch_claim_fails_invalid_state() -> None: task_svc.claim.return_value = None deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] == "invalid_state" @@ -253,7 +296,14 @@ async def test_needs_revision_branch_start_fails() -> None: task_svc.start.return_value = None deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] == "invalid_state" @@ -1147,7 +1197,14 @@ async def test_i_will_work_on_envelope_carries_introspection_on_success() -> Non task_svc.start.return_value = claimed_task deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="ok plan", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) body = env.as_dict() assert body["error"] is None assert body["current_state"] == "in_progress" diff --git a/tests/unit/gateway/test_claim_arg_order.py b/tests/unit/gateway/test_claim_arg_order.py index 6b3e5edd..3b14cb21 100644 --- a/tests/unit/gateway/test_claim_arg_order.py +++ b/tests/unit/gateway/test_claim_arg_order.py @@ -36,6 +36,21 @@ _STEPS = [ ), } ] +# Full parity: a fresh dev claim authors the same rich plan a PM does. +# These satisfy _dev_plan_gate (plan/approach >= 150 chars, +# technical_considerations, risks). +_GOOD_PLAN = ( + "Append the timestamp HTML comment to the very bottom of README.md without " + "touching any other line, then commit it on the task branch and open a PR. " + "Verify the diff is a single-line addition before submitting for QA." +) +_GOOD_TC = ["Use a trailing newline so the comment sits on its own line."] +_GOOD_RISKS = [ + { + "risk": "An accidental reformat of README.md balloons the diff.", + "mitigation": "Append only; assert the diff touches one line pre-commit.", + } +] def _make_deps(**overrides: Any) -> ChoreographerDeps: @@ -132,7 +147,14 @@ async def test_i_will_work_on_pending_calls_claim_with_task_id_first() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="x", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) # Service signature is (task_id, agent_id, ...) — pin that order. task_svc.claim.assert_awaited_once_with(task_id, agent_id) @@ -184,7 +206,14 @@ async def test_i_will_work_on_needs_revision_calls_start_with_task_id_first() -> deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) task_svc.start.assert_awaited_once_with(task_id, agent_id) task_svc.claim.assert_awaited_once_with(task_id, agent_id) diff --git a/tests/unit/gateway/test_dev_steps_gate.py b/tests/unit/gateway/test_dev_steps_gate.py index 08e907d7..ea6e8e1f 100644 --- a/tests/unit/gateway/test_dev_steps_gate.py +++ b/tests/unit/gateway/test_dev_steps_gate.py @@ -22,6 +22,31 @@ _GOOD_STEP_DESC = ( "be-dev-1 prepends the smoke-test HTML comment above the README H1, " "leaving the rest of the file untouched, then stages the change." ) +# Full parity: a fresh dev claim authors the same rich plan a PM does. These +# satisfy _dev_plan_gate (plan/approach >= 150 chars, substantive steps, +# technical_considerations, risks). +_GOOD_PLAN = ( + "Append the timestamp HTML comment to the very bottom of README.md without " + "touching any other line, then commit it on the task branch and open a PR. " + "Verify the diff is a single-line addition before submitting for QA." +) +_GOOD_TC = ["Use a trailing newline so the comment sits on its own line."] +_GOOD_RISKS = [ + { + "risk": "An accidental reformat of README.md balloons the diff.", + "mitigation": "Append only; assert the diff touches one line pre-commit.", + } +] + + +def _full_plan_kwargs(steps: list[dict[str, str]]) -> dict[str, Any]: + """The full rich-plan kwargs a fresh dev claim must supply post-parity.""" + return { + "plan": _GOOD_PLAN, + "steps": steps, + "technical_considerations": _GOOD_TC, + "risks": _GOOD_RISKS, + } def _make_deps(**overrides: Any) -> ChoreographerDeps: @@ -137,22 +162,42 @@ async def test_dev_with_substantive_steps_passes_gate_and_persists_checklist() - {"title": "Edit README", "description": _GOOD_STEP_DESC}, {"title": "Commit + open PR", "description": _GOOD_STEP_DESC}, ] - env = await c.i_will_work_on( - dev_id, - task_id, - plan="implement the README change end to end", - steps=steps_in, - ) + env = await c.i_will_work_on(dev_id, task_id, **_full_plan_kwargs(steps_in)) body = env.as_dict() assert body.get("error") != "incomplete_input", body - # Steps were layered into the panel-shaped plan dict and persisted. + # The full rich plan was layered into the panel-shaped dict and persisted, + # so the dev leaf's Plan tab renders like a PM's (approach + sub_tasks + + # technical_considerations + risks). svc.set_plan.assert_awaited_once() persisted = svc.set_plan.await_args.args[1] assert isinstance(persisted, dict), persisted sub_tasks = persisted.get("sub_tasks") or [] assert len(sub_tasks) == len(steps_in), persisted assert all(st.get("title") for st in sub_tasks), sub_tasks - assert persisted.get("text") == "implement the README change end to end" + assert persisted.get("approach") == _GOOD_PLAN + assert persisted.get("technical_considerations") == _GOOD_TC + assert len(persisted.get("risks") or []) == 1 + + +@pytest.mark.asyncio +async def test_dev_fresh_claim_missing_considerations_and_risks_rejected() -> None: + """Full parity: substantive steps + long plan are not enough — a fresh dev + claim must also carry technical_considerations and risks.""" + dev_id = uuid4() + task_id = uuid4() + c = Choreographer(_make_deps(task=_dev_task_svc(task_id))) + + env = await c.i_will_work_on( + dev_id, + task_id, + plan=_GOOD_PLAN, + steps=[{"title": "Edit README", "description": _GOOD_STEP_DESC}], + ) + body = env.as_dict() + assert body["error"] == "incomplete_input", body + missing = body.get("missing") or [] + assert "technical_considerations" in missing, body + assert "risks" in missing, body @pytest.mark.asyncio diff --git a/tests/unit/gateway/test_work_session_auto_create.py b/tests/unit/gateway/test_work_session_auto_create.py index 3372f20f..7345cd11 100644 --- a/tests/unit/gateway/test_work_session_auto_create.py +++ b/tests/unit/gateway/test_work_session_auto_create.py @@ -30,6 +30,21 @@ _STEPS = [ ), } ] +# Full parity: a fresh dev claim authors the same rich plan a PM does. +# These satisfy _dev_plan_gate (plan/approach >= 150 chars, +# technical_considerations, risks). +_GOOD_PLAN = ( + "Append the timestamp HTML comment to the very bottom of README.md without " + "touching any other line, then commit it on the task branch and open a PR. " + "Verify the diff is a single-line addition before submitting for QA." +) +_GOOD_TC = ["Use a trailing newline so the comment sits on its own line."] +_GOOD_RISKS = [ + { + "risk": "An accidental reformat of README.md balloons the diff.", + "mitigation": "Append only; assert the diff touches one line pre-commit.", + } +] def _make_task_svc(agent_id, task_id, *, status: str): @@ -132,7 +147,14 @@ async def test_i_will_work_on_calls_ensure_work_session() -> None: deps = _make_deps(task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) assert env.error is None, f"Expected ok, got error={env.error} msg={env.message}" assert env.status == "in_progress" @@ -267,7 +289,14 @@ async def test_ensure_work_session_not_called_when_start_fails() -> None: deps = _make_deps(task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) + env = await c.i_will_work_on( + agent_id, + task_id, + plan=_GOOD_PLAN, + steps=_STEPS, + technical_considerations=_GOOD_TC, + risks=_GOOD_RISKS, + ) assert env.error == "invalid_state" task_svc.ensure_work_session.assert_not_awaited() diff --git a/tests/unit/mcp_servers/test_flow_server.py b/tests/unit/mcp_servers/test_flow_server.py index 7da5ebf3..199ccced 100644 --- a/tests/unit/mcp_servers/test_flow_server.py +++ b/tests/unit/mcp_servers/test_flow_server.py @@ -144,6 +144,9 @@ def test_i_will_work_on_passes_plan(flow_module: types.ModuleType) -> None: "task_id": "task-uuid", "plan": "my plan", "steps": [], + "technical_considerations": [], + "risks": [], + "open_questions": [], } assert "/api/v2/flow/developer/i_will_work_on" in args[0] @@ -155,7 +158,14 @@ def test_i_will_work_on_plan_defaults_to_none(flow_module: types.ModuleType) -> flow_module.i_will_work_on("task-uuid") _, kwargs = fake_client.post.call_args - assert kwargs["json"] == {"task_id": "task-uuid", "plan": None, "steps": []} + assert kwargs["json"] == { + "task_id": "task-uuid", + "plan": None, + "steps": [], + "technical_considerations": [], + "risks": [], + "open_questions": [], + } def test_i_will_work_on_passes_steps(flow_module: types.ModuleType) -> None: @@ -176,6 +186,9 @@ def test_i_will_work_on_passes_steps(flow_module: types.ModuleType) -> None: "task_id": "task-uuid", "plan": "p", "steps": steps, + "technical_considerations": [], + "risks": [], + "open_questions": [], }