mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(pr-review): fleet PRs are ours by branch ownership, not author identity (#668)
With a GitHub App bound, fleet PRs are authored by <app-slug>[bot] whose author_association is NONE — the inbound classifier's author heuristics read that as an outsider and ingested the org's own dev-stream PR as external_pr for adversarial review (2026-07-23 live: PR #667). The repo-owner author check only ever covered the PAT era. _ingest_pr_if_reviewable now skips any same-repo PR whose head branch an active task owns BEFORE the author-based classification, and active_task_owns_branch widens from the single polled project to every project sharing its git_url (the poll collapses a monorepo's cell-projects to one canonical project, so a sibling cell's ownership must count — the same sibling scope external_review_task_exists already uses, now shared via _repo_sibling_project_ids). A deleted-fork head (GitHub sends head.repo=null) now classifies as fork, failing closed to review instead of risking a silent ownership skip on a branch-name collision. Residual, documented: an org PR whose task went terminal with the PR left open falls through to the author heuristics. Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -9508,9 +9508,11 @@ Start by:
|
|||||||
Repo-aware: collapses active projects to one canonical project per
|
Repo-aware: collapses active projects to one canonical project per
|
||||||
distinct repo (so a monorepo product yields ONE review per PR, not one
|
distinct repo (so a monorepo product yields ONE review per PR, not one
|
||||||
per cell-project), lists each repo's open PRs, and ingests a de-duped
|
per cell-project), lists each repo's open PRs, and ingests a de-duped
|
||||||
review task for each reviewable one — external/fork PRs, and (when
|
review task for each reviewable one. A same-repo PR whose head branch
|
||||||
internal review is on) org-repo PRs not tied to an active task. Commits
|
an active task owns (repo-wide, sibling cell-projects included) is the
|
||||||
once at the end.
|
org's own and never ingested, regardless of author — the rest split
|
||||||
|
into external/fork PRs and (when internal review is on) org-repo PRs
|
||||||
|
opened outside the task flow. Commits once at the end.
|
||||||
"""
|
"""
|
||||||
from roboco.services.git import GitService
|
from roboco.services.git import GitService
|
||||||
from roboco.services.project import get_project_service
|
from roboco.services.project import get_project_service
|
||||||
@@ -9554,6 +9556,20 @@ Start by:
|
|||||||
# 422), and re-reviewing the org's own in-flight PRs every poll is noise.
|
# 422), and re-reviewing the org's own in-flight PRs every poll is noise.
|
||||||
if pr.get("author_is_owner"):
|
if pr.get("author_is_owner"):
|
||||||
return False
|
return False
|
||||||
|
# The org's own in-flight PRs are recognized by BRANCH OWNERSHIP, not
|
||||||
|
# author identity: with a GitHub App bound, fleet PRs are authored by
|
||||||
|
# <app-slug>[bot] whose author_association is NONE, which the external
|
||||||
|
# heuristic below reads as an outsider (2026-07-23 live incident: a
|
||||||
|
# same-repo dev-stream PR was ingested as external_pr and adversarially
|
||||||
|
# reviewed). A same-repo head branch owned by an active task is ours
|
||||||
|
# regardless of who authored the PR, so this check must run BEFORE the
|
||||||
|
# author-based classification. Residual: an org PR whose task went
|
||||||
|
# terminal with the PR left open falls through to the author heuristics.
|
||||||
|
if not pr.get("is_fork") and await task_service.active_task_owns_branch(
|
||||||
|
str(pr.get("head_ref") or ""),
|
||||||
|
cast("UUID", project.id),
|
||||||
|
):
|
||||||
|
return False
|
||||||
if self._is_external_pr(pr):
|
if self._is_external_pr(pr):
|
||||||
if not settings.external_pr_enabled or not self._pr_author_allowed(
|
if not settings.external_pr_enabled or not self._pr_author_allowed(
|
||||||
pr, allowlist
|
pr, allowlist
|
||||||
@@ -9563,11 +9579,6 @@ Start by:
|
|||||||
else:
|
else:
|
||||||
if not settings.internal_pr_enabled:
|
if not settings.internal_pr_enabled:
|
||||||
return False
|
return False
|
||||||
if await task_service.active_task_owns_branch(
|
|
||||||
str(pr.get("head_ref") or ""),
|
|
||||||
cast("UUID", project.id),
|
|
||||||
):
|
|
||||||
return False
|
|
||||||
source = "internal_pr"
|
source = "internal_pr"
|
||||||
created = await task_service.ingest_external_pr(
|
created = await task_service.ingest_external_pr(
|
||||||
project_id=cast("UUID", project.id),
|
project_id=cast("UUID", project.id),
|
||||||
|
|||||||
@@ -2316,7 +2316,11 @@ class GitService(BaseService):
|
|||||||
"title": pr.get("title") or "",
|
"title": pr.get("title") or "",
|
||||||
"head_ref": head.get("ref"),
|
"head_ref": head.get("ref"),
|
||||||
"head_sha": head.get("sha"),
|
"head_sha": head.get("sha"),
|
||||||
"is_fork": bool(head_full and head_full != base_full),
|
# A null head repo (GitHub sends head.repo=null when the fork was
|
||||||
|
# deleted) is NOT ours — fail closed to fork/external so the
|
||||||
|
# branch-ownership skip can never silently swallow a genuine fork
|
||||||
|
# whose head_ref collides with an org branch name.
|
||||||
|
"is_fork": (head_full != base_full) if head_full else True,
|
||||||
"user_login": login,
|
"user_login": login,
|
||||||
# The reviewer reviews PRs the org did NOT author. A PR opened by the
|
# The reviewer reviews PRs the org did NOT author. A PR opened by the
|
||||||
# repo-owner account is a self-review (GitHub 422s REQUEST_CHANGES on
|
# repo-owner account is a self-review (GitHub 422s REQUEST_CHANGES on
|
||||||
|
|||||||
+39
-30
@@ -1407,27 +1407,7 @@ class TaskService(BaseService):
|
|||||||
- a legacy/markerless task exists, or ``head_sha`` is unknown -> True;
|
- a legacy/markerless task exists, or ``head_sha`` is unknown -> True;
|
||||||
- tasks exist but all cover OTHER SHAs -> False (new commits — re-review).
|
- tasks exist but all cover OTHER SHAs -> False (new commits — re-review).
|
||||||
"""
|
"""
|
||||||
# Resolve every project sharing this project's repo (git_url) so the
|
scope_ids = await self._repo_sibling_project_ids(project_id)
|
||||||
# dedupe spans the whole monorepo, not just the one project. An unknown
|
|
||||||
# project_id falls back to itself (can't widen).
|
|
||||||
git_url = (
|
|
||||||
await self.session.execute(
|
|
||||||
select(ProjectTable.git_url).where(ProjectTable.id == project_id)
|
|
||||||
)
|
|
||||||
).scalar_one_or_none()
|
|
||||||
if git_url:
|
|
||||||
sibling_ids = (
|
|
||||||
(
|
|
||||||
await self.session.execute(
|
|
||||||
select(ProjectTable.id).where(ProjectTable.git_url == git_url)
|
|
||||||
)
|
|
||||||
)
|
|
||||||
.scalars()
|
|
||||||
.all()
|
|
||||||
)
|
|
||||||
scope_ids = list(sibling_ids) or [project_id]
|
|
||||||
else:
|
|
||||||
scope_ids = [project_id]
|
|
||||||
result = await self.session.execute(
|
result = await self.session.execute(
|
||||||
select(TaskTable.orchestration_markers).where(
|
select(TaskTable.orchestration_markers).where(
|
||||||
TaskTable.project_id.in_(scope_ids),
|
TaskTable.project_id.in_(scope_ids),
|
||||||
@@ -1514,26 +1494,55 @@ class TaskService(BaseService):
|
|||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
return task
|
return task
|
||||||
|
|
||||||
async def active_task_owns_branch(self, branch_name: str, project_id: UUID) -> bool:
|
async def _repo_sibling_project_ids(self, project_id: UUID) -> list[UUID]:
|
||||||
"""True if a non-terminal task on ``project_id`` already owns this branch.
|
"""Every project id sharing ``project_id``'s repo (``git_url``).
|
||||||
|
|
||||||
Lets the internal-PR reviewer skip the org's own in-flight integration
|
The inbound-PR poll collapses a monorepo's cell-projects to one
|
||||||
|
canonical project per repo, so any repo-level lookup keyed by the
|
||||||
|
canonical id must widen back out to the siblings — a branch/review
|
||||||
|
owned by a sibling cell-project is still "this repo's". Falls back to
|
||||||
|
``[project_id]`` when the project is unknown or has no git_url.
|
||||||
|
"""
|
||||||
|
git_url = (
|
||||||
|
await self.session.execute(
|
||||||
|
select(ProjectTable.git_url).where(ProjectTable.id == project_id)
|
||||||
|
)
|
||||||
|
).scalar_one_or_none()
|
||||||
|
if not git_url:
|
||||||
|
return [project_id]
|
||||||
|
sibling_ids = (
|
||||||
|
(
|
||||||
|
await self.session.execute(
|
||||||
|
select(ProjectTable.id).where(ProjectTable.git_url == git_url)
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.scalars()
|
||||||
|
.all()
|
||||||
|
)
|
||||||
|
return list(sibling_ids) or [project_id]
|
||||||
|
|
||||||
|
async def active_task_owns_branch(self, branch_name: str, project_id: UUID) -> bool:
|
||||||
|
"""True if a non-terminal task on this REPO already owns this branch.
|
||||||
|
|
||||||
|
Lets the inbound-PR reviewer skip the org's own in-flight integration
|
||||||
PRs — those whose head branch a live task created via the agent
|
PRs — those whose head branch a live task created via the agent
|
||||||
task-flow (and which therefore already pass QA + PM review) — and review
|
task-flow (and which therefore already pass QA + PM review) — and review
|
||||||
only org-repo PRs opened outside that flow.
|
only org-repo PRs opened outside that flow.
|
||||||
|
|
||||||
Scoped to the polled project: a branch in project A's repo can only be
|
Scoped to the REPO (every project sharing ``project_id``'s git_url),
|
||||||
owned by a task whose ``project_id == A`` (each task branches in its
|
not the single polled project: the poll collapses a monorepo's
|
||||||
own project's repo, including each root-subtask of a multi-repo
|
cell-projects to one canonical project, so a fleet PR whose owning
|
||||||
MegaTask). An unscoped lookup would match the wrong project's task on a
|
task lives on a sibling cell-project must still count as ours.
|
||||||
cross-project branch_name collision and false-skip project A's PR.
|
Cross-REPO branch-name collisions stay excluded — a different repo's
|
||||||
|
task can never own this repo's branch.
|
||||||
"""
|
"""
|
||||||
if not branch_name:
|
if not branch_name:
|
||||||
return False
|
return False
|
||||||
|
scope_ids = await self._repo_sibling_project_ids(project_id)
|
||||||
result = await self.session.execute(
|
result = await self.session.execute(
|
||||||
select(TaskTable.id).where(
|
select(TaskTable.id).where(
|
||||||
TaskTable.branch_name == branch_name,
|
TaskTable.branch_name == branch_name,
|
||||||
TaskTable.project_id == project_id,
|
TaskTable.project_id.in_(scope_ids),
|
||||||
TaskTable.status.notin_([TaskStatus.COMPLETED, TaskStatus.CANCELLED]),
|
TaskTable.status.notin_([TaskStatus.COMPLETED, TaskStatus.CANCELLED]),
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,17 +1,16 @@
|
|||||||
"""active_task_owns_branch must be scoped to the polled project.
|
"""active_task_owns_branch must be scoped to the polled project's REPO.
|
||||||
|
|
||||||
The internal-PR reviewer (orchestrator) calls this to skip the org's own
|
The inbound-PR reviewer (orchestrator) calls this to skip the org's own
|
||||||
in-flight integration PRs — a PR on project A's repo is "ours" only if a
|
in-flight integration PRs. Scope is the repo (every project sharing the
|
||||||
non-terminal task ON PROJECT A owns its head branch. The query was unscoped
|
polled project's ``git_url``), not one project: the poll collapses a
|
||||||
(``WHERE branch_name = ?``), so a cross-project branch_name collision (two
|
monorepo's cell-projects to one canonical project, so a fleet PR whose
|
||||||
tasks sharing an 8-char-UUID-prefix branch on different projects) made it
|
owning task lives on a sibling cell-project must still count as ours
|
||||||
match the WRONG project's task — project A's leftover PR was skipped because
|
(2026-07-23: a GitHub-App-authored dev-stream PR was ingested as external_pr
|
||||||
project B happened to have an active task with the same branch_name.
|
because the canonical project's id didn't match the owning cell's).
|
||||||
|
|
||||||
Scoping by ``project_id`` is correct for both single-project tasks and
|
Cross-REPO branch_name collisions stay excluded — the original bug this file
|
||||||
MegaTask multi-repo batches: each root-subtask carries its own ``project_id``
|
pinned: an unscoped query (``WHERE branch_name = ?``) matched the WRONG
|
||||||
matching its own repo, so a branch on project A's repo is owned only by a
|
repo's task on an 8-char-UUID-prefix collision and false-skipped a real PR.
|
||||||
task whose ``project_id == A``.
|
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
@@ -40,7 +39,9 @@ SYSTEM_UUID = _foundation.AGENTS["system"].uuid
|
|||||||
_BRANCH = "feature/backend/collide001"
|
_BRANCH = "feature/backend/collide001"
|
||||||
|
|
||||||
|
|
||||||
async def _seed_project(db: AsyncSession, slug: str) -> ProjectTable:
|
async def _seed_project(
|
||||||
|
db: AsyncSession, slug: str, git_url: str | None = None
|
||||||
|
) -> ProjectTable:
|
||||||
if await db.get(AgentTable, SYSTEM_UUID) is None:
|
if await db.get(AgentTable, SYSTEM_UUID) is None:
|
||||||
db.add(
|
db.add(
|
||||||
AgentTable(
|
AgentTable(
|
||||||
@@ -62,7 +63,7 @@ async def _seed_project(db: AsyncSession, slug: str) -> ProjectTable:
|
|||||||
id=uuid4(),
|
id=uuid4(),
|
||||||
name=slug,
|
name=slug,
|
||||||
slug=slug,
|
slug=slug,
|
||||||
git_url=f"https://github.com/rennf93/{slug}",
|
git_url=git_url or f"https://github.com/rennf93/{slug}",
|
||||||
assigned_cell=Team.BACKEND,
|
assigned_cell=Team.BACKEND,
|
||||||
created_by=SYSTEM_UUID,
|
created_by=SYSTEM_UUID,
|
||||||
)
|
)
|
||||||
@@ -118,6 +119,27 @@ async def test_branch_owned_only_by_its_own_project(db_session: AsyncSession) ->
|
|||||||
assert await svc.active_task_owns_branch(_BRANCH, cast("UUID", proj_b.id)) is False
|
assert await svc.active_task_owns_branch(_BRANCH, cast("UUID", proj_b.id)) is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_monorepo_sibling_project_ownership_counts(
|
||||||
|
db_session: AsyncSession,
|
||||||
|
) -> None:
|
||||||
|
"""Two cell-projects share one git_url (monorepo). The active task lives
|
||||||
|
on the FRONTEND cell; the poll queries with the CANONICAL sibling's id.
|
||||||
|
Repo-scoping must still recognize the branch as ours — the 2026-07-23
|
||||||
|
incident shape."""
|
||||||
|
repo = "https://github.com/rennf93/gca-mono"
|
||||||
|
canonical = await _seed_project(db_session, "gca-mono-api", git_url=repo)
|
||||||
|
frontend = await _seed_project(db_session, "gca-mono-panel", git_url=repo)
|
||||||
|
branch = "feature/frontend/mono0001--child001"
|
||||||
|
db_session.add(
|
||||||
|
_task(cast("UUID", frontend.id), branch=branch, status=TaskStatus.IN_PROGRESS)
|
||||||
|
)
|
||||||
|
await db_session.flush()
|
||||||
|
svc = get_task_service(db_session)
|
||||||
|
|
||||||
|
assert await svc.active_task_owns_branch(branch, cast("UUID", canonical.id)) is True
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_empty_branch_never_owned(db_session: AsyncSession) -> None:
|
async def test_empty_branch_never_owned(db_session: AsyncSession) -> None:
|
||||||
proj = await _seed_project(db_session, "gca-collide-empty")
|
proj = await _seed_project(db_session, "gca-collide-empty")
|
||||||
|
|||||||
@@ -198,6 +198,78 @@ async def test_skip_internal_when_disabled(monkeypatch: pytest.MonkeyPatch) -> N
|
|||||||
svc.ingest_external_pr.assert_not_awaited()
|
svc.ingest_external_pr.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_skip_app_bot_authored_fleet_pr(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
|
"""The 2026-07-23 live incident: with a GitHub App bound, fleet PRs are
|
||||||
|
authored by <app-slug>[bot] whose author_association is NONE — the external
|
||||||
|
heuristic reads that as an outsider. Branch ownership must win: a same-repo
|
||||||
|
PR whose head an active task owns is the org's own, whoever authored it."""
|
||||||
|
monkeypatch.setattr(orch_mod.settings, "external_pr_enabled", True)
|
||||||
|
monkeypatch.setattr(orch_mod.settings, "internal_pr_enabled", True)
|
||||||
|
svc = _svc(owns_branch=True)
|
||||||
|
pr = {
|
||||||
|
"number": 667,
|
||||||
|
"is_fork": False,
|
||||||
|
"author_is_owner": False,
|
||||||
|
"user_login": "roboco-app[bot]",
|
||||||
|
"author_association": "NONE",
|
||||||
|
"head_ref": "feature/frontend/170c9578--f1957610",
|
||||||
|
}
|
||||||
|
ok = await _orch()._ingest_pr_if_reviewable(
|
||||||
|
svc, SimpleNamespace(id=uuid4()), pr, uuid4(), set()
|
||||||
|
)
|
||||||
|
assert ok is False
|
||||||
|
svc.ingest_external_pr.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_app_bot_pr_without_owning_task_still_reviews(
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""The documented residual: a same-repo bot-authored PR with NO active
|
||||||
|
owning task falls through to the author heuristics and reviews as
|
||||||
|
external — orphaned/unknown bot branches (dependabot included) keep
|
||||||
|
getting the read-only adversarial review."""
|
||||||
|
monkeypatch.setattr(orch_mod.settings, "external_pr_enabled", True)
|
||||||
|
monkeypatch.setattr(orch_mod.settings, "internal_pr_enabled", True)
|
||||||
|
svc = _svc(owns_branch=False)
|
||||||
|
pr = {
|
||||||
|
"number": 42,
|
||||||
|
"is_fork": False,
|
||||||
|
"author_is_owner": False,
|
||||||
|
"user_login": "dependabot[bot]",
|
||||||
|
"author_association": "NONE",
|
||||||
|
"head_ref": "dependabot/npm_and_yarn/foo-1.2.3",
|
||||||
|
}
|
||||||
|
ok = await _orch()._ingest_pr_if_reviewable(
|
||||||
|
svc, SimpleNamespace(id=uuid4()), pr, uuid4(), set()
|
||||||
|
)
|
||||||
|
assert ok is True
|
||||||
|
assert svc.ingest_external_pr.await_args.kwargs["source"] == "external_pr"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_fork_pr_never_consults_branch_ownership(
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""A fork PR is external by definition — the ownership pre-check must not
|
||||||
|
run for it (a fork head ref can coincide with an org branch name)."""
|
||||||
|
monkeypatch.setattr(orch_mod.settings, "external_pr_enabled", True)
|
||||||
|
svc = _svc(owns_branch=True)
|
||||||
|
pr = {
|
||||||
|
"number": 7,
|
||||||
|
"is_fork": True,
|
||||||
|
"user_login": "outsider",
|
||||||
|
"head_ref": "feature/frontend/copycat",
|
||||||
|
}
|
||||||
|
ok = await _orch()._ingest_pr_if_reviewable(
|
||||||
|
svc, SimpleNamespace(id=uuid4()), pr, uuid4(), set()
|
||||||
|
)
|
||||||
|
assert ok is True
|
||||||
|
svc.active_task_owns_branch.assert_not_awaited()
|
||||||
|
assert svc.ingest_external_pr.await_args.kwargs["source"] == "external_pr"
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_skip_owner_authored_pr(monkeypatch: pytest.MonkeyPatch) -> None:
|
async def test_skip_owner_authored_pr(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
# The org's own account opened the PR → self-review, never ingest (even with
|
# The org's own account opened the PR → self-review, never ingest (even with
|
||||||
|
|||||||
@@ -104,6 +104,22 @@ async def test_list_open_prs_normalizes_and_flags_fork() -> None:
|
|||||||
assert internal["author_is_owner"] is False
|
assert internal["author_is_owner"] is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_normalize_deleted_fork_head_is_fork() -> None:
|
||||||
|
"""GitHub sends head.repo=null when a fork was deleted — that head is NOT
|
||||||
|
ours, so it must classify as a fork (fail-closed to review) rather than
|
||||||
|
fall into the same-repo path where a branch-name collision could skip it."""
|
||||||
|
pr = {
|
||||||
|
"number": 11,
|
||||||
|
"html_url": "https://github.com/acme/repo/pull/11",
|
||||||
|
"title": "ghost fork",
|
||||||
|
"head": {"ref": "feature-x", "sha": "cafebabe", "repo": None},
|
||||||
|
"user": {"login": "ghost"},
|
||||||
|
"author_association": "NONE",
|
||||||
|
}
|
||||||
|
out = GitService._normalize_open_pr(pr, "acme/repo")
|
||||||
|
assert out["is_fork"] is True
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_list_open_prs_flags_owner_authored_pr() -> None:
|
async def test_list_open_prs_flags_owner_authored_pr() -> None:
|
||||||
"""A PR opened by the repo-owner account is flagged author_is_owner."""
|
"""A PR opened by the repo-owner account is flagged author_is_owner."""
|
||||||
|
|||||||
Reference in New Issue
Block a user