External/roboco
1
0
Fork 0
mirror of https://github.com/rennf93/roboco.git synced 2026-08-03 07:23:24 +02:00
Code Issues Packages Projects Releases Wiki Activity

[fix] submit_root: hard unchanged-PR gate stops the pr_fail re-submit loop

Browse Source 
The 2026-06-27 infinite pr_fail loop: a Main-PM root (PR #139) was pr_fail'd,
routed to needs_revision, and re-submitted byte-identical → awaiting_pr_review
→ pr_fail again, forever. The prior hint/a2a steer was ignored by the weak
coordinator model — hints don't stop a model that won't read them. A HARD gate
refuses the re-submit when the assembled root PR's head SHA is unchanged since
the last pr_fail (no new cell work → identical diff); a different SHA ⇒ the
branch advanced ⇒ allow. Every ambiguous case fails open (no prior fail, no
recorded SHA, no pr_number, unresolvable slug, git error, closed PR) — only the
exact-unchanged case is hard-blocked.

- content/models: PrReviewContent.head_sha (optional; JSON col → no migration).
- git: get_pr_head_sha (GitHub pulls API; None on any failure → fail-open).
- pr_gate: pr_fail captures head_sha into the verdict record; pr_pass does not.
- _impl: submit_root runs _submit_root_unchanged_pr_guard after _submit_up_guard;
  _current_root_pr_head_sha resolves slug + current SHA (fail-open).
- pr_review: extract module-level resolve_task_project_slug, shared by the mixin
  and the gate helper (_LegacyChoreographer reaches it via cast to the
  ChoreographerHelpers typed view — it doesn't inherit the helpers mixin).
- tests: test_submit_root_unchanged_pr_guard (11 — refuse/allow/6 fail-open/3
  capture-side, mypy-clean via cc:Any spy idiom, zero type:ignore) +
  test_pr_gate_notifies_pm capture-path stub.
This commit is contained in:
Renn F
2026-06-28 02:02:50 +02:00
parent 676a87985f
commit e52fd05d59
7 changed files with 682 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
roboco/services/gateway/choreographer/_impl.py
+95 -1
View File
@@ -14,7 +14,7 @@ from __future__ import annotations
import contextlib
from dataclasses import dataclass
from datetime import UTC, datetime
from typing import Any, ClassVar, cast
from typing import TYPE_CHECKING, Any, ClassVar, cast
from uuid import UUID
import structlog
@@ -56,6 +56,13 @@ from roboco.services.gateway.remediation import (
logger = structlog.get_logger()
if TYPE_CHECKING:
# The composed ``Choreographer`` resolves mixin helpers (``_project_slug_for``,
# etc.) via MRO, but ``_LegacyChoreographer`` itself does not inherit
# ``ChoreographerHelpers`` — so mypy can't see those names on ``self`` here.
# The cast below reaches the typed view the mixins use (``_Base`` pattern).
from roboco.services.gateway.choreographer._protocol import ChoreographerHelpers
# Minimum character length enforced on rich_plan["approach"] by the PM
# sub-tasks gate. Must match the Pydantic min_length on
# IWillPlanRequest.approach. Raised 20→150: plans were vague
@@ -5671,6 +5678,78 @@ class Choreographer:
)
return None
async def _submit_root_unchanged_pr_guard(
self, t: Any, briefing: dict[str, Any]
) -> Envelope | None:
"""Refuse to re-submit a root PR whose diff is unchanged since the last fail.
The hard loop-stopper for the 2026-06-27 ``pr_fail`` re-submit loop. A
prior ``pr_fail`` stamped the assembled PR's head SHA into
``notes_structured.pr_review.head_sha`` (see ``_capture_pr_head_sha`` /
``_record_gate_verdict``). On the next ``submit_root`` this looks up the
PR's CURRENT head SHA and compares. Equal ⇒ no new cell work landed on
the root branch since the fail ⇒ the diff the reviewer would see is
byte-identical to the one just rejected ⇒ refuse, so no model can loop
itself back into ``awaiting_pr_review``. Different ⇒ the branch advanced
⇒ allow. Returns ``None`` (proceed) on every ambiguous case so the gate
FAILS OPEN: no prior ``pr_fail`` verdict, no recorded ``head_sha`` (e.g.
a verdict written before this field existed), no ``pr_number``, no
resolvable project, or a git/closed-PR lookup that returns ``None``.
Only the exact-unchanged case is hard-blocked; the rest fall through to
the reviewer, who can still ``pr_fail`` if the diff is bad.
"""
pr_review = (getattr(t, "notes_structured", None) or {}).get("pr_review") or {}
if pr_review.get("verdict") != "failed":
return None
recorded = pr_review.get("head_sha")
if not recorded:
return None
current = await self._current_root_pr_head_sha(t)
if current is None or current != recorded:
return None
return Envelope.invalid_state(
message=(
"the assembled root PR is unchanged since the last pr_fail"
f" (head {current[:7]}). No new cell work has landed on the"
" root branch, so re-submitting would re-open the exact diff"
" the reviewer just rejected and loop straight back to"
" awaiting_pr_review."
),
remediate=(
"re-delegate the fixes to the owning cell PM(s) via"
" delegate(...) and wait for the cell subtasks to complete"
" and the root branch to be re-assembled. Do NOT call"
" submit_root again until new cell work has advanced the"
" root branch HEAD."
),
context_briefing=briefing,
)
async def _current_root_pr_head_sha(self, t: Any) -> str | None:
"""Best-effort current head SHA of the task's assembled PR (fail-open).
The lookup the unchanged-PR gate compares against. Returns ``None`` on
every ambiguous case (no ``pr_number``, no resolvable project slug, a
git error, or a closed/missing PR) so the gate fails open rather than
wedging the PM — only the exact-unchanged case is hard-blocked.
"""
pr_number = getattr(t, "pr_number", None)
if not pr_number:
return None
try:
# ``_LegacyChoreographer`` doesn't inherit ``ChoreographerHelpers``,
# so cast to the typed view the mixins use to reach the shared
# ``_project_slug_for`` resolver (it delegates to the module-level
# ``resolve_task_project_slug``). Same resolver the pr_fail capture
# path uses, so one stub controls both gate paths.
slug = await cast("ChoreographerHelpers", self)._project_slug_for(t)
if not slug:
return None
sha = await self.git.get_pr_head_sha(slug, int(pr_number))
return sha if isinstance(sha, str) else None
except Exception:
return None
async def submit_root(
self, main_pm_agent_id: UUID, task_id: UUID, notes: str
) -> Envelope:
@@ -5725,6 +5804,21 @@ class Choreographer:
# journal:decision + subtasks-terminal + branch-present. submit_root's
# tracing requirements mirror submit_up's, so the shared guard applies.
guard = await self._submit_up_guard(main_pm_agent_id, task_id, t, notes)
# Hard unchanged-PR gate (the 2026-06-27 pr_fail re-submit loop-stopper).
# A hint (the pr_fail a2a / next-hint steer "do NOT re-submit") is ignored
# by a weak coordinator (minimax-m3 re-submitted PR #139 byte-identical),
# so this refuses the re-submit STRUCTURALLY: if the last pr_fail stamped
# the assembled PR's head SHA and the current PR head SHA is the same, no
# new cell work has landed on the root branch since the fail — re-running
# submit_root would re-open / re-push the exact diff the reviewer just
# rejected, looping straight back to awaiting_pr_review → pr_fail. Force
# the PM to re-delegate the fixes and wait for re-assembly instead.
# Fail-open on ANY ambiguity (no prior fail, no recorded sha, no
# resolvable project, git error, closed/missing PR) — only the
# exact-unchanged case is hard-blocked; everything else proceeds and
# relies on the reviewer to re-fail if the diff is still bad.
if guard is None:
guard = await self._submit_root_unchanged_pr_guard(t, briefing)
if guard is not None:
guard.with_introspection(task=t, role=role_str)
return await self._emit_rejection(
roboco/services/gateway/choreographer/pr_gate.py
+60 -3
View File
@@ -244,8 +244,17 @@ class PRGateMixin(_Base):
# it is persisted by the same commit (mirrors post_pr_review). This is
# what keeps notes_structured.pr_review in lock-step with the decision —
# a later pr_fail overwrites an earlier pr_pass verdict instead of
# leaving a stale "passed" on a task that was just sent back.
self._record_gate_verdict(t, verb, notes, issues=issues)
# leaving a stale "passed" on a task that was just sent back. On pr_fail
# also stamp the assembled PR's head SHA so the next submit_root can
# structurally refuse to re-submit the unchanged root (the 2026-06-27
# infinite pr_fail re-submit loop). Best-effort: a capture failure
# (no token, no PR, GitHub error) leaves head_sha absent and the
# submit_root gate fails open rather than wedging the PM.
if verb == "pr_fail":
head_sha = await self._capture_pr_head_sha(t)
self._record_gate_verdict(t, verb, notes, issues=issues, head_sha=head_sha)
else:
self._record_gate_verdict(t, verb, notes, issues=issues)
runner = self._verb_runner()
try:
t = await runner.run_intent(verb, t, agent, spec_ctx)
@@ -346,7 +355,13 @@ class PRGateMixin(_Base):
return None
def _record_gate_verdict(
self, t: Any, verb: str, notes: str, issues: tuple[str, ...] = ()
self,
t: Any,
verb: str,
notes: str,
issues: tuple[str, ...] = (),
*,
head_sha: str | None = None,
) -> None:
"""Persist the gate verdict as the canonical ``pr_review`` note.
@@ -361,6 +376,12 @@ class PRGateMixin(_Base):
PM's briefing that mirrors it — gets the concrete change-requests.
Best-effort: content validation (e.g. a too-short summary) must never
roll back the gate, so a malformed payload is logged and skipped.
On ``pr_fail`` the assembled PR's head SHA is stamped into the slot
(``head_sha``) so the next ``submit_root`` can structurally refuse to
re-submit the unchanged root — the 2026-06-27 infinite ``pr_fail``
re-submit loop. ``None`` (the default) leaves it absent, which the
``submit_root`` gate treats as fail-open.
"""
from roboco.foundation.policy.content import ContentValidationError
from roboco.services.content_notes import apply_structured_note
@@ -387,6 +408,8 @@ class PRGateMixin(_Base):
}
if issues:
payload["issues"] = list(issues)
if verb == "pr_fail" and head_sha:
payload["head_sha"] = head_sha
try:
apply_structured_note(t, "pr_review", payload)
except ContentValidationError:
@@ -396,6 +419,40 @@ class PRGateMixin(_Base):
task_id=str(getattr(t, "id", "")),
)
async def _capture_pr_head_sha(self, t: Any) -> str | None:
"""Best-effort capture of the assembled PR's head SHA at ``pr_fail`` time.
Resolves the project slug via ``_project_slug_for`` (handles a
Main-PM root that carries only a ``product_id``) and asks
``git.get_pr_head_sha``. Returns ``None`` on ANY failure (no PR number,
no resolvable project, git error) so the ``submit_root`` unchanged-PR
gate fails open rather than wedging the PM — only the exact-unchanged
case is hard-blocked.
"""
pr_number = getattr(t, "pr_number", None)
if not pr_number:
return None
try:
slug = await self._project_slug_for(t)
except Exception:
logger.exception(
"pr_fail head-sha capture: slug resolve failed",
task_id=str(getattr(t, "id", "")),
)
return None
if not slug:
return None
try:
sha = await self.git.get_pr_head_sha(slug, int(pr_number))
return sha if isinstance(sha, str) else None
except Exception:
logger.exception(
"pr_fail head-sha capture: git lookup failed",
task_id=str(getattr(t, "id", "")),
pr=pr_number,
)
return None
async def _post_gate_review_to_pr(
self, t: Any, verb: str, reviewer_slug: str, notes: str
) -> None:
roboco/services/gateway/choreographer/pr_review.py
+50 -38
View File
@@ -544,45 +544,57 @@ class PRReviewerMixin(_Base):
async def _project_slug_for(self, t: Any) -> str | None:
"""Resolve the project slug for a task (read-only PR API calls + the
in-path gate's PR comment).
A normal task carries ``project_id``. A Main-PM coordination root — the
only task a root→master PR ever sits on — often carries just a
``product_id`` (the cell→repo map) and no project of its own; its
``feature/main_pm/{root}`` branch + PR live in the product's repo. Fall
through to the product's first distinct project (a monorepo product maps
every cell to one repo) so the gate verdict reaches the PR instead of
silently no-op'ing. Purely additive: a task WITH ``project_id`` resolves
exactly as before.
in-path gate's PR comment). Delegates to the module-level resolver so
the unchanged-PR gate in ``_impl.py`` reuses the EXACT same path (it
can't see this mixin method — ``_LegacyChoreographer`` does not inherit
``ChoreographerHelpers``). See ``resolve_task_project_slug``.
"""
from uuid import UUID
return await resolve_task_project_slug(self.task.session, t)
from roboco.services.project import get_project_service
project_service = get_project_service(self.task.session)
if t.project_id is not None:
project = await project_service.get(t.project_id)
async def resolve_task_project_slug(session: Any, t: Any) -> str | None:
"""Resolve the project slug for a task (read-only PR API calls + the
in-path gate's PR comment). Module-level so it is shared by
``PRReviewerMixin._project_slug_for`` and the unchanged-PR gate's
``_current_root_pr_head_sha`` (in ``_impl.py``) — DRY, and the only way the
legacy choreographer class can reach the resolver without inheriting
``ChoreographerHelpers``.
A normal task carries ``project_id``. A Main-PM coordination root — the
only task a root→master PR ever sits on — often carries just a
``product_id`` (the cell→repo map) and no project of its own; its
``feature/main_pm/{root}`` branch + PR live in the product's repo. Fall
through to the product's first distinct project (a monorepo product maps
every cell to one repo) so the gate verdict reaches the PR instead of
silently no-op'ing. Purely additive: a task WITH ``project_id`` resolves
exactly as before.
"""
from uuid import UUID
from roboco.services.project import get_project_service
project_service = get_project_service(session)
if t.project_id is not None:
project = await project_service.get(t.project_id)
return project.slug if project is not None else None
product_id = getattr(t, "product_id", None)
if product_id is not None:
from roboco.services.product import get_product_service
product_service = get_product_service(session)
project_ids = await product_service.distinct_project_ids(UUID(str(product_id)))
if not project_ids:
return None
project = await project_service.get(project_ids[0])
return project.slug if project is not None else None
# Ad-hoc per-cell map root-subtask: mirror the product root's first-project
# resolution so the gate verdict reaches the PR in the mapped repo.
cell_map = getattr(t, "cell_projects", None) or []
seen: set[UUID] = set()
for mapping in sorted(cell_map, key=lambda m: m.team.value):
pid = UUID(str(mapping.project_id))
if pid not in seen:
seen.add(pid)
project = await project_service.get(pid)
return project.slug if project is not None else None
product_id = getattr(t, "product_id", None)
if product_id is not None:
from roboco.services.product import get_product_service
product_service = get_product_service(self.task.session)
project_ids = await product_service.distinct_project_ids(
UUID(str(product_id))
)
if not project_ids:
return None
project = await project_service.get(project_ids[0])
return project.slug if project is not None else None
# Ad-hoc per-cell map root-subtask: mirror the product root's first-project
# resolution so the gate verdict reaches the PR in the mapped repo.
cell_map = getattr(t, "cell_projects", None) or []
seen: set[UUID] = set()
for mapping in sorted(cell_map, key=lambda m: m.team.value):
pid = UUID(str(mapping.project_id))
if pid not in seen:
seen.add(pid)
project = await project_service.get(pid)
return project.slug if project is not None else None
return None
return None