fix(create-pr): add marker-based cleanup for stranded synthetic commits

[jamesadevine]

Closes #192.

Problem

generate_patch() creates a synthetic commit, runs git format-patch, then resets it. If the process is hard-killed (OOM, SIGKILL) between commit and reset, the synthetic commit persists.

Fix

Marker file approach:

1. Write .ado-aw-synthetic-commit marker before creating the synthetic commit
2. Remove marker after successful reset
3. On next invocation, if marker exists, clean up the stranded commit

Also adds documentation explaining why a git stash approach was considered but not used (format-patch needs proper commit history for rename detection and binary handling).

Risk Assessment

Low probability event in an ephemeral AWF container. The marker-based cleanup adds a safety net without changing the proven format-patch flow.

Testing

All 810 tests pass.

[jamesadevine]

This PR and issue makes no sense given the execution context

[github-actions]

🔍 Rust PR Review

Summary: Has a concrete bug — the marker file placement in the working tree creates a false-positive cleanup path that can reset a real (non-synthetic) commit.

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs:273 — Marker file is inside the working tree, not .git/

  let marker_path = git_dir.join(".ado-aw-synthetic-commit");

  git_dir is the working tree root (bounding_directory or a sub-repo). Placing the marker there means:

  1. It shows up in git status --porcelain as an untracked file.
  2. has_uncommitted is evaluated before the marker cleanup block (line ~265 vs line ~274), so if the marker is the only thing present, has_uncommitted is incorrectly true.
  3. git add -A stages and commits the marker into the synthetic commit.
  4. After git reset --mixed HEAD~1 the marker file re-materialises as an untracked file.

  This creates a critical failure path: if the process is killed after the successful reset but before remove_file (a perfectly real race — the reset is a process exit + a very short file I/O), the next invocation sees:

  • has_uncommitted = true (the stale untracked marker is the only change)
  • Marker exists → cleanup runs git reset HEAD~1
  • But there is no synthetic commit at HEAD — the reset removes the real last commit, silently destroying work.

  Fix: store the marker inside .git/ where git ignores it completely:

  let marker_path = git_dir.join(".git").join("ado-aw-synthetic-commit");

  This file is invisible to git status, never touched by git add -A, survives hard kills, and still scopes to the correct repository.

• src/mcp.rs:274 — Cleanup runs without verifying HEAD is the synthetic commit

  The cleanup unconditionally issues git reset HEAD~1 whenever the marker exists. There is no check that the current HEAD is actually the expected synthetic commit (e.g. comparing the SHA stored in the marker against git rev-parse HEAD). Storing the commit SHA in the marker file — instead of the static string "synthetic-commit-pending\n" — would let the cleanup verify before destructively resetting:

  tokio::fs::write(&marker_path, format!("{}\n", head_sha_after_commit)).await

  Then in cleanup, read the SHA and compare with HEAD before resetting. (This concern is secondary to the working-tree placement issue above, but worth addressing at the same time.)

✅ What Looks Good

• The decision to avoid git stash is well-justified in the comment — format-patch's rename detection and binary handling genuinely require proper commit history.
• Cleanup errors are silently ignored (let _ = ...) which is the right call: a failed cleanup attempt should not abort the current run.
• The format-patch result is deliberately deferred until after the reset, preserving the "always undo the commit first" invariant.

Generated by Rust PR Reviewer for issue #203 · ● 389.5K · ◷