mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F046] pr_gate: guard None runner result on concurrent transition (pr_pass/pr_fail)
_gate_decision dereferenced the verb-runner result without a None guard. run_intent returns None when a concurrent transition (cancel or a racing reviewer) moves the task out of awaiting_pr_review between the precondition gate and the runner's final composed action (the verb runner's documented last-action source-status contract). The subsequent t.assigned_to / t.status / _post_gate_review_to_pr(t, ...) dereferences then crashed the gate with a 500 AttributeError. Add a None guard that surfaces a clean invalid_state rejection (re-fetch + re-issue) before any dereference; no PR post or a2a runs against a None task. TDD test_pr_gate_notifies_pm.py (+2).
This commit is contained in:
@@ -269,6 +269,34 @@ class PRGateMixin(_Base):
|
|||||||
task_id=task_id,
|
task_id=task_id,
|
||||||
verb=verb,
|
verb=verb,
|
||||||
)
|
)
|
||||||
|
# F046: a concurrent transition (cancel, or a racing reviewer) between
|
||||||
|
# the precondition gate and the runner's final composed action makes
|
||||||
|
# the source-status check fail mid-flight and run_intent returns None
|
||||||
|
# (the verb runner's documented contract for a last-action source-status
|
||||||
|
# failure). Without this guard the dereferences below (t.assigned_to,
|
||||||
|
# t.status, _post_gate_review_to_pr(t, ...)) crash the gate with a 500
|
||||||
|
# AttributeError. Surface a clean invalid_state rejection so the
|
||||||
|
# reviewer re-fetches with evidence(task_id) and re-issues — the
|
||||||
|
# already-authored verdict note is harmless (the task is no longer in
|
||||||
|
# the gate state) and no PR post / a2a runs against a None task.
|
||||||
|
if t is None:
|
||||||
|
return await self._emit_rejection(
|
||||||
|
Envelope.invalid_state(
|
||||||
|
message=(
|
||||||
|
f"{verb}: the task moved out of awaiting_pr_review before"
|
||||||
|
" the decision committed — a concurrent transition"
|
||||||
|
" (cancel or a racing reviewer) beat you to it."
|
||||||
|
),
|
||||||
|
remediate=(
|
||||||
|
"re-fetch with evidence(task_id) and re-issue your gate"
|
||||||
|
" verb once the task is back in awaiting_pr_review"
|
||||||
|
),
|
||||||
|
context_briefing=briefing,
|
||||||
|
),
|
||||||
|
agent_id=reviewer_agent_id,
|
||||||
|
task_id=task_id,
|
||||||
|
verb=verb,
|
||||||
|
)
|
||||||
# Leave the gate verdict on the PR itself so there's a visible trail on
|
# Leave the gate verdict on the PR itself so there's a visible trail on
|
||||||
# the very PR the PM (or CEO) merges. Best-effort and AFTER the DB
|
# the very PR the PM (or CEO) merges. Best-effort and AFTER the DB
|
||||||
# transition — a GitHub failure must not roll back the gate decision.
|
# transition — a GitHub failure must not roll back the gate decision.
|
||||||
|
|||||||
@@ -238,3 +238,62 @@ async def test_pr_fail_a2a_failure_is_swallowed() -> None:
|
|||||||
env = await c.pr_fail(reviewer_id, task_id, ["a concrete actionable issue"])
|
env = await c.pr_fail(reviewer_id, task_id, ["a concrete actionable issue"])
|
||||||
# Verdict still landed — the owning PM is in needs_revision.
|
# Verdict still landed — the owning PM is in needs_revision.
|
||||||
assert env.status == "needs_revision"
|
assert env.status == "needs_revision"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_pr_fail_returns_invalid_state_when_runner_returns_none() -> None:
|
||||||
|
"""F046: if a concurrent transition (cancel or a racing reviewer) moved the
|
||||||
|
task out of ``awaiting_pr_review`` between the precondition gate and the
|
||||||
|
runner's final composed action, ``run_intent`` returns None (the verb
|
||||||
|
runner's documented contract for a last-action source-status failure).
|
||||||
|
``_gate_decision`` must surface a clean ``invalid_state`` rejection so the
|
||||||
|
reviewer re-fetches and re-issues — NOT dereference None and crash the
|
||||||
|
gate with a 500 AttributeError on ``t.assigned_to`` / ``t.status``.
|
||||||
|
"""
|
||||||
|
reviewer_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
t_before = MagicMock(
|
||||||
|
id=task_id,
|
||||||
|
assigned_to=reviewer_id,
|
||||||
|
pr_number=44,
|
||||||
|
parent_task_id=uuid4(),
|
||||||
|
status="awaiting_pr_review",
|
||||||
|
)
|
||||||
|
|
||||||
|
c = _make_choreographer()
|
||||||
|
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=None)
|
||||||
|
|
||||||
|
env = await c.pr_fail(reviewer_id, task_id, ["a concrete actionable issue"])
|
||||||
|
|
||||||
|
# Clean rejection, not a 500.
|
||||||
|
assert env.error == "invalid_state"
|
||||||
|
# No PR post / no a2a against a None task.
|
||||||
|
c._post_gate_review_to_pr.assert_not_awaited()
|
||||||
|
c.a2a.send.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_pr_pass_returns_invalid_state_when_runner_returns_none() -> None:
|
||||||
|
"""F046: the same None-guard covers pr_pass — a concurrent cancel between
|
||||||
|
gate and runner must surface invalid_state, not crash on ``str(t.status)``.
|
||||||
|
"""
|
||||||
|
reviewer_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
t_before = MagicMock(
|
||||||
|
id=task_id,
|
||||||
|
assigned_to=reviewer_id,
|
||||||
|
pr_number=45,
|
||||||
|
parent_task_id=uuid4(),
|
||||||
|
status="awaiting_pr_review",
|
||||||
|
)
|
||||||
|
|
||||||
|
c = _make_choreographer()
|
||||||
|
_stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=None)
|
||||||
|
|
||||||
|
env = await c.pr_pass(
|
||||||
|
reviewer_id, task_id, "Assembled root scope is clean and covered."
|
||||||
|
)
|
||||||
|
|
||||||
|
assert env.error == "invalid_state"
|
||||||
|
c._post_gate_review_to_pr.assert_not_awaited()
|
||||||
|
c.a2a.send.assert_not_awaited()
|
||||||
|
|||||||
Reference in New Issue
Block a user