mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Renn F
742e0ebfaf
Both verbs compose a single atomic action whose None return (the verb's own result) flowed out of run_intent and was dereferenced as t.status, HTTP 500-ing with no actionable rejection: - F016 submit_root: submit_for_review returns None when the root->master PR was already opened / the task raced out of in_progress. Post-runner None-guard extracted into _submit_root_finalize -> invalid_state (re-fetch; if awaiting_pr_review the PR is open, wait for reviewer; else re-delegate fixes and retry) instead of None.status. - F017 i_am_blocked: escalate returns None in four cases (no task, no agent, no resolvable escalation-target slug, no target agent row) e.g. a developer whose role has no PM above it. _run_i_am_blocked_intent now guards updated is None -> (t, invalid_state rejection) with remediation (re-fetch + escalate to CEO directly / retry) instead of the caller deref'ing None.status -> 500 + respawn-loop. TDD red->green; ruff + mypy clean; gateway suite green (58 passed).
433 lines
17 KiB
Python
433 lines
17 KiB
Python
"""The hard ``submit_root`` unchanged-PR gate — the pr_fail re-submit loop-stopper.
|
|
|
|
The 2026-06-27 infinite ``pr_fail`` loop: a Main-PM-owned root (S1
|
|
"chart-first Metrics", PR #139) was ``pr_fail``'d for a real code defect, routed
|
|
to ``needs_revision``, the Main PM re-claimed + re-delegated nothing, and
|
|
re-submitted the **unchanged** root → ``awaiting_pr_review`` → ``pr_fail`` again,
|
|
forever. The prior fixes (the ``pr_fail`` a2a steer + the ``next_hint`` "do NOT
|
|
re-submit") are *hints* — a weak coordinator (minimax-m3:cloud) ignored them and
|
|
re-submitted PR #139 byte-identical. Hints do not stop a model that won't read
|
|
them; only a structural refusal does.
|
|
|
|
This gate refuses the re-submit when the assembled root PR's head SHA is
|
|
unchanged since the last ``pr_fail`` (no new cell work landed on the root
|
|
branch). ``pr_fail`` stamps that SHA into ``notes_structured.pr_review.head_sha``
|
|
(``_capture_pr_head_sha`` + ``_record_gate_verdict``); ``submit_root`` reads it
|
|
back and compares against the PR's current head SHA. Equal ⇒ refuse; different
|
|
⇒ the branch advanced ⇒ allow. Every ambiguous case FAILS OPEN (no prior fail,
|
|
no recorded SHA, no PR number, no resolvable project, git/closed-PR lookup
|
|
returns ``None``) — only the exact-unchanged case is hard-blocked.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from datetime import UTC, datetime
|
|
from typing import Any
|
|
from unittest.mock import AsyncMock, MagicMock
|
|
from uuid import uuid4
|
|
|
|
import pytest
|
|
from roboco.foundation.policy import lifecycle as spec_module
|
|
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
|
|
|
|
SHA_OLD = "aaaa1111bbbb2222cccc3333dddd4444eeee5555"
|
|
SHA_NEW = "9999888877776666555544443333222211110000"
|
|
|
|
|
|
def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
|
base: dict[str, Any] = {
|
|
"task": AsyncMock(),
|
|
"work_session": AsyncMock(),
|
|
"git": AsyncMock(),
|
|
"a2a": AsyncMock(),
|
|
"journal": AsyncMock(),
|
|
"audit": AsyncMock(),
|
|
"evidence_repo": AsyncMock(),
|
|
}
|
|
base.update(overrides)
|
|
base["journal"].has_decision_for_task.return_value = True
|
|
base["journal"].latest_decision_at.return_value = datetime.now(UTC)
|
|
base["journal"].has_reflect_for_task.return_value = True
|
|
return ChoreographerDeps(**base)
|
|
|
|
|
|
def _resubmit_root(
|
|
*,
|
|
notes_structured: dict[str, Any] | None,
|
|
pr_number: int | None = 139,
|
|
) -> tuple[Choreographer, Any, Any]:
|
|
"""A Main-PM root re-submitted from ``in_progress`` after a ``pr_fail``.
|
|
|
|
Mirrors the live c80e19ff / PR #139 re-submit: the root is back in
|
|
``in_progress`` (re-claimed out of ``needs_revision``), carries the prior
|
|
``pr_fail`` verdict in ``notes_structured.pr_review``, and the PR is still
|
|
open. The ``_submit_up_guard`` preflight is satisfied (owned, journal
|
|
decision, subtasks terminal, branch present, notes long enough) so the
|
|
unchanged-PR gate is the thing under test.
|
|
"""
|
|
main_pm_id = uuid4()
|
|
root_task_id = uuid4()
|
|
in_prog = MagicMock(
|
|
id=root_task_id,
|
|
status="in_progress",
|
|
assigned_to=main_pm_id,
|
|
pr_number=pr_number,
|
|
branch_name="feature/main_pm/c80e19ff",
|
|
parent_task_id=None,
|
|
batch_id=None,
|
|
team="main_pm",
|
|
notes_structured=notes_structured,
|
|
)
|
|
gated = MagicMock(**{**in_prog.__dict__, "status": "awaiting_pr_review"})
|
|
task_svc = AsyncMock()
|
|
task_svc.get.return_value = in_prog
|
|
task_svc.submit_for_review.return_value = gated
|
|
task_svc.all_subtasks_terminal.return_value = True
|
|
task_svc.uncovered_parent_acceptance_criteria.return_value = []
|
|
task_svc.agent_for.return_value = MagicMock(role="main_pm", team="main_pm")
|
|
task_svc.session.begin_nested = MagicMock(
|
|
return_value=MagicMock(__aenter__=AsyncMock(), __aexit__=AsyncMock())
|
|
)
|
|
c = Choreographer(_make_deps(task=task_svc, git=AsyncMock()))
|
|
# Real _project_slug_for would walk a mock session into a MagicMock slug; the
|
|
# gate under test needs a real string slug + a controllable head SHA. Alias to
|
|
# ``Any`` so mypy doesn't flag the method-spy assignment (no type:ignore owed).
|
|
cc: Any = c
|
|
cc._project_slug_for = AsyncMock(return_value="proj-slug")
|
|
return c, main_pm_id, root_task_id
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# The hard block — refuse the byte-identical re-submit
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_refuses_unchanged_pr_after_pr_fail() -> None:
|
|
"""The loop-stopper: prior pr_fail stamped head SHA X, the PR head is still
|
|
X (no new cell work on the root branch) → refuse, do not open the gate."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={
|
|
"pr_review": {"verdict": "failed", "head_sha": SHA_OLD, "summary": "..."}
|
|
}
|
|
)
|
|
c.git.get_pr_head_sha = AsyncMock(return_value=SHA_OLD)
|
|
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submitting the root after the fix"
|
|
)
|
|
|
|
assert env.error is not None, env.as_dict()
|
|
assert env.error == "invalid_state"
|
|
assert "unchanged" in (env.message or "").lower()
|
|
remediate = env.remediate or ""
|
|
assert "re-delegate" in remediate
|
|
assert "submit_root" in remediate
|
|
# The PR was NOT re-opened / re-pushed — the runner never ran.
|
|
c.task.submit_for_review.assert_not_awaited()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_allows_after_root_branch_advanced() -> None:
|
|
"""A different current head SHA ⇒ cell work landed on the root branch ⇒
|
|
the diff changed ⇒ allow the re-submit into the gate."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={
|
|
"pr_review": {"verdict": "failed", "head_sha": SHA_OLD, "summary": "..."}
|
|
}
|
|
)
|
|
c.git.get_pr_head_sha = AsyncMock(return_value=SHA_NEW)
|
|
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submitting after the cell re-assembly"
|
|
)
|
|
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
c.task.submit_for_review.assert_awaited_once()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Fail-open — ambiguous cases proceed and rely on the reviewer to re-fail
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_fail_open_when_no_prior_pr_fail_verdict() -> None:
|
|
"""No pr_review (first submit) or a passed verdict ⇒ nothing to compare ⇒
|
|
allow."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(notes_structured=None)
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="first root submit; nothing to compare yet"
|
|
)
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_fail_open_when_prior_fail_recorded_no_head_sha() -> None:
|
|
"""A pr_fail verdict written before this field existed has no ``head_sha`` ⇒
|
|
cannot compare ⇒ allow (fail open, not wedge)."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={"pr_review": {"verdict": "failed", "summary": "..."}}
|
|
)
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submit; no recorded sha to compare"
|
|
)
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_fail_open_when_git_lookup_returns_none() -> None:
|
|
"""A closed/missing PR or a git error returns ``None`` ⇒ ambiguous ⇒ allow
|
|
(the reviewer can still pr_fail if the diff is bad)."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={
|
|
"pr_review": {"verdict": "failed", "head_sha": SHA_OLD, "summary": "..."}
|
|
}
|
|
)
|
|
c.git.get_pr_head_sha = AsyncMock(return_value=None)
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submit; the prior PR was closed or missing"
|
|
)
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_fail_open_when_no_pr_number() -> None:
|
|
"""A root with no ``pr_number`` has nothing to look up ⇒ allow."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={
|
|
"pr_review": {"verdict": "failed", "head_sha": SHA_OLD, "summary": "..."}
|
|
},
|
|
pr_number=None,
|
|
)
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submit; this root has no pr number"
|
|
)
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_fail_open_when_slug_unresolvable() -> None:
|
|
"""No resolvable project slug (a product-only root the product service can't
|
|
expand) ⇒ can't query git ⇒ allow."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={
|
|
"pr_review": {"verdict": "failed", "head_sha": SHA_OLD, "summary": "..."}
|
|
}
|
|
)
|
|
cc: Any = c
|
|
cc._project_slug_for = AsyncMock(return_value=None)
|
|
c.git.get_pr_head_sha = AsyncMock(return_value=SHA_OLD)
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submit; project slug unresolvable here"
|
|
)
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_fail_open_when_git_lookup_raises() -> None:
|
|
"""A git lookup that raises must not 500 the PM — the gate swallows it and
|
|
proceeds (fail open)."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(
|
|
notes_structured={
|
|
"pr_review": {"verdict": "failed", "head_sha": SHA_OLD, "summary": "..."}
|
|
}
|
|
)
|
|
c.git.get_pr_head_sha = AsyncMock(side_effect=RuntimeError("boom"))
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submit; git head-sha lookup raised an error"
|
|
)
|
|
assert env.error is None, env.as_dict()
|
|
assert env.status == "awaiting_pr_review"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# The capture side — pr_fail stamps the head SHA into notes_structured
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _make_choreographer_for_gate() -> Choreographer:
|
|
"""A choreographer wired to drive ``_gate_decision`` past preflight/tracing
|
|
and into the verdict-record step without exercising the heavy ownership
|
|
logic (those have their own tests). Mirrors test_pr_gate_notifies_pm."""
|
|
base: dict[str, Any] = {
|
|
"task": AsyncMock(),
|
|
"work_session": AsyncMock(),
|
|
"git": AsyncMock(),
|
|
"a2a": AsyncMock(),
|
|
"journal": AsyncMock(),
|
|
"audit": AsyncMock(),
|
|
"evidence_repo": AsyncMock(),
|
|
}
|
|
return Choreographer(ChoreographerDeps(**base))
|
|
|
|
|
|
def _stub_gate_path(
|
|
c: Choreographer, *, reviewer_id: Any, t_before: Any, t_after: Any
|
|
) -> MagicMock:
|
|
"""Drive ``_gate_decision`` past preflight/tracing/post and into the verdict
|
|
record step. Returns the ``_record_gate_verdict`` spy so callers can assert
|
|
on the recorded kwargs. The ``cc: Any`` alias is the method-spy idiom: mypy
|
|
doesn't flag attribute assignment on ``Any`` (no method-assign / no
|
|
attr-defined), so no ``type: ignore`` is owed and ruff's B010 (no ``setattr``
|
|
with a constant) is sidestepped too.
|
|
"""
|
|
cc: Any = c
|
|
agent = MagicMock(role="pr_reviewer", slug="be-pr-reviewer")
|
|
cc._gate_preflight = AsyncMock(
|
|
return_value=(
|
|
t_before,
|
|
agent,
|
|
"pr_reviewer",
|
|
{},
|
|
spec_module.Context(actor_id=reviewer_id),
|
|
)
|
|
)
|
|
cc._gate_tracing = AsyncMock(return_value=None)
|
|
# Spy on the verdict record so we can assert the head_sha kwarg without
|
|
# running the real apply_structured_note (which needs a real ORM task).
|
|
record_spy = MagicMock()
|
|
cc._record_gate_verdict = record_spy
|
|
cc._post_gate_review_to_pr = AsyncMock()
|
|
runner = MagicMock()
|
|
runner.run_intent = AsyncMock(return_value=t_after)
|
|
cc._verb_runner = MagicMock(return_value=runner)
|
|
return record_spy
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_pr_fail_captures_head_sha_into_verdict() -> None:
|
|
"""pr_fail resolves the PR's head SHA and threads it into the verdict record
|
|
so the next submit_root can compare against it."""
|
|
reviewer_id = uuid4()
|
|
pm_id = uuid4()
|
|
task_id = uuid4()
|
|
t_before = MagicMock(
|
|
id=task_id,
|
|
assigned_to=reviewer_id,
|
|
pr_number=139,
|
|
parent_task_id=uuid4(),
|
|
status="awaiting_pr_review",
|
|
)
|
|
t_after = MagicMock(
|
|
id=task_id,
|
|
assigned_to=pm_id,
|
|
pr_number=139,
|
|
parent_task_id=uuid4(),
|
|
status="needs_revision",
|
|
)
|
|
|
|
c = _make_choreographer_for_gate()
|
|
record_spy = _stub_gate_path(
|
|
c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after
|
|
)
|
|
cc: Any = c
|
|
cc._project_slug_for = AsyncMock(return_value="proj-slug")
|
|
c.git.get_pr_head_sha = AsyncMock(return_value=SHA_OLD)
|
|
|
|
await c.pr_fail(reviewer_id, task_id, ["duplicate TimeseriesChart export"])
|
|
|
|
record_spy.assert_called_once()
|
|
kwargs = record_spy.call_args.kwargs
|
|
assert kwargs["head_sha"] == SHA_OLD
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_pr_fail_capture_best_effort_when_git_raises() -> None:
|
|
"""A git head-sha lookup that raises must not crash the gate — head_sha
|
|
falls back to None (the submit_root gate then fails open) and the
|
|
transition still proceeds to needs_revision."""
|
|
reviewer_id = uuid4()
|
|
pm_id = uuid4()
|
|
task_id = uuid4()
|
|
t_before = MagicMock(
|
|
id=task_id,
|
|
assigned_to=reviewer_id,
|
|
pr_number=139,
|
|
parent_task_id=uuid4(),
|
|
status="awaiting_pr_review",
|
|
)
|
|
t_after = MagicMock(
|
|
id=task_id, assigned_to=pm_id, pr_number=139, status="needs_revision"
|
|
)
|
|
|
|
c = _make_choreographer_for_gate()
|
|
record_spy = _stub_gate_path(
|
|
c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after
|
|
)
|
|
cc: Any = c
|
|
cc._project_slug_for = AsyncMock(return_value="proj-slug")
|
|
c.git.get_pr_head_sha = AsyncMock(side_effect=RuntimeError("github 503"))
|
|
|
|
env = await c.pr_fail(reviewer_id, task_id, ["a concrete issue"])
|
|
|
|
assert env.status == "needs_revision"
|
|
record_spy.assert_called_once()
|
|
assert record_spy.call_args.kwargs["head_sha"] is None
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_pr_pass_does_not_capture_head_sha() -> None:
|
|
"""Only pr_fail stamps a head SHA — pr_pass must not (there is no loop to
|
|
guard against a pass)."""
|
|
reviewer_id = uuid4()
|
|
pm_id = uuid4()
|
|
task_id = uuid4()
|
|
t_before = MagicMock(
|
|
id=task_id,
|
|
assigned_to=reviewer_id,
|
|
pr_number=42,
|
|
parent_task_id=uuid4(),
|
|
status="awaiting_pr_review",
|
|
)
|
|
t_after = MagicMock(
|
|
id=task_id, assigned_to=pm_id, pr_number=42, status="awaiting_pm_review"
|
|
)
|
|
|
|
c = _make_choreographer_for_gate()
|
|
record_spy = _stub_gate_path(
|
|
c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after
|
|
)
|
|
cc: Any = c
|
|
cc._project_slug_for = AsyncMock(return_value="proj-slug")
|
|
c.git.get_pr_head_sha = AsyncMock(return_value=SHA_OLD)
|
|
|
|
await c.pr_pass(reviewer_id, task_id, "Assembled root scope is clean.")
|
|
|
|
record_spy.assert_called_once()
|
|
# pr_pass path never calls _capture_pr_head_sha, so head_sha is absent
|
|
# from the kwargs (the default None is not passed).
|
|
assert "head_sha" not in record_spy.call_args.kwargs
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# F016 — submit_root must not 500 when submit_for_review returns None
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_submit_root_invalid_state_when_submit_for_review_returns_none() -> None:
|
|
"""F016: submit_for_review returns None when the root->master PR was already
|
|
opened (the task raced out of in_progress, or a prior call already
|
|
transitioned it). create_root_pr already ran as the pre-side-effect, so the
|
|
PR exists, but the transition did not happen. submit_root must surface an
|
|
invalid_state envelope, not dereference None.status and 500."""
|
|
c, main_pm_id, root_task_id = _resubmit_root(notes_structured=None)
|
|
# The transition did not happen (PR already opened / task raced).
|
|
c.task.submit_for_review.return_value = None
|
|
|
|
env = await c.submit_root(
|
|
main_pm_id, root_task_id, notes="re-submit; transition returned nothing"
|
|
)
|
|
|
|
assert env.error is not None, env.as_dict()
|
|
assert env.error == "invalid_state"
|
|
remediate = env.remediate or ""
|
|
assert "evidence" in remediate.lower() or "re-fetch" in remediate.lower()
|