From 4c397e1768d46724026c790dc39d8557691b7655 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sat, 16 May 2026 10:48:23 +0200 Subject: [PATCH] feat(gateway): developer i_will_work_on takes a substantive step checklist (#172) The dev plan was a free string with only a presence gate, so the executing dev had no checklist for plan-driven progress (#173). - IWillWorkOnRequest gains `steps` (same SubTask shape as a PM's sub_tasks); flow_dev route threads it through. - i_will_work_on layers steps onto the narrative plan via the same panel-shaped path PMs use, so task.plan.sub_tasks is populated (panel render + #173 progress). - New _dev_steps_gate (mirrors _pm_sub_tasks_gate, runs after the spec gate): a developer FRESH claim must supply a non-empty steps list with every description >= _PM_SUBTASK_DESC_MIN_LEN. Re-entry/recovery short-circuit before the gate (extracted _dev_reentry + _fresh_dev_claim keep i_will_work_on within the return-count + cyclomatic gates). - developer role prompt: steps template + "thin steps rejected" + the progress(plan_step=...) handoff. - Updated every dev-fresh-claim test fixture across the suite to pass substantive steps; added dedicated _dev_steps_gate coverage. Commit 2 of 3 for the plan/progress quality work (#171/#172/#173). --- agents/prompts/roles/developer.md | 2 +- roboco/api/routes/v2/flow_dev.py | 4 +- roboco/api/schemas/v2/flow.py | 10 + .../services/gateway/choreographer/_impl.py | 156 ++++++++++++++-- .../test_full_lifecycle_real_db.py | 21 ++- tests/integration/test_lifecycle_real_db.py | 23 ++- .../v2/test_full_pending_to_completed.py | 1 + .../test_choreographer_claim_guards.py | 32 +++- tests/unit/gateway/test_choreographer_dev.py | 17 +- .../test_choreographer_impl_branches.py | 39 ++-- tests/unit/gateway/test_claim_arg_order.py | 18 +- tests/unit/gateway/test_dev_steps_gate.py | 171 ++++++++++++++++++ .../gateway/test_work_session_auto_create.py | 16 +- 13 files changed, 458 insertions(+), 52 deletions(-) create mode 100644 tests/unit/gateway/test_dev_steps_gate.py diff --git a/agents/prompts/roles/developer.md b/agents/prompts/roles/developer.md index 658f54fc..5e68c903 100644 --- a/agents/prompts/roles/developer.md +++ b/agents/prompts/roles/developer.md @@ -18,7 +18,7 @@ You write code; you do not coordinate. If you find yourself thinking "let me als | Verb | What it does | Preconditions | |---|---|---| | `give_me_work()` | Returns your highest-priority task or `idle`. | None. | -| `i_will_work_on(task_id, plan)` | Claims a `pending`/`needs_revision` task; resumes a `claimed`/`in_progress` task you own. Auto-creates branch on first claim. **`plan` is REQUIRED** — even on first claim. The gateway returns `tracing_gap missing=['plan']` if you call without it. On resume, pass `plan='resume: '`. | Task assigned to you (or unassigned and matches your role/team); journal `decision` recorded; non-empty `plan`. | +| `i_will_work_on(task_id, plan, steps)` | Claims a `pending`/`needs_revision` task; resumes a `claimed`/`in_progress` task you own. Auto-creates branch on first claim. **`plan` is REQUIRED** (narrative; `tracing_gap missing=['plan']` if absent). **On a FRESH claim `steps` is REQUIRED and gated** — a non-empty list of `{title, description}` where every `description` is **≥60 chars** saying what that step actually does. `steps` is your execution checklist AND your progress checklist: as you finish each, call `progress(task_id, plan_step=, message=...)` and the % is computed from the checklist for you. Thin/title-only steps are rejected. Example step: `{"title": "Edit README", "description": "prepend the smoke-test HTML comment above the H1, leaving the rest of the file untouched"}`. **On resume** (`claimed`/`in_progress` you own) pass `plan='resume: '`; steps are not re-required. | Task assigned to you (or unassigned and matches your role/team); journal `decision` recorded; non-empty `plan`; substantive `steps` on fresh claim. | | `commit(message)` | Makes the git commit, auto-prefixes `[task-id]`, records a progress entry. This is the ONLY way to commit — the gateway covers the actual git operation. | Task in `in_progress`; on your branch. | | `open_pr(task_id)` | Push your branch and open a PR. Run after your last commit, before `i_am_done`. `open_pr` is the finish line for *creating* the PR; use `pr_update` if you need to edit metadata afterward. | Task assigned to you; at least one commit; no PR yet. | | `pr_update(task_id, title?, body?, reviewers?)` | Update an existing PR's title, body, or reviewer list. Use after `open_pr` if you need to correct title/body or assign a reviewer. At least one field must be set. **Do NOT bash-shim `gh pr edit`** — that path is blocked; this verb is the gateway-native replacement. | Task has `pr_number`; you are the assignee (or your PM). | diff --git a/roboco/api/routes/v2/flow_dev.py b/roboco/api/routes/v2/flow_dev.py index 716d3cd0..0dde197f 100644 --- a/roboco/api/routes/v2/flow_dev.py +++ b/roboco/api/routes/v2/flow_dev.py @@ -48,7 +48,9 @@ async def i_will_work_on( x_agent_id: _AgentIdHeader, choreographer: _ChoreographerDep, ) -> dict: - env = await choreographer.i_will_work_on(x_agent_id, body.task_id, body.plan) + env = await choreographer.i_will_work_on( + x_agent_id, body.task_id, body.plan, steps=body.steps + ) return envelope_to_response(env, request) diff --git a/roboco/api/schemas/v2/flow.py b/roboco/api/schemas/v2/flow.py index 27f35df9..501faa93 100644 --- a/roboco/api/schemas/v2/flow.py +++ b/roboco/api/schemas/v2/flow.py @@ -12,6 +12,16 @@ class GiveMeWorkRequest(BaseModel): class IWillWorkOnRequest(BaseModel): task_id: UUID plan: str | None = None + # #172: the executing developer's plan is a step checklist (same + # SubTask shape as IWillPlanRequest.sub_tasks). It is both the + # execution plan AND the progress checklist (#173): completing a + # step advances progress. Depth is enforced server-side in + # choreographer._dev_steps_gate (a title with no real description is + # not a step). Server assigns id + order. + steps: list[dict[str, str]] = Field( + default_factory=list, + description="Ordered execution steps — list of {title, description}", + ) class OpenPrRequest(BaseModel): diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index c2d31c8f..21102189 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -959,7 +959,11 @@ class Choreographer: ).with_introspection(task=t, role=role_str) async def i_will_work_on( - self, agent_id: UUID, task_id: UUID, plan: str | None = None + self, + agent_id: UUID, + task_id: UUID, + plan: str | None = None, + steps: list[dict[str, Any]] | None = None, ) -> Envelope: """Claim a task and start work on it. @@ -968,6 +972,15 @@ class Choreographer: savepoint by the runner so a mid-sequence failure rolls back the DB. Idempotent re-entry: a respawned dev re-calling on a task they already own in_progress just refreshes the heartbeat. + + #172: ``steps`` is the developer's execution checklist (same + SubTask shape as a PM's sub_tasks). Persisted into + ``task.plan.sub_tasks`` via the panel-shaped path so it renders + identically AND feeds plan-driven progress (#173). A developer + on a fresh claim must supply substantive steps — + ``_dev_steps_gate`` enforces depth; the re-entry / recovery + paths short-circuit before the gate so a respawned dev is never + re-blocked for steps it already submitted. """ t = await self.task.get(task_id) if t is None: @@ -993,8 +1006,17 @@ 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} + ) spec_ctx = spec_module.Context( - plan=plan, + plan=effective_plan, actor_id=agent_id, actor_slug=getattr(agent, "slug", None) if agent is not None else None, original_developer_slug=_extract_original_developer(t), @@ -1005,13 +1027,34 @@ class Choreographer: task=t, role_str=role_str, briefing=briefing, - plan=plan, + plan=effective_plan, verb_name="i_will_work_on", ) + if reentry := await self._dev_reentry( + ctx, t, agent_id, task_id, role_str, briefing + ): + return reentry + return await self._fresh_dev_claim( + ctx, role, spec_ctx, agent, steps, role_str, t, agent_id, task_id, briefing + ) + + async def _dev_reentry( + self, + ctx: _ClaimPlanStartContext, + t: Any, + agent_id: UUID, + task_id: UUID, + role_str: str, + briefing: dict[str, Any], + ) -> Envelope | None: + """Re-entry short-circuits for i_will_work_on (extracted to keep + i_will_work_on under the cyclomatic-complexity gate; mirrors + _handle_pm_reentry). Returns an Envelope to short-circuit, or None + to fall through to the fresh-claim path. + """ # Idempotent re-entry: agent already owns the task in_progress. - # Touch heartbeat and short-circuit before the spec gate (which - # would otherwise reject because in_progress is not a source state - # for the composed `claim` action). + # Short-circuit before the spec gate (in_progress is not a source + # state for the composed `claim` action). if str(t.status) == "in_progress" and t.assigned_to == agent_id: await self._touch(task_id) return Envelope.ok( @@ -1020,25 +1063,112 @@ class Choreographer: next=spec_module._INTENT_VERBS["i_will_work_on"].next_hint(t), context_briefing=briefing, ).with_introspection(task=t, role=role_str) - # Recovery re-entry: task stuck in `claimed` (e.g. orchestrator restart - # or a partial-claim race) and the agent already owns it. The spec - # `claim` action's source-statuses do NOT include CLAIMED, so the spec - # gate would reject. Surface this as a runner call that runs only - # set_plan + start. Without this block, an agent reclaiming from a - # crashed mid-sequence would loop forever (Bug A from the 2026-05-09 - # smoke test). + # Recovery re-entry: task stuck in `claimed` (orchestrator restart + # or partial-claim race) and the agent already owns it. The spec + # `claim` source-statuses exclude CLAIMED, so run only set_plan + + # start (Bug A from the 2026-05-09 smoke test). if str(t.status) == "claimed" and t.assigned_to == agent_id: envelope = await self._resume_from_claimed(ctx) return await self._post_claim_journal_gate( "i_will_work_on", agent_id, task_id, envelope ) + return None + + async def _fresh_dev_claim( + self, + ctx: _ClaimPlanStartContext, + role: Any, + spec_ctx: Any, + agent: Any, + steps: list[dict[str, Any]] | None, + 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 + gate → claim/plan/start → post-claim journal gate. Extracted so + i_will_work_on stays within the return-count budget; the dev-steps + 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( + role_str=role_str, + steps=steps, + task=t, + agent_id=agent_id, + task_id=task_id, + briefing=briefing, + ): + return rejection envelope = await self._claim_plan_start_run(ctx, agent, spec_ctx) return await self._post_claim_journal_gate( "i_will_work_on", agent_id, task_id, envelope ) + async def _dev_steps_gate( + self, + *, + role_str: str, + steps: list[dict[str, Any]] | None, + 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. + """ + 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 + @staticmethod def _with_briefing(env: Envelope, briefing: dict[str, Any]) -> Envelope: """Attach a context_briefing to an Envelope (mutate-and-return helper).""" diff --git a/tests/integration/test_full_lifecycle_real_db.py b/tests/integration/test_full_lifecycle_real_db.py index f8e7f8b9..5b541222 100644 --- a/tests/integration/test_full_lifecycle_real_db.py +++ b/tests/integration/test_full_lifecycle_real_db.py @@ -41,6 +41,17 @@ from roboco.models.base import ( from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps from roboco.services.task import TaskService +# #172: a developer fresh claim must carry a substantive step checklist. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + if TYPE_CHECKING: from collections.abc import AsyncIterator @@ -318,7 +329,9 @@ async def test_dev_can_claim_pending_task_via_gateway( ) c = Choreographer(deps) - env = await c.i_will_work_on(dev_agent.id, task.id, plan="add the route") + env = await c.i_will_work_on( + dev_agent.id, task.id, plan="add the route", steps=_STEPS + ) assert env.error is None, f"claim failed: {env.message}" assert env.status == "in_progress" @@ -360,7 +373,9 @@ async def test_dev_full_chain_through_awaiting_qa( c = Choreographer(deps) # 1. Claim - env = await c.i_will_work_on(dev_agent.id, task.id, plan="add the route") + env = await c.i_will_work_on( + dev_agent.id, task.id, plan="add the route", steps=_STEPS + ) assert env.error is None assert env.status == "in_progress" @@ -424,7 +439,7 @@ async def test_full_chain_through_doc_handoff( c = Choreographer(deps) # Drive the dev side first (same as test_dev_full_chain_through_awaiting_qa). - await c.i_will_work_on(dev_agent.id, task.id, plan="add the route") + await c.i_will_work_on(dev_agent.id, task.id, plan="add the route", steps=_STEPS) await stub_git.commit( branch_name=_BRANCH, message=f"[{str(task.id)[:8]}] feat(api): add /healthz", diff --git a/tests/integration/test_lifecycle_real_db.py b/tests/integration/test_lifecycle_real_db.py index 45d78518..c6214192 100644 --- a/tests/integration/test_lifecycle_real_db.py +++ b/tests/integration/test_lifecycle_real_db.py @@ -39,6 +39,17 @@ from roboco.services.gateway.choreographer import Choreographer, ChoreographerDe from roboco.services.task import TaskService from sqlalchemy import delete +# #172: a developer fresh claim must carry a substantive step checklist. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + if TYPE_CHECKING: from collections.abc import AsyncIterator @@ -387,7 +398,9 @@ async def test_dev_full_chain_through_awaiting_qa( ) c = Choreographer(deps) - env = await c.i_will_work_on(dev_agent.id, task.id, plan="add the route") + env = await c.i_will_work_on( + dev_agent.id, task.id, plan="add the route", steps=_STEPS + ) assert env.error is None, f"i_will_work_on failed: {env.message}" assert env.status == Status.IN_PROGRESS.value @@ -753,7 +766,9 @@ async def test_block_then_unblock_restore( c = _build_choreographer(db_session, task, task_service) # Drive into in_progress via the real claim+start sequence. - env = await c.i_will_work_on(dev_agent.id, task.id, plan="implement /healthz") + env = await c.i_will_work_on( + dev_agent.id, task.id, plan="implement /healthz", steps=_STEPS + ) assert env.error is None, f"i_will_work_on failed: {env.message}" assert env.status == Status.IN_PROGRESS.value @@ -807,7 +822,9 @@ async def test_pause_then_resume( task_service = TaskService(db_session) c = _build_choreographer(db_session, task, task_service) - env = await c.i_will_work_on(dev_agent.id, task.id, plan="implement /healthz") + env = await c.i_will_work_on( + dev_agent.id, task.id, plan="implement /healthz", steps=_STEPS + ) assert env.error is None, f"i_will_work_on failed: {env.message}" assert env.status == Status.IN_PROGRESS.value diff --git a/tests/integration/v2/test_full_pending_to_completed.py b/tests/integration/v2/test_full_pending_to_completed.py index 4224633b..ea3eba2a 100644 --- a/tests/integration/v2/test_full_pending_to_completed.py +++ b/tests/integration/v2/test_full_pending_to_completed.py @@ -62,6 +62,7 @@ class _MockChoreographer: _agent_id: object, _task_id: object, _plan: object = None, + **_kwargs: object, ) -> Envelope: self._state["task_status"] = "in_progress" return Envelope.ok( diff --git a/tests/unit/gateway/test_choreographer_claim_guards.py b/tests/unit/gateway/test_choreographer_claim_guards.py index 1430218b..c4f7e5d6 100644 --- a/tests/unit/gateway/test_choreographer_claim_guards.py +++ b/tests/unit/gateway/test_choreographer_claim_guards.py @@ -22,6 +22,18 @@ from uuid import uuid4 import pytest from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps +# #172: a developer fresh claim must carry a substantive step checklist. +# Inert on re-entry/error/non-dev paths, so safe to pass everywhere. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + def _make_deps(**overrides: Any) -> ChoreographerDeps: base: dict[str, Any] = { @@ -132,7 +144,7 @@ async def test_i_will_work_on_blocks_when_earlier_sibling_open() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, target_id, plan="x") + env = await c.i_will_work_on(agent_id, target_id, plan="x", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" assert "sequence" in body["message"].lower() @@ -177,7 +189,7 @@ 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) + env = await c.i_will_work_on(agent_id, target_id, steps=_STEPS) assert env.error is None task_svc.claim.assert_awaited_once_with(target_id, agent_id) @@ -207,7 +219,7 @@ 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) + env = await c.i_will_work_on(agent_id, target_id, steps=_STEPS) assert env.error is None # Sequence check should not have queried siblings on a root task task_svc.get_subtasks.assert_not_awaited() @@ -238,7 +250,7 @@ async def test_i_will_work_on_blocks_when_agent_has_in_progress_task() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, target_id, plan="x") + env = await c.i_will_work_on(agent_id, target_id, plan="x", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" assert str(other_id) in body["message"] or str(other_id) in body["remediate"] @@ -272,7 +284,7 @@ async def test_i_will_work_on_resumption_does_not_self_block() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id) + env = await c.i_will_work_on(agent_id, task_id, steps=_STEPS) assert env.error is None task_svc.start.assert_awaited_once_with(task_id, agent_id) @@ -302,7 +314,7 @@ async def test_i_will_work_on_blocks_when_agent_has_paused_task() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, target_id, plan="x") + env = await c.i_will_work_on(agent_id, target_id, plan="x", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" assert str(paused_id) in body["remediate"] @@ -333,7 +345,7 @@ async def test_cell_pm_cannot_claim_code_task_via_i_will_work_on() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(pm_id, task_id, plan="x") + env = await c.i_will_work_on(pm_id, task_id, plan="x", steps=_STEPS) body = env.as_dict() assert body["error"] == "not_authorized" # Spec produces "role 'cell_pm' may not call 'i_will_work_on'". @@ -360,7 +372,7 @@ async def test_main_pm_cannot_claim_code_task_via_i_will_work_on() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(pm_id, task_id, plan="x") + env = await c.i_will_work_on(pm_id, task_id, plan="x", steps=_STEPS) body = env.as_dict() assert body["error"] == "not_authorized" @@ -510,7 +522,7 @@ async def test_developer_cannot_claim_qa_status_task() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(dev_id, task_id) + env = await c.i_will_work_on(dev_id, task_id, steps=_STEPS) body = env.as_dict() assert body["error"] == "not_authorized" assert "developer" in body["message"] @@ -601,7 +613,7 @@ async def test_non_developer_role_cannot_claim_via_i_will_work_on() -> None: deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(doc_id, task_id, plan="x") + env = await c.i_will_work_on(doc_id, task_id, plan="x", steps=_STEPS) body = env.as_dict() # Role-typed claim refuses with not_authorized assert body["error"] == "not_authorized" diff --git a/tests/unit/gateway/test_choreographer_dev.py b/tests/unit/gateway/test_choreographer_dev.py index a1c2822b..88089488 100644 --- a/tests/unit/gateway/test_choreographer_dev.py +++ b/tests/unit/gateway/test_choreographer_dev.py @@ -9,6 +9,17 @@ from uuid import uuid4 import pytest from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps +# #172: a developer fresh claim must carry a substantive step checklist. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + def _make_deps(**overrides: AsyncMock) -> ChoreographerDeps: task = overrides.get("task", AsyncMock()) @@ -138,7 +149,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) assert env.error is None assert env.status == "in_progress" task_svc.claim.assert_awaited_once_with(task_id, agent_id) @@ -227,7 +238,7 @@ 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) + env = await c.i_will_work_on(agent_id, task_id, steps=_STEPS) assert env.status == "in_progress" task_svc.start.assert_awaited_once_with(task_id, agent_id) @@ -332,7 +343,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) 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 3222e387..7cbefc04 100644 --- a/tests/unit/gateway/test_choreographer_impl_branches.py +++ b/tests/unit/gateway/test_choreographer_impl_branches.py @@ -18,6 +18,19 @@ from roboco.services.gateway.choreographer import Choreographer, ChoreographerDe from roboco.services.gateway.choreographer._impl import DelegateInputs from roboco.services.gateway.envelope import Envelope +# #172: a developer fresh claim must carry a substantive step checklist. +# Inert on re-entry/error/non-dev paths (the gate is skipped or the call +# short-circuits before it), so it is safe to pass everywhere. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + def _wire_dev_task_svc( task_id, *, status: str, assigned_to=None, plan=None, parent_task_id=None @@ -139,7 +152,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="plan", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" assert "verb runner failed" in body["message"] @@ -155,7 +168,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="plan", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" @@ -177,7 +190,7 @@ async def test_i_will_work_on_pending_no_plan_tracing_gap() -> None: task_svc.claim.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=None) + env = await c.i_will_work_on(agent_id, task_id, plan=None, steps=_STEPS) body = env.as_dict() assert body["error"] == "tracing_gap" @@ -201,7 +214,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="ok plan", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" assert "start failed" in body["message"] @@ -224,7 +237,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" @@ -240,7 +253,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" @@ -261,7 +274,7 @@ async def test_claimed_branch_returns_start_failed() -> 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") + env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" @@ -282,7 +295,7 @@ async def test_i_will_work_on_in_progress_assigned_to_self_idempotent() -> None: task_svc.heartbeat = AsyncMock() deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="ok") + env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) body = env.as_dict() # No error — re-entry pass. assert "error" not in body or body.get("error") is None @@ -1016,7 +1029,7 @@ async def test_claimed_branch_already_active_guard() -> None: task_svc.list_in_progress_for_agent.return_value = [in_prog] deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="ok") + env = await c.i_will_work_on(agent_id, task_id, plan="ok", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" @@ -1151,7 +1164,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="ok plan", steps=_STEPS) body = env.as_dict() assert body["error"] is None assert body["current_state"] == "in_progress" @@ -1172,7 +1185,7 @@ async def test_i_will_work_on_envelope_carries_introspection_on_rejection() -> N task_svc = _wire_dev_task_svc(task_id, status="completed", assigned_to=agent_id) deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="x") + env = await c.i_will_work_on(agent_id, task_id, plan="x", steps=_STEPS) body = env.as_dict() assert body["error"] == "invalid_state" assert body["current_state"] == "completed" @@ -1233,7 +1246,7 @@ async def test_i_will_work_on_missing_plan_does_not_claim_pending_task() -> None task_svc = _wire_dev_task_svc(task_id, status="pending") deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan=None) + env = await c.i_will_work_on(agent_id, task_id, plan=None, steps=_STEPS) body = env.as_dict() assert body["error"] == "tracing_gap" assert "plan" in body["missing"] @@ -1268,7 +1281,7 @@ async def test_i_will_work_on_claimed_with_no_plan_accepts_recovery_plan() -> No task_svc.start.return_value = started deps = _make_deps(task=task_svc) c = Choreographer(deps) - env = await c.i_will_work_on(agent_id, task_id, plan="recovery plan") + env = await c.i_will_work_on(agent_id, task_id, plan="recovery plan", steps=_STEPS) body = env.as_dict() assert body["error"] is None, f"expected success, got {body}" task_svc.set_plan.assert_awaited_once() diff --git a/tests/unit/gateway/test_claim_arg_order.py b/tests/unit/gateway/test_claim_arg_order.py index ead06111..6b3e5edd 100644 --- a/tests/unit/gateway/test_claim_arg_order.py +++ b/tests/unit/gateway/test_claim_arg_order.py @@ -25,6 +25,18 @@ from uuid import uuid4 import pytest from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps +# #172: a developer fresh claim must carry a substantive step checklist. +# Inert on re-entry/error/non-dev paths, so safe to pass everywhere. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + def _make_deps(**overrides: Any) -> ChoreographerDeps: base = { @@ -120,7 +132,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="x", steps=_STEPS) # Service signature is (task_id, agent_id, ...) — pin that order. task_svc.claim.assert_awaited_once_with(task_id, agent_id) @@ -172,7 +184,7 @@ 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) + env = await c.i_will_work_on(agent_id, task_id, steps=_STEPS) task_svc.start.assert_awaited_once_with(task_id, agent_id) task_svc.claim.assert_awaited_once_with(task_id, agent_id) @@ -220,7 +232,7 @@ async def test_i_will_work_on_claimed_resumption_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) + env = await c.i_will_work_on(agent_id, task_id, steps=_STEPS) task_svc.start.assert_awaited_once_with(task_id, agent_id) assert env.error is None diff --git a/tests/unit/gateway/test_dev_steps_gate.py b/tests/unit/gateway/test_dev_steps_gate.py new file mode 100644 index 00000000..08e907d7 --- /dev/null +++ b/tests/unit/gateway/test_dev_steps_gate.py @@ -0,0 +1,171 @@ +"""#172: a developer's i_will_work_on must carry a substantive step checklist. + +The dev plan was a free string with only a presence gate. Plan-driven +progress (#173) needs a checklist on the executing dev's task too, so +i_will_work_on now takes structured `steps` (same SubTask shape as a +PM's sub_tasks), gated for depth like the PM plan, persisted into +task.plan.sub_tasks via the panel-shaped path. Re-entry/recovery +short-circuit before the gate so a respawned dev is never re-blocked. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + +_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." +) + + +def _make_deps(**overrides: Any) -> ChoreographerDeps: + base: dict[str, Any] = { + "task": AsyncMock(), + "work_session": AsyncMock(), + "git": AsyncMock(), + "a2a": AsyncMock(), + "journal": AsyncMock(), + "audit": AsyncMock(), + "evidence_repo": AsyncMock(), + } + base.update(overrides) + repo = base["evidence_repo"] + for m in ( + "list_unread_a2a", + "list_unread_mentions", + "list_pending_notifications", + "task_metadata_gaps", + "recent_team_activity", + "blockers_in_lane", + "journal_highlights_for_task", + ): + getattr(repo, m).return_value = [] + _ldef = base["journal"].latest_decision_at.return_value + if type(_ldef).__name__ in ("MagicMock", "AsyncMock"): + base["journal"].latest_decision_at.return_value = datetime.now(UTC) + return ChoreographerDeps(**base) + + +def _dev_task_svc(task_id: object, *, status: str = "pending") -> AsyncMock: + svc = AsyncMock() + svc.get.return_value = MagicMock( + id=task_id, + status=status, + plan=None, + assigned_to=None, + task_type="code", + parent_task_id=uuid4(), + sequence=0, + team="backend", + commits=[], + pr_number=None, + branch_name=None, + quick_context=None, + ) + svc.agent_for.return_value = MagicMock( + id=uuid4(), role="developer", team="backend", slug="be-dev-1" + ) + svc.list_in_progress_for_agent.return_value = [] + svc.list_paused_for_agent.return_value = [] + svc.get_subtasks.return_value = [] + svc.session = MagicMock() + svc.session.begin_nested = MagicMock( + return_value=MagicMock( + __aenter__=AsyncMock(return_value=None), + __aexit__=AsyncMock(return_value=False), + ) + ) + return svc + + +@pytest.mark.asyncio +async def test_dev_fresh_claim_without_steps_is_rejected() -> None: + 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="do the thing") + body = env.as_dict() + assert body["error"] == "incomplete_input", body + assert "steps" in (body.get("missing") or []), body + + +@pytest.mark.asyncio +async def test_dev_thin_step_description_is_rejected() -> None: + 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="do the thing", + steps=[{"title": "Edit README", "description": "edit it"}], + ) + body = env.as_dict() + assert body["error"] == "incomplete_input", body + assert "steps" in (body.get("missing") or []), body + + +@pytest.mark.asyncio +async def test_dev_with_substantive_steps_passes_gate_and_persists_checklist() -> None: + dev_id = uuid4() + task_id = uuid4() + svc = _dev_task_svc(task_id) + claimed = MagicMock( + id=task_id, status="claimed", plan=None, assigned_to=dev_id, task_type="code" + ) + started = MagicMock( + id=task_id, + status="in_progress", + plan={"text": "x"}, + assigned_to=dev_id, + task_type="code", + ) + svc.claim.return_value = claimed + svc.set_plan.return_value = claimed + svc.start.return_value = started + c = Choreographer(_make_deps(task=svc)) + + steps_in = [ + {"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, + ) + body = env.as_dict() + assert body.get("error") != "incomplete_input", body + # Steps were layered into the panel-shaped plan dict and persisted. + 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" + + +@pytest.mark.asyncio +async def test_dev_reentry_in_progress_short_circuits_before_steps_gate() -> None: + """A respawned dev re-calling on a task it owns in_progress with NO + steps must short-circuit to OK, not be re-blocked for steps.""" + dev_id = uuid4() + task_id = uuid4() + svc = _dev_task_svc(task_id, status="in_progress") + svc.get.return_value.assigned_to = dev_id + c = Choreographer(_make_deps(task=svc)) + + env = await c.i_will_work_on(dev_id, task_id, plan="resume: keep going") + body = env.as_dict() + assert body.get("error") is None, body + assert body.get("status") == "in_progress", body diff --git a/tests/unit/gateway/test_work_session_auto_create.py b/tests/unit/gateway/test_work_session_auto_create.py index 5ce718f6..3372f20f 100644 --- a/tests/unit/gateway/test_work_session_auto_create.py +++ b/tests/unit/gateway/test_work_session_auto_create.py @@ -19,6 +19,18 @@ from uuid import uuid4 import pytest from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps +# #172: a developer fresh claim must carry a substantive step checklist. +# Inert on re-entry/error/non-dev paths, so safe to pass everywhere. +_STEPS = [ + { + "title": "Implement the change", + "description": ( + "edit the target file, add tests, run them, and stage the " + "change for commit on the task branch" + ), + } +] + def _make_task_svc(agent_id, task_id, *, status: str): """Build a TaskService AsyncMock that completes the (claim, set_plan, start) @@ -120,7 +132,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) assert env.error is None, f"Expected ok, got error={env.error} msg={env.message}" assert env.status == "in_progress" @@ -255,7 +267,7 @@ 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") + env = await c.i_will_work_on(agent_id, task_id, plan="do x then y", steps=_STEPS) assert env.error == "invalid_state" task_svc.ensure_work_session.assert_not_awaited()