mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
e97f46af6e
* fix(git): reviewer reads must prefer origin over a diverged local ref _resolve_head_ref (GitService.diff/list_changed_files/read_file_at_branch) kept local priority on ANY divergence from origin, real or rewritten. A reviewer's clone parked on pre-rebase history after the branch's routine force-push sync stayed frozen there across every subsequent review round, while origin held every fix commit — QA repeatedly bounced work that had already landed. Every caller here is a reader, never the branch's own author mid-write, so origin now wins whenever it carries anything the local ref lacks; local keeps priority only when it strictly contains origin (unpushed commits, or equal). The read-only git MCP surface (roboco_git_log) hit the same staleness through a separate path: /api/git/log resolved the requested branch as a bare name straight off whatever the caller's own clone had on disk, with no fetch at all. It now routes through the same fixed _resolve_head_ref. * test(e2e): give the armed flow-verb timeout real headroom The armed value is also verb-2's entire execution budget (claim + every claim guard + set_plan + start + tracing gate), which grows as guards land; 1s flaked on loaded CI runners while passing locally. The cancel-and-release semantics only need the timeout far below the hang. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
369 lines
15 KiB
Python
369 lines
15 KiB
Python
"""Task #161: GitService diff base falls back to default branch.
|
|
|
|
A leaf dev branch's parent (per parent_branch_for) is the cell-PM
|
|
branch feature/{team}/{root}--{cellpm}, which is NEVER pushed — only
|
|
devs push their own leaf branch. Diffing against a non-existent
|
|
origin/<parent> returns an empty diff, so QA / docs saw nothing.
|
|
_resolve_diff_base must fall back to the repo default branch when the
|
|
parent ref is absent on origin.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from pathlib import Path
|
|
from typing import Any
|
|
from unittest.mock import AsyncMock, patch
|
|
|
|
import pytest
|
|
from roboco.services.git import GitService
|
|
|
|
|
|
def _git_service() -> Any:
|
|
return GitService.__new__(GitService)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_diff_base_uses_parent_when_pushed() -> None:
|
|
"""When origin/<parent> exists, use it (normal case)."""
|
|
svc = _git_service()
|
|
svc._run_git = AsyncMock()
|
|
svc._ref_exists = AsyncMock(return_value=True)
|
|
ws = Path("/tmp/ws")
|
|
|
|
base = await svc._resolve_diff_base(
|
|
ws, "feature/backend/root1234--cellpm56--dev78901"
|
|
)
|
|
# parent_branch_for strips the last --segment.
|
|
assert base == "origin/feature/backend/root1234--cellpm56"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_diff_base_falls_back_when_parent_absent() -> None:
|
|
"""When origin/<parent> does NOT exist (cell-PM branch never pushed),
|
|
fall back to the repo default branch via origin/HEAD."""
|
|
svc = _git_service()
|
|
svc._run_git = AsyncMock()
|
|
# parent ref absent → _ref_exists False for the parent check.
|
|
svc._ref_exists = AsyncMock(return_value=False)
|
|
svc._default_branch_ref = AsyncMock(return_value="origin/master")
|
|
ws = Path("/tmp/ws")
|
|
|
|
base = await svc._resolve_diff_base(
|
|
ws, "feature/backend/root1234--cellpm56--dev78901"
|
|
)
|
|
assert base == "origin/master"
|
|
svc._default_branch_ref.assert_awaited_once()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_default_branch_ref_prefers_origin_head() -> None:
|
|
"""origin/HEAD symbolic-ref is the canonical default-branch pointer."""
|
|
svc = _git_service()
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
|
if args[:2] == ["symbolic-ref", "--quiet"]:
|
|
return type(
|
|
"R", (), {"returncode": 0, "stdout": "refs/remotes/origin/main\n"}
|
|
)()
|
|
return type("R", (), {"returncode": 1, "stdout": ""})()
|
|
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
ref = await svc._default_branch_ref(Path("/tmp/ws"))
|
|
assert ref == "origin/main"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_default_branch_ref_fallback_when_no_head() -> None:
|
|
"""No origin/HEAD → probe origin/master then origin/main; final
|
|
hard fallback is origin/master so the git invocation stays valid."""
|
|
svc = _git_service()
|
|
|
|
async def fake_run(_ws: Any, _args: list[str], **_kw: Any) -> Any:
|
|
# symbolic-ref fails; fetches succeed but ref never verifies.
|
|
return type("R", (), {"returncode": 1, "stdout": ""})()
|
|
|
|
svc._ref_exists = AsyncMock(return_value=False)
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
ref = await svc._default_branch_ref(Path("/tmp/ws"))
|
|
assert ref == "origin/master"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Task #161 (facet): the diff HEAD side must resolve in the inspecting
|
|
# clone. The local <branch> ref only exists in the dev's own clone (the
|
|
# clone that ran `git checkout -b` at claim). QA / doc / PM diff from
|
|
# their OWN clones, which only have origin/<branch> after a fetch. Diffing
|
|
# against the bare local name there yields an empty diff (smoke-14: QA saw
|
|
# no changes on a real PR). _resolve_head_ref + diff()/list_changed_files
|
|
# must prefer the local branch, then origin/<branch>.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_BR = "feature/backend/root1234--cellpm56--dev78901"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_head_ref_prefers_local_branch_in_dev_clone() -> None:
|
|
"""Dev's own clone has the local branch, ahead of/equal to origin
|
|
(origin has nothing local lacks) — use it unchanged."""
|
|
svc = _git_service()
|
|
svc._run_git = AsyncMock(
|
|
return_value=type("R", (), {"returncode": 0, "stdout": "0"})()
|
|
)
|
|
svc._ref_exists = AsyncMock(return_value=True)
|
|
|
|
head = await svc._resolve_head_ref(Path("/tmp/ws"), _BR)
|
|
assert head == _BR
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_head_ref_falls_back_to_origin_in_foreign_clone() -> None:
|
|
"""QA/doc/PM clone has no local branch but origin/<branch> exists
|
|
(open_pr pushed it) — diff must target origin/<branch>."""
|
|
svc = _git_service()
|
|
svc._run_git = AsyncMock()
|
|
|
|
async def ref_exists(_ws: Any, ref: str) -> bool:
|
|
# local branch absent; only the remote-tracking ref resolves.
|
|
return ref == f"origin/{_BR}"
|
|
|
|
with patch.object(svc, "_ref_exists", new=ref_exists):
|
|
head = await svc._resolve_head_ref(Path("/tmp/ws"), _BR)
|
|
assert head == f"origin/{_BR}"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_head_ref_fetches_branch_before_resolving() -> None:
|
|
"""The branch is fetched into the workspace so origin/<branch> is
|
|
available even on a clone that never saw it."""
|
|
svc = _git_service()
|
|
calls: list[list[str]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
|
calls.append(args)
|
|
return type("R", (), {"returncode": 0, "stdout": ""})()
|
|
|
|
svc._ref_exists = AsyncMock(return_value=True)
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
await svc._resolve_head_ref(Path("/tmp/ws"), _BR)
|
|
assert ["fetch", "origin", _BR] in calls
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_diff_targets_origin_head_in_foreign_clone() -> None:
|
|
"""Regression for smoke-14: diff() from QA's clone must compare
|
|
base...origin/<branch>, not base...<bare-local-branch> (which is
|
|
unresolvable there and silently produced an empty diff)."""
|
|
svc = _git_service()
|
|
svc._workspace_for_branch = AsyncMock(return_value=Path("/tmp/qa-ws"))
|
|
svc._resolve_diff_base = AsyncMock(return_value="origin/master")
|
|
svc._resolve_head_ref = AsyncMock(return_value=f"origin/{_BR}")
|
|
svc._token_for_branch = AsyncMock(return_value="tok")
|
|
captured: list[list[str]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
|
captured.append(args)
|
|
return type("R", (), {"returncode": 0, "stdout": "diff body"})()
|
|
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
out = await svc.diff(branch_name=_BR)
|
|
assert out == "diff body"
|
|
assert captured == [["diff", f"origin/master...origin/{_BR}"]]
|
|
# #168: the resolved project token is threaded into ref resolution so
|
|
# the fetches authenticate (unauth fails on private repos).
|
|
svc._resolve_head_ref.assert_awaited_once_with(Path("/tmp/qa-ws"), _BR, token="tok")
|
|
svc._resolve_diff_base.assert_awaited_once_with(
|
|
Path("/tmp/qa-ws"), _BR, token="tok", preferred_parent=None
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_list_changed_files_targets_origin_head_in_foreign_clone() -> None:
|
|
"""Same fix on the files_changed path (#154 evidence)."""
|
|
svc = _git_service()
|
|
svc._workspace_for_branch = AsyncMock(return_value=Path("/tmp/qa-ws"))
|
|
svc._resolve_diff_base = AsyncMock(return_value="origin/master")
|
|
svc._resolve_head_ref = AsyncMock(return_value=f"origin/{_BR}")
|
|
svc._token_for_branch = AsyncMock(return_value="tok")
|
|
captured: list[list[str]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
|
captured.append(args)
|
|
return type("R", (), {"returncode": 0, "stdout": "README.md\nsrc/app.py\n"})()
|
|
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
files = await svc.list_changed_files(branch_name=_BR)
|
|
assert files == ["README.md", "src/app.py"]
|
|
assert captured == [["diff", "--name-only", f"origin/master...origin/{_BR}"]]
|
|
svc._resolve_diff_base.assert_awaited_once_with(
|
|
Path("/tmp/qa-ws"), _BR, token="tok", preferred_parent=None
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_diff_honours_explicit_base_with_resolved_head() -> None:
|
|
"""The incremental dev path (base=HEAD~1) still works: explicit base
|
|
is preserved, head still goes through _resolve_head_ref."""
|
|
svc = _git_service()
|
|
svc._workspace_for_branch = AsyncMock(return_value=Path("/tmp/dev-ws"))
|
|
svc._resolve_diff_base = AsyncMock(return_value="SHOULD_NOT_BE_USED")
|
|
svc._resolve_head_ref = AsyncMock(return_value=_BR)
|
|
svc._token_for_branch = AsyncMock(return_value=None)
|
|
captured: list[list[str]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
|
captured.append(args)
|
|
return type("R", (), {"returncode": 0, "stdout": ""})()
|
|
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
await svc.diff(branch_name=_BR, base="HEAD~1")
|
|
assert captured == [["diff", f"HEAD~1...{_BR}"]]
|
|
svc._resolve_diff_base.assert_not_awaited()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# In-path PR-review gate cross-team fix: an explicit ``preferred_parent``
|
|
# (resolve_parent_branch's real parent-task branch) overrides the derived
|
|
# parent_branch_for, fetched + qualified exactly like the derived one — and
|
|
# falls back to the same repo-default when it was never pushed. An explicit
|
|
# literal ``base`` (e.g. HEAD~1 above) still wins outright and ignores it.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_diff_base_uses_preferred_parent_when_pushed() -> None:
|
|
svc = _git_service()
|
|
svc._run_git = AsyncMock()
|
|
svc._ref_exists = AsyncMock(return_value=True)
|
|
ws = Path("/tmp/ws")
|
|
|
|
base = await svc._resolve_diff_base(
|
|
ws,
|
|
"feature/frontend/f7d0a61a--e56e6543--e2b50b06",
|
|
preferred_parent="feature/main_pm/f7d0a61a--e56e6543",
|
|
)
|
|
# NOT the same-team derivation (feature/frontend/f7d0a61a--e56e6543).
|
|
assert base == "origin/feature/main_pm/f7d0a61a--e56e6543"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_diff_base_preferred_parent_falls_back_when_absent() -> None:
|
|
"""A preferred_parent that was never pushed (unassembled branchless
|
|
parent) still falls back to the repo default branch — never crashes."""
|
|
svc = _git_service()
|
|
svc._run_git = AsyncMock()
|
|
svc._ref_exists = AsyncMock(return_value=False)
|
|
svc._default_branch_ref = AsyncMock(return_value="origin/master")
|
|
ws = Path("/tmp/ws")
|
|
|
|
base = await svc._resolve_diff_base(
|
|
ws, "feature/main_pm/f7d0a61a--e56e6543", preferred_parent="master"
|
|
)
|
|
assert base == "origin/master"
|
|
svc._default_branch_ref.assert_awaited_once()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_diff_threads_preferred_parent_into_resolve_diff_base() -> None:
|
|
"""diff()/list_changed_files() forward preferred_parent only when base is
|
|
omitted — the gate's evidence-build path (no explicit base)."""
|
|
svc = _git_service()
|
|
svc._workspace_for_branch = AsyncMock(return_value=Path("/tmp/ws"))
|
|
svc._resolve_head_ref = AsyncMock(return_value=_BR)
|
|
svc._token_for_branch = AsyncMock(return_value="tok")
|
|
svc._ref_exists = AsyncMock(return_value=True)
|
|
svc._run_git = AsyncMock(
|
|
return_value=type("R", (), {"returncode": 0, "stdout": "diff body"})()
|
|
)
|
|
|
|
out = await svc.diff(branch_name=_BR, preferred_parent="feature/main_pm/root")
|
|
assert out == "diff body"
|
|
svc._run_git.assert_any_call(
|
|
Path("/tmp/ws"),
|
|
["diff", f"origin/feature/main_pm/root...{_BR}"],
|
|
check=False,
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_explicit_base_ignores_preferred_parent() -> None:
|
|
"""An explicit literal base wins outright — preferred_parent is only
|
|
consulted when base is omitted."""
|
|
svc = _git_service()
|
|
svc._workspace_for_branch = AsyncMock(return_value=Path("/tmp/ws"))
|
|
svc._resolve_head_ref = AsyncMock(return_value=_BR)
|
|
svc._token_for_branch = AsyncMock(return_value=None)
|
|
svc._resolve_diff_base = AsyncMock(return_value="SHOULD_NOT_BE_USED")
|
|
captured: list[list[str]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
|
captured.append(args)
|
|
return type("R", (), {"returncode": 0, "stdout": ""})()
|
|
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
await svc.diff(
|
|
branch_name=_BR, base="HEAD~1", preferred_parent="feature/main_pm/root"
|
|
)
|
|
assert captured == [["diff", f"HEAD~1...{_BR}"]]
|
|
svc._resolve_diff_base.assert_not_awaited()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Task #168: the diff base must be CURRENT. In an inspecting clone
|
|
# origin/HEAD is set, so _default_branch_ref early-returns the ref NAME
|
|
# without fetching, leaving the base the stale clone-time tip — a
|
|
# three-dot diff then spans the whole repo delta (smoke-15: 41 files vs a
|
|
# 1-line change). _resolve_diff_base must re-fetch the resolved base, and
|
|
# every fetch in the diff path must authenticate (unauth fails on a
|
|
# private repo: "could not read Username for github.com").
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_diff_base_refetches_default_branch_with_token() -> None:
|
|
"""Parent absent → default branch resolved → it must be re-fetched
|
|
authenticated so the base is current, not the stale clone-time ref."""
|
|
svc = _git_service()
|
|
calls: list[tuple[list[str], str | None]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **kw: Any) -> Any:
|
|
calls.append((args, kw.get("token")))
|
|
return type("R", (), {"returncode": 0, "stdout": ""})()
|
|
|
|
# parent ref never exists → fall back to default branch.
|
|
svc._ref_exists = AsyncMock(return_value=False)
|
|
svc._default_branch_ref = AsyncMock(return_value="origin/master")
|
|
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
base = await svc._resolve_diff_base(Path("/tmp/ws"), _BR, token="tok")
|
|
assert base == "origin/master"
|
|
# The resolved default branch ('master') was fetched, authenticated.
|
|
assert (["fetch", "origin", "master"], "tok") in calls
|
|
# The parent fetch is also authenticated.
|
|
assert all(tok == "tok" for args, tok in calls if args[:2] == ["fetch", "origin"])
|
|
svc._default_branch_ref.assert_awaited_once_with(Path("/tmp/ws"), token="tok")
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_resolve_head_ref_fetch_is_authenticated() -> None:
|
|
"""The branch fetch in _resolve_head_ref must carry the token too."""
|
|
svc = _git_service()
|
|
seen: list[tuple[list[str], str | None]] = []
|
|
|
|
async def fake_run(_ws: Any, args: list[str], **kw: Any) -> Any:
|
|
seen.append((args, kw.get("token")))
|
|
return type("R", (), {"returncode": 0, "stdout": ""})()
|
|
|
|
svc._ref_exists = AsyncMock(return_value=True)
|
|
with patch.object(svc, "_run_git", new=fake_run):
|
|
await svc._resolve_head_ref(Path("/tmp/ws"), _BR, token="tok")
|
|
assert (["fetch", "origin", _BR], "tok") in seen
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_token_for_branch_is_best_effort_none() -> None:
|
|
"""Unresolvable branch/project must yield None (degrade to unauth),
|
|
never raise inside the evidence-assembly path."""
|
|
svc = _git_service()
|
|
svc._task_for_branch = AsyncMock(return_value=None)
|
|
assert await svc._token_for_branch(_BR) is None
|