mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
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.
This commit is contained in:
+108
-14
@@ -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;
|
||||
|
||||
+174
-19
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user