From 2db11c78339d7c14520ae29352be2f9f2744aae8 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 14 Jun 2026 04:44:21 +0200 Subject: [PATCH] feat(qa): require a per-acceptance-criterion verdict before pass_review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA may no longer pass a task with a single gestalt approval — pass_review now takes ac_verdicts (one verification entry per acceptance criterion) and the gateway rejects a pass that does not cover every criterion. If a criterion does not hold, QA fails the review instead. The verdicts are folded into the persisted qa_notes for the audit trail. Threaded through the flow MCP tool, the HTTP request schema, and the route; QA prompt updated. --- agents/prompts/roles/qa.md | 4 +- roboco/api/routes/v1/flow_qa.py | 4 +- roboco/api/schemas/v1/flow.py | 8 +++ roboco/mcp/flow_server.py | 16 ++++- roboco/services/gateway/choreographer/qa.py | 70 ++++++++++++++++++- .../test_full_lifecycle_real_db.py | 7 +- tests/integration/test_lifecycle_real_db.py | 7 +- .../v1/test_full_pending_to_completed.py | 1 + tests/unit/gateway/test_qa_ac_coverage.py | 67 ++++++++++++++++++ 9 files changed, 174 insertions(+), 10 deletions(-) create mode 100644 tests/unit/gateway/test_qa_ac_coverage.py diff --git a/agents/prompts/roles/qa.md b/agents/prompts/roles/qa.md index 70cb195d..059cddb2 100644 --- a/agents/prompts/roles/qa.md +++ b/agents/prompts/roles/qa.md @@ -18,7 +18,7 @@ A pass without evidence is a betrayal of your role: the entire downstream chain |---|---|---| | `give_me_work()` | Returns a task in `awaiting_qa` for your team or `idle`. | None. | | `claim_review(task_id)` | Claims the QA task; returns PR data inline. | Task in `awaiting_qa`; you are not the original developer. | -| `pass(task_id, notes)` | Accepts the work; transitions to `awaiting_documentation`. | Task claimed by you; `notes` >= 80 chars; journal `learning` entry recorded. | +| `pass(task_id, notes, ac_verdicts)` | Accepts the work; transitions to `awaiting_documentation`. `ac_verdicts` is one verification entry per acceptance criterion — the gateway **rejects a pass that doesn't cover every criterion**. | Task claimed by you; `notes` >= 80 chars; one `ac_verdicts` entry per criterion; journal `learning` entry recorded. | | `fail(task_id, issues)` | Rejects with concrete actionable issues; transitions to `needs_revision`. | Task claimed by you; each issue references criterion/file/line. | | `unclaim(task_id)` | Release this claim back to pending. Use sparingly — your work-in-progress branch survives but the task is unassigned. | Task assigned to you and in claimed/in_progress. | | `resume(task_id)` | Resume a paused task. Transitions paused → in_progress. | Task assigned to you and in paused state. | @@ -53,7 +53,7 @@ A pass without evidence is a betrayal of your role: the entire downstream chain 6. Run tests/lint via `Bash` (e.g. `make quality` or `pytest`) — even if the dev says they passed, you re-run. 7. `note(scope='struggle', text='...')` if you can't decide — flag the ambiguity rather than guess. Then `dm(recipient=, text='')` to ask before failing. 8. `note(scope='learning', text="")` — required before pass/fail. -9. Pass: `pass(task_id, notes="<>=80 chars: what you reviewed, which acceptance criteria were verified by which artifacts, edge cases tested, any caveats>")`. Fail: `fail(task_id, issues=["", "", ...])` — each issue is a single string. Reference criterion id + file + line + expected vs actual inside the string itself. +9. Pass: `pass(task_id, notes="<>=80 chars: overall review summary, edge cases tested, any caveats>", ac_verdicts=["criterion 1 — verified by ", "criterion 2 — verified by ", ...])` — **one entry per acceptance criterion, in the task's criterion order**; the gateway rejects a pass that leaves any criterion uncovered. If even one criterion does not hold, do NOT pass — `fail` instead. Fail: `fail(task_id, issues=["", "", ...])` — each issue is a single string. Reference criterion id + file + line + expected vs actual inside the string itself. ## Journaling cadence diff --git a/roboco/api/routes/v1/flow_qa.py b/roboco/api/routes/v1/flow_qa.py index 998a8854..5c73732f 100644 --- a/roboco/api/routes/v1/flow_qa.py +++ b/roboco/api/routes/v1/flow_qa.py @@ -58,7 +58,9 @@ async def qa_pass( x_agent_id: _AgentIdHeader, choreographer: _ChoreographerDep, ) -> dict: - env = await choreographer.pass_review(x_agent_id, body.task_id, body.notes) + env = await choreographer.pass_review( + x_agent_id, body.task_id, body.notes, body.ac_verdicts + ) return envelope_to_response(env, request) diff --git a/roboco/api/schemas/v1/flow.py b/roboco/api/schemas/v1/flow.py index 692c2ff2..734873d6 100644 --- a/roboco/api/schemas/v1/flow.py +++ b/roboco/api/schemas/v1/flow.py @@ -110,6 +110,14 @@ class ClaimReviewRequest(BaseModel): class PassReviewRequest(BaseModel): task_id: UUID notes: str = Field(..., min_length=1) + ac_verdicts: list[str] | None = Field( + default=None, + description=( + "One verification entry per acceptance criterion (in criterion " + "order) stating how QA verified it. Every criterion must be " + "covered before a pass is allowed." + ), + ) class FailReviewRequest(BaseModel): diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index 4c1b40da..922dc46f 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -332,9 +332,19 @@ def claim_review(task_id: str) -> dict[str, Any]: return _post(_role_path("claim_review"), {"task_id": task_id}) -def pass_review(task_id: str, notes: str) -> dict[str, Any]: - """QA: accept the work. notes >= 80 chars; journal:learning required.""" - return _post(_role_path("pass"), {"task_id": task_id, "notes": notes}) +def pass_review( + task_id: str, notes: str, ac_verdicts: list[str] | None = None +) -> dict[str, Any]: + """QA: accept the work. notes >= 80 chars; journal:learning required. + + ac_verdicts: one entry per acceptance criterion (in criterion order) stating + how you verified it. Every criterion must be covered — a pass is rejected + until all are. If any criterion does not hold, call fail_review instead. + """ + payload: dict[str, Any] = {"task_id": task_id, "notes": notes} + if ac_verdicts is not None: + payload["ac_verdicts"] = ac_verdicts + return _post(_role_path("pass"), payload) def fail_review(task_id: str, issues: list[str]) -> dict[str, Any]: diff --git a/roboco/services/gateway/choreographer/qa.py b/roboco/services/gateway/choreographer/qa.py index 2ed3b47a..b8d0042b 100644 --- a/roboco/services/gateway/choreographer/qa.py +++ b/roboco/services/gateway/choreographer/qa.py @@ -332,8 +332,66 @@ class QAMixin(_Base): ) return None, agent, role_str + @staticmethod + def _nonblank_verdicts(ac_verdicts: list[str] | None) -> list[str]: + """Non-empty verdict strings (defensive against blanks / non-strings).""" + return [ + v.strip() for v in (ac_verdicts or []) if isinstance(v, str) and v.strip() + ] + + @classmethod + def _qa_ac_coverage_check( + cls, task: Any, ac_verdicts: list[str] | None + ) -> Envelope | None: + """Per-acceptance-criterion verification gate for pass_review. + + QA may not pass a task until it has recorded a verification for EVERY + acceptance criterion. A single gestalt "looks good" approval is how a + silently-unbuilt criterion slips through; requiring one verdict per + criterion forces QA to check each individually. If a criterion does not + hold, the QA fails the review instead of passing a partial. + """ + criteria = list(getattr(task, "acceptance_criteria", None) or []) + if not criteria: + return None + verdicts = cls._nonblank_verdicts(ac_verdicts) + if len(verdicts) >= len(criteria): + return None + return Envelope.invalid_state( + message=( + f"pass_review needs a verification for each of the " + f"{len(criteria)} acceptance criteria; got {len(verdicts)}." + ), + remediate=( + "Re-call pass_review with ac_verdicts=[...] — one entry per " + "acceptance criterion (in the task's criterion order) stating " + "how you verified it. If any criterion does NOT hold, call " + "fail_review with the specific gaps instead of passing a partial." + ), + context_briefing={}, + ) + + @classmethod + def _merge_ac_verdicts_into_notes( + cls, notes: str, ac_verdicts: list[str] | None + ) -> str: + """Fold the per-criterion verdicts into the persisted QA notes. + + Keeps the per-criterion verification in the audit trail (qa_notes), so + PM/CEO can see exactly which criteria QA checked and how. + """ + lines = cls._nonblank_verdicts(ac_verdicts) + if not lines: + return notes + body = "\n".join(f"- {line}" for line in lines) + return f"{notes}\n\nPer-criterion verification:\n{body}" + async def pass_review( - self, qa_agent_id: UUID, task_id: UUID, notes: str + self, + qa_agent_id: UUID, + task_id: UUID, + notes: str, + ac_verdicts: list[str] | None = None, ) -> Envelope: """QA passes the task; transitions awaiting_qa → awaiting_documentation. @@ -368,13 +426,21 @@ class QAMixin(_Base): ) if gate_rejection is not None: return gate_rejection + ac_rejection = self._qa_ac_coverage_check(t, ac_verdicts) + if ac_rejection is not None: + return await self._emit_rejection( + ac_rejection.with_introspection(task=t, role=role_str), + agent_id=qa_agent_id, + task_id=task_id, + verb="pass_review", + ) briefing = await self._briefing_for(qa_agent_id, task_id) spec_ctx = spec_module.Context( actor_id=qa_agent_id, actor_slug=getattr(agent, "slug", None) if agent is not None else None, original_developer_slug=_extract_original_developer(t), - notes=notes, + notes=self._merge_ac_verdicts_into_notes(notes, ac_verdicts), ) runner = self._verb_runner() try: diff --git a/tests/integration/test_full_lifecycle_real_db.py b/tests/integration/test_full_lifecycle_real_db.py index 1e697b79..99bb5979 100644 --- a/tests/integration/test_full_lifecycle_real_db.py +++ b/tests/integration/test_full_lifecycle_real_db.py @@ -494,7 +494,12 @@ async def test_full_chain_through_doc_handoff( "Reviewed the diff; route returns 200 OK with timestamp. Tests cover " "both acceptance criteria. Approving." ) - env = await c.pass_review(qa_agent.id, task.id, notes=qa_notes) + env = await c.pass_review( + qa_agent.id, + task.id, + notes=qa_notes, + ac_verdicts=[f"verified: {crit}" for crit in task.acceptance_criteria], + ) assert env.error is None, f"pass_review failed: {env.message}" assert env.status == "awaiting_documentation" diff --git a/tests/integration/test_lifecycle_real_db.py b/tests/integration/test_lifecycle_real_db.py index 04cf0bf7..78a11561 100644 --- a/tests/integration/test_lifecycle_real_db.py +++ b/tests/integration/test_lifecycle_real_db.py @@ -495,7 +495,12 @@ async def test_qa_pass_path( assert str(after_claim.status) == Status.AWAITING_QA.value assert after_claim.assigned_to == qa_agent.id - env = await c.pass_review(qa_agent.id, task.id, notes=_QA_PASS_NOTES) + env = await c.pass_review( + qa_agent.id, + task.id, + notes=_QA_PASS_NOTES, + ac_verdicts=[f"verified: {crit}" for crit in task.acceptance_criteria], + ) assert env.error is None, f"pass_review failed: {env.message}" assert env.status == Status.AWAITING_DOCUMENTATION.value diff --git a/tests/integration/v1/test_full_pending_to_completed.py b/tests/integration/v1/test_full_pending_to_completed.py index c5b29755..a89bf76e 100644 --- a/tests/integration/v1/test_full_pending_to_completed.py +++ b/tests/integration/v1/test_full_pending_to_completed.py @@ -103,6 +103,7 @@ class _MockChoreographer: _agent_id: object, _task_id: object, _notes: object, + _ac_verdicts: object = None, ) -> Envelope: self._state["task_status"] = "awaiting_documentation" return Envelope.ok( diff --git a/tests/unit/gateway/test_qa_ac_coverage.py b/tests/unit/gateway/test_qa_ac_coverage.py new file mode 100644 index 00000000..13cb617a --- /dev/null +++ b/tests/unit/gateway/test_qa_ac_coverage.py @@ -0,0 +1,67 @@ +"""pass_review requires a verification for every acceptance criterion. + +A gestalt "looks good" approval is how a silently-unbuilt criterion slips +through QA; the coverage gate forces one verdict per criterion before a pass is +allowed. If a criterion does not hold, QA fails the review instead. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +from roboco.services.gateway.choreographer import Choreographer + + +def _task(criteria: list[str]) -> SimpleNamespace: + return SimpleNamespace(acceptance_criteria=criteria) + + +def test_no_criteria_imposes_no_requirement() -> None: + assert Choreographer._qa_ac_coverage_check(_task([]), None) is None + + +def test_full_coverage_passes() -> None: + t = _task(["returns 200", "includes timestamp"]) + assert ( + Choreographer._qa_ac_coverage_check(t, ["200 ok via test", "ts in diff"]) + is None + ) + + +def test_partial_coverage_is_rejected() -> None: + t = _task(["a", "b", "c"]) + env = Choreographer._qa_ac_coverage_check(t, ["only one verified"]) + assert env is not None + body = env.as_dict() + assert body["error"] == "invalid_state", body + assert "fail_review" in (env.remediate or "") + + +def test_missing_verdicts_entirely_is_rejected() -> None: + t = _task(["a", "b"]) + env = Choreographer._qa_ac_coverage_check(t, None) + assert env is not None + + +def test_blank_verdicts_do_not_count() -> None: + t = _task(["a", "b"]) + env = Choreographer._qa_ac_coverage_check(t, ["a ok", " "]) + assert env is not None, "whitespace-only verdict must not satisfy a criterion" + + +def test_extra_verdicts_are_allowed() -> None: + t = _task(["a"]) + assert Choreographer._qa_ac_coverage_check(t, ["a ok", "bonus note"]) is None + + +def test_verdicts_fold_into_persisted_notes() -> None: + merged = Choreographer._merge_ac_verdicts_into_notes( + "base review", ["a ok", "b ok"] + ) + assert "Per-criterion verification" in merged + assert "- a ok" in merged + assert "- b ok" in merged + + +def test_merge_with_no_verdicts_returns_notes_unchanged() -> None: + assert Choreographer._merge_ac_verdicts_into_notes("base", None) == "base"