From 161b36b563687c86d65833b81ffc5dbe13b2d356 Mon Sep 17 00:00:00 2001 From: Renn F Date: Wed, 22 Jul 2026 03:30:40 +0200 Subject: [PATCH] fix(git): stage only codegen-produced drift and link the auto-commit The pre-push codegen auto-commit staged with a blanket 'git add -A', so any pre-existing dirty file in the worktree (e.g. crash-orphaned edits a resume never cleans) was silently swept into the 'regenerate generated artifacts' commit. Now the porcelain state is snapshotted before and after the codegen run and only newly-dirty paths are staged with 'git add --'. The commit is also recorded via _link_commit_to_task with the pushing agent's id threaded from both push call sites, so it shows up in the task's commit history like every other commit. --- roboco/services/git.py | 122 +++++++++++++++++--- tests/unit/services/test_git.py | 193 ++++++++++++++++++++++++++++---- 2 files changed, 282 insertions(+), 33 deletions(-) diff --git a/roboco/services/git.py b/roboco/services/git.py index 51e7a0e4..b42f5d42 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -299,6 +299,11 @@ def _git_ownership_scope(args: list[str]) -> str: # Expected number of parts in various git outputs _REV_LIST_PARTS = 2 +# `git status --porcelain`: 2 status columns + 1 space precede the path +_PORCELAIN_PATH_OFFSET = 3 +# A quoted path needs at least its two surrounding quote characters +_MIN_QUOTED_TOKEN_LEN = 2 + # GitHub REST API status codes _GH_UNPROCESSABLE = 422 # merges-API success codes: 201 = merge commit created + pushed; 204 = nothing @@ -1784,7 +1789,9 @@ class GitService(BaseService): workspace = await self.get_workspace(project.slug, agent_id) # Regenerate + commit any codegen drift BEFORE the push carries it — # a no-op unless the project sets codegen_command. - await self._run_codegen_and_commit(str(task.branch_name), workspace) + await self._run_codegen_and_commit( + str(task.branch_name), workspace, actor_agent_id=agent_id + ) # Push the task's branch BY NAME, independent of the current checkout. # The dev's clone is shared across tasks, so by the QA-submission / # open_pr boundary it is usually parked on a LATER task's branch; the @@ -4257,7 +4264,79 @@ class GitService(BaseService): command = getattr(project, "codegen_command", None) return command if isinstance(command, str) and command else None - async def _run_codegen_and_commit(self, branch_name: str, workspace: Path) -> None: + @staticmethod + def _porcelain_paths(status_output: str) -> set[str]: + """Path(s) named by each ``git status --porcelain`` line. + + A rename line (``R old -> new``) contributes BOTH sides — staging + only the new path would leave the old path's deletion unstaged. A + quoted path (git wraps a path containing unusual characters in + double quotes) has its surrounding quotes stripped. + """ + paths: set[str] = set() + for line in status_output.splitlines(): + if len(line) <= _PORCELAIN_PATH_OFFSET: + continue + rest = line[_PORCELAIN_PATH_OFFSET:] + raw_tokens = rest.split(" -> ") if " -> " in rest else (rest,) + for raw_token in raw_tokens: + candidate = raw_token.strip() + if ( + len(candidate) >= _MIN_QUOTED_TOKEN_LEN + and candidate[0] == '"' + and candidate[-1] == '"' + ): + candidate = candidate[1:-1] + if candidate: + paths.add(candidate) + return paths + + @classmethod + def _new_codegen_drift_paths(cls, before: str, after: str) -> list[str]: + """Paths newly dirty in ``after`` that were clean in ``before``. + + Identity is the path, not the full status line, so a file already + dirty pre-codegen is excluded even if codegen also touched it — + only genuinely new drift gets staged. + """ + return sorted(cls._porcelain_paths(after) - cls._porcelain_paths(before)) + + async def _link_codegen_commit( + self, + worktree: Path, + task: Any, + task_id: UUID, + message: str, + actor_agent_id: UUID | None, + ) -> None: + """Best-effort link of the just-made codegen commit to its task. + + Mirrors every other commit path's ``_link_commit_to_task`` call so + this commit lands in ``task.commits``/the WorkSession instead of + being invisible to QA/PM/CEO review surfaces. A resolution failure + here only logs — the commit already landed and the caller's push + must proceed regardless (see ``_run_codegen_and_commit``). + """ + link_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id) + if link_agent_id is None: + self.log.warning( + "codegen_commit_link_skipped_no_agent", task_id=str(task_id) + ) + return + sha_result = await self._run_git(worktree, ["rev-parse", "HEAD"], check=False) + sha = sha_result.stdout.strip() + if not sha: + self.log.warning("codegen_commit_sha_unresolved", task_id=str(task_id)) + return + await self._link_commit_to_task(task_id, sha, message, link_agent_id) + + async def _run_codegen_and_commit( + self, + branch_name: str, + workspace: Path, + *, + actor_agent_id: UUID | None = None, + ) -> None: """Regenerate + commit codegen drift in the branch's worktree before push. Some projects check in generated artifacts (rendered docs, generated @@ -4268,6 +4347,17 @@ class GitService(BaseService): ``codegen_command`` here means any drift lands in the SAME push that's about to open/update the PR, so CI never sees it stale. + Staging is scoped to what codegen ITSELF newly dirtied: a + ``git status --porcelain`` snapshot taken before the codegen command + runs is diffed against one taken after, and only the paths absent + from the first are staged. A file already dirty in the worktree + before codegen ran (e.g. crash-orphaned edits the resume path left + behind) is never swept into this commit, even if codegen also + touched it — CI's own drift gate stays the safety net for any + residual drift. The resulting commit is linked to the task/work + session (`_link_codegen_commit`) so it isn't invisible to + QA/PM/CEO review surfaces. + Fail-open by design: a broken/timing-out codegen command, or any resolution failure (missing task/project, worktree trouble), logs a warning and is skipped — the push proceeds without a commit rather @@ -4288,6 +4378,9 @@ class GitService(BaseService): task_id = require_uuid(task.id) worktree = self._worktree_for_task(workspace, task_id) await self._ensure_worktree_for_commit(workspace, worktree, branch_name) + before = await self._run_git( + worktree, ["status", "--porcelain"], check=False + ) result = await run_quality_commands(worktree, [("codegen", command)]) if not result.passed: self.log.warning( @@ -4297,19 +4390,18 @@ class GitService(BaseService): output=result.output_excerpt, ) return - status = await self._run_git( + after = await self._run_git( worktree, ["status", "--porcelain"], check=False ) - if not status.stdout.strip(): - return # codegen ran clean — nothing to commit - await self._run_git(worktree, ["add", "-A"]) - await self._run_git( - worktree, - [ - "commit", - "-m", - f"[{str(task_id)[:8]}] regenerate generated artifacts", - ], + new_paths = self._new_codegen_drift_paths(before.stdout, after.stdout) + if not new_paths: + self.log.info("codegen_no_new_drift", task_id=str(task_id)) + return # codegen touched nothing beyond pre-existing drift + await self._run_git(worktree, ["add", "--", *new_paths]) + message = f"[{str(task_id)[:8]}] regenerate generated artifacts" + await self._run_git(worktree, ["commit", "-m", message]) + await self._link_codegen_commit( + worktree, task, task_id, message, actor_agent_id ) except Exception as exc: self.log.warning( @@ -4453,7 +4545,9 @@ class GitService(BaseService): ) # Regenerate + commit any codegen drift BEFORE this first push opens # the PR — a no-op unless the project sets codegen_command. - await self._run_codegen_and_commit(branch_name, workspace) + await self._run_codegen_and_commit( + branch_name, workspace, actor_agent_id=actor_agent_id + ) # Push the NAMED branch, not the workspace's current checkout. The # clone root is shared across a dev's tasks and (F123) parked on the # default branch while the task branch lives in a per-task worktree; diff --git a/tests/unit/services/test_git.py b/tests/unit/services/test_git.py index 08aaefbf..2cffa0f3 100644 --- a/tests/unit/services/test_git.py +++ b/tests/unit/services/test_git.py @@ -178,9 +178,12 @@ async def test_push_task_branch_runs_codegen_before_push() -> None: push_mock = AsyncMock(return_value=("feature/backend/abc", _PUSHED_COMMIT_COUNT)) _bind(svc, "push", push_mock) - await svc.push_task_branch(uuid4(), uuid4()) + agent_id = uuid4() + await svc.push_task_branch(agent_id, uuid4()) - codegen_mock.assert_awaited_once_with("feature/backend/abc", Path("/tmp/ws")) + codegen_mock.assert_awaited_once_with( + "feature/backend/abc", Path("/tmp/ws"), actor_agent_id=agent_id + ) @pytest.mark.asyncio @@ -224,9 +227,12 @@ async def test_push_branch_runs_codegen_before_push() -> None: push_mock = AsyncMock(return_value=(branch_name, _PUSHED_COMMIT_COUNT)) _bind(svc, "push", push_mock) - await svc.push_branch(branch_name) + actor_id = uuid4() + await svc.push_branch(branch_name, actor_agent_id=actor_id) - codegen_mock.assert_awaited_once_with(branch_name, Path("/tmp/ws")) + codegen_mock.assert_awaited_once_with( + branch_name, Path("/tmp/ws"), actor_agent_id=actor_id + ) @pytest.mark.asyncio @@ -452,9 +458,109 @@ async def test_run_codegen_and_commit_noop_when_no_task() -> None: @pytest.mark.asyncio async def test_run_codegen_and_commit_commits_drift() -> None: - """Codegen runs clean but produces drift -> add -A + a task-prefixed commit.""" + """Codegen runs clean but produces NEW drift -> scoped `git add --` (not + `-A`) + a task-prefixed commit, and the commit is linked to the task.""" task_id = uuid4() - task = MagicMock(id=task_id, branch_name="feature/backend/abc") + agent_id = uuid4() + task = MagicMock(id=task_id, branch_name="feature/backend/abc", assigned_to=None) + project = MagicMock(codegen_command="make codegen", slug="roboco") + svc = _service() + _bind(svc, "_task_for_branch", AsyncMock(return_value=task)) + _bind(svc, "_project_for_task", AsyncMock(return_value=project)) + _bind(svc, "_ensure_worktree_for_commit", AsyncMock()) + link_mock = AsyncMock() + _bind(svc, "_link_commit_to_task", link_mock) + calls: list[list[str]] = [] + status_calls = 0 + + async def _run_git(_ws: object, args: list[str], **_kw: object) -> MagicMock: + nonlocal status_calls + calls.append(args) + res = MagicMock() + res.returncode = 0 + if args == ["status", "--porcelain"]: + status_calls += 1 + # Clean before codegen runs; codegen dirties one new path. + res.stdout = "" if status_calls == 1 else " M docs/rag/lifecycle/foo.md\n" + elif args[0] == "rev-parse": + res.stdout = "abc123deadbeef\n" + else: + res.stdout = "" + return res + + _bind(svc, "_run_git", AsyncMock(side_effect=_run_git)) + gate_result = GateResult(passed=True, output="ok") + with patch( + "roboco.services.git.run_quality_commands", + AsyncMock(return_value=gate_result), + ): + await svc._run_codegen_and_commit( + "feature/backend/abc", Path("/tmp/ws"), actor_agent_id=agent_id + ) + + assert ["add", "--", "docs/rag/lifecycle/foo.md"] in calls + assert not any(c[:2] == ["add", "-A"] for c in calls) + expected_message = f"[{str(task_id)[:8]}] regenerate generated artifacts" + commit_call = next(c for c in calls if c[0] == "commit") + assert commit_call == ["commit", "-m", expected_message] + link_mock.assert_awaited_once_with( + task_id, "abc123deadbeef", expected_message, agent_id + ) + + +@pytest.mark.asyncio +async def test_run_codegen_and_commit_excludes_pre_existing_dirty_file() -> None: + """A file already dirty before codegen ran (e.g. crash-orphaned edits the + resume no-op left behind) must never be swept into the auto-commit, even + though codegen also touches it — only genuinely NEW drift is staged.""" + task_id = uuid4() + task = MagicMock(id=task_id, branch_name="feature/backend/abc", assigned_to=None) + project = MagicMock(codegen_command="make codegen", slug="roboco") + svc = _service() + _bind(svc, "_task_for_branch", AsyncMock(return_value=task)) + _bind(svc, "_project_for_task", AsyncMock(return_value=project)) + _bind(svc, "_ensure_worktree_for_commit", AsyncMock()) + _bind(svc, "_link_commit_to_task", AsyncMock()) + calls: list[list[str]] = [] + status_calls = 0 + + async def _run_git(_ws: object, args: list[str], **_kw: object) -> MagicMock: + nonlocal status_calls + calls.append(args) + res = MagicMock() + res.returncode = 0 + if args == ["status", "--porcelain"]: + status_calls += 1 + res.stdout = ( + " M crash_orphaned.txt\n" + if status_calls == 1 + else " M crash_orphaned.txt\n M docs/rag/lifecycle/foo.md\n" + ) + elif args[0] == "rev-parse": + res.stdout = "deadbeef\n" + else: + res.stdout = "" + return res + + _bind(svc, "_run_git", AsyncMock(side_effect=_run_git)) + gate_result = GateResult(passed=True, output="ok") + with patch( + "roboco.services.git.run_quality_commands", + AsyncMock(return_value=gate_result), + ): + await svc._run_codegen_and_commit("feature/backend/abc", Path("/tmp/ws")) + + add_call = next(c for c in calls if c[0] == "add") + assert add_call == ["add", "--", "docs/rag/lifecycle/foo.md"] + assert "crash_orphaned.txt" not in add_call + + +@pytest.mark.asyncio +async def test_run_codegen_and_commit_noop_when_no_new_drift() -> None: + """Codegen only touches what was already dirty pre-run -> the newly-dirty + set is empty, so nothing is staged or committed even though the worktree + is dirty both before and after.""" + task = MagicMock(id=uuid4(), branch_name="feature/backend/abc") project = MagicMock(codegen_command="make codegen", slug="roboco") svc = _service() _bind(svc, "_task_for_branch", AsyncMock(return_value=task)) @@ -466,7 +572,7 @@ async def test_run_codegen_and_commit_commits_drift() -> None: calls.append(args) res = MagicMock() res.returncode = 0 - res.stdout = " M docs/rag/lifecycle/foo.md\n" if args[0] == "status" else "" + res.stdout = " M pre_existing.txt\n" if args[0] == "status" else "" return res _bind(svc, "_run_git", AsyncMock(side_effect=_run_git)) @@ -477,13 +583,7 @@ async def test_run_codegen_and_commit_commits_drift() -> None: ): await svc._run_codegen_and_commit("feature/backend/abc", Path("/tmp/ws")) - assert ["add", "-A"] in calls - commit_call = next(c for c in calls if c[0] == "commit") - assert commit_call == [ - "commit", - "-m", - f"[{str(task_id)[:8]}] regenerate generated artifacts", - ] + assert not any(c[0] in ("add", "commit") for c in calls) @pytest.mark.asyncio @@ -525,8 +625,16 @@ async def test_run_codegen_and_commit_fail_open_on_command_failure() -> None: _bind(svc, "_task_for_branch", AsyncMock(return_value=task)) _bind(svc, "_project_for_task", AsyncMock(return_value=project)) _bind(svc, "_ensure_worktree_for_commit", AsyncMock()) - run_git_mock = AsyncMock() - _bind(svc, "_run_git", run_git_mock) + calls: list[list[str]] = [] + + async def _run_git(_ws: object, args: list[str], **_kw: object) -> MagicMock: + calls.append(args) + res = MagicMock() + res.returncode = 0 + res.stdout = "" + return res + + _bind(svc, "_run_git", AsyncMock(side_effect=_run_git)) gate_result = GateResult(passed=False, failures=("codegen",), output="boom") with patch( "roboco.services.git.run_quality_commands", @@ -534,9 +642,56 @@ async def test_run_codegen_and_commit_fail_open_on_command_failure() -> None: ): await svc._run_codegen_and_commit("feature/backend/abc", Path("/tmp/ws")) - # Never even checks git status — a failed codegen command skips straight - # through without touching git. - run_git_mock.assert_not_awaited() + # Only the pre-codegen baseline status is read (needed for the eventual + # diff); a failed codegen command skips the post-run status, add, and + # commit entirely. + assert calls == [["status", "--porcelain"]] + + +@pytest.mark.asyncio +async def test_run_codegen_and_commit_links_commit_to_task() -> None: + """The auto-commit is linked to the task/work-session the same way every + other commit path is (`_link_commit_to_task`), using the resolved HEAD + sha and the threaded-through actor agent id.""" + task_id = uuid4() + agent_id = uuid4() + task = MagicMock(id=task_id, branch_name="feature/backend/abc", assigned_to=None) + project = MagicMock(codegen_command="make codegen", slug="roboco") + svc = _service() + _bind(svc, "_task_for_branch", AsyncMock(return_value=task)) + _bind(svc, "_project_for_task", AsyncMock(return_value=project)) + _bind(svc, "_ensure_worktree_for_commit", AsyncMock()) + link_mock = AsyncMock() + _bind(svc, "_link_commit_to_task", link_mock) + status_calls = 0 + + async def _run_git(_ws: object, args: list[str], **_kw: object) -> MagicMock: + nonlocal status_calls + res = MagicMock() + res.returncode = 0 + if args == ["status", "--porcelain"]: + status_calls += 1 + res.stdout = "" if status_calls == 1 else " M docs/rag/lifecycle/foo.md\n" + elif args[0] == "rev-parse": + res.stdout = "cafebabe1234\n" + else: + res.stdout = "" + return res + + _bind(svc, "_run_git", AsyncMock(side_effect=_run_git)) + gate_result = GateResult(passed=True, output="ok") + with patch( + "roboco.services.git.run_quality_commands", + AsyncMock(return_value=gate_result), + ): + await svc._run_codegen_and_commit( + "feature/backend/abc", Path("/tmp/ws"), actor_agent_id=agent_id + ) + + expected_message = f"[{str(task_id)[:8]}] regenerate generated artifacts" + link_mock.assert_awaited_once_with( + task_id, "cafebabe1234", expected_message, agent_id + ) @pytest.mark.asyncio