From 4fdde2b0822e4aac5a401e447e33ddd760e81420 Mon Sep 17 00:00:00 2001 From: Renn F Date: Fri, 15 May 2026 06:31:38 +0200 Subject: [PATCH] fix(gateway): evidence/QA/doc paths populate files_changed from git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- roboco/api/deps.py | 3 +- .../services/gateway/choreographer/_impl.py | 11 +- roboco/services/gateway/choreographer/doc.py | 12 +- roboco/services/gateway/choreographer/qa.py | 15 +- roboco/services/gateway/content_actions.py | 31 ++- roboco/services/git.py | 30 +++ .../test_full_lifecycle_real_db.py | 6 + tests/integration/test_lifecycle_real_db.py | 6 + tests/unit/gateway/test_choreographer_doc.py | 2 +- tests/unit/gateway/test_choreographer_qa.py | 2 +- tests/unit/gateway/test_content_actions.py | 6 + .../gateway/test_content_actions_ownership.py | 6 + .../test_evidence_populates_files_changed.py | 182 ++++++++++++++++++ 13 files changed, 294 insertions(+), 18 deletions(-) create mode 100644 tests/unit/gateway/test_evidence_populates_files_changed.py diff --git a/roboco/api/deps.py b/roboco/api/deps.py index dff10bf2..cf43f8ba 100644 --- a/roboco/api/deps.py +++ b/roboco/api/deps.py @@ -519,7 +519,7 @@ async def get_choreographer( async def get_content_actions( db_session: DbSession, ) -> ContentActions: - """Build a ContentActions with all 7 service dependencies wired up.""" + """Build a ContentActions with all service dependencies wired up.""" return ContentActions( ContentActionsDeps( task=TaskService(db_session), @@ -530,6 +530,7 @@ async def get_content_actions( workspace=WorkspaceService(db_session), notifications=NotificationService(), notification_delivery=NotificationDeliveryService(db_session), + evidence_repo=EvidenceRepo(db_session), ) ) diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index a63641b5..57c0bbfd 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1556,13 +1556,18 @@ class Choreographer: async def _build_i_am_done_ok( self, agent_id: UUID, task_id: UUID, t: Any ) -> 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( task_id ) files_changed: list[str] = [] - if t.work_session_id: - files_changed = await self.work_session.files_changed(t.work_session_id) + if t.branch_name: + files_changed = await self.git.list_changed_files(branch_name=t.branch_name) evidence = build_evidence_for_task( t, journal_highlights=journal_highlights, diff --git a/roboco/services/gateway/choreographer/doc.py b/roboco/services/gateway/choreographer/doc.py index ea4ab3c8..c55c3bf7 100644 --- a/roboco/services/gateway/choreographer/doc.py +++ b/roboco/services/gateway/choreographer/doc.py @@ -175,13 +175,19 @@ class DocMixin(_Base): ).with_introspection(task=t, role=role_str) 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] = [] - if task.work_session_id: - files_changed = await self.work_session.files_changed(task.work_session_id) diff = "" if 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( task_id ) diff --git a/roboco/services/gateway/choreographer/qa.py b/roboco/services/gateway/choreographer/qa.py index ea4effb3..dbe976e3 100644 --- a/roboco/services/gateway/choreographer/qa.py +++ b/roboco/services/gateway/choreographer/qa.py @@ -182,16 +182,21 @@ class QAMixin(_Base): async def _build_qa_claim_evidence(self, t: Any, task_id: UUID) -> Any: """Assemble the inline evidence payload returned by claim_review. - Bundles files_changed (from work_session) + pr_diff_summary (from - git) + journal_highlights so the QA agent has the full PR - context up-front and can't miss a piece. + Bundles files_changed + pr_diff_summary (both from git, the + authoritative source) + journal_highlights so the QA agent has + 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] = [] - if t.work_session_id: - files_changed = await self.work_session.files_changed(t.work_session_id) diff_summary = "" if 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( task_id ) diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index 8afdfe6c..194f533a 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -217,6 +217,10 @@ class ContentActionsDeps: # `NotificationDeliveryService`, not `NotificationService`. Keeping # them separate so the sender vs receiver concerns stay split. 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) @@ -254,6 +258,10 @@ class ContentActions: def notifications(self) -> Any: return self._deps.notifications + @property + def evidence_repo(self) -> Any: + return self._deps.evidence_repo + async def commit( self, *, @@ -648,6 +656,13 @@ class ContentActions: Allows inspection when caller is assignee OR task is unassigned (post-handoff transient state) — strict ownership only blocks 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) if t is None: @@ -659,13 +674,21 @@ class ContentActions: agent_id=agent_id, branch_name=t.branch_name ) diff = "" + files_changed: list[str] = [] if t.branch_name: - base = "HEAD~1" if t.commits else None - diff = await self.git.diff(branch_name=t.branch_name, base=base) + diff = await self.git.diff( + 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( t, - journal_highlights=[], - files_changed=[], + journal_highlights=journal_highlights, + files_changed=files_changed, pr_diff_summary=diff, ) return Envelope.ok( diff --git a/roboco/services/git.py b/roboco/services/git.py index 113068ec..6a30553b 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -2135,6 +2135,36 @@ class GitService(BaseService): diff_result = await self._run_git(workspace, diff_args, check=False) 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( self, *, diff --git a/tests/integration/test_full_lifecycle_real_db.py b/tests/integration/test_full_lifecycle_real_db.py index b3d4772c..f8e7f8b9 100644 --- a/tests/integration/test_full_lifecycle_real_db.py +++ b/tests/integration/test_full_lifecycle_real_db.py @@ -119,6 +119,12 @@ class _StubGit: del branch_name, base, actor_agent_id 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: del pr_number, actor_agent_id return "main" diff --git a/tests/integration/test_lifecycle_real_db.py b/tests/integration/test_lifecycle_real_db.py index 80e342f0..45d78518 100644 --- a/tests/integration/test_lifecycle_real_db.py +++ b/tests/integration/test_lifecycle_real_db.py @@ -121,6 +121,12 @@ class _StubGit: del branch_name, base, actor_agent_id 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: del pr_number, actor_agent_id return "main" diff --git a/tests/unit/gateway/test_choreographer_doc.py b/tests/unit/gateway/test_choreographer_doc.py index 92159075..f6bebf02 100644 --- a/tests/unit/gateway/test_choreographer_doc.py +++ b/tests/unit/gateway/test_choreographer_doc.py @@ -70,9 +70,9 @@ async def test_claim_doc_task_returns_evidence() -> None: task_svc.list_paused_for_agent.return_value = [] task_svc.doc_claim.return_value = after work_svc = AsyncMock() - work_svc.files_changed.return_value = ["README.md"] git_svc = AsyncMock() 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) c = Choreographer(deps) diff --git a/tests/unit/gateway/test_choreographer_qa.py b/tests/unit/gateway/test_choreographer_qa.py index 685d7d54..d5017274 100644 --- a/tests/unit/gateway/test_choreographer_qa.py +++ b/tests/unit/gateway/test_choreographer_qa.py @@ -78,9 +78,9 @@ async def test_claim_review_returns_evidence_inline() -> None: task_svc.list_paused_for_agent.return_value = [] task_svc.qa_claim.return_value = t_claimed work_svc = AsyncMock() - work_svc.files_changed.return_value = ["README.md"] git_svc = AsyncMock() 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) c = Choreographer(deps) diff --git a/tests/unit/gateway/test_content_actions.py b/tests/unit/gateway/test_content_actions.py index 14ed2723..ecff1883 100644 --- a/tests/unit/gateway/test_content_actions.py +++ b/tests/unit/gateway/test_content_actions.py @@ -37,6 +37,11 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps: workspace = overrides.get("workspace", AsyncMock()) notifications = overrides.get("notifications", 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( task=task, git=git, @@ -46,6 +51,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps: workspace=workspace, notifications=notifications, notification_delivery=notification_delivery, + evidence_repo=evidence_repo, ) diff --git a/tests/unit/gateway/test_content_actions_ownership.py b/tests/unit/gateway/test_content_actions_ownership.py index fd1c2a06..189597d4 100644 --- a/tests/unit/gateway/test_content_actions_ownership.py +++ b/tests/unit/gateway/test_content_actions_ownership.py @@ -44,6 +44,11 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps: journal = overrides.get("journal", AsyncMock()) workspace = overrides.get("workspace", 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( task=task, git=git, @@ -52,6 +57,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps: journal=journal, workspace=workspace, notifications=notifications, + evidence_repo=evidence_repo, ) diff --git a/tests/unit/gateway/test_evidence_populates_files_changed.py b/tests/unit/gateway/test_evidence_populates_files_changed.py new file mode 100644 index 00000000..464e06a3 --- /dev/null +++ b/tests/unit/gateway/test_evidence_populates_files_changed.py @@ -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()