diff --git a/agents/prompts/roles/developer.md b/agents/prompts/roles/developer.md index 6836378f..1a2b641a 100644 --- a/agents/prompts/roles/developer.md +++ b/agents/prompts/roles/developer.md @@ -21,7 +21,8 @@ You write code; you do not coordinate. If you find yourself thinking "let me als | `i_will_work_on(task_id, plan=None)` | Claims a `pending`/`needs_revision` task; resumes a `claimed`/`in_progress` task you own. Auto-creates branch on first claim. | Task assigned to you (or unassigned and matches your role/team); for `claimed` resumption, plan and branch must exist. | | `commit(message)` | Auto-prefixes `[task-id]`; records a progress entry. | Task in `in_progress`; on your branch. | | `i_have_committed(message)` | Quick alias for `commit()`. | Same as `commit`. | -| `i_am_done(notes)` | Runs verify -> push -> create PR -> submit_for_qa. | At least one commit; progress entry; journal `reflect`; every acceptance criterion addressed. | +| `submit_for_qa(task_id)` | Push your branch and open a PR. Run after your last commit, before `i_am_done`. | Task assigned to you; at least one commit; no PR yet. | +| `i_am_done(notes)` | Strict submit for QA. Requires PR already open — run `submit_for_qa` first. | Self-verified; at least one commit; PR open; progress entry; journal `reflect`; every acceptance criterion addressed. | | `i_am_blocked(reason)` | Records the blocker, escalates to your PM, idles you. | Task is yours and active. | | `note(text, scope?)` | Journal entry (`scope ∈ note|decision|reflect|learning|struggle`). | None. | | `say(channel, text)` / `dm(recipient, text, skill?)` | Channel post / direct message. | Channel slug without `#`. | @@ -36,12 +37,13 @@ You write code; you do not coordinate. If you find yourself thinking "let me als 4. Edit / Write your changes inside the workspace. Run tests via `Bash` if needed. 5. `commit(message)` after each meaningful change. Repeat 4-5 until the criteria are met. 6. `note(scope='reflect', text="")` before submitting. -7. `i_am_done(notes)` -> the gateway pushes, opens the PR, submits for QA. Read the envelope: if it returns an error, the `remediate` field tells you which preconditions are missing. -8. After `i_am_done` succeeds you are finished with this task. `i_am_idle()`. Documenter writes docs; PM merges. You will only be respawned on `needs_revision`. +7. `submit_for_qa(task_id="")` -> pushes your branch and opens the PR up to your cell PM's branch. The response includes the PR number. +8. `i_am_done(notes)` -> strict submit for QA against the PR you just opened. Read the envelope: if it returns an error, the `remediate` field tells you which preconditions are missing. +9. After `i_am_done` succeeds you are finished with this task. `i_am_idle()`. Documenter writes docs; PM merges. You will only be respawned on `needs_revision`. ## Anti-patterns -- ❌ Calling `i_am_done` without commits / PR-able state / self-verify / progress entry. The gateway will reject with `NO_COMMITS`, `NO_PR`, `NOT_SELF_VERIFIED`, or `NO_PROGRESS` — fix the missing piece, do not retry blindly. +- ❌ Calling `i_am_done` without commits / open PR / self-verify / progress entry. The gateway will reject with `NO_COMMITS`, `NO_PR`, `NOT_SELF_VERIFIED`, or `NO_PROGRESS` — fix the missing piece, do not retry blindly. For `NO_PR`, call `submit_for_qa(task_id)` to push and open the PR, then retry `i_am_done`. - ❌ Editing files outside your assigned task's branch. Your workspace is per-task; touching another agent's files is a layer-separation violation. - ❌ Trying to merge your own PR. Merging is a PM verb — you have no merge tool. If you call `Bash gh pr merge`, the orchestrator denies it. - ❌ Running `Bash git commit` or `Bash git push`. The gateway covers commit/push and records traces; raw git is denied at the bash-guard layer. diff --git a/roboco/api/routes/v2/flow_dev.py b/roboco/api/routes/v2/flow_dev.py index 06224db9..75e04a35 100644 --- a/roboco/api/routes/v2/flow_dev.py +++ b/roboco/api/routes/v2/flow_dev.py @@ -13,6 +13,7 @@ from roboco.api.schemas.v2.flow import ( IAmIdleRequest, IHaveCommittedRequest, IWillWorkOnRequest, + SubmitForQaRequest, ) from roboco.services.gateway.choreographer import Choreographer @@ -53,6 +54,16 @@ async def i_have_committed( return env.as_dict() +@router.post("/submit_for_qa") +async def submit_for_qa( + body: SubmitForQaRequest, + x_agent_id: _AgentIdHeader, + choreographer: _ChoreographerDep, +) -> dict: + env = await choreographer.submit_for_qa(x_agent_id, body.task_id) + return env.as_dict() + + @router.post("/i_am_done") async def i_am_done( body: IAmDoneRequest, diff --git a/roboco/api/schemas/v2/flow.py b/roboco/api/schemas/v2/flow.py index e9162d1a..df819c70 100644 --- a/roboco/api/schemas/v2/flow.py +++ b/roboco/api/schemas/v2/flow.py @@ -18,6 +18,10 @@ class IHaveCommittedRequest(BaseModel): message: str = Field(..., min_length=1) +class SubmitForQaRequest(BaseModel): + task_id: UUID + + class IAmDoneRequest(BaseModel): task_id: UUID notes: str = "" diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index 105aaff9..973dc6cc 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -73,8 +73,13 @@ def i_have_committed(message: str) -> dict[str, Any]: return _post(_role_path("i_have_committed"), {"message": message}) +def submit_for_qa(task_id: str) -> dict[str, Any]: + """Push your branch and open a PR. Run after your last commit, before i_am_done.""" + return _post(_role_path("submit_for_qa"), {"task_id": task_id}) + + def i_am_done(task_id: str, notes: str = "") -> dict[str, Any]: - """Submit work for QA. Runs verify/push/PR/submit-qa as needed.""" + """Submit for QA. Strict — PR must be open (call submit_for_qa first).""" return _post(_role_path("i_am_done"), {"task_id": task_id, "notes": notes}) @@ -205,6 +210,7 @@ _TOOLS: dict[str, Any] = { "give_me_work": give_me_work, "i_will_work_on": i_will_work_on, "i_have_committed": i_have_committed, + "submit_for_qa": submit_for_qa, "i_am_done": i_am_done, "i_am_blocked": i_am_blocked, "i_am_idle": i_am_idle, diff --git a/roboco/services/gateway/choreographer.py b/roboco/services/gateway/choreographer.py index 63dfc8bd..f36eb33e 100644 --- a/roboco/services/gateway/choreographer.py +++ b/roboco/services/gateway/choreographer.py @@ -340,6 +340,67 @@ class Choreographer: context_briefing=await self._briefing_for(agent_id, t.id), ) + async def submit_for_qa(self, agent_id: UUID, task_id: UUID) -> Envelope: + """Push the dev's branch and open a PR. Does NOT submit for QA itself — + the dev calls ``i_am_done`` after this verb returns success. + + Gate E made ``i_am_done`` strict: it requires ``pr_number`` set. The + catch-up shortcut lives off the dev manifest, so before this verb + existed devs had no escape from the NO_PR rejection. ``submit_for_qa`` + is the explicit push + open-PR step, leaving ``i_am_done`` to do the + strict submit. + + Pre-flight: caller must own the task, have committed at least once, + and not already have a PR open. If a PR is already open, this verb + is idempotent — it points the dev at ``i_am_done``. + """ + t = await self.task.get(task_id) + if t is None: + return Envelope.not_found(message=f"task {task_id} not found") + briefing = await self._briefing_for(agent_id, task_id) + if t.assigned_to != agent_id: + return Envelope.not_authorized( + message=f"task {task_id} is not assigned to you", + remediate="call give_me_work() to find your work", + context_briefing=briefing, + ) + if not t.commits: + return Envelope.invalid_state( + message="no commits on this task yet", + remediate=( + "commit at least one change before submitting for QA — " + "call commit(message='')" + ), + context_briefing=briefing, + ) + if t.pr_number is not None: + return Envelope.ok( + status=str(t.status), + task_id=str(task_id), + next=( + f"PR #{t.pr_number} already open; call " + f"i_am_done(task_id, notes='...') when self-verified" + ), + context_briefing=briefing, + ) + + await self._touch(task_id) + await self.git.push_branch(t.branch_name) + parent = parent_branch_for(t.branch_name) + pr = await self.git.create_pr( + t.branch_name, parent=parent, is_root_pr=False + ) + + return Envelope.ok( + status=str(t.status), + task_id=str(task_id), + next=( + f"PR #{pr['pr_number']} opened; call " + f"i_am_done(task_id, notes='...') when self-verified" + ), + context_briefing=briefing, + ) + async def i_am_done(self, agent_id: UUID, task_id: UUID, notes: str) -> Envelope: """Submit work for QA — strict path. diff --git a/roboco/services/gateway/role_config.py b/roboco/services/gateway/role_config.py index f78d84d2..de4f8366 100644 --- a/roboco/services/gateway/role_config.py +++ b/roboco/services/gateway/role_config.py @@ -26,6 +26,7 @@ _DEV_FLOW = ( "give_me_work", "i_will_work_on", "i_have_committed", + "submit_for_qa", "i_am_done", "i_am_blocked", "i_am_idle", diff --git a/tests/unit/gateway/test_submit_for_qa.py b/tests/unit/gateway/test_submit_for_qa.py new file mode 100644 index 00000000..ccc25da3 --- /dev/null +++ b/tests/unit/gateway/test_submit_for_qa.py @@ -0,0 +1,175 @@ +"""submit_for_qa pushes the current branch and opens a PR. + +Gate E (commit c5c2016) made `i_am_done` strict — requires `pr_number` set. +The catch-up flow is off the dev manifest. Devs need an explicit verb that +pushes + opens the PR so a subsequent `i_am_done` can do the strict submit. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + + +def _make_deps(**overrides: AsyncMock) -> ChoreographerDeps: + """Local dep-builder. Established pattern: per-test-file, not centralized.""" + task = overrides.get("task", AsyncMock()) + work_session = overrides.get("work_session", AsyncMock()) + git = overrides.get("git", AsyncMock()) + a2a = overrides.get("a2a", AsyncMock()) + journal = overrides.get("journal", AsyncMock()) + audit = overrides.get("audit", AsyncMock()) + evidence_repo = overrides.get("evidence_repo", AsyncMock()) + for method in ( + "list_unread_a2a", + "list_unread_mentions", + "list_pending_notifications", + "task_metadata_gaps", + "recent_team_activity", + "blockers_in_lane", + ): + getattr(evidence_repo, method).return_value = [] + return ChoreographerDeps( + task=task, + work_session=work_session, + git=git, + a2a=a2a, + journal=journal, + audit=audit, + evidence_repo=evidence_repo, + ) + + +@pytest.mark.asyncio +async def test_submit_for_qa_pushes_and_opens_pr() -> None: + aid = uuid4() + tid = uuid4() + t = MagicMock( + id=tid, + status="in_progress", + assigned_to=aid, + plan="x", + commits=[{"sha": "abc"}], + pr_number=None, + branch_name="feature/backend/abc12345", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + git_svc = AsyncMock() + git_svc.push_branch.return_value = ("feature/backend/abc12345", 1) + git_svc.create_pr.return_value = { + "pr_number": 42, + "pr_url": "https://gh/x/42", + "is_root_pr": False, + } + work_session_svc = AsyncMock() + deps = _make_deps(task=task_svc, git=git_svc, work_session=work_session_svc) + c = Choreographer(deps) + + env = await c.submit_for_qa(aid, tid) + + git_svc.push_branch.assert_awaited() + git_svc.create_pr.assert_awaited() + assert env.error is None + assert env.next is not None + assert "42" in env.next # remediate points to i_am_done with PR ref + + +@pytest.mark.asyncio +async def test_submit_for_qa_rejects_when_not_assigned() -> None: + aid = uuid4() + other = uuid4() + tid = uuid4() + t = MagicMock( + id=tid, + status="in_progress", + assigned_to=other, + plan="x", + commits=[{"sha": "abc"}], + pr_number=None, + branch_name="feature/backend/abc12345", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + git_svc = AsyncMock() + deps = _make_deps(task=task_svc, git=git_svc) + c = Choreographer(deps) + + env = await c.submit_for_qa(aid, tid) + + git_svc.push_branch.assert_not_awaited() + git_svc.create_pr.assert_not_awaited() + assert env.error == "not_authorized" + + +@pytest.mark.asyncio +async def test_submit_for_qa_rejects_when_no_commits() -> None: + aid = uuid4() + tid = uuid4() + t = MagicMock( + id=tid, + status="in_progress", + assigned_to=aid, + plan="x", + commits=[], + pr_number=None, + branch_name="feature/backend/abc12345", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + git_svc = AsyncMock() + deps = _make_deps(task=task_svc, git=git_svc) + c = Choreographer(deps) + + env = await c.submit_for_qa(aid, tid) + + git_svc.push_branch.assert_not_awaited() + git_svc.create_pr.assert_not_awaited() + assert env.error == "invalid_state" + assert env.remediate is not None + assert "commit" in env.remediate.lower() + + +@pytest.mark.asyncio +async def test_submit_for_qa_idempotent_when_pr_already_open() -> None: + aid = uuid4() + tid = uuid4() + t = MagicMock( + id=tid, + status="in_progress", + assigned_to=aid, + plan="x", + commits=[{"sha": "abc"}], + pr_number=7, + branch_name="feature/backend/abc12345", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + git_svc = AsyncMock() + deps = _make_deps(task=task_svc, git=git_svc) + c = Choreographer(deps) + + env = await c.submit_for_qa(aid, tid) + + git_svc.push_branch.assert_not_awaited() + git_svc.create_pr.assert_not_awaited() + assert env.error is None + assert env.next is not None + assert "7" in env.next + + +@pytest.mark.asyncio +async def test_submit_for_qa_returns_not_found_for_unknown_task() -> None: + aid = uuid4() + tid = uuid4() + task_svc = AsyncMock() + task_svc.get.return_value = None + deps = _make_deps(task=task_svc) + c = Choreographer(deps) + + env = await c.submit_for_qa(aid, tid) + + assert env.error == "not_found"