mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): evidence/QA/doc paths populate files_changed from git
Bug:
ContentActions.evidence() hard-coded files_changed=[] and diffed
against HEAD~1 instead of the branch's parent. QA's _build_qa_claim_
evidence (and doc/_impl mirrors) sourced files_changed from
work_session.files_modified, which the gateway commit() never
populates (no add_files_modified plumbing). Result: QA / docs / PM
reviewers saw an empty change list on real PRs and only the latest
commit's delta — flagged in smoke-9 when PR #20 showed the README
change on GitHub but evidence() reported empty.
Fix:
Added GitService.list_changed_files (git diff --name-only against
parent branch). evidence(), _build_qa_claim_evidence,
_claim_doc_evidence, and _build_i_am_done_ok all source files_changed
from this — git is the authoritative source. evidence() also drops
the HEAD~1 base so the diff is the full PR.
Wired EvidenceRepo into ContentActionsDeps so evidence() returns
journal_highlights too, matching the QA/doc shape.
This commit is contained in:
+2
-1
@@ -519,7 +519,7 @@ async def get_choreographer(
|
|||||||
async def get_content_actions(
|
async def get_content_actions(
|
||||||
db_session: DbSession,
|
db_session: DbSession,
|
||||||
) -> ContentActions:
|
) -> ContentActions:
|
||||||
"""Build a ContentActions with all 7 service dependencies wired up."""
|
"""Build a ContentActions with all service dependencies wired up."""
|
||||||
return ContentActions(
|
return ContentActions(
|
||||||
ContentActionsDeps(
|
ContentActionsDeps(
|
||||||
task=TaskService(db_session),
|
task=TaskService(db_session),
|
||||||
@@ -530,6 +530,7 @@ async def get_content_actions(
|
|||||||
workspace=WorkspaceService(db_session),
|
workspace=WorkspaceService(db_session),
|
||||||
notifications=NotificationService(),
|
notifications=NotificationService(),
|
||||||
notification_delivery=NotificationDeliveryService(db_session),
|
notification_delivery=NotificationDeliveryService(db_session),
|
||||||
|
evidence_repo=EvidenceRepo(db_session),
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -1556,13 +1556,18 @@ class Choreographer:
|
|||||||
async def _build_i_am_done_ok(
|
async def _build_i_am_done_ok(
|
||||||
self, agent_id: UUID, task_id: UUID, t: Any
|
self, agent_id: UUID, task_id: UUID, t: Any
|
||||||
) -> Envelope:
|
) -> Envelope:
|
||||||
"""Assemble the success envelope for i_am_done / _with_catchup."""
|
"""Assemble the success envelope for i_am_done / _with_catchup.
|
||||||
|
|
||||||
|
Task #154: files_changed sourced from git (authoritative) so the
|
||||||
|
i_am_done envelope shows the same file list QA / docs / PMs will
|
||||||
|
see — independent of legacy ``add_files_modified`` plumbing.
|
||||||
|
"""
|
||||||
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
||||||
task_id
|
task_id
|
||||||
)
|
)
|
||||||
files_changed: list[str] = []
|
files_changed: list[str] = []
|
||||||
if t.work_session_id:
|
if t.branch_name:
|
||||||
files_changed = await self.work_session.files_changed(t.work_session_id)
|
files_changed = await self.git.list_changed_files(branch_name=t.branch_name)
|
||||||
evidence = build_evidence_for_task(
|
evidence = build_evidence_for_task(
|
||||||
t,
|
t,
|
||||||
journal_highlights=journal_highlights,
|
journal_highlights=journal_highlights,
|
||||||
|
|||||||
@@ -175,13 +175,19 @@ class DocMixin(_Base):
|
|||||||
).with_introspection(task=t, role=role_str)
|
).with_introspection(task=t, role=role_str)
|
||||||
|
|
||||||
async def _claim_doc_evidence(self, task: Any, task_id: UUID) -> dict[str, Any]:
|
async def _claim_doc_evidence(self, task: Any, task_id: UUID) -> dict[str, Any]:
|
||||||
"""Build the evidence dict surfaced inline on claim_doc_task ok envelopes."""
|
"""Build the evidence dict surfaced inline on claim_doc_task ok envelopes.
|
||||||
|
|
||||||
|
Task #154: files_changed sourced from git (authoritative) instead
|
||||||
|
of ``work_session.files_modified``, which the gateway commit()
|
||||||
|
does not populate. The docs writer sees an accurate file list.
|
||||||
|
"""
|
||||||
files_changed: list[str] = []
|
files_changed: list[str] = []
|
||||||
if task.work_session_id:
|
|
||||||
files_changed = await self.work_session.files_changed(task.work_session_id)
|
|
||||||
diff = ""
|
diff = ""
|
||||||
if task.branch_name:
|
if task.branch_name:
|
||||||
diff = await self.git.diff(branch_name=task.branch_name)
|
diff = await self.git.diff(branch_name=task.branch_name)
|
||||||
|
files_changed = await self.git.list_changed_files(
|
||||||
|
branch_name=task.branch_name
|
||||||
|
)
|
||||||
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
||||||
task_id
|
task_id
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -182,16 +182,21 @@ class QAMixin(_Base):
|
|||||||
async def _build_qa_claim_evidence(self, t: Any, task_id: UUID) -> Any:
|
async def _build_qa_claim_evidence(self, t: Any, task_id: UUID) -> Any:
|
||||||
"""Assemble the inline evidence payload returned by claim_review.
|
"""Assemble the inline evidence payload returned by claim_review.
|
||||||
|
|
||||||
Bundles files_changed (from work_session) + pr_diff_summary (from
|
Bundles files_changed + pr_diff_summary (both from git, the
|
||||||
git) + journal_highlights so the QA agent has the full PR
|
authoritative source) + journal_highlights so the QA agent has
|
||||||
context up-front and can't miss a piece.
|
the full PR context up-front and can't miss a piece.
|
||||||
|
|
||||||
|
Task #154: files_changed comes from ``git.list_changed_files``
|
||||||
|
instead of ``work_session.files_modified``. The legacy
|
||||||
|
``add_files_modified`` HTTP path that populated files_modified
|
||||||
|
is not called by the gateway ``commit()``, so the work_session
|
||||||
|
list was always empty — QA saw no files even on real PRs.
|
||||||
"""
|
"""
|
||||||
files_changed: list[str] = []
|
files_changed: list[str] = []
|
||||||
if t.work_session_id:
|
|
||||||
files_changed = await self.work_session.files_changed(t.work_session_id)
|
|
||||||
diff_summary = ""
|
diff_summary = ""
|
||||||
if t.branch_name:
|
if t.branch_name:
|
||||||
diff_summary = await self.git.diff(branch_name=t.branch_name)
|
diff_summary = await self.git.diff(branch_name=t.branch_name)
|
||||||
|
files_changed = await self.git.list_changed_files(branch_name=t.branch_name)
|
||||||
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
||||||
task_id
|
task_id
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -217,6 +217,10 @@ class ContentActionsDeps:
|
|||||||
# `NotificationDeliveryService`, not `NotificationService`. Keeping
|
# `NotificationDeliveryService`, not `NotificationService`. Keeping
|
||||||
# them separate so the sender vs receiver concerns stay split.
|
# them separate so the sender vs receiver concerns stay split.
|
||||||
notification_delivery: Any = None
|
notification_delivery: Any = None
|
||||||
|
# Task #154: evidence() returns journal_highlights for QA/reviewer
|
||||||
|
# context. Matches the choreographer's EvidenceRepo wiring so both
|
||||||
|
# paths surface the same shape.
|
||||||
|
evidence_repo: Any = None
|
||||||
|
|
||||||
|
|
||||||
_VALID_NOTIFY_PRIORITIES: frozenset[str] = frozenset(p.value for p in _comms.Priority)
|
_VALID_NOTIFY_PRIORITIES: frozenset[str] = frozenset(p.value for p in _comms.Priority)
|
||||||
@@ -254,6 +258,10 @@ class ContentActions:
|
|||||||
def notifications(self) -> Any:
|
def notifications(self) -> Any:
|
||||||
return self._deps.notifications
|
return self._deps.notifications
|
||||||
|
|
||||||
|
@property
|
||||||
|
def evidence_repo(self) -> Any:
|
||||||
|
return self._deps.evidence_repo
|
||||||
|
|
||||||
async def commit(
|
async def commit(
|
||||||
self,
|
self,
|
||||||
*,
|
*,
|
||||||
@@ -648,6 +656,13 @@ class ContentActions:
|
|||||||
Allows inspection when caller is assignee OR task is unassigned
|
Allows inspection when caller is assignee OR task is unassigned
|
||||||
(post-handoff transient state) — strict ownership only blocks
|
(post-handoff transient state) — strict ownership only blocks
|
||||||
cross-agent inspection of an actively-owned task.
|
cross-agent inspection of an actively-owned task.
|
||||||
|
|
||||||
|
Task #154: ``files_changed`` and ``pr_diff_summary`` are pulled
|
||||||
|
from git (against the branch's parent) — the authoritative source.
|
||||||
|
Earlier versions hard-coded ``files_changed=[]`` and used
|
||||||
|
``HEAD~1`` for the diff base, so QA / reviewers saw an empty
|
||||||
|
change list and only the latest commit's delta even when the PR
|
||||||
|
on GitHub had a multi-commit change set.
|
||||||
"""
|
"""
|
||||||
t = await self.task.get(task_id)
|
t = await self.task.get(task_id)
|
||||||
if t is None:
|
if t is None:
|
||||||
@@ -659,13 +674,21 @@ class ContentActions:
|
|||||||
agent_id=agent_id, branch_name=t.branch_name
|
agent_id=agent_id, branch_name=t.branch_name
|
||||||
)
|
)
|
||||||
diff = ""
|
diff = ""
|
||||||
|
files_changed: list[str] = []
|
||||||
if t.branch_name:
|
if t.branch_name:
|
||||||
base = "HEAD~1" if t.commits else None
|
diff = await self.git.diff(
|
||||||
diff = await self.git.diff(branch_name=t.branch_name, base=base)
|
branch_name=t.branch_name, actor_agent_id=agent_id
|
||||||
|
)
|
||||||
|
files_changed = await self.git.list_changed_files(
|
||||||
|
branch_name=t.branch_name, actor_agent_id=agent_id
|
||||||
|
)
|
||||||
|
journal_highlights = await self.evidence_repo.journal_highlights_for_task(
|
||||||
|
task_id
|
||||||
|
)
|
||||||
ev = build_evidence_for_task(
|
ev = build_evidence_for_task(
|
||||||
t,
|
t,
|
||||||
journal_highlights=[],
|
journal_highlights=journal_highlights,
|
||||||
files_changed=[],
|
files_changed=files_changed,
|
||||||
pr_diff_summary=diff,
|
pr_diff_summary=diff,
|
||||||
)
|
)
|
||||||
return Envelope.ok(
|
return Envelope.ok(
|
||||||
|
|||||||
@@ -2135,6 +2135,36 @@ class GitService(BaseService):
|
|||||||
diff_result = await self._run_git(workspace, diff_args, check=False)
|
diff_result = await self._run_git(workspace, diff_args, check=False)
|
||||||
return diff_result.stdout
|
return diff_result.stdout
|
||||||
|
|
||||||
|
async def list_changed_files(
|
||||||
|
self,
|
||||||
|
*,
|
||||||
|
branch_name: str,
|
||||||
|
base: str | None = None,
|
||||||
|
actor_agent_id: UUID | None = None,
|
||||||
|
) -> list[str]:
|
||||||
|
"""Return the file paths changed on `branch_name` relative to `base`.
|
||||||
|
|
||||||
|
Mirrors ``diff`` but invokes ``git diff --name-only`` so the
|
||||||
|
gateway evidence path can populate ``files_changed`` from the
|
||||||
|
authoritative git state — independent of whether the agent
|
||||||
|
ever called the legacy ``add_files_modified`` HTTP endpoint
|
||||||
|
(which the gateway commit() does not call). Empty paths are
|
||||||
|
skipped; output preserves git's order.
|
||||||
|
"""
|
||||||
|
from roboco.services.gateway.merge_chain import parent_branch_for
|
||||||
|
|
||||||
|
workspace = await self._workspace_for_branch(
|
||||||
|
branch_name, actor_agent_id=actor_agent_id
|
||||||
|
)
|
||||||
|
if base is None:
|
||||||
|
parent = parent_branch_for(branch_name)
|
||||||
|
await self._run_git(workspace, ["fetch", "origin", parent], check=False)
|
||||||
|
args = ["diff", "--name-only", f"origin/{parent}...{branch_name}"]
|
||||||
|
else:
|
||||||
|
args = ["diff", "--name-only", f"{base}...{branch_name}"]
|
||||||
|
result = await self._run_git(workspace, args, check=False)
|
||||||
|
return [line for line in result.stdout.splitlines() if line.strip()]
|
||||||
|
|
||||||
async def commit(
|
async def commit(
|
||||||
self,
|
self,
|
||||||
*,
|
*,
|
||||||
|
|||||||
@@ -119,6 +119,12 @@ class _StubGit:
|
|||||||
del branch_name, base, actor_agent_id
|
del branch_name, base, actor_agent_id
|
||||||
return "stub diff"
|
return "stub diff"
|
||||||
|
|
||||||
|
async def list_changed_files(
|
||||||
|
self, *, branch_name: str, base: Any = None, actor_agent_id: Any = None
|
||||||
|
) -> list[str]:
|
||||||
|
del branch_name, base, actor_agent_id
|
||||||
|
return []
|
||||||
|
|
||||||
async def pr_target(self, pr_number: int, *, actor_agent_id: Any = None) -> str:
|
async def pr_target(self, pr_number: int, *, actor_agent_id: Any = None) -> str:
|
||||||
del pr_number, actor_agent_id
|
del pr_number, actor_agent_id
|
||||||
return "main"
|
return "main"
|
||||||
|
|||||||
@@ -121,6 +121,12 @@ class _StubGit:
|
|||||||
del branch_name, base, actor_agent_id
|
del branch_name, base, actor_agent_id
|
||||||
return "stub diff"
|
return "stub diff"
|
||||||
|
|
||||||
|
async def list_changed_files(
|
||||||
|
self, *, branch_name: str, base: Any = None, actor_agent_id: Any = None
|
||||||
|
) -> list[str]:
|
||||||
|
del branch_name, base, actor_agent_id
|
||||||
|
return []
|
||||||
|
|
||||||
async def pr_target(self, pr_number: int, *, actor_agent_id: Any = None) -> str:
|
async def pr_target(self, pr_number: int, *, actor_agent_id: Any = None) -> str:
|
||||||
del pr_number, actor_agent_id
|
del pr_number, actor_agent_id
|
||||||
return "main"
|
return "main"
|
||||||
|
|||||||
@@ -70,9 +70,9 @@ async def test_claim_doc_task_returns_evidence() -> None:
|
|||||||
task_svc.list_paused_for_agent.return_value = []
|
task_svc.list_paused_for_agent.return_value = []
|
||||||
task_svc.doc_claim.return_value = after
|
task_svc.doc_claim.return_value = after
|
||||||
work_svc = AsyncMock()
|
work_svc = AsyncMock()
|
||||||
work_svc.files_changed.return_value = ["README.md"]
|
|
||||||
git_svc = AsyncMock()
|
git_svc = AsyncMock()
|
||||||
git_svc.diff.return_value = "+++ diff"
|
git_svc.diff.return_value = "+++ diff"
|
||||||
|
git_svc.list_changed_files.return_value = ["README.md"]
|
||||||
deps = _make_deps(task=task_svc, work_session=work_svc, git=git_svc)
|
deps = _make_deps(task=task_svc, work_session=work_svc, git=git_svc)
|
||||||
c = Choreographer(deps)
|
c = Choreographer(deps)
|
||||||
|
|
||||||
|
|||||||
@@ -78,9 +78,9 @@ async def test_claim_review_returns_evidence_inline() -> None:
|
|||||||
task_svc.list_paused_for_agent.return_value = []
|
task_svc.list_paused_for_agent.return_value = []
|
||||||
task_svc.qa_claim.return_value = t_claimed
|
task_svc.qa_claim.return_value = t_claimed
|
||||||
work_svc = AsyncMock()
|
work_svc = AsyncMock()
|
||||||
work_svc.files_changed.return_value = ["README.md"]
|
|
||||||
git_svc = AsyncMock()
|
git_svc = AsyncMock()
|
||||||
git_svc.diff.return_value = "+++ diff content"
|
git_svc.diff.return_value = "+++ diff content"
|
||||||
|
git_svc.list_changed_files.return_value = ["README.md"]
|
||||||
deps = _make_deps(task=task_svc, work_session=work_svc, git=git_svc)
|
deps = _make_deps(task=task_svc, work_session=work_svc, git=git_svc)
|
||||||
c = Choreographer(deps)
|
c = Choreographer(deps)
|
||||||
|
|
||||||
|
|||||||
@@ -37,6 +37,11 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
workspace = overrides.get("workspace", AsyncMock())
|
workspace = overrides.get("workspace", AsyncMock())
|
||||||
notifications = overrides.get("notifications", AsyncMock())
|
notifications = overrides.get("notifications", AsyncMock())
|
||||||
notification_delivery = overrides.get("notification_delivery", AsyncMock())
|
notification_delivery = overrides.get("notification_delivery", AsyncMock())
|
||||||
|
if "evidence_repo" in overrides:
|
||||||
|
evidence_repo = overrides["evidence_repo"]
|
||||||
|
else:
|
||||||
|
evidence_repo = AsyncMock()
|
||||||
|
evidence_repo.journal_highlights_for_task.return_value = []
|
||||||
return ContentActionsDeps(
|
return ContentActionsDeps(
|
||||||
task=task,
|
task=task,
|
||||||
git=git,
|
git=git,
|
||||||
@@ -46,6 +51,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
workspace=workspace,
|
workspace=workspace,
|
||||||
notifications=notifications,
|
notifications=notifications,
|
||||||
notification_delivery=notification_delivery,
|
notification_delivery=notification_delivery,
|
||||||
|
evidence_repo=evidence_repo,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -44,6 +44,11 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
journal = overrides.get("journal", AsyncMock())
|
journal = overrides.get("journal", AsyncMock())
|
||||||
workspace = overrides.get("workspace", AsyncMock())
|
workspace = overrides.get("workspace", AsyncMock())
|
||||||
notifications = overrides.get("notifications", AsyncMock())
|
notifications = overrides.get("notifications", AsyncMock())
|
||||||
|
if "evidence_repo" in overrides:
|
||||||
|
evidence_repo = overrides["evidence_repo"]
|
||||||
|
else:
|
||||||
|
evidence_repo = AsyncMock()
|
||||||
|
evidence_repo.journal_highlights_for_task.return_value = []
|
||||||
return ContentActionsDeps(
|
return ContentActionsDeps(
|
||||||
task=task,
|
task=task,
|
||||||
git=git,
|
git=git,
|
||||||
@@ -52,6 +57,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
journal=journal,
|
journal=journal,
|
||||||
workspace=workspace,
|
workspace=workspace,
|
||||||
notifications=notifications,
|
notifications=notifications,
|
||||||
|
evidence_repo=evidence_repo,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,182 @@
|
|||||||
|
"""Task #154: evidence() must populate files_changed + use full PR diff.
|
||||||
|
|
||||||
|
Bug:
|
||||||
|
ContentActions.evidence() hard-coded ``files_changed=[]`` and called
|
||||||
|
``git.diff(branch_name=..., base="HEAD~1")``. Result: QA / reviewers
|
||||||
|
inspecting a real PR saw an empty change list and only the latest
|
||||||
|
commit's delta, even when GitHub showed a multi-commit change set.
|
||||||
|
|
||||||
|
Fix:
|
||||||
|
Pull files via ``git.list_changed_files(branch_name=...)`` (no base
|
||||||
|
→ full diff vs parent branch). Pull diff with ``base=None`` so the
|
||||||
|
full PR diff comes through. Both use git as the authoritative source
|
||||||
|
instead of the legacy ``work_session.files_modified`` field, which
|
||||||
|
the gateway ``commit()`` does not populate.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from unittest.mock import AsyncMock, MagicMock
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.services.gateway.content_actions import ContentActions, ContentActionsDeps
|
||||||
|
|
||||||
|
|
||||||
|
def _deps_for_evidence(
|
||||||
|
task_svc: AsyncMock,
|
||||||
|
git_svc: AsyncMock,
|
||||||
|
workspace_svc: AsyncMock,
|
||||||
|
evidence_repo: AsyncMock,
|
||||||
|
) -> ContentActionsDeps:
|
||||||
|
return ContentActionsDeps(
|
||||||
|
task=task_svc,
|
||||||
|
git=git_svc,
|
||||||
|
messaging=AsyncMock(),
|
||||||
|
a2a=AsyncMock(),
|
||||||
|
journal=AsyncMock(),
|
||||||
|
workspace=workspace_svc,
|
||||||
|
notifications=AsyncMock(),
|
||||||
|
notification_delivery=AsyncMock(),
|
||||||
|
evidence_repo=evidence_repo,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _task_with_pr(task_id: object, *, commits: list[str]) -> MagicMock:
|
||||||
|
return MagicMock(
|
||||||
|
id=task_id,
|
||||||
|
status="awaiting_qa",
|
||||||
|
assigned_to=None,
|
||||||
|
branch_name="feature/backend/abc12345--def67890",
|
||||||
|
work_session_id=uuid4(),
|
||||||
|
commits=commits,
|
||||||
|
pr_number=20,
|
||||||
|
pr_url="https://github.com/org/repo/pull/20",
|
||||||
|
dev_notes="see PR description",
|
||||||
|
acceptance_criteria_status=[],
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_evidence_populates_files_changed_from_git() -> None:
|
||||||
|
"""The smoke-9 regression: PR #20 has README change on GitHub but
|
||||||
|
evidence() reports files_changed=[]. The fix queries git directly."""
|
||||||
|
agent_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
task_svc.get.return_value = _task_with_pr(task_id, commits=["abc", "def"])
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
git_svc.diff.return_value = "diff --git a/README.md b/README.md\n+added line\n"
|
||||||
|
git_svc.list_changed_files.return_value = ["README.md", "docs/guide.md"]
|
||||||
|
workspace_svc = AsyncMock()
|
||||||
|
evidence_repo = AsyncMock()
|
||||||
|
evidence_repo.journal_highlights_for_task.return_value = []
|
||||||
|
|
||||||
|
ca = ContentActions(
|
||||||
|
_deps_for_evidence(task_svc, git_svc, workspace_svc, evidence_repo)
|
||||||
|
)
|
||||||
|
env = await ca.evidence(agent_id=agent_id, task_id=task_id)
|
||||||
|
body = env.as_dict()
|
||||||
|
|
||||||
|
assert body["error"] is None
|
||||||
|
assert body["evidence"]["files_changed"] == ["README.md", "docs/guide.md"]
|
||||||
|
assert "diff --git" in body["evidence"]["pr_diff_summary"]
|
||||||
|
git_svc.list_changed_files.assert_awaited_once()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_evidence_uses_full_pr_diff_not_head_minus_one() -> None:
|
||||||
|
"""git.diff must be called with base=None (full PR diff vs parent),
|
||||||
|
not base='HEAD~1' (only the last commit)."""
|
||||||
|
agent_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
# Multi-commit branch — the pre-fix code passed base='HEAD~1' when
|
||||||
|
# task.commits was non-empty, masking earlier commits' changes.
|
||||||
|
task_svc.get.return_value = _task_with_pr(task_id, commits=["sha1", "sha2", "sha3"])
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
git_svc.diff.return_value = "full diff"
|
||||||
|
git_svc.list_changed_files.return_value = []
|
||||||
|
workspace_svc = AsyncMock()
|
||||||
|
evidence_repo = AsyncMock()
|
||||||
|
evidence_repo.journal_highlights_for_task.return_value = []
|
||||||
|
|
||||||
|
ca = ContentActions(
|
||||||
|
_deps_for_evidence(task_svc, git_svc, workspace_svc, evidence_repo)
|
||||||
|
)
|
||||||
|
await ca.evidence(agent_id=agent_id, task_id=task_id)
|
||||||
|
|
||||||
|
git_svc.diff.assert_awaited_once()
|
||||||
|
call_kwargs = git_svc.diff.await_args.kwargs
|
||||||
|
# Pre-fix bug: kwargs['base'] would be 'HEAD~1' for any multi-commit
|
||||||
|
# branch. Post-fix: base is omitted (or explicitly None).
|
||||||
|
base = call_kwargs.get("base")
|
||||||
|
assert base in (None, ""), (
|
||||||
|
f"git.diff must use full-PR diff (base=None), got base={base!r}"
|
||||||
|
)
|
||||||
|
assert call_kwargs.get("branch_name") == "feature/backend/abc12345--def67890"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_evidence_populates_journal_highlights() -> None:
|
||||||
|
"""evidence() must return journal_highlights so QA gets the dev's
|
||||||
|
decision/reflection context — same as qa.py's claim_review evidence."""
|
||||||
|
agent_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
task_svc.get.return_value = _task_with_pr(task_id, commits=["abc"])
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
git_svc.diff.return_value = ""
|
||||||
|
git_svc.list_changed_files.return_value = []
|
||||||
|
workspace_svc = AsyncMock()
|
||||||
|
evidence_repo = AsyncMock()
|
||||||
|
highlights = [
|
||||||
|
{"scope": "decision", "title": "Use README format X", "content": "..."},
|
||||||
|
{"scope": "reflect", "title": "Lesson learned", "content": "..."},
|
||||||
|
]
|
||||||
|
evidence_repo.journal_highlights_for_task.return_value = highlights
|
||||||
|
|
||||||
|
ca = ContentActions(
|
||||||
|
_deps_for_evidence(task_svc, git_svc, workspace_svc, evidence_repo)
|
||||||
|
)
|
||||||
|
env = await ca.evidence(agent_id=agent_id, task_id=task_id)
|
||||||
|
body = env.as_dict()
|
||||||
|
assert body["evidence"]["journal_highlights"] == highlights
|
||||||
|
evidence_repo.journal_highlights_for_task.assert_awaited_once_with(task_id)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_evidence_no_branch_skips_git_calls() -> None:
|
||||||
|
"""A task without a branch_name has no PR yet — skip git entirely,
|
||||||
|
still return a valid envelope with empty files_changed."""
|
||||||
|
agent_id = uuid4()
|
||||||
|
task_id = uuid4()
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
no_branch = MagicMock(
|
||||||
|
id=task_id,
|
||||||
|
status="claimed",
|
||||||
|
assigned_to=agent_id,
|
||||||
|
branch_name=None,
|
||||||
|
work_session_id=None,
|
||||||
|
commits=[],
|
||||||
|
pr_number=None,
|
||||||
|
pr_url=None,
|
||||||
|
dev_notes=None,
|
||||||
|
acceptance_criteria_status=[],
|
||||||
|
)
|
||||||
|
task_svc.get.return_value = no_branch
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
workspace_svc = AsyncMock()
|
||||||
|
evidence_repo = AsyncMock()
|
||||||
|
evidence_repo.journal_highlights_for_task.return_value = []
|
||||||
|
|
||||||
|
ca = ContentActions(
|
||||||
|
_deps_for_evidence(task_svc, git_svc, workspace_svc, evidence_repo)
|
||||||
|
)
|
||||||
|
env = await ca.evidence(agent_id=agent_id, task_id=task_id)
|
||||||
|
body = env.as_dict()
|
||||||
|
assert body["error"] is None
|
||||||
|
assert body["evidence"]["files_changed"] == []
|
||||||
|
assert body["evidence"]["pr_diff_summary"] == ""
|
||||||
|
git_svc.diff.assert_not_awaited()
|
||||||
|
git_svc.list_changed_files.assert_not_awaited()
|
||||||
Reference in New Issue
Block a user