Rectify: merge_worktree Silently Merges Into Wrong Branch#705
Conversation
Tests 1.1-1.4 verify perform_merge rejects branch mismatches between main_repo and base_branch, and that into_branch/cwd reflect verified state. Test 1.6 verifies create_unique_branch reports base_ref. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduces GitMergeTarget dataclass and _verify_merge_target() to ensure main_repo's checked-out branch matches base_branch before executing the merge. Return value into_branch now derives from verified state instead of echoing caller input. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Query git branch --show-current before checkout -b to report the starting branch/commit in the return value. Reports DETACHED_HEAD when HEAD is detached, making the starting point visible to callers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update _push_full_success_sequence to accept base_branch param and use it in worktree list mock. Add step 7.5 (branch --show-current) mock to all success-path tests. Align porcelain branch output with base_branch parameter in all test_tools_git.py tests. Add branch --show-current mock to create_unique_branch tests for HEAD state reporting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lock the invariant that into_branch reflects verified state and that merge commands execute with the correct cwd (main_repo path). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ver init tests Three tests missed the step 7.5 mock (git branch --show-current on main_repo) added by the branch verification feature. Also fixed test_server_init worktree list mock to use refs/heads/dev instead of refs/heads/main to match base_branch="dev". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested (9 actionable warnings, 1 decision finding, 4 info)
| ), | ||
| "failed_step": MergeFailedStep.MERGE, | ||
| "state": MergeState.WORKTREE_INTACT, | ||
| "worktree_path": "", # filled in by caller |
There was a problem hiding this comment.
[warning] slop: Inline comment # filled in by caller documents a manual mutation convention on a returned dict. The worktree_path should either be populated here or the function signature redesigned.
There was a problem hiding this comment.
Investigated — this is intentional. The worktree_path is not in scope inside _verify_merge_target (it is only known to perform_merge). The caller at L401-403 immediately populates it via target_or_err['worktree_path'] = worktree_path. This is a deliberate API boundary, not a slop pattern.
| return result.returncode, result.stdout, result.stderr | ||
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) |
There was a problem hiding this comment.
[warning] cohesion: GitMergeTarget fields path and branch are generic names that lose semantic signal compared to main_repo and base_branch they replace at call sites (L408, L491). Consider main_repo_path and verified_branch for clarity.
There was a problem hiding this comment.
Valid observation — flagged for design decision. The fields path and branch hold verified values after confirmation. The docstring already conveys the 'verified' semantics, but renaming to main_repo_path/verified_branch adds specificity at the cost of verbosity. This is a naming style question requiring human input.
| main_repo: str, | ||
| expected_branch: str, | ||
| runner: SubprocessRunner, | ||
| ) -> GitMergeTarget | dict[str, Any]: |
There was a problem hiding this comment.
[info] defense: Return type GitMergeTarget | dict[str, Any] is a loosely typed union. A dedicated error dataclass would provide a typed boundary and prevent reliance on dict key conventions invisible to static analysis.
There was a problem hiding this comment.
Valid observation — flagged for design decision. GitMergeTarget | dict[str, Any] is consistent with the surrounding codebase where all perform_merge error returns are dict[str, Any]. A dedicated error dataclass would tighten the type boundary but requires broader refactoring of the entire perform_merge error surface beyond this PR's scope.
| "worktree_path": worktree_path, | ||
| } | ||
|
|
||
| # 7.5 Verify main_repo is on base_branch |
There was a problem hiding this comment.
[info] slop: Step comment # 7.5 Verify main_repo is on base_branch is a plan annotation. Step numbering in production code is fragile if steps are reordered.
| assert wt in remove_calls | ||
|
|
||
|
|
||
| class TestPerformMergeTargetBranchVerification: |
There was a problem hiding this comment.
[warning] tests: test_succeeds_when_main_repo_branch_matches, test_merge_command_cwd_is_main_repo, and test_into_branch_reflects_verified_state all set up identical mock sequences. The CWD and into_branch assertions could be consolidated into a single success-path test.
| assert "impl-test" in sidecar_calls | ||
| # Verify merge command cwd is main_repo (/repo) | ||
| merge_call = next( | ||
| args for args in runner.call_args_list if len(args[0]) > 1 and args[0][1] == "merge" |
There was a problem hiding this comment.
[warning] tests: Same fragile next() pattern at L581-L584. If no merge call exists, StopIteration with no diagnostic. Use next(..., None) + assert.
| assert result["cleanup_succeeded"] is True | ||
| assert result["worktree_removed"] is True | ||
| assert result["branch_deleted"] is True | ||
| # Verify merge command cwd is the main_repo (/repo) |
There was a problem hiding this comment.
[warning] tests: Fragile next() for merge call lookup (L185-L191). StopIteration on no match gives no diagnostic message. Use next(..., None) + assert.
| branch: str | ||
|
|
||
|
|
||
| async def _verify_merge_target( |
There was a problem hiding this comment.
[warning] cohesion: GitMergeTarget is only constructed by private _verify_merge_target but used as part of perform_merge return chain. If future callers need the type without verification, they must import a type only created by a private function.
There was a problem hiding this comment.
Investigated — this is intentional. GitMergeTarget is private to this module (underscore-prefixed constructor _verify_merge_target, no public export). It functions as a typed proof-of-verification value within git.py. The concern about future callers needing the type without verification is speculative and does not reflect any actual issue in the current codebase — verification and construction are deliberately coupled.
…y_merge_target When git branch --show-current fails (rc != 0), the error message now reports the command failure directly instead of reporting an empty/garbled branch name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…message fragment Replace loose "main" in error / "dev" in error checks with a precise "is on branch 'main', expected 'dev'" assertion that pins the check to the exact _verify_merge_target error path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
perform_merge()executes 13+ precondition checks but has zero verification at the criticalstep 7 -> 8 transition: after discovering
main_repoviagit worktree list --porcelain, itimmediately runs
git mergeagainst whatever branchmain_repohas checked out. Thebase_branchparameter is used for protection checks, rebase targeting, and the return valuelabel, but never for verifying the actual merge target. The return value
"into_branch": base_branchechoes the caller's input, masking any mismatch.The fix introduces a Verified Target pattern: a
GitMergeTargetdataclassthat can only be constructed by querying git, making it structurally impossible to skip branch
verification. Return values are derived from verified state, not echoed input. Test
infrastructure is hardened to make branch/cwd mismatches instantly visible.
Architecture Impact
Process Flow Diagram
%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 50, 'curve': 'basis'}}}%% flowchart TB %% CLASS DEFINITIONS %% classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff; classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff; classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff; classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff; %% TERMINALS %% START(["START<br/>━━━━━━━━━━<br/>● merge_worktree tool<br/>tools_git.py"]) COMPLETE(["COMPLETE<br/>━━━━━━━━━━<br/>merge_succeeded: true<br/>into_branch: target.branch"]) ERROR(["ERROR<br/>━━━━━━━━━━<br/>error + failed_step<br/>+ state + worktree_path"]) subgraph Validation ["Phase 1: Validation Gates (Steps 1–3d)"] direction TB V1{"Step 1<br/>━━━━━━━━━━<br/>Path exists?"} V1P{"Step 1.5<br/>━━━━━━━━━━<br/>Protected<br/>branch?"} V2{"Step 2<br/>━━━━━━━━━━<br/>Git worktree?<br/>/worktrees/ in<br/>git-dir"} V3{"Step 3<br/>━━━━━━━━━━<br/>Branch name<br/>detected?"} V3B{"Pre-cond<br/>━━━━━━━━━━<br/>Active rebase<br/>in progress?"} V3C["Step 3c<br/>━━━━━━━━━━<br/>Strip tracked<br/>generated files"] V3D{"Step 3d<br/>━━━━━━━━━━<br/>Dirty tree?<br/>uncommitted<br/>non-generated"} end subgraph TestGate ["Phase 2: Pre-Merge Test Gate (Step 4)"] direction TB T1{"Step 4<br/>━━━━━━━━━━<br/>test_gate_on_merge?<br/>Tests pass?"} end subgraph FetchRebase ["Phase 3: Fetch, Pre-Rebase Checks & Rebase (Steps 5–6.5)"] direction TB F1["Step 5<br/>━━━━━━━━━━<br/>git fetch origin"] F1D{"Fetch OK?"} F2{"Step 5.5<br/>━━━━━━━━━━<br/>Remote tracking<br/>ref exists?"} F3{"Step 5.6<br/>━━━━━━━━━━<br/>Merge commits<br/>in history?"} R1["Step 6<br/>━━━━━━━━━━<br/>git rebase<br/>--autostash<br/>origin/base"] R1D{"Rebase OK?"} R2{"Step 6.5<br/>━━━━━━━━━━<br/>Post-rebase<br/>tests pass?"} end subgraph TargetVerify ["Phase 4: Target Discovery & ● Verification (Steps 7–7.5)"] direction TB M0["Step 7<br/>━━━━━━━━━━<br/>git worktree list<br/>discover main repo"] M0D{"Main repo<br/>found?"} M1["● Step 7.5<br/>━━━━━━━━━━<br/>● _verify_merge_target<br/>● git branch --show-current<br/>● on main_repo"] M1D{"● Current branch<br/>━━━━━━━━━━<br/>● == base_branch?"} end subgraph MergeCleanup ["Phase 5: Merge, Guard & Cleanup (Steps 8–9)"] direction TB M2["Step 8<br/>━━━━━━━━━━<br/>git merge --no-edit<br/>worktree_branch<br/>into target.path"] M2D{"Merge OK?"} M3{"Step 8.5<br/>━━━━━━━━━━<br/>● Editable install<br/>guard — scan<br/>site-packages"} M4["Step 9<br/>━━━━━━━━━━<br/>Cleanup:<br/>remove worktree<br/>delete branch"] end %% FLOW %% START --> V1 V1 -->|"no"| ERROR V1 -->|"yes"| V1P V1P -->|"protected"| ERROR V1P -->|"allowed"| V2 V2 -->|"not worktree"| ERROR V2 -->|"valid"| V3 V3 -->|"fail / detached"| ERROR V3 -->|"branch found"| V3B V3B -->|"rebase active"| ERROR V3B -->|"clean"| V3C V3C --> V3D V3D -->|"dirty"| ERROR V3D -->|"clean"| T1 T1 -->|"fail"| ERROR T1 -->|"pass / skip"| F1 F1 --> F1D F1D -->|"fail"| ERROR F1D -->|"ok"| F2 F2 -->|"no ref"| ERROR F2 -->|"exists"| F3 F3 -->|"merge commits"| ERROR F3 -->|"linear"| R1 R1 --> R1D R1D -->|"fail → abort"| ERROR R1D -->|"ok"| R2 R2 -->|"fail"| ERROR R2 -->|"pass / skip"| M0 M0 --> M0D M0D -->|"not found"| ERROR M0D -->|"found"| M1 M1 --> M1D M1D -->|"● mismatch"| ERROR M1D -->|"● match"| M2 M2 --> M2D M2D -->|"fail → abort"| ERROR M2D -->|"ok"| M3 M3 -->|"poisoned"| ERROR M3 -->|"clean"| M4 M4 --> COMPLETE %% CLASS ASSIGNMENTS %% class START,COMPLETE,ERROR terminal; class V1,V1P,V2,V3,V3B,V3D detector; class V3C handler; class T1 detector; class F1 handler; class F1D,F2,F3 stateNode; class R1 handler; class R1D,R2 stateNode; class M0 handler; class M0D stateNode; class M1 newComponent; class M1D newComponent; class M2 handler; class M2D stateNode; class M3 detector; class M4 phase;Error/Resilience Diagram
%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 50, 'curve': 'basis'}}}%% flowchart TB %% CLASS DEFINITIONS %% classDef cli fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff; classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff; classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff; classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000; classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; %% ENTRY %% START(["merge_worktree<br/>MCP tool call"]) subgraph HookLayer ["HOOK LAYER — Pre-Call Interception"] HOOK_BRANCH["branch_protection_guard<br/>━━━━━━━━━━<br/>PreToolUse hook<br/>Denies protected branches"] HOOK_DENY(["DENIED<br/>━━━━━━━━━━<br/>Tool call rejected<br/>before Python runs"]) end subgraph ToolLayer ["● MCP TOOL LAYER — tools_git.py"] GATE{"Kitchen<br/>gate<br/>open?"} GATE_ERR(["GATE ERROR<br/>━━━━━━━━━━<br/>subtype: gate_error<br/>is_error: true"]) TOOL_WRAP["@track_response_size<br/>━━━━━━━━━━<br/>Exception safety net<br/>catches unhandled Exception"] TOOL_EXC(["TOOL EXCEPTION<br/>━━━━━━━━━━<br/>subtype: tool_exception<br/>exit_code: -1"]) end subgraph PreMergeGates ["● VALIDATION GATES — git.py (Fail-Fast)"] direction TB G1["Gate 1: Path Exists<br/>━━━━━━━━━━<br/>os.path.isdir"] G1_5["Gate 1.5: Protected Branch<br/>━━━━━━━━━━<br/>is_protected_branch"] G2["Gate 2: Worktree Identity<br/>━━━━━━━━━━<br/>/worktrees/ in git-dir"] G3["Gate 3: Branch Detection<br/>━━━━━━━━━━<br/>git branch --show-current<br/>Detached HEAD check"] G3B["Gate 3b: Active Rebase<br/>━━━━━━━━━━<br/>rebase-merge/ or<br/>rebase-apply/ dir"] G3C["Gate 3c: Generated Files<br/>━━━━━━━━━━<br/>git rm --cached + commit"] G3D["Gate 3d: Dirty Tree<br/>━━━━━━━━━━<br/>git status --porcelain<br/>filter GENERATED_FILES"] G4["Gate 4: Pre-Rebase Tests<br/>━━━━━━━━━━<br/>tester.run if enabled"] end subgraph RebasePhase ["● REBASE PHASE — git.py"] direction TB G5["Gate 5: Fetch<br/>━━━━━━━━━━<br/>git fetch origin<br/>60s timeout"] G5_5["Gate 5.5: Remote Ref<br/>━━━━━━━━━━<br/>refs/remotes/origin/base<br/>must exist"] G5_6["Gate 5.6: Merge Commits<br/>━━━━━━━━━━<br/>git log --merges<br/>blocks rebase"] REBASE["Rebase<br/>━━━━━━━━━━<br/>git rebase --autostash<br/>120s timeout"] REBASE_ABORT{"Abort<br/>succeeds?"} G6_5["Gate 6.5: Post-Rebase Tests<br/>━━━━━━━━━━<br/>tester.run after rebase"] end subgraph MergePhase ["● MERGE PHASE — git.py"] direction TB G7["Gate 7: Main Repo Discovery<br/>━━━━━━━━━━<br/>git worktree list --porcelain"] G7_5["● Gate 7.5: Branch Verify<br/>━━━━━━━━━━<br/>main repo branch ==<br/>expected base_branch"] MERGE["Merge<br/>━━━━━━━━━━<br/>git merge --no-edit<br/>in main repo, 60s timeout"] MERGE_ABORT{"Abort<br/>succeeds?"} end subgraph PostMerge ["● POST-MERGE — _editable_guard.py"] EDIT_GUARD["● Gate 8.5: Editable Guard<br/>━━━━━━━━━━<br/>scan PEP 610 direct_url.json<br/>Fail-open design"] end subgraph Cleanup ["CLEANUP — git.py (Non-Fatal)"] direction TB WT_RM["Worktree Remove<br/>━━━━━━━━━━<br/>shutil.rmtree fallback"] SIDECAR_RM["Sidecar Remove<br/>━━━━━━━━━━<br/>Best-effort"] BRANCH_DEL["Branch Delete<br/>━━━━━━━━━━<br/>git branch -D"] end %% TERMINAL STATES %% T_INTACT(["WORKTREE_INTACT<br/>━━━━━━━━━━<br/>Safe to retry"]) T_DIRTY(["WORKTREE_DIRTY<br/>━━━━━━━━━━<br/>Needs manual fix"]) T_ABORT_FAIL(["ABORT_FAILED<br/>━━━━━━━━━━<br/>Repo may be damaged"]) T_MAIN_ABORT(["MAIN_REPO_ABORTED<br/>━━━━━━━━━━<br/>Merge conflict in main"]) T_CLEANUP_BLOCKED(["MERGE_SUCCEEDED<br/>CLEANUP_BLOCKED<br/>━━━━━━━━━━<br/>Editable install blocks<br/>worktree deletion"]) T_SUCCESS(["SUCCESS<br/>━━━━━━━━━━<br/>merge_succeeded: true"]) %% FLOW %% START --> HOOK_BRANCH HOOK_BRANCH -->|"branch in protected list"| HOOK_DENY HOOK_BRANCH -->|"allowed"| GATE GATE -->|"closed"| GATE_ERR GATE -->|"open"| TOOL_WRAP TOOL_WRAP -->|"unhandled exception"| TOOL_EXC TOOL_WRAP -->|"normal"| G1 G1 -->|"not a dir"| T_INTACT G1 -->|"exists"| G1_5 G1_5 -->|"protected"| T_INTACT G1_5 -->|"allowed"| G2 G2 -->|"not a worktree"| T_INTACT G2 -->|"valid"| G3 G3 -->|"detection fail / detached HEAD"| T_INTACT G3 -->|"branch found"| G3B G3B -->|"mid-rebase"| T_DIRTY G3B -->|"clean"| G3C G3C -->|"rm/commit fail"| T_INTACT G3C -->|"ok"| G3D G3D -->|"dirty files"| T_DIRTY G3D -->|"clean"| G4 G4 -->|"tests fail"| T_INTACT G4 -->|"pass / disabled"| G5 G5 -->|"fetch fail"| T_INTACT G5 -->|"ok"| G5_5 G5_5 -->|"ref missing"| T_INTACT G5_5 -->|"exists"| G5_6 G5_6 -->|"merge commits found"| T_INTACT G5_6 -->|"clean history"| REBASE REBASE -->|"conflict"| REBASE_ABORT REBASE_ABORT -->|"yes"| T_INTACT REBASE_ABORT -->|"no"| T_ABORT_FAIL REBASE -->|"ok"| G6_5 G6_5 -->|"tests fail"| T_INTACT G6_5 -->|"pass / disabled"| G7 G7 -->|"not found"| T_INTACT G7 -->|"found"| G7_5 G7_5 -->|"wrong branch"| T_INTACT G7_5 -->|"matches"| MERGE MERGE -->|"conflict"| MERGE_ABORT MERGE_ABORT -->|"yes"| T_MAIN_ABORT MERGE_ABORT -->|"no"| T_ABORT_FAIL MERGE -->|"ok"| EDIT_GUARD EDIT_GUARD -->|"poisoned installs found"| T_CLEANUP_BLOCKED EDIT_GUARD -->|"clean / fail-open"| WT_RM WT_RM -->|"fail: log warning"| SIDECAR_RM WT_RM -->|"ok"| SIDECAR_RM SIDECAR_RM --> BRANCH_DEL BRANCH_DEL --> T_SUCCESS %% CLASS ASSIGNMENTS %% class START,HOOK_DENY,GATE_ERR,TOOL_EXC terminal; class HOOK_BRANCH detector; class GATE stateNode; class TOOL_WRAP handler; class G1,G1_5,G2,G3,G3B,G3C,G3D,G4 detector; class G5,G5_5,G5_6,G6_5,G7,G7_5 detector; class REBASE,MERGE handler; class REBASE_ABORT,MERGE_ABORT stateNode; class EDIT_GUARD detector; class WT_RM,SIDECAR_RM,BRANCH_DEL output; class T_INTACT,T_SUCCESS terminal; class T_DIRTY,T_CLEANUP_BLOCKED gap; class T_ABORT_FAIL,T_MAIN_ABORT gap;Closes #700
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/remediation-20260409-220428-653353/.autoskillit/temp/rectify/rectify_merge_worktree_wrong_branch_2026-04-09_220428.md🤖 Generated with Claude Code via AutoSkillit