diff --git a/src/forge/prompts/v1/implement-review-fix.md b/src/forge/prompts/v1/implement-review-fix.md index a91bb050c..58d398b20 100644 --- a/src/forge/prompts/v1/implement-review-fix.md +++ b/src/forge/prompts/v1/implement-review-fix.md @@ -11,9 +11,17 @@ Implement the code changes described in `.forge/review-plan.md`, then ensure the - Run the linter/formatter on changed files - Verify the build compiles -4. Commit everything (implementation + any generated or formatted files) in a single commit: - `git commit -m "[{ticket_key}] review: address PR feedback"` +4. Amend the existing HEAD commit so review fixes stay on the original commit + (preserves the original message and avoids a second commit that may fail + per-commit CI checks): + ``` + git add -A + git commit --amend --no-edit + ``` + Do **not** create a new commit with a different message. Only create a new + commit if `git commit --amend` is impossible (for example, HEAD has no + commits yet); in that rare case keep the original project commit style. -5. Do NOT push — the orchestrator handles that. +5. Do NOT push — the orchestrator handles that (force-push after amend). Ticket: {ticket_key} diff --git a/src/forge/workflow/nodes/implement_review.py b/src/forge/workflow/nodes/implement_review.py index 88dcb1ce1..2bd17cc22 100644 --- a/src/forge/workflow/nodes/implement_review.py +++ b/src/forge/workflow/nodes/implement_review.py @@ -324,24 +324,24 @@ async def implement_review(state: WorkflowState) -> WorkflowState: ) state = merge_review_exhaustion(state, result, ticket_key, "implement_review_fix") - # Commit any uncommitted changes the container left + # Fold leftover work into HEAD so review fixes do not create a + # second commit that can fail per-commit message CI gates. if git.has_uncommitted_changes(): - git.stage_all() - git.commit(f"[{ticket_key}] review: address PR feedback") + git.amend_commit() - # ── Push only if there are new commits ─────────────────────────────── + # ── Push only if HEAD moved relative to the remote ──────────────────── if fork_owner and fork_repo: git.add_fork_remote(fork_owner, fork_repo) remote_ref = f"fork/{branch_name}" else: remote_ref = f"origin/{branch_name}" - unpushed = git._run_git( - "log", f"{remote_ref}..HEAD", "--oneline", check=False - ).stdout.strip() + local_sha = git._run_git("rev-parse", "HEAD", check=False).stdout.strip() + remote_sha = git._run_git("rev-parse", remote_ref, check=False).stdout.strip() + head_diverged = bool(local_sha) and local_sha != remote_sha - if unpushed: - # Run post-change review before pushing (only when there are commits) + if head_diverged: + # Run post-change review before pushing (only when HEAD changed) _, review_result = await run_post_change_review( workspace_path=workspace_path, ticket_key=ticket_key, @@ -354,11 +354,12 @@ async def implement_review(state: WorkflowState) -> WorkflowState: if review_result is not None: state = merge_review_exhaustion(state, review_result, ticket_key, "code_review") + # Amend rewrites history; force-push is required to update the PR. if fork_owner and fork_repo: - git.push_to_fork(force=False) + git.push_to_fork(force=True) else: - git.push(force=False) - logger.info(f"Review implementation pushed for {ticket_key}") + git.push(force=True) + logger.info("Review implementation force-pushed for %s", ticket_key) await sync_pr_description( state, @@ -391,7 +392,7 @@ async def implement_review(state: WorkflowState) -> WorkflowState: if contested_comments: next_node = "review_response_gate" else: - next_node = "wait_for_ci_gate" if unpushed else "human_review_gate" + next_node = "wait_for_ci_gate" if head_diverged else "human_review_gate" return update_state_timestamp( { diff --git a/src/forge/workspace/git_ops.py b/src/forge/workspace/git_ops.py index 625164efe..131000f9e 100644 --- a/src/forge/workspace/git_ops.py +++ b/src/forge/workspace/git_ops.py @@ -262,6 +262,55 @@ def commit(self, message: str, author_name: str = "Forge") -> bool: logger.info(f"Committed: {message[:50]}...") return True + def amend_commit( + self, + message: str | None = None, + author_name: str = "Forge", + ) -> bool: + """Amend HEAD with staged/unstaged user-facing changes. + + Preserves the existing commit message when ``message`` is None. + Returns False when there is nothing to amend into HEAD. + + Args: + message: Optional replacement commit message. When None, keep HEAD's + message via ``--no-edit``. + author_name: Author name recorded on the amended commit. + + Returns: + True if HEAD was amended, False if there was nothing to commit. + """ + result = self._run_git("status", "--porcelain", check=False) + if not result.stdout.strip() and message is None: + logger.info("Nothing to amend") + return False + + self.stage_all() + # Re-check after staging exclusions (.forge) — may be empty. + staged = self._run_git("diff", "--cached", "--name-only", check=False) + if not staged.stdout.strip() and message is None: + logger.info("Nothing to amend after staging") + return False + + args = [ + "-c", + f"user.name={self.settings.git_user_name}", + "-c", + f"user.email={self.settings.git_user_email}", + "commit", + "--amend", + "--author", + f"{author_name} <{self.settings.git_user_email}>", + ] + if message is None: + args.append("--no-edit") + else: + args.extend(["-m", message]) + + self._run_git(*args) + logger.info("Amended HEAD commit%s", "" if message is None else f": {message[:50]}...") + return True + def remote_branch_exists(self, branch_name: str, remote: str = "origin") -> bool: """Check whether a branch exists on the given remote. diff --git a/tests/unit/workspace/test_git_ops_amend.py b/tests/unit/workspace/test_git_ops_amend.py new file mode 100644 index 000000000..00d9543e5 --- /dev/null +++ b/tests/unit/workspace/test_git_ops_amend.py @@ -0,0 +1,86 @@ +"""Tests for amending commits via GitOperations.""" + +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +from forge.workspace.git_ops import GitOperations +from forge.workspace.manager import Workspace + + +def _run_git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + text=True, + ) + + +def _init_repo_with_commit(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + _run_git(repo, "init") + _run_git(repo, "config", "user.email", "dev@example.com") + _run_git(repo, "config", "user.name", "Dev") + (repo / "file.txt").write_text("v1\n") + _run_git(repo, "add", "file.txt") + _run_git(repo, "commit", "-m", "openflow: fix drain pending messages") + return repo + + +def test_amend_commit_preserves_original_message(tmp_path, monkeypatch): + """Review fixes should fold into HEAD without creating a second commit.""" + repo = _init_repo_with_commit(tmp_path) + original = _run_git(repo, "rev-parse", "HEAD").stdout.strip() + original_msg = _run_git(repo, "log", "-1", "--format=%s").stdout.strip() + + (repo / "file.txt").write_text("v2\n") + + settings = MagicMock( + git_user_name="Forge Bot", + git_user_email="forge-bot@example.com", + ) + workspace = Workspace( + path=repo, + repo_name="org/repo", + branch_name="forge/test-123", + ticket_key="TEST-123", + ) + with patch("forge.workspace.git_ops.get_settings", return_value=settings): + git = GitOperations(workspace) + + empty_global_config = tmp_path / "empty-gitconfig" + empty_global_config.touch() + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(empty_global_config)) + + assert git.amend_commit() is True + + new_sha = _run_git(repo, "rev-parse", "HEAD").stdout.strip() + new_msg = _run_git(repo, "log", "-1", "--format=%s").stdout.strip() + count = int(_run_git(repo, "rev-list", "--count", "HEAD").stdout.strip()) + + assert new_sha != original + assert new_msg == original_msg == "openflow: fix drain pending messages" + assert count == 1 + assert (repo / "file.txt").read_text() == "v2\n" + + +def test_amend_commit_noop_when_clean(tmp_path): + """Amend with no changes and no message rewrite returns False.""" + repo = _init_repo_with_commit(tmp_path) + settings = MagicMock( + git_user_name="Forge Bot", + git_user_email="forge-bot@example.com", + ) + workspace = Workspace( + path=repo, + repo_name="org/repo", + branch_name="forge/test-123", + ticket_key="TEST-123", + ) + with patch("forge.workspace.git_ops.get_settings", return_value=settings): + git = GitOperations(workspace) + + assert git.amend_commit() is False