mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(git): authenticate diff-path fetches so QA's diff base is current (#168)
Smoke-15: QA's claim_review diff was `origin/master...origin/<branch>`
but origin/master in QA's clone was the STALE clone-time tip
(47c674d) — the three-dot diff spanned the whole session delta
(41 files / +2740) instead of the 1-line README change.
Two compounding causes:
- _default_branch_ref early-returns the ref NAME when origin/HEAD is
set (it was) WITHOUT fetching it, so the base stayed stale.
- Every fetch in the diff path ran unauthenticated; the repo is
private, so `git fetch` failed ("could not read Username for
github.com") and could never refresh the ref. (The documenter path
was correct only because #162 uses the PAT-injected
workspace.fetch_branch_for_inspection.)
Fix: new best-effort _token_for_branch resolves the project PAT
(None on any failure → degrades to unauth, never raises in the
evidence path). diff()/list_changed_files() thread it into
_resolve_head_ref + _resolve_diff_base, whose fetches now pass
token= (uses the existing _run_git http.extraheader Basic-auth
injection). _resolve_diff_base additionally re-fetches the resolved
default branch so the base is current even when origin/HEAD shortcut
skipped the fetch.
This commit is contained in:
+61
-11
@@ -1817,6 +1817,30 @@ class GitService(BaseService):
|
||||
workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id)
|
||||
return await self.get_workspace(project.slug, agent_id=workspace_agent_id)
|
||||
|
||||
async def _token_for_branch(self, branch_name: str) -> str | None:
|
||||
"""Best-effort project PAT for authenticated fetch in the diff path.
|
||||
|
||||
Task #168: an unauthenticated ``git fetch`` fails on a private
|
||||
repo ("could not read Username for github.com"), so the diff base
|
||||
stays the stale clone-time ``origin/<default>`` and the three-dot
|
||||
diff spans the whole repo delta instead of the branch's change.
|
||||
Returns None on ANY resolution failure so the fetch degrades to
|
||||
unauthenticated (prior behaviour) rather than raising inside an
|
||||
evidence-assembly path — authentication is an optimisation here,
|
||||
never a hard dependency of producing a diff.
|
||||
"""
|
||||
try:
|
||||
task = await self._task_for_branch(branch_name)
|
||||
if task is None:
|
||||
return None
|
||||
project_service = get_project_service(self.session)
|
||||
project = await project_service.get(UUID(str(task.project_id)))
|
||||
if project is None:
|
||||
return None
|
||||
return await self._get_project_token_or_raise(project.slug)
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
async def checkout_branch_in_agent_workspace(
|
||||
self,
|
||||
branch_name: str,
|
||||
@@ -2137,14 +2161,17 @@ class GitService(BaseService):
|
||||
)
|
||||
return result.returncode == 0
|
||||
|
||||
async def _default_branch_ref(self, workspace: Any) -> str:
|
||||
async def _default_branch_ref(
|
||||
self, workspace: Any, token: str | None = None
|
||||
) -> str:
|
||||
"""Resolve the repo's default remote branch ref.
|
||||
|
||||
Tries ``origin/HEAD`` (the canonical pointer), then common
|
||||
defaults. Always returns a usable ref string; falls back to
|
||||
``origin/master`` so the diff command is still well-formed even
|
||||
on a misconfigured remote (an empty/garbage diff is recoverable;
|
||||
a malformed git invocation is not).
|
||||
a malformed git invocation is not). This only resolves the ref
|
||||
NAME — the caller is responsible for fetching it fresh (#168).
|
||||
"""
|
||||
head = await self._run_git(
|
||||
workspace,
|
||||
@@ -2160,12 +2187,15 @@ class GitService(BaseService):
|
||||
workspace,
|
||||
["fetch", "origin", candidate.split("/", 1)[1]],
|
||||
check=False,
|
||||
token=token,
|
||||
)
|
||||
if await self._ref_exists(workspace, candidate):
|
||||
return candidate
|
||||
return "origin/master"
|
||||
|
||||
async def _resolve_diff_base(self, workspace: Any, branch_name: str) -> str:
|
||||
async def _resolve_diff_base(
|
||||
self, workspace: Any, branch_name: str, token: str | None = None
|
||||
) -> str:
|
||||
"""Best diff base for `branch_name` when no explicit base is given.
|
||||
|
||||
Task #161: a leaf dev branch's ``parent_branch_for`` is the
|
||||
@@ -2174,16 +2204,32 @@ class GitService(BaseService):
|
||||
against a non-existent ``origin/<parent>`` returns an empty
|
||||
diff, so QA / docs see nothing. Fall back to the repo default
|
||||
branch when the computed parent ref is absent on origin.
|
||||
|
||||
Task #168: the default-branch ref in an inspecting clone is the
|
||||
stale clone-time tip (origin/HEAD is set, so _default_branch_ref
|
||||
early-returns its NAME without fetching). A three-dot diff against
|
||||
a stale base spans the whole repo delta, not the branch's change.
|
||||
Re-fetch the resolved base authenticated (unauth fails on private
|
||||
repos) so the base is current.
|
||||
"""
|
||||
from roboco.services.gateway.merge_chain import parent_branch_for
|
||||
|
||||
parent = parent_branch_for(branch_name)
|
||||
await self._run_git(workspace, ["fetch", "origin", parent], check=False)
|
||||
await self._run_git(
|
||||
workspace, ["fetch", "origin", parent], check=False, token=token
|
||||
)
|
||||
if await self._ref_exists(workspace, f"origin/{parent}"):
|
||||
return f"origin/{parent}"
|
||||
return await self._default_branch_ref(workspace)
|
||||
default = await self._default_branch_ref(workspace, token=token)
|
||||
short = default.split("/", 1)[1] if "/" in default else default
|
||||
await self._run_git(
|
||||
workspace, ["fetch", "origin", short], check=False, token=token
|
||||
)
|
||||
return default
|
||||
|
||||
async def _resolve_head_ref(self, workspace: Any, branch_name: str) -> str:
|
||||
async def _resolve_head_ref(
|
||||
self, workspace: Any, branch_name: str, token: str | None = None
|
||||
) -> str:
|
||||
"""Ref for the branch tip that actually resolves in `workspace`.
|
||||
|
||||
Task #161 (facet): the local ``<branch_name>`` ref only exists in
|
||||
@@ -2199,7 +2245,9 @@ class GitService(BaseService):
|
||||
back to ``origin/<branch>``; last resort the bare name so the
|
||||
diff command stays well-formed.
|
||||
"""
|
||||
await self._run_git(workspace, ["fetch", "origin", branch_name], check=False)
|
||||
await self._run_git(
|
||||
workspace, ["fetch", "origin", branch_name], check=False, token=token
|
||||
)
|
||||
if await self._ref_exists(workspace, branch_name):
|
||||
return branch_name
|
||||
if await self._ref_exists(workspace, f"origin/{branch_name}"):
|
||||
@@ -2227,11 +2275,12 @@ class GitService(BaseService):
|
||||
workspace = await self._workspace_for_branch(
|
||||
branch_name, actor_agent_id=actor_agent_id
|
||||
)
|
||||
head_ref = await self._resolve_head_ref(workspace, branch_name)
|
||||
token = await self._token_for_branch(branch_name)
|
||||
head_ref = await self._resolve_head_ref(workspace, branch_name, token=token)
|
||||
base_ref = (
|
||||
base
|
||||
if base is not None
|
||||
else await self._resolve_diff_base(workspace, branch_name)
|
||||
else await self._resolve_diff_base(workspace, branch_name, token=token)
|
||||
)
|
||||
diff_result = await self._run_git(
|
||||
workspace, ["diff", f"{base_ref}...{head_ref}"], check=False
|
||||
@@ -2258,11 +2307,12 @@ class GitService(BaseService):
|
||||
workspace = await self._workspace_for_branch(
|
||||
branch_name, actor_agent_id=actor_agent_id
|
||||
)
|
||||
head_ref = await self._resolve_head_ref(workspace, branch_name)
|
||||
token = await self._token_for_branch(branch_name)
|
||||
head_ref = await self._resolve_head_ref(workspace, branch_name, token=token)
|
||||
base_ref = (
|
||||
base
|
||||
if base is not None
|
||||
else await self._resolve_diff_base(workspace, branch_name)
|
||||
else await self._resolve_diff_base(workspace, branch_name, token=token)
|
||||
)
|
||||
result = await self._run_git(
|
||||
workspace,
|
||||
|
||||
@@ -162,6 +162,9 @@ async def test_diff_targets_origin_head_in_foreign_clone() -> None:
|
||||
svc._resolve_head_ref = AsyncMock( # type: ignore[method-assign]
|
||||
return_value=f"origin/{_BR}"
|
||||
)
|
||||
svc._token_for_branch = AsyncMock( # type: ignore[method-assign]
|
||||
return_value="tok"
|
||||
)
|
||||
captured: list[list[str]] = []
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
@@ -173,6 +176,12 @@ async def test_diff_targets_origin_head_in_foreign_clone() -> None:
|
||||
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"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -188,6 +197,9 @@ async def test_list_changed_files_targets_origin_head_in_foreign_clone() -> None
|
||||
svc._resolve_head_ref = AsyncMock( # type: ignore[method-assign]
|
||||
return_value=f"origin/{_BR}"
|
||||
)
|
||||
svc._token_for_branch = AsyncMock( # type: ignore[method-assign]
|
||||
return_value="tok"
|
||||
)
|
||||
captured: list[list[str]] = []
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
@@ -199,6 +211,9 @@ async def test_list_changed_files_targets_origin_head_in_foreign_clone() -> None
|
||||
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"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -215,6 +230,9 @@ async def test_diff_honours_explicit_base_with_resolved_head() -> None:
|
||||
svc._resolve_head_ref = AsyncMock( # type: ignore[method-assign]
|
||||
return_value=_BR
|
||||
)
|
||||
svc._token_for_branch = AsyncMock( # type: ignore[method-assign]
|
||||
return_value=None
|
||||
)
|
||||
captured: list[list[str]] = []
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
@@ -225,3 +243,66 @@ async def test_diff_honours_explicit_base_with_resolved_head() -> None:
|
||||
await svc.diff(branch_name=_BR, base="HEAD~1")
|
||||
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": ""})()
|
||||
|
||||
svc._run_git = fake_run # type: ignore[method-assign]
|
||||
# parent ref never exists → fall back to default branch.
|
||||
svc._ref_exists = AsyncMock(return_value=False) # type: ignore[method-assign]
|
||||
svc._default_branch_ref = AsyncMock( # type: ignore[method-assign]
|
||||
return_value="origin/master"
|
||||
)
|
||||
|
||||
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._run_git = fake_run # type: ignore[method-assign]
|
||||
svc._ref_exists = AsyncMock(return_value=True) # type: ignore[method-assign]
|
||||
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) # type: ignore[method-assign]
|
||||
assert await svc._token_for_branch(_BR) is None
|
||||
|
||||
Reference in New Issue
Block a user