feat(qa): require a per-acceptance-criterion verdict before pass_review

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.
This commit is contained in:
Renn F
2026-06-14 04:44:21 +02:00
parent 8affb283f5
commit 2db11c7833
9 changed files with 174 additions and 10 deletions
+2 -2
View File
@@ -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=<dev>, text='<question>')` to ask before failing.
8. `note(scope='learning', text="<what worked / what would have caught the issue earlier / what pattern this work establishes>")` — 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=["<concrete actionable issue>", "<another>", ...])` — 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 <commit/file/line>", "criterion 2 — verified by <artifact>", ...])`**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=["<concrete actionable issue>", "<another>", ...])` — each issue is a single string. Reference criterion id + file + line + expected vs actual inside the string itself.
## Journaling cadence
+3 -1
View File
@@ -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)
+8
View File
@@ -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):
+13 -3
View File
@@ -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]:
+68 -2
View File
@@ -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:
@@ -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"
+6 -1
View File
@@ -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
@@ -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(
+67
View File
@@ -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"