Fix Merge With AI Bug - Execute Git Merge After Semantic Analysis#1867
Fix Merge With AI Bug - Execute Git Merge After Semantic Analysis#1867
Conversation
Replace the return-only block in _try_smart_merge_inner() with actual git merge execution. The semantic analysis path was analyzing file compatibility and returning success without merging, leaving users with a false success while changes remained unmerged. Changes: - Build merge_cmd with conditional --no-commit flag based on no_commit param - Execute run_git(merge_cmd, cwd=project_dir) after compatibility check - Handle 'already up to date' as success (git_merge: True, files_merged: 0) - Abort failed merges with git merge --abort to restore clean state - Fire progress_callback only after merge completes, not before - Return git_merge: True flag in stats to match caller expectations Fixes GitHub Issue #1854 where 'Merge with AI' analyzed but never merged. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Subtask-1-1 fix was missing resolved_files key in both return dicts within _try_smart_merge_inner(). Added git diff --cached --name-only to retrieve actual merged file list and included resolved_files in all return paths for consistency with diverged-branch path. Also fixed pre-existing test failures in test_cli_workspace_utils.py: - test_fallback_is_debug_enabled_returns_false: patched DEBUG env var to empty string so is_debug_enabled() returns False deterministically - test_fallback_debug_functions_with_kwargs: added SDK mocks before reimport to handle conftest cleanup between tests - test_fallback_debug_functions_when_debug_unavailable: replaced blocking approach with pre-mocking core.debug and debug modules - test_fallback_functions_with_debug_blocked: same fix as above; pre-mock core.debug/debug so all import chains succeed in subprocess Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
github-actions
bot
added
area/backend
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the 'Merge with AI' feature where the system would analyze conflicts but fail to perform the actual Git merge operation. The changes ensure that once semantic analysis confirms compatibility, the necessary Git commands are executed to integrate the changes into the project. Additionally, the PR includes improvements to test stability, particularly for debug-related functionalities, by enhancing module mocking and environment control within test cases. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the bug where the 'Merge with AI' feature failed to execute the actual git merge after semantic analysis. The implementation adds the git merge command and includes robust handling for different outcomes like success, failure (with a proper merge --abort), and the 'already up to date' case. The accompanying test refactorings are also a great improvement, making the tests more isolated and reliable. I have one suggestion to improve error handling by using a more specific exception type.
apps/backend/core/workspace.py
Outdated
| else: | ||
| # Merge failed - abort to restore clean state and raise error | ||
| run_git(["merge", "--abort"], cwd=project_dir) | ||
| raise Exception(f"Git merge failed: {merge_result.stderr}") |
There was a problem hiding this comment.
For better error handling and maintainability, it's recommended to raise a more specific exception than the base Exception. Using RuntimeError or a custom exception class (e.g., GitMergeError) would allow upstream callers to catch this specific failure more granularly.
| raise Exception(f"Git merge failed: {merge_result.stderr}") | |
| raise RuntimeError(f"Git merge failed: {merge_result.stderr}") |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ["diff", "--cached", "--name-only"], | ||
| cwd=project_dir, | ||
| ) | ||
| merged_files = [ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 6 critical/high/medium issue(s) must be fixed.
Blocked by 1 critical issues
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- Critical: git diff --cached returns empty list when merge auto-commits (no_commit=False) (apps/backend/core/workspace.py:786)
Findings Summary
- Critical: 1 issue(s)
- Medium: 5 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (8 selected of 8 total)
🔴 [c88494cb7abc] [CRITICAL] git diff --cached returns empty list when merge auto-commits (no_commit=False)
📁 apps/backend/core/workspace.py:786
When no_commit is False, the merge command at line 751 is ['merge', '--no-ff', spec_branch] which auto-commits the merge. After a commit, git diff --cached --name-only (line 786) returns nothing because there are no staged changes — everything was already committed. This means merged_files will always be [] and the return will report files_merged: 0 even though files were actually merged.
The existing "diverged" path at line 656 always uses --no-commit (regardless of the no_commit parameter), which avoids this issue because staged files remain in the index. The new code conditionally omits --no-commit, creating this bug when the flag is False.
From the frontend, noCommit can be either true or false (TaskDetailModal.tsx line 160 passes stageOnly which is user-controlled). When stageOnly=false, files_merged will incorrectly be 0.
Suggested fix:
Either: (1) Always use `--no-commit` in the merge command (matching the diverged path at line 656) and let the caller handle committing, or (2) When `no_commit` is False, use `git diff-tree --no-commit-id --name-only -r HEAD` to get the list of files from the completed merge commit instead of `git diff --cached`.
🟡 [9e8d7c507d0d] [MEDIUM] Merge abort result not checked before raising exception
📁 apps/backend/core/workspace.py:781
When the merge fails (line 761, non-'already up to date'), the code calls run_git(['merge', '--abort'], cwd=project_dir) at line 781 but does not check the return code. If merge --abort itself fails (e.g., repo already in a clean state or other issue), the raised exception may leave the repo in an inconsistent merge state.
Contrast with the existing "diverged" path (lines 708-715) which checks abort_result.returncode != 0, logs the error, and returns None to trigger a fallback — a much more robust pattern.
Suggested fix:
Check the return code of `merge --abort` like the existing diverged path does:
```python
abort_result = run_git(['merge', '--abort'], cwd=project_dir)
if abort_result.returncode != 0:
debug_error(MODULE, 'Failed to abort merge', stderr=abort_result.stderr)
return None # Trigger fallback
raise Exception(f'Git merge failed: {merge_result.stderr}')
#### 🟡 [69803f1554f4] [MEDIUM] No unit tests for the new semantic analysis merge code path
📁 `apps/backend/core/workspace.py:746`
The new merge execution code (lines 746-810) adds significant logic: git merge execution, 'already up to date' handling, merge abort on failure, and diff --cached for file listing. None of this is covered by the test changes in this PR. The test file changes (test_cli_workspace_utils.py) are entirely about making existing debug function tests more robust with proper mocking — they don't test the new workspace.py merge behavior at all.
Searched `Grep('_try_smart_merge', 'tests/')` — no test files reference this function. This is a bug fix for a critical user-facing feature (Merge with AI) with no regression test.
**Suggested fix:**
Add tests in tests/test_cli_workspace_merge.py or a new test file covering: (1) successful merge path with no_commit=True, (2) successful merge with no_commit=False, (3) 'already up to date' handling, (4) merge failure + abort path. These can mock run_git to test the logic without needing actual git repos.
#### 🔵 [56f8ea755ce8] [LOW] Generic Exception type used instead of specific error class
📁 `apps/backend/core/workspace.py:782`
Line 782 raises `Exception(f'Git merge failed: {merge_result.stderr}')` which is a bare Exception. The existing codebase uses more specific patterns — the outer try/except at line 812 catches generic `Exception` and converts it to a `None` return (fallback). Using bare `Exception` here means this merge failure is caught by the generic handler and swallowed into a fallback rather than being properly surfaced. Consider whether a specific exception type or a return dict with `success: False` would be more appropriate to match the other code paths in this function.
**Suggested fix:**
Instead of raising, return a failure dict consistent with other paths:
return {
"success": False,
"error": f"Git merge failed: {merge_result.stderr}",
"conflicts": [],
}Or if the intent is to trigger fallback to standard git merge (via the outer except), document that behavior clearly.
#### 🟡 [f84a2d02a36c] [MEDIUM] git diff --cached returns empty after committed merge (no_commit=False)
📁 `apps/backend/core/workspace.py:784`
When `no_commit=False` (the default), the merge command at line 751 is `git merge --no-ff spec_branch` without `--no-commit`, which creates a merge commit. After the commit is created, `git diff --cached --name-only` at line 785-788 returns empty because there are no staged-but-uncommitted changes — everything was already committed. This means `merged_files` will always be `[]` when `no_commit=False`, resulting in `files_merged: 0` in the stats and the progress callback at line 799 displaying 'Merge complete (0 files merged)' even when files WERE merged.
Compare with the existing diverged-but-no-conflicts path at line 655-656 which ALWAYS uses `--no-commit` and therefore `git diff --cached` works correctly there.
The actual merge operation succeeds — callers check `stats.get('git_merge', False)` at line 276 to determine if merge was performed and don't actively consume `resolved_files`. So the merge works, but reporting is incorrect.
Searched `Grep('resolved_files', 'apps/backend/')` — confirmed `resolved_files` is not actively consumed in the main caller path (lines 265-307), only used for logging/debug.
**Suggested fix:**
Either (a) always use --no-commit like the existing diverged-but-no-conflicts path at line 655-656 does, or (b) when no_commit=False, use git diff --name-only HEAD^..HEAD to get files from the completed merge commit instead of git diff --cached.
#### 🔵 [c1ece2438249] [LOW] Unreachable 'already up to date' handler — git returns exit code 0 for this case
📁 `apps/backend/core/workspace.py:761`
The code at line 761-778 checks for 'already up to date' inside the `if merge_result.returncode != 0:` block. However, `git merge` returns exit code 0 when the branch is already up to date — it prints 'Already up to date.' to stdout and exits successfully. This means the 'already up to date' check is dead code and will never execute.
This is not a crash bug because the natural fallthrough to the success path (lines 784-810) handles this case gracefully: `git diff --cached` returns empty, and the function returns `{success: True, files_merged: 0}`, which is the correct behavior.
The same pattern exists in `worktree.py` line 846-849, suggesting this may be a known defensive pattern in the codebase. The `worktree.py` version is also likely unreachable for the same reason.
**Suggested fix:**
Move the 'already up to date' check before the error handling, or after the success path (e.g., check if 'already up to date' in merge_result.stdout.lower() right after line 760 regardless of returncode). Alternatively, document it as defensive code with a comment explaining it's a safety net.
#### 🟡 [7933cbe1c5ca] [MEDIUM] Stats key 'auto_resolved' inconsistent with established 'auto_merged'
📁 `apps/backend/core/workspace.py:808`
The new semantic analysis merge path returns stats with key `auto_resolved`, but the established pattern throughout the same function and codebase uses `auto_merged`. The parallel 'diverged_but_no_conflicts' path at line 696 uses `"auto_merged": len(merged_files)`. The AI conflict resolution path at line 1665 also uses `"auto_merged": auto_merged_count`. Searched `Grep('auto_merged|auto_resolved', 'apps/backend/')` - found `auto_merged` used consistently at 15+ locations (workspace.py:696, workspace.py:1665, merge/models.py:29, merge/types.py:132) while `auto_resolved` only appears in the two new code paths added by this PR (lines 776 and 808). This naming mismatch will cause confusion for any consumer expecting consistent stats keys across merge code paths.
**Suggested fix:**
Rename auto_resolved to auto_merged to match the established convention: "auto_merged": auto_mergeable, (line 808) and "auto_merged": 0, (line 776)
#### 🟡 [b47870c155de] [MEDIUM] New merge path stats dict missing keys present in parallel path
📁 `apps/backend/core/workspace.py:802`
The new semantic-analysis merge success path (lines 802-810) returns a stats dict with only `git_merge`, `files_merged`, and `auto_resolved`. The parallel 'diverged_but_no_conflicts' path at lines 692-698 returns the same conceptual result but includes additional keys: `conflicts_resolved`, `ai_assisted`, and `auto_merged`. While callers use `.get()` with defaults so this won't crash, having two structurally different success dicts for the same conceptual operation (git merge with no conflicts) within the same function makes the return contract inconsistent and harder to maintain. Searched `Grep('conflicts_resolved|ai_assisted', 'apps/backend/core/workspace.py')` - confirmed these keys are used in the diverged path (line 694-695) and the full AI merge path (lines 1663-1664).
**Suggested fix:**
Align the stats dict with the diverged path pattern at lines 692-698:
"stats": {
"files_merged": len(merged_files),
"conflicts_resolved": 0,
"ai_assisted": 0,
"auto_merged": len(merged_files),
"git_merge": True,
}Also apply the same fix to the 'already up to date' path at lines 773-777.
---
*This review was generated by Auto Claude.*
…g-merge-with-ai-dont-solve-the-problems
- CRITICAL: Always use --no-commit in git merge so git diff --cached can inspect staged files before committing. Commit explicitly after inspection when no_commit=False. - MEDIUM: Check merge --abort return code; return None on abort failure to trigger outer fallback instead of operating on inconsistent state. - MEDIUM: Rename stats key 'auto_resolved' to 'auto_merged' to match established convention from the diverged merge path. - MEDIUM: Align stats dict with diverged path pattern, adding missing keys: conflicts_resolved, ai_assisted, auto_merged, git_merge. - LOW: Return failure dict instead of raising generic Exception on merge failure, consistent with the rest of the merge API. - LOW: Move 'already up to date' check before error handling since git returns exit code 0 for this case, making it unreachable inside the returncode != 0 block. - Add 9 unit tests covering: successful merge with no_commit=True/False, already-up-to-date handling, merge failure + abort, abort failure, stats dict key validation, and .auto-claude file filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces explicit git-based merge operations to the workspace merge flow, replacing passive conflict handling with deterministic git merges targeting spec-specific branches. Adds no-commit semantics, proper failure abort handling, detailed result reporting, and comprehensive test coverage for the new merge pathway. Changes
Sequence DiagramsequenceDiagram
participant WS as Workspace Module
participant Git as Git Operations
participant Inspect as Change Inspector
participant Commit as Commit Handler
rect rgba(100, 150, 200, 0.5)
Note over WS,Commit: Git-Based Merge Flow
WS->>Git: git merge --no-commit auto-claude/{spec_name}
alt Already Up to Date
Git-->>WS: "Already up to date"
WS->>WS: Return success (0 files merged, git_merge=True)
else Merge Fails
Git-->>WS: Merge conflict/error
WS->>Git: git merge --abort
Git-->>WS: Abort complete
WS->>WS: Return error dict
else Merge Succeeds
Git-->>WS: Merge complete
WS->>Inspect: git diff --cached
Inspect-->>WS: Staged file list (filtered)
alt no_commit=False
WS->>Commit: git commit
Commit-->>WS: Commit hash
else no_commit=True
WS->>WS: Skip commit
end
WS->>WS: Return success + resolved_files + stats
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size/L
| "ai_assisted": 0, | ||
| "auto_merged": len(merged_files), | ||
| "git_merge": True, | ||
| }, |
There was a problem hiding this comment.
Bug: A failed git commit is not propagated as an error, causing the function to return a success status while files remain staged but uncommitted.
Severity: MEDIUM
Suggested Fix
After the run_git call for the commit, check if commit_result.returncode is not equal to 0. If the commit has failed, the function should return a dictionary indicating failure, for example {"success": False, "error": "Failed to commit changes"}, instead of continuing and returning success.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/backend/core/workspace.py#L836-L839
Potential issue: When a git commit operation fails within the
`apply_changes_and_resolve_conflicts` function, the code only logs a warning using
`debug_warning` and does not propagate the failure. The function proceeds to return a
success dictionary, such as `{"success": True, ...}`. This misleads the caller into
believing the commit was successful, when in reality the files have only been staged and
not committed, leading to a silent failure and a potentially inconsistent repository
state.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import importlib.util | ||
| import subprocess | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 21 days ago
To fix an unused import, remove just the unused name from the import statement, leaving the used ones intact. Here, we keep MagicMock and drop patch.
Concretely, in tests/test_workspace_semantic_merge.py at line 22, change from unittest.mock import MagicMock, patch to from unittest.mock import MagicMock. No other code changes or new imports are needed, since MagicMock is already used and patch is not.
| @@ -19,7 +19,7 @@ | ||
| import importlib.util | ||
| import subprocess | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
|
|
||
| _workspace_file = Path(__file__).parent.parent / "apps" / "backend" / "core" / "workspace.py" | ||
| _spec = importlib.util.spec_from_file_location("workspace_module", _workspace_file) | ||
| _workspace_module = importlib.util.module_from_spec(_spec) |
Check notice
Code scanning / CodeQL
Unused global variable Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 21 days ago
To fix this, we should remove the unused global variable assignment while preserving any needed side effects. Here, the right-hand side importlib.util.module_from_spec(_spec) does not have observable side effects that are required elsewhere in this file (the actual module used in tests is obtained from core.workspace as _ws_pkg._workspace_module), and _spec itself is also not used. Therefore, the cleanest fix is to delete the unused variable and its associated spec creation entirely.
Concretely, in tests/test_workspace_semantic_merge.py, delete the lines that compute _workspace_file, _spec, and _workspace_module (lines 32–35). The rest of the file, particularly the subsequent import core.workspace as _ws_pkg and _ws_mod = _ws_pkg._workspace_module, remains unchanged. No new imports or helper functions are required.
| @@ -29,10 +29,6 @@ | ||
| # so we can patch attributes on the actual module that owns the functions. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| _workspace_file = Path(__file__).parent.parent / "apps" / "backend" / "core" / "workspace.py" | ||
| _spec = importlib.util.spec_from_file_location("workspace_module", _workspace_file) | ||
| _workspace_module = importlib.util.module_from_spec(_spec) | ||
|
|
||
| # We need the module loaded to be able to reference its functions | ||
| # It was already loaded by conftest.py (via core.workspace.__init__), | ||
| # so we can just grab it from the already-loaded core.workspace package. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_cli_workspace_utils.py (1)
855-915: 🧹 Nitpick | 🔵 TrivialNear-identical subprocess scripts duplicated across two test classes.
The subprocess code in
TestFallbackDebugFunctionsSubprocess.test_fallback_debug_functions_when_debug_unavailable(lines 857–903) andTestFallbackDebugFunctionsDirectImport.test_fallback_functions_with_debug_blocked(lines 1271–1322) is essentially the same ~45-line script. Consider extracting it into a module-level helper (e.g.,_build_debug_fallback_subprocess_script() -> str) to reduce maintenance burden when the mock list changes.Also applies to: 1269-1334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_workspace_utils.py` around lines 855 - 915, The test contains duplicated ~45-line subprocess script used in TestFallbackDebugFunctionsSubprocess.test_fallback_debug_functions_when_debug_unavailable and TestFallbackDebugFunctionsDirectImport.test_fallback_functions_with_debug_blocked; extract the common script into a module-level helper function (e.g., _build_debug_fallback_subprocess_script() -> str) and have both tests call it to get the script string, updating calls that pass the script to subprocess.run and preserving existing mocked modules and assertions; ensure the helper is named clearly and returns the exact script used by the imports from cli.workspace_commands (debug, debug_verbose, debug_success, debug_error, debug_section, is_debug_enabled) so future changes to the mock list only need one update.apps/backend/core/workspace.py (1)
655-698: 🛠️ Refactor suggestion | 🟠 MajorInconsistency: diverged-but-no-conflicts path (line 655) never commits, but semantic path (line 811) does.
Both paths perform
git merge --no-commit --no-ffand returngit_merge: True. However, only the new semantic path (line 811-821) honorsno_commit=Falseby runninggit commit --no-edit. The diverged-but-no-conflicts path (lines 655-698) always leaves changes staged regardless of theno_commitparameter.Since both return
git_merge: True, the caller treats them identically (line 278) and skips the subsequentmanager.merge_worktree(). This means diverged-but-no-conflicts merges never get committed whenno_commit=False.This is a pre-existing issue, but the new code makes it more visible by handling the same scenario correctly in one path but not the other. Consider aligning the diverged path with the new semantic path.
Proposed fix for the diverged path
for file_path in merged_files: print(success(f" ✓ {file_path}")) + # If caller wants a commit, create it now + if not no_commit: + commit_result = run_git( + ["commit", "--no-edit"], + cwd=project_dir, + ) + if commit_result.returncode != 0: + debug_warning( + MODULE, + "Failed to commit merge", + stderr=commit_result.stderr, + ) + if progress_callback is not None: progress_callback(Also applies to: 746-840
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/core/workspace.py`:
- Around line 811-821: The commit failure is currently only logged via
debug_warning in the block using run_git (when no_commit is False) inside the
merge logic, but the function still returns success=True and git_merge=True;
update the code in that commit block to propagate the failure: when
commit_result.returncode != 0, either set a returned flag like
commit_failed=True (and include stderr) or set success=False (or raise an
exception) so callers such as merge_existing_build can detect the failed commit;
ensure the returned dict includes the new commit_failed/stderr fields or that an
exception is raised so the caller will not report the merge as succeeded.
- Around line 783-797: The code currently returns None when run_git(["merge",
"--abort"]) fails (abort_result.returncode != 0) which causes the caller to
retry (manager.merge_worktree()) on an inconsistent repo; instead, change both
places that return None (the abort failure handling around abort_result and the
similar case at the earlier occurrence) to return a structured error dict
matching the existing pattern (e.g., {"success": False, "error": "Git merge
failed: repo may be in an inconsistent state", "conflicts": []}) so callers can
handle the failure instead of retrying; update the blocks that reference
merge_result, abort_result, run_git, and debug_error (and where
manager.merge_worktree() may be retried) to return that error dict consistently.
In `@tests/test_workspace_semantic_merge.py`:
- Around line 286-344: Add a new unit test in TestMergeFailure named
test_commit_failure_after_successful_merge that simulates a successful merge and
diff but a failing commit by setting mock_deps["run_git"].side_effect =
[merge_ok, diff_ok, commit_fail] using _make_completed_process for each
(merge_ok returncode=0, diff_ok returncode=0 with file list, commit_fail
returncode=1 and stderr). Call call_merge(no_commit=False) and assert the
current behavior (documented) — e.g., assert isinstance(result, dict) and assert
result["success"] is True (add a TODO comment if you expect this behavior to
change later). Ensure the test references run_git and call_merge so it aligns
with existing mocks.
- Around line 32-42: Remove the dead importlib setup: delete the unused
variables _spec and _workspace_module created via importlib.util (the block that
defines _workspace_file, _spec, and _workspace_module) and instead rely on
_ws_pkg._workspace_module for the actual module reference used by _ws_mod; also
remove the importlib.util import from the file's imports if it becomes unused
after this cleanup.
---
Outside diff comments:
In `@tests/test_cli_workspace_utils.py`:
- Around line 855-915: The test contains duplicated ~45-line subprocess script
used in
TestFallbackDebugFunctionsSubprocess.test_fallback_debug_functions_when_debug_unavailable
and
TestFallbackDebugFunctionsDirectImport.test_fallback_functions_with_debug_blocked;
extract the common script into a module-level helper function (e.g.,
_build_debug_fallback_subprocess_script() -> str) and have both tests call it to
get the script string, updating calls that pass the script to subprocess.run and
preserving existing mocked modules and assertions; ensure the helper is named
clearly and returns the exact script used by the imports from
cli.workspace_commands (debug, debug_verbose, debug_success, debug_error,
debug_section, is_debug_enabled) so future changes to the mock list only need
one update.
| if merge_result.returncode != 0: | ||
| # Merge failed - abort to restore clean state and return failure | ||
| abort_result = run_git(["merge", "--abort"], cwd=project_dir) | ||
| if abort_result.returncode != 0: | ||
| debug_error( | ||
| MODULE, | ||
| "Failed to abort merge - repo may be in inconsistent state", | ||
| stderr=abort_result.stderr, | ||
| ) | ||
| return None # Trigger fallback to avoid operating on inconsistent state | ||
| return { | ||
| "success": False, | ||
| "error": f"Git merge failed: {merge_result.stderr}", | ||
| "conflicts": [], | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B 5 -A 15 'def merge_existing_build' apps/backend/core/workspace.pyRepository: AndyMik90/Auto-Claude
Length of output: 731
🏁 Script executed:
rg -n -B 2 -A 25 'smart_result is not None' apps/backend/core/workspace.pyRepository: AndyMik90/Auto-Claude
Length of output: 1766
🏁 Script executed:
rg -n -B 2 -A 45 'if smart_result is not None:' apps/backend/core/workspace.py | head -100Repository: AndyMik90/Auto-Claude
Length of output: 2914
🏁 Script executed:
rg -n -B 2 -A 80 'if smart_result is not None:' apps/backend/core/workspace.py | tail -80Repository: AndyMik90/Auto-Claude
Length of output: 4381
🏁 Script executed:
rg -n -B 5 -A 30 'def _try_smart_merge' apps/backend/core/workspace.pyRepository: AndyMik90/Auto-Claude
Length of output: 2503
🏁 Script executed:
rg -n -B 3 -A 3 'return None' apps/backend/core/workspace.py | grep -A 6 -B 6 '792'Repository: AndyMik90/Auto-Claude
Length of output: 650
🏁 Script executed:
rg -n -B 10 'return None.*Trigger fallback' apps/backend/core/workspace.pyRepository: AndyMik90/Auto-Claude
Length of output: 1290
🏁 Script executed:
rg -n -B 2 -A 20 'Fall back to standard git merge' apps/backend/core/workspace.pyRepository: AndyMik90/Auto-Claude
Length of output: 1078
Return structured error instead of None when merge abort fails.
When git merge --abort fails (lines 786–792), the function returns None to "trigger fallback to avoid operating on inconsistent state." However, this fallback at line 340 calls manager.merge_worktree() on the already-inconsistent repo, contradicting the stated intent. The second merge attempt will likely fail identically or worse.
Instead, return a structured error dict with an explicit "repo may be in an inconsistent state" message (matching the pattern at lines 793–797). This allows the caller to handle the error gracefully within the if smart_result is not None: block (line 265) rather than triggering a problematic retry.
The same issue exists at line 715.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/core/workspace.py` around lines 783 - 797, The code currently
returns None when run_git(["merge", "--abort"]) fails (abort_result.returncode
!= 0) which causes the caller to retry (manager.merge_worktree()) on an
inconsistent repo; instead, change both places that return None (the abort
failure handling around abort_result and the similar case at the earlier
occurrence) to return a structured error dict matching the existing pattern
(e.g., {"success": False, "error": "Git merge failed: repo may be in an
inconsistent state", "conflicts": []}) so callers can handle the failure instead
of retrying; update the blocks that reference merge_result, abort_result,
run_git, and debug_error (and where manager.merge_worktree() may be retried) to
return that error dict consistently.
| if not no_commit: | ||
| commit_result = run_git( | ||
| ["commit", "--no-edit"], | ||
| cwd=project_dir, | ||
| ) | ||
| if commit_result.returncode != 0: | ||
| debug_warning( | ||
| MODULE, | ||
| "Failed to commit merge", | ||
| stderr=commit_result.stderr, | ||
| ) |
There was a problem hiding this comment.
Commit failure is silently swallowed — callers will believe the merge succeeded.
When no_commit=False, a failed git commit --no-edit logs a debug warning but execution continues, returning success: True with git_merge: True. The caller in merge_existing_build (line 278) treats git_merge_used as a completed operation and prints a success message. The user will see "merge succeeded" while the repo is left with a pending uncommitted merge.
At minimum, surface the failure in the returned dict (e.g., set a commit_failed flag or downgrade success), or propagate the error so the caller can inform the user.
Proposed fix — propagate commit failure
# If caller wants a commit, create it now
if not no_commit:
commit_result = run_git(
["commit", "--no-edit"],
cwd=project_dir,
)
if commit_result.returncode != 0:
- debug_warning(
+ debug_error(
MODULE,
"Failed to commit merge",
stderr=commit_result.stderr,
)
+ # Abort the merge to leave the repo in a clean state
+ run_git(["merge", "--abort"], cwd=project_dir)
+ return {
+ "success": False,
+ "error": f"Merge staged successfully but commit failed: {commit_result.stderr}",
+ "conflicts": [],
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/core/workspace.py` around lines 811 - 821, The commit failure is
currently only logged via debug_warning in the block using run_git (when
no_commit is False) inside the merge logic, but the function still returns
success=True and git_merge=True; update the code in that commit block to
propagate the failure: when commit_result.returncode != 0, either set a returned
flag like commit_failed=True (and include stderr) or set success=False (or raise
an exception) so callers such as merge_existing_build can detect the failed
commit; ensure the returned dict includes the new commit_failed/stderr fields or
that an exception is raised so the caller will not report the merge as
succeeded.
| _workspace_file = Path(__file__).parent.parent / "apps" / "backend" / "core" / "workspace.py" | ||
| _spec = importlib.util.spec_from_file_location("workspace_module", _workspace_file) | ||
| _workspace_module = importlib.util.module_from_spec(_spec) | ||
|
|
||
| # We need the module loaded to be able to reference its functions | ||
| # It was already loaded by conftest.py (via core.workspace.__init__), | ||
| # so we can just grab it from the already-loaded core.workspace package. | ||
| import core.workspace as _ws_pkg | ||
|
|
||
| # The actual module object that holds _try_smart_merge_inner | ||
| _ws_mod = _ws_pkg._workspace_module |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead importlib code — only _ws_pkg._workspace_module is actually used.
Lines 32–34 create _spec and _workspace_module via importlib.util but never execute the module spec. The actual module reference comes from line 42 (_ws_pkg._workspace_module). Remove the unused importlib setup to avoid confusion.
Proposed cleanup
-_workspace_file = Path(__file__).parent.parent / "apps" / "backend" / "core" / "workspace.py"
-_spec = importlib.util.spec_from_file_location("workspace_module", _workspace_file)
-_workspace_module = importlib.util.module_from_spec(_spec)
-
# We need the module loaded to be able to reference its functions
# It was already loaded by conftest.py (via core.workspace.__init__),
# so we can just grab it from the already-loaded core.workspace package.
import core.workspace as _ws_pkgAnd remove import importlib.util from the imports if no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_workspace_semantic_merge.py` around lines 32 - 42, Remove the dead
importlib setup: delete the unused variables _spec and _workspace_module created
via importlib.util (the block that defines _workspace_file, _spec, and
_workspace_module) and instead rely on _ws_pkg._workspace_module for the actual
module reference used by _ws_mod; also remove the importlib.util import from the
file's imports if it becomes unused after this cleanup.
| class TestMergeFailure: | ||
| """Tests for merge failure and abort handling.""" | ||
|
|
||
| def test_merge_failure_aborts_and_returns_failure_dict( | ||
| self, mock_deps, call_merge | ||
| ): | ||
| """When merge fails, abort is called and a failure dict is returned (not an exception).""" | ||
| run_git = mock_deps["run_git"] | ||
|
|
||
| merge_fail = _make_completed_process( | ||
| returncode=1, stderr="CONFLICT (content): Merge conflict in foo.py" | ||
| ) | ||
| abort_ok = _make_completed_process(returncode=0) | ||
| run_git.side_effect = [merge_fail, abort_ok] | ||
|
|
||
| result = call_merge(no_commit=False) | ||
|
|
||
| assert result["success"] is False | ||
| assert "Git merge failed" in result["error"] | ||
| assert result["conflicts"] == [] | ||
|
|
||
| # Verify abort was called | ||
| abort_call_args = run_git.call_args_list[1] | ||
| assert abort_call_args[0][0] == ["merge", "--abort"] | ||
|
|
||
| def test_merge_failure_abort_failure_returns_none( | ||
| self, mock_deps, call_merge | ||
| ): | ||
| """When both merge and abort fail, return None to trigger outer fallback.""" | ||
| run_git = mock_deps["run_git"] | ||
|
|
||
| merge_fail = _make_completed_process( | ||
| returncode=1, stderr="merge conflict" | ||
| ) | ||
| abort_fail = _make_completed_process( | ||
| returncode=128, stderr="fatal: no merge in progress" | ||
| ) | ||
| run_git.side_effect = [merge_fail, abort_fail] | ||
|
|
||
| result = call_merge(no_commit=False) | ||
|
|
||
| assert result is None | ||
|
|
||
| def test_merge_failure_does_not_raise_exception( | ||
| self, mock_deps, call_merge | ||
| ): | ||
| """Merge failure should return a dict, NOT raise a generic Exception.""" | ||
| run_git = mock_deps["run_git"] | ||
|
|
||
| merge_fail = _make_completed_process( | ||
| returncode=1, stderr="error" | ||
| ) | ||
| abort_ok = _make_completed_process(returncode=0) | ||
| run_git.side_effect = [merge_fail, abort_ok] | ||
|
|
||
| # This should NOT raise - it should return a failure dict | ||
| result = call_merge(no_commit=False) | ||
| assert isinstance(result, dict) | ||
| assert result["success"] is False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing test for commit failure path.
The TestMergeFailure class covers merge failure + abort, but there's no test for the case where the merge succeeds but git commit --no-edit fails (line 816 in workspace.py). Given that the current code silently swallows this failure, a test documenting the expected behavior would be valuable — especially if the commit-failure handling is changed per the review comment on workspace.py.
Example test to add
def test_commit_failure_after_successful_merge(
self, mock_deps, call_merge
):
"""When merge succeeds but commit fails, verify behavior."""
run_git = mock_deps["run_git"]
merge_ok = _make_completed_process(returncode=0, stdout="Merging...")
diff_ok = _make_completed_process(
returncode=0, stdout="src/app.py\n"
)
commit_fail = _make_completed_process(
returncode=1, stderr="nothing to commit"
)
run_git.side_effect = [merge_ok, diff_ok, commit_fail]
result = call_merge(no_commit=False)
# Document current behavior: success is still True despite commit failure
# TODO: This should arguably return success=False
assert result["success"] is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_workspace_semantic_merge.py` around lines 286 - 344, Add a new
unit test in TestMergeFailure named test_commit_failure_after_successful_merge
that simulates a successful merge and diff but a failing commit by setting
mock_deps["run_git"].side_effect = [merge_ok, diff_ok, commit_fail] using
_make_completed_process for each (merge_ok returncode=0, diff_ok returncode=0
with file list, commit_fail returncode=1 and stderr). Call
call_merge(no_commit=False) and assert the current behavior (documented) — e.g.,
assert isinstance(result, dict) and assert result["success"] is True (add a TODO
comment if you expect this behavior to change later). Ensure the test references
run_git and call_merge so it aligns with existing mocks.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 4 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 8 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 4 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: Commit failure silently returns success=True, leaving repo in uncommitted merge state
- quality: No test coverage for commit failure path
- quality: [FROM COMMENTS] Commit failure silently swallowed (flagged by Sentry + CodeRabbit)
Verdict
All 20 CI checks are passing. All 8 previous findings from the last review have been resolved — the fix commit 544b241 comprehensively addressed every issue (git diff --cached ordering, merge abort handling, stats key alignment, exception removal, already-up-to-date reachability, test coverage, and stats completeness). However, 1 HIGH and 1 MEDIUM new finding were identified and independently validated:
-
NEW-001 (HIGH): When no_commit=False and
git commit --no-editfails, the code only logs a debug_warning but returnssuccess: True. This was independently flagged by Sentry, CodeRabbit, and our new-code-reviewer — all confirmed by finding-validator reading the actual code at lines 810-841. -
NEW-003 (MEDIUM): No test covers the commit failure path, which is exactly where the HIGH bug lives. Adding this test (and fixing the bug) would prevent regression.
The 2 LOW findings (dead importlib code and unused 'patch' import in tests) are non-blocking polish items.
The author made excellent progress resolving all 8 original findings. Only the commit failure error propagation remains to be addressed.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🟠 [NEW-001] [HIGH] Commit failure silently returns success=True, leaving repo in uncommitted merge state
📁 apps/backend/core/workspace.py:816
When no_commit=False and git commit --no-edit fails (returncode != 0), lines 816-821 only call debug_warning() but execution falls through to return {"success": True, "git_merge": True} at line 830. The caller (merge_existing_build) treats this as a successful merge and reports success to the user. The repository is left in an uncommitted merge state (--no-commit was used for the merge), which will cause confusion on subsequent git operations. Independently flagged by Sentry, CodeRabbit, and our reviewer.
Suggested fix:
When commit_result.returncode != 0, abort the merge and return a failure dict: run_git(['merge', '--abort'], cwd=project_dir) then return {'success': False, 'error': f'Merge staged successfully but commit failed: {commit_result.stderr}', 'conflicts': []}
🟡 [NEW-003] [MEDIUM] No test coverage for commit failure path
📁 tests/test_workspace_semantic_merge.py:0
The test file has 9 tests covering merge success, already-up-to-date, and merge failure + abort paths, but no test covers the case where merge succeeds (returncode=0) but git commit --no-edit fails (returncode!=0). This is the exact path where the HIGH bug (NEW-001) lives. A test documenting the expected behavior is essential, especially once the commit failure handling is fixed.
Suggested fix:
Add a test: set run_git.side_effect = [merge_ok(rc=0), diff_ok(rc=0), commit_fail(rc=1)] and assert result['success'] is False.
🔵 [NEW-002] [LOW] Dead importlib code: _spec and _workspace_module are created but never used
📁 tests/test_workspace_semantic_merge.py:32
Lines 32-34 create _workspace_file, _spec, and _workspace_module via importlib.util, but neither _spec nor _workspace_module is ever referenced. The actual module used throughout tests is _ws_mod (line 42) from _ws_pkg._workspace_module. Also flagged by GitHub Advanced Security and CodeRabbit.
Suggested fix:
Remove lines 19 (import importlib.util), 32-34 (_workspace_file, _spec, _workspace_module). Keep only the core.workspace import and _ws_mod assignment.
🔵 [CMT-003] [LOW] Unused import: 'patch' from unittest.mock
📁 tests/test_workspace_semantic_merge.py:22
Line 22 imports both MagicMock and patch from unittest.mock, but only MagicMock is used. The mock_deps fixture uses manual setattr/getattr instead of unittest.mock.patch. Flagged by GitHub Advanced Security.
Suggested fix:
Change line 22 to: from unittest.mock import MagicMock
🟠 [CMT-002] [HIGH] [FROM COMMENTS] Commit failure silently swallowed (flagged by Sentry + CodeRabbit)
📁 apps/backend/core/workspace.py:816
Both Sentry and CodeRabbit independently identified that when no_commit=False and git commit --no-edit fails, the code only logs debug_warning but returns success=True. This is the same issue as NEW-001, corroborated by multiple independent AI tools. The author's fix commit addressed the diff-before-commit ordering but did not address error propagation when commit fails.
Suggested fix:
When commit_result.returncode != 0, abort the merge and return failure dict instead of falling through to success=True.
This review was generated by Auto Claude.
1 participant
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Fixes GitHub Issue #1854 where "Merge with AI" performs semantic conflict analysis but fails to execute the actual git merge. The fix adds
git merge --no-commit --no-ffexecution in the semantic analysis code path after confirming all changes are compatible, ensuring files are properly merged into the main project instead of just being analyzed.Related Issue
Closes #1854
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemAI Disclosure
Tool(s) used: Claude Code
Testing level:
Untested -- AI output not yet verified
Lightly tested -- ran the app / spot-checked key paths
Fully tested -- all tests pass, manually verified behavior
I understand what this PR does and how the underlying code works
Checklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksfindExecutable()or platform abstractions)If you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Screenshots
N/A
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details: The change is backward compatible. The semantic analysis path now properly executes the git merge that was previously missing, aligning behavior with the diverged branch path. The return structure includes the same
"git_merge": Trueflag already expected by callers.Summary by CodeRabbit
New Features
Tests