mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F008] evidence_builder: surface persisted pr_review verdict+issues in the PM task_handoff
The pr_fail a2a steer to the owning PM is fire-and-forget; a PM respawned into needs_revision later read none of it (build_task_handoff never looked at notes_structured), saw a generic 'needs revision' with zero concrete change-requests, and re-submitted the same PR (the 2026-06-27 infinite pr_fail loop on 9980d0a0 / PR #138). The signal-gap was only partially closed by the a2a. build_task_handoff now extracts notes_structured.pr_review (verdict/summary/issues/head_sha — the slot pr_fail authors on every fail) into a pr_review field on the handoff, so every PM briefing for the task carries the concrete change-requests. A prior pr_fail alone now counts as prior-work-worth-resuming. Type-guarded + capped; absent => no key (no misleading empty slot). TDD red->green; ruff + mypy clean; evidence_builder suite green (14 passed).
This commit is contained in:
@@ -106,6 +106,15 @@ def build_task_handoff(
|
||||
# Upstream dependencies that completed and were cleared — present only on a
|
||||
# just-unblocked task, so the revived dependent knows what it can build on.
|
||||
completed_deps = _typed(getattr(task, "completed_dependency_ids", None), list, [])
|
||||
# F008 — the persisted in-path PR-review gate verdict + concrete issues.
|
||||
# ``pr_fail`` authors ``notes_structured.pr_review`` (verdict / summary /
|
||||
# issues / head_sha) on every fail, but the a2a steer to the owning PM is
|
||||
# fire-and-forget — a PM respawned into ``needs_revision`` later read none
|
||||
# of it (build_task_handoff never looked at notes_structured), saw a generic
|
||||
# "needs revision" with zero change-requests, and re-submitted the same PR
|
||||
# (the 2026-06-27 infinite pr_fail loop on 9980d0a0 / PR #138). Surfacing it
|
||||
# here puts the concrete issues in every PM briefing for the task.
|
||||
pr_review = _extract_pr_review(getattr(task, "notes_structured", None))
|
||||
has_prior = bool(
|
||||
commits
|
||||
or acceptance
|
||||
@@ -113,10 +122,11 @@ def build_task_handoff(
|
||||
or pr_number is not None
|
||||
or dev_summary
|
||||
or completed_deps
|
||||
or pr_review is not None
|
||||
)
|
||||
if not has_prior:
|
||||
return None
|
||||
return {
|
||||
handoff: dict[str, Any] = {
|
||||
"pr_number": pr_number,
|
||||
"pr_url": _typed(task.pr_url, str, None),
|
||||
"branch_name": _typed(task.branch_name, str, None),
|
||||
@@ -129,6 +139,40 @@ def build_task_handoff(
|
||||
str(d) for d in completed_deps[:BRIEFING_LIST_CAP]
|
||||
],
|
||||
}
|
||||
if pr_review is not None:
|
||||
handoff["pr_review"] = pr_review
|
||||
return handoff
|
||||
|
||||
|
||||
def _extract_pr_review(notes_structured: Any) -> dict[str, Any] | None:
|
||||
"""Pull the canonical ``pr_review`` slot out of ``notes_structured``.
|
||||
|
||||
Returns ``None`` when there is no structured note, no ``pr_review`` key, or
|
||||
the slot isn't a dict — so the handoff omits the field entirely (no
|
||||
misleading empty slot) for a task with no prior gate verdict. Only the
|
||||
well-typed scalar/list fields the gate writes are forwarded; anything else
|
||||
degrades to a safe default so a malformed slot never leaks a non-JSON
|
||||
object into the briefing.
|
||||
"""
|
||||
if not isinstance(notes_structured, dict):
|
||||
return None
|
||||
raw = notes_structured.get("pr_review")
|
||||
if not isinstance(raw, dict):
|
||||
return None
|
||||
verdict = _typed(raw.get("verdict"), str, None)
|
||||
summary = _typed(raw.get("summary"), str, None)
|
||||
issues = _typed(raw.get("issues"), list, [])
|
||||
head_sha = _typed(raw.get("head_sha"), str, None)
|
||||
if not (verdict or summary or issues or head_sha):
|
||||
return None
|
||||
surface: dict[str, Any] = {"issues": list(issues[:BRIEFING_LIST_CAP])}
|
||||
if verdict:
|
||||
surface["verdict"] = verdict
|
||||
if summary:
|
||||
surface["summary"] = summary
|
||||
if head_sha:
|
||||
surface["head_sha"] = head_sha
|
||||
return surface
|
||||
|
||||
|
||||
def build_context_briefing(inputs: BriefingInputs) -> dict[str, Any]:
|
||||
|
||||
@@ -187,3 +187,52 @@ class TestTaskHandoff:
|
||||
assert digest["journal_highlights"] == []
|
||||
assert digest["pr_url"] is None
|
||||
assert digest["branch_name"] is None
|
||||
|
||||
|
||||
class TestPrReviewSurface:
|
||||
"""F008 — the persisted pr_fail verdict + issues must surface in the PM
|
||||
briefing's task_handoff, not just the fire-and-forget a2a. A PM respawned
|
||||
into ``needs_revision`` after a pr_fail otherwise sees a generic "needs
|
||||
revision" with zero concrete change-requests and re-submits the same PR."""
|
||||
|
||||
def test_surfaces_pr_fail_verdict_and_issues(self) -> None:
|
||||
t = _task(pr_number=138, commits=[{"sha": "abc", "message": "feat: x"}])
|
||||
t.notes_structured = {
|
||||
"pr_review": {
|
||||
"verdict": "failed",
|
||||
"summary": "In-path PR-review gate requested changes.",
|
||||
"issues": ["missing null guard", "no test for the edge case"],
|
||||
"head_sha": "aaaa1111bbbb2222",
|
||||
}
|
||||
}
|
||||
digest = build_task_handoff(t, [])
|
||||
assert digest is not None
|
||||
pr_review = digest["pr_review"]
|
||||
assert pr_review["verdict"] == "failed"
|
||||
assert pr_review["issues"] == [
|
||||
"missing null guard",
|
||||
"no test for the edge case",
|
||||
]
|
||||
assert pr_review["head_sha"] == "aaaa1111bbbb2222"
|
||||
|
||||
def test_no_pr_review_field_when_none_present(self) -> None:
|
||||
t = _task(pr_number=8, commits=[{"sha": "abc", "message": "feat: x"}])
|
||||
t.notes_structured = None
|
||||
digest = build_task_handoff(t, [])
|
||||
assert digest is not None
|
||||
# Absent pr_review ⇒ no key (not a None-valued key) so a PM without a
|
||||
# prior gate verdict doesn't see a misleading empty slot.
|
||||
assert "pr_review" not in digest
|
||||
|
||||
def test_pr_review_alone_is_prior_work_worth_resuming(self) -> None:
|
||||
"""A task with no commits/dev-notes but a prior pr_fail verdict still
|
||||
surfaces the handoff so the owning PM reads the change-requests."""
|
||||
t = _task(pr_number=None, pr_url=None, dev_notes="")
|
||||
t.commits = []
|
||||
t.acceptance_criteria_status = []
|
||||
t.notes_structured = {
|
||||
"pr_review": {"verdict": "failed", "issues": ["fix the off-by-one"]}
|
||||
}
|
||||
digest = build_task_handoff(t, [])
|
||||
assert digest is not None
|
||||
assert digest["pr_review"]["issues"] == ["fix the off-by-one"]
|
||||
|
||||
Reference in New Issue
Block a user