feat(create-pr): align with gh-aw create-pull-request implementation

[jamesadevine]

Summary

Closes the gap between ado-aw and gh-aw's create-pull-request safe output implementation. Identified via a systematic comparison of both codebases.

Security/Correctness

• File protection system — blocks manifests (package.json, go.mod, Cargo.toml, Dockerfile, etc.), CI configs (.github/, .pipelines/), and agent instruction files (.agents/, .claude/, .copilot/) by default. Override with protected-files: allowed.
• Max files per PR — caps at 100 files (configurable via max-files)
• Patch conflict fallback — git am --3way with git apply --3way fallback (with conflict marker detection) instead of hard failure
• Branch salt randomness — cryptographic random hex via rand crate instead of predictable timestamp
• Branch collision handling — queries ADO refs API before push; retries with new suffix if branch already exists
• Patch path validation — consistent parsing for rename/copy paths (including paths with spaces)
• ADO response sanitization — ##vso[ commands neutralized in error messages via sanitize_text()

Feature Parity

• Draft PR support — default draft: true, operator-enforced via ADO isDraft field
• Fallback on failure — records branch info on PR creation failure for manual recovery (fallback-record-branch: true)
• Excluded files — excluded-files: glob patterns strip files from patches before application (handles deletions correctly)
• No-changes config — if-no-changes: warn|error|ignore controls behavior on empty patches
• Label validation — agent can pass labels in tool params, validated against allowed-labels allowlist. Operator labels always applied unconditionally.
• Title prefix — title-prefix: "[Bot] " prepended to all PR titles
• Base commit recording — merge-base SHA saved at patch generation time for reliable Stage 2 application even if target branch advances

Patch Format Upgrade

• Migrated from raw git diff to git format-patch for proper commit metadata, rename detection, and binary file handling
• Stage 2 applies patches via git am --3way with git apply --3way fallback
• Added git to default bash command allowlist so agents can commit
• Updated tool description to encourage staging commits before PR creation

Other

• Provenance footer appended to PR body with timestamp and compiler version
• ExecutionResult::failure_with_data and ExecutionResult::warning for structured error/warning responses
• Exit code 2 → SucceededWithIssues pipeline integration
• Protected manifests expanded: Bun, Deno, Elixir, Haskell, uv, additional .NET packages

New Config Fields

safe-outputs:
  create-pull-request:
    draft: true                        # default: true (NEW)
    title-prefix: "[Bot] "            # NEW
    if-no-changes: warn               # warn|error|ignore (NEW)
    max-files: 100                    # NEW
    protected-files: blocked          # blocked|allowed (NEW)
    excluded-files: ["*.lock"]        # NEW
    allowed-labels: ["automated"]     # NEW
    fallback-record-branch: true      # default: true (NEW)

Testing

All 757 tests pass.

[github-actions]

🔍 Rust PR Review

Summary: Mostly solid — the feature parity work is well-structured and the security additions are a real improvement — but there are two correctness bugs and an edge case worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/tools/create_pr.rs — if-no-changes: "warn" is identical to "error"
  Both the excluded-files path (~line 594) and the no-changes path (~line 828) use a match where _ (the "warn" catch-all) returns ExecutionResult::failure() — the same as "error". Operators who configure if-no-changes: warn will still see a pipeline failure, which defeats the purpose of a distinct "warn" mode. The _ arm should return ExecutionResult::success() (mirroring "ignore") while logging a warning. Right now the three-variant enum exposed in the docs/config effectively has only two behaviors.

• src/mcp.rs — Silently-ignored git reset HEAD~1 is a correctness risk
  The temporary commit reset is fire-and-forget:

  let _ = Command::new("git")
      .args(["reset", "HEAD~1", "--mixed", "--quiet"])
      .current_dir(&git_dir)
      .output()
      .await;

  If this fails (e.g. permissions, detached HEAD, or filesystem error), the agent's repository silently retains the synthetic "agent changes" commit. Any subsequent git operations in the same pipeline run — including a second create-pull-request call — will operate on top of an unexpected extra commit. At minimum this should propagate an error; ideally, the failure is surfaced rather than swallowed.

⚠️ Suggestions

• src/tools/create_pr.rs — extract_paths_from_patch / filter_excluded_files_from_patch break on paths with spaces
  Both functions use split_whitespace() to parse diff --git a/path b/path headers. Git format-patch quotes space-containing filenames in the diff header (e.g. diff --git "a/path with spaces" "b/path with spaces") — splitting by whitespace will produce garbage paths for those cases. This means max-files counting and excluded-files filtering silently misbehave for repos with spaces in filenames. A more robust approach is to parse the --- a/ / +++ b/ lines instead, or use a dedicated diff-header parser.

• src/tools/create_pr.rs — find_disallowed_files is #[cfg(test)] dead code in production
  The function lives under #[cfg(test)], but the tests use it with file path globs (not labels), hinting at a planned "allowed-files" feature that was never wired into the production executor. If there's no follow-up intent, this should be removed to avoid confusion; if there is, a tracking comment would help.

✅ What Looks Good

• The switch from git diff to git format-patch + git am --3way with git apply --3way fallback is a meaningful robustness upgrade and handles the rename/binary file cases cleanly.
• PROTECTED_MANIFEST_BASENAMES / PROTECTED_PATH_PREFIXES are thoughtfully comprehensive and the case-insensitive matching is correct.
• Cryptographic branch salt via rand::rng().random::<u32>() is a clear improvement over the millisecond-timestamp approach.
• The label allowlist validation (merge operator labels + validate agent labels) is implemented correctly in production — inline config.allowed_labels.contains(l) does the right thing.
• collect_changes_from_diff_tree correctly handles the M/A/D/R diff-tree status codes and calls validate_single_path for all paths including both sides of a rename.

Generated by Rust PR Reviewer for issue #155 · ● 882.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Needs changes — good security additions, but the patch generation approach has a fundamental design conflict between the implementation and the updated tool description, plus binary file support is incomplete despite being claimed.

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs:194-290 — Format-patch captures uncommitted state only, but tool description says to commit first (critical)

  The synthetic-commit approach (git add -A && git commit && format-patch -1 && reset HEAD~1) only captures working-tree and staged changes — it cannot see previously committed work. Yet the updated create-pull-request tool description explicitly instructs agents:

  "stage and commit your changes using git add and git commit — each logical change should be its own commit"

  If an agent follows this advice and has committed their changes, the working tree is clean. git add -A stages nothing, git commit --allow-empty produces an empty commit, and git format-patch -1 yields a patch with no file changes. The result is an empty patch → if-no-changes: warn fires, and no PR is created. The previous git diff origin/main approach captured committed changes correctly.

  Either the implementation needs to use git format-patch origin/main..HEAD (or equivalent) to capture real commits, or the tool description must be reverted to "do not commit before calling this tool."

• src/tools/create_pr.rs:1100-1175 — Binary files silently fail despite "binary file handling" being listed as a feature

  collect_changes_from_diff_tree reads file content with tokio::fs::read_to_string, which returns an Err for non-UTF-8 files. The ADO Pushes API supports contentType: "base64encoded" for binary files, but this code always sends contentType: "rawtext". Binary file support exists in Stage 1 (format-patch captures them) but is broken in Stage 2.

• src/tools/create_pr.rs:1155-1175 — Renamed+modified files lose content changes

  When collect_changes_from_diff_tree encounters an R (rename) status entry, it emits only a rename change without reading the new file's content. If the file was simultaneously modified (partial rename similarity score, e.g., R80), the content delta is silently dropped. The ADO rename changetype only moves the path — it doesn't apply content edits.

• src/main.rs:243 — Fragile success_count - warning_count arithmetic

  success_count - warning_count  // usize subtraction

  This assumes the invariant warning == true → success == true, which holds for all current constructors but is not enforced by the type system. A future ExecutionResult { success: false, warning: true, .. } — e.g., in failure_with_data — would cause a debug-mode panic. At minimum, add a debug_assert!(warning_count <= success_count) to make the assumption visible, or count with filter(|r| r.success && !r.warning) directly (as done in execute.rs, but main.rs uses the subtraction form).

⚠️ Suggestions

• src/tools/create_pr.rs — excluded-files glob patterns don't match nested paths

  glob_match::glob_match("*.lock", "subdir/Cargo.lock") returns false — * in the glob-match crate does not cross path separators. A user writing excluded-files: ["*.lock"] expecting to exclude all lockfiles will be surprised when services/api/package-lock.json slips through. Either document this prominently in the config doccomment, or normalise patterns (prepend **/ when the pattern contains no /) to match user intent.

• src/tools/create_pr.rs:382,388 — Unvalidated string enums

  Both if_no_changes and protected_files are free-form String fields with no validation. A typo like protected-files: "disbaled" silently enables file protection (fail-safe, but silent). Consider adding a compile-time validation step in CreatePrResult::execute that rejects unrecognised values with a clear error, or change these to proper enums.

• src/mcp.rs:206-222 — git commit may fail if git identity is not configured

  The synthetic commit requires user.email and user.name to be set in git config. In a fresh ADO agent, these may not be configured, causing git commit to fail with a "Please tell me who you are" error. Consider using -c user.email=agent@ado-aw -c user.name="ADO Agent" flags on the commit invocation (or the equivalent GIT_AUTHOR_*/GIT_COMMITTER_* env vars) to ensure the step is self-contained.

✅ What Looks Good

• Cryptographic random suffix (rand::rng().random()) replacing the timestamp-based one is a solid improvement.
• The filter_excluded_files_from_patch patch-block parser correctly handles the format-patch preamble headers (lines before the first diff --git) and quoted paths with spaces.
• extract_paths_from_patch correctly excludes /dev/null from new-file entries.
• The warning / ExecutionResult::warning / exit-code-2 / ##vso[task.complete result=SucceededWithIssues] chain is a clean end-to-end implementation.
• Protected-files list and path-prefix detection are comprehensive and case-insensitive.
• Label allowlist validation (allow-all when allowed_labels is empty, reject-unknown otherwise) is a sensible operator-friendly default.

Generated by Rust PR Reviewer for issue #155 · ● 1.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security thinking, but has one correctness bug in the merge-base fallback, a misleading feature name, and a couple of edge cases worth addressing.

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs — find_merge_base fallback breaks on single-commit repos
  When neither origin/HEAD nor origin/main is reachable, the function returns the literal string "HEAD~1". On a repo with only one commit (e.g. fresh git init + one commit), HEAD~1 doesn't exist, so git format-patch HEAD~1..HEAD fails with fatal: bad revision 'HEAD~1..HEAD'. The error surface at that point is an opaque format-patch failure rather than a useful message. Fix: resolve the initial commit instead:

  // Fallback: diff from root commit
  let root = Command::new("git")
      .args(["rev-list", "--max-parents=0", "HEAD"])
      .current_dir(git_dir).output().await.ok();
  if let Some(out) = root.filter(|o| o.status.success()) {
      let sha = String::from_utf8_lossy(&out.stdout).trim().to_string();
      if !sha.is_empty() { return Ok(sha); }
  }
  // True fallback if even root fails (empty repo)
  return Err(anyhow_to_mcp_error(anyhow::anyhow!("Cannot determine diff base: no commits or remote")));

• src/tools/create_pr.rs:extract_paths_from_patch — renames counted twice toward max_files
  Both --- a/<old> and +++ b/<new> lines are collected, so a rename counts as 2 entries and inflates the file count check. For the max_files budget only the destination path (+++ b/) should be counted; source paths are redundant for adds/edits too (where --- a/ and +++ b/ are the same file). The protected-files check is unaffected since it guards both old and new path names.

• src/tools/create_pr.rs:filter_excluded_files_from_patch — empty-commit envelopes in multi-commit format-patch output
  When all diffs within a single commit block are excluded, the From <SHA> Mon Sep 17 ... / Subject: envelope remains in the output without any diff hunks. git am accepts this as an empty commit, which is mostly harmless but could leave unexpected empty commits on the branch. Worth noting in a comment since it's a non-obvious behaviour change from the old git diff path.

⚠️ Suggestions

• fallback_as_work_item name is misleading
  The config key and PR description both imply a work item is automatically created on PR failure. In reality, ExecutionResult::failure_with_data just surfaces structured JSON in the failure log — no ADO work-item API is called. Operators who enable this expecting an auto-created work item will be surprised. Consider renaming to record-branch-on-failure (or documenting explicitly that it records branch info only) before this ships.

• if_no_changes and protected_files validated only at Stage 2
  Both fields are String types with runtime enum validation. Invalid values (protected-files: bloced) only surface at execution time, not at ado-aw compile. Modelling them as Rust enums with #[serde(rename_all = "kebab-case")] would give pipeline authors compile-time feedback instead. This is consistent with how other strongly-typed fields in types.rs work.

• src/tools/create_pr.rs — collect_changes_from_worktree "RM" handling emits both a rename + edit
  The new code adds a second edit change entry for the new path when status is "RM" (renamed + modified in working tree). Confirm the ADO pushes API correctly handles a rename and a subsequent edit for the same new path in a single commit payload; otherwise the push may fail with a duplicate-path error.

✅ What Looks Good

• Cryptographic random for branch IDs — upgrading from a predictable timestamp suffix to rand::rng().random::<u32>() is a correct security improvement.
• ##vso[task.complete result=SucceededWithIssues] pattern in base.yml — the exit-code 2 convention piped through the ADO task result marker is exactly right for surfacing warnings without failing the pipeline.
• Synthetic-commit cleanup ordering — the git reset HEAD~1 --mixed runs before checking format_patch_output.status, so the worktree is always restored even when format-patch fails. Clean.
• Protected-files list — comprehensive, covers supply-chain attack vectors (manifests), CI configs, and agent instruction directories. Good defaults.
• Binary file support via base64 — read_file_change now UTF-8-probes and falls back to base64encoded, which is the correct ADO push API content type for binary blobs.

Generated by Rust PR Reviewer for issue #155 · ● 984.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Needs changes — one security bug in the file protection system undermines the key safety feature.

---

Findings

🐛 Bugs / Logic Issues

find_protected_files — case comparison is broken for mixed-case manifest names (src/tools/create_pr.rs)

The function lowercases the path basename but then compares against the original (mixed-case) manifest constant:

let lower_basename = basename.to_lowercase();
for manifest in PROTECTED_MANIFEST_BASENAMES {
    if lower_basename == *manifest {   // ← comparing "cargo.toml" != "Cargo.toml"

PROTECTED_MANIFEST_BASENAMES contains several mixed-case entries: "Cargo.toml", "Cargo.lock", "Pipfile", "Pipfile.lock", "Gemfile", "Gemfile.lock", "Directory.Build.props", "Directory.Build.targets". None of these will ever be detected as protected — a Cargo.toml in any path silently passes the check.

The existing tests only cover all-lowercase manifests (package.json, go.mod), so this escaped test coverage.

Fix — one of:

// Option A: normalise both sides
if lower_basename == manifest.to_lowercase() {

// Option B: ensure PROTECTED_MANIFEST_BASENAMES is all-lowercase at the source

The exact-paths check uses exact.to_lowercase() correctly (so CODEOWNERS is caught), and the path-prefix check uses all-lowercase constants correctly. Only the manifest basename check has this defect.

---

⚠️ Suggestions

filter_excluded_files_from_patch — if-no-changes not triggered when all files are excluded from a format-patch (src/tools/create_pr.rs)

The early-return check is:

if filtered.trim().is_empty() {
    match config.if_no_changes { ... }
}

For git format-patch output, even when all diffs are excluded the commit envelope (From <SHA>, Subject:, etc.) remains, so filtered.trim().is_empty() is always false. The if-no-changes decision is then deferred to the changes.is_empty() check after git am. If git am rejects a zero-hunk patch email (implementation-dependent), you'd get an opaque failure instead of the configured warn/ignore behaviour. The code comment acknowledges the empty-commit case but not this error path.

excluded-files silently precedes protected-files (src/tools/create_pr.rs)

Exclusion is applied before the protection check, so any file in PROTECTED_MANIFEST_BASENAMES can be excluded via excluded-files and the protection check will never see it. The net effect is the file change is simply dropped from the PR rather than blocked. This may be intentional, but it's worth a comment explaining the ordering and that exclusion implicitly overrides protection.

ExecutionResult.warning invariant is soft (src/tools/result.rs)

The struct's fields are pub, so warning: true, success: false is constructable. The counting logic in execute.rs and main.rs assumes warnings imply success: true. Consider either making warning private with accessor methods, or adding a #[cfg(debug_assertions)] invariant check. Low priority, but the mismatch would silently corrupt the summary counts.

---

✅ What Looks Good

• rand::rng().random() for branch suffix — straightforward fix for the predictable timestamp bias; test updated correctly.
• git am --3way → git apply --3way fallback — good layered resilience; the git am --abort cleanup before the fallback is correct.
• git format-patch upgrade — removing the manual untracked-file diff construction is a clear correctness win; binary file support via base64encoded is a nice addition.
• collect_changes_from_diff_tree — rename-with-modification (R100 detection) mirrors the RM handling in collect_changes_from_worktree cleanly.
• Label allowlist validation — agent labels rejected before the ADO API call with clear error messaging.
• Provenance footer — timestamp and version are both safe compile-time/runtime values; no injection risk.
• base.yml exit-code 2 wrapper — mapping the warning exit code to SucceededWithIssues via ##vso[task.complete] is the right ADO pattern.

Generated by Rust PR Reviewer for issue #155 · ● 1.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: needs changes — two interacting correctness bugs in the multi-commit format-patch flow that will silently produce incomplete or malformed patches when agents make more than one commit.

Findings

🐛 Bugs / Logic Issues

[src/tools/create_pr.rs — collect_changes_from_diff_tree] Multi-commit format-patch produces silently incomplete PRs

git diff-tree --no-commit-id -r --name-status HEAD compares HEAD to its immediate parent only. When git am successfully applies a format-patch with N > 1 commits, HEAD is the last commit in the series, and diff-tree HEAD only captures the files changed in that final commit. Changes from commits 1...N-1 are silently dropped — the PR is created with a subset of the intended files.

This is directly triggered by the PR description's own instruction to agents: "each logical change should be its own commit with a descriptive message." A 2-commit patch applied by git am = last commit's diff only.

The fix is to record the worktree HEAD SHA before git am runs and diff against that:

// before git am
let base_sha = get_current_head(&worktree_path).await?;
// ... git am ...
// then:
git diff-tree --no-commit-id -r --name-status <base_sha> HEAD
```
Or more simply, just use `git diff <target_branch_initial_sha> HEAD` after applying.

---

**[src/tools/create_pr.rs — `filter_excluded_files_from_patch`] Commit envelope misattribution corrupts multi-commit patches**

A `git format-patch --stdout` stream has this structure:

```
From <sha1> ...            ← commit 1 envelope
Subject: [PATCH 1/2] ...
---
diff --git a/file1.rs ...  ← file1 diff block
...
                           ← blank line (end of commit 1 diff)
From <sha2> ...            ← commit 2 envelope
Subject: [PATCH 2/2] ...
---
diff --git a/file2.rs ...  ← file2 diff block

The current block-splitting logic only splits on diff --git lines. Commit 2's envelope (From <sha2>...Subject:...---) appears between file1's diff and file2's diff --git, so it gets appended to file1's block.

Result:

• If file1 is excluded: commit 2's envelope is dropped with it → file2's diff starts without a format-patch header → git am fails on the corrupted patch, falls through to git apply --3way (unintentional but functionally recovers)
• If file1 is not excluded: the output is a malformed patch with commit 2's envelope embedded mid-block → git am misparses, likely fails

This only manifests with multi-commit patches, which are now the default path and explicitly encouraged. A correct implementation needs to track and independently emit commit envelopes (lines between -- \n terminator and the next diff --git).

---

⚠️ Suggestions

[src/tools/create_pr.rs — find_protected_files] Windows backslash check is dead code

|| lower_path.starts_with(&prefix.replace('/', "\\"))

Paths extracted from Git diffs always use forward slashes regardless of OS. This condition can never be true and the replace allocation runs for every path × every prefix. Safe to remove.

---

[src/tools/create_pr.rs — label allowlist] Case-sensitive comparison may surprise operators

.filter(|l| !config.allowed_labels.contains(l))

An agent providing "Automated" when the allowlist contains "automated" produces a hard failure. Consider .any(|al| al.eq_ignore_ascii_case(l)) or document the case-sensitivity contract clearly.

---

✅ What Looks Good

• Cryptographic random suffix for branch names (rand::rng().random::<u32>()) is a clean fix over the predictable timestamp approach.
• The ExecutionResult::warning / is_warning() design with success: true, warning: true invariant is well-structured, and the exit-code-2 pipeline plumbing through base.yml is correct.
• find_merge_base with graceful fallback chain and explicit warning log is solid.
• read_file_change with UTF-8 probe → base64 fallback is a good improvement for binary files.
• Protected files list is comprehensive and the case-insensitive matching approach is correct.
• extract_paths_from_patch correctly avoids collecting /dev/null (new/deleted file sentinels) — the test covers this.

Generated by Rust PR Reviewer for issue #155 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good overall — solid security additions and cleaner patch format, but there are a few real correctness issues worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs — find_merge_base too narrow: Only tries origin/HEAD and origin/main before falling back to the root commit. In repos where the default remote branch is something other than main (e.g. master, develop) and origin/HEAD isn't auto-configured (common with manual clones), the fallback fires and git format-patch root..HEAD captures the entire repo history across all branches. This could produce enormous patches or incorrect diffs that include commits from other branches. Consider also trying origin/master and — better yet — checking git symbolic-ref refs/remotes/origin/HEAD first to discover the actual default branch name before hard-coding the fallback list.

• src/tools/create_pr.rs — label dedup is case-sensitive while the allowlist check is case-insensitive: The allowed-labels filter uses eq_ignore_ascii_case (correct), but the dedup when merging agent labels into operator labels uses all_labels.contains(label) (byte-equal). If config.labels contains "Automated" and the agent sends "automated", both land in all_labels. Minor, but produces duplicate labels on ADO PRs.

• src/tools/create_pr.rs — filter_excluded_files_from_patch leaves ghost commit envelopes: When all diff blocks within a multi-commit format-patch are excluded, the commit envelope (From <SHA>, Subject:, date headers) is preserved in the output. On some git versions git am treats an envelope with no diff as an error ("patch does not change any file"), causing it to abort and fall through to the git apply --3way fallback, which also produces no changes and triggers the if-no-changes handler. The code comment acknowledges this but the behavior is surprising and version-dependent. Worth stripping envelope-only commits or documenting the observed git version behaviour.

⚠️ Suggestions

• src/tools/create_pr.rs — draft: true is a breaking default: Existing agents that don't set draft: will now have all their PRs silently created as drafts. ADO drafts cannot be auto-completed or merged by branch policies until promoted. This is likely to break existing scheduled pipelines. Consider defaulting to false (matching ADO's own default) and making true opt-in, or at minimum calling it out loudly in the CHANGELOG.

• src/mcp.rs — synthetic commit leaves behind a dirty state on reset failure: If git reset HEAD~1 fails the function returns a McpError. At that point the synthetic commit is still present in the agent's working repo, and the agent process will continue (or the pipeline will retry). The user-facing error message is good ("repository may retain synthetic commit"), but consider also logging the commit SHA so operators can identify and clean it up.

• templates/base.yml — exit code 2 handling: The EXIT_CODE=$? capture and ##vso[task.complete result=SucceededWithIssues;] emission is correct. Make sure the displayName on this step is distinct enough for operators to distinguish it from the old single-line version in pipeline logs (currently both say "Process safe outputs").

✅ What Looks Good

• The ExecutionResult::warning() invariant (warning ⟹ success) is clearly documented and correctly maintained throughout execute.rs and main.rs.
• Binary file support via read_file_change → base64 is a clean addition and handles the String::from_utf8 fallback idiom well.
• Protected-files detection is comprehensive and the case-insensitive basename comparison avoids Windows/macOS casing bypass.
• The collect_changes_from_diff_tree function correctly handles the R<similarity> format from git diff-tree --name-status and emits an extra edit entry only when similarity < 100%.
• Cryptographic randomness (rand::rng().random()) for branch suffixes is the right call over the timestamp-based approach.

Generated by Rust PR Reviewer for issue #155 · ● 820.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security foundations, but has a correctness bug on the draft default and a reliability issue with the synthetic commit cleanup path.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/create_pr.rs — draft default contradicts PR description

  The PR description explicitly states "default: draft: true" as a safety feature (operator-enforced). But the code defaults to false:

  /// Whether to create the PR as a draft (default: false)
  #[serde(default)]
  pub draft: bool,
  // Default impl: draft: false,

  If the intent is that all agent-created PRs should be drafts until an operator explicitly opens them (a sensible safety posture), the serde default and Default impl should be true. As shipped, agents will silently create non-draft PRs unless the operator adds draft: true to every pipeline's front matter.

• src/mcp.rs — Synthetic commit can leak on git format-patch spawn failure

  In generate_patch(), if Command::new("git").args(["format-patch", ...]).output().await fails at the OS level (e.g., OOM, ENOMEM), the ? operator returns early before the if made_synthetic_commit { git reset HEAD~1 } block is reached. The synthetic commit is then permanently baked into the agent's working repository.

  // Step 1: commit (made_synthetic_commit = true)
  let format_patch_output = Command::new("git")
      .args(["format-patch", ...])
      .output()
      .await
      .map_err(|e| { ... })?;  // ← early return here skips reset!
  
  // Step 2 (never reached on spawn failure):
  if made_synthetic_commit {
      // git reset HEAD~1 ...
  }

  The fix is to reset before checking the map_err result, or use a drop guard. A simple approach: capture the IO error, attempt the reset, then return the error.

⚠️ Suggestions

• src/tools/create_pr.rs:PROTECTED_MANIFEST_BASENAMES — Dockerfile is absent

  Dockerfile and docker-compose.yml are common supply-chain attack vectors (they specify base images and build commands) but aren't in the protected manifest list. The CI/pipeline prefix list covers .github/ and .pipelines/ but doesn't guard against Dockerfile at the repo root or in service subdirectories. Worth adding for consistency with the stated threat model.

• src/tools/create_pr.rs:collect_changes_from_diff_tree — redundant path validation for renames

  let old_path = file_path;  // already validated via validate_single_path(file_path) above
  let new_path = parts[2];
  validate_single_path(old_path)?;  // ← re-validates file_path unnecessarily
  validate_single_path(new_path)?;  // ← correct and needed

  Harmless but noisy.

• src/mcp.rs:filter_excluded_files_from_patch — lines() silently normalises \r\n

  The parser iterates patch_content.lines() (which strips \r) and re-emits with push('\n'). On Windows or for patches generated on Windows, this converts CRLF → LF, which can cause git am to reject the patched result with a "trailing whitespace" or line-ending warning. Low risk in the Azure Pipelines environment (Linux agents) but worth noting.

• src/tools/create_pr.rs — fallback_description embeds raw ADO API response body

  let fallback_description = format!(
      "...**Error:** {} - {}\n\n...",
      status, body,  // ← body is the raw ADO HTTP error response

  If the ADO error response contains sensitive content (tokens, internal identifiers), it gets written to the failure data returned from the executor. In practice ADO error bodies are benign, but it's worth sanitising or truncating body to a reasonable max length (e.g., 500 chars) before embedding.

---

✅ What Looks Good

• Cryptographic branch suffix: Replacing the timestamp-based generate_short_id() with rand::rng().random::<u32>() is strictly correct. The old 6-digit hex timestamp (only 24 bits of millis) was predictable within a second.
• Protected files system: The layered check (basename, path prefix, exact path, case-insensitive) is thorough and the test coverage is good. Including agent instruction dirs (.agents/, .claude/, .copilot/) in PROTECTED_PATH_PREFIXES is exactly the right call.
• git am --3way + git apply --3way fallback: Proper handling of the format-patch → am pipeline with a sensible fallback for plain-diff backward compatibility.
• collect_changes_from_diff_tree vs collect_changes_from_worktree split: Cleanly separates the committed-changes (diff-tree) path from the staged-changes (status) path. The base_sha..HEAD range in diff-tree correctly handles multi-commit patches.
• Binary file support via base64: The read_file_change helper trying UTF-8 first and falling back to base64 is the right approach for the ADO Push API.
• Exit code 2 → SucceededWithIssues: The ##vso[task.complete result=SucceededWithIssues;] integration is clean and the pipeline template change is correct.
• Label allowlist validation: Rejecting disallowed agent labels before pushing is the right ordering (fail fast before the branch is created).

Generated by Rust PR Reviewer for issue #155 · ● 900.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: solid implementation with one correctness bug in the synthetic-commit path and a few worth-flagging gaps.

---

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs — generate_patch() mutates index state on completion
  The synthetic-commit path does git add -A + git commit → git reset HEAD~1 --mixed. A --mixed reset undoes the commit and unstages the index back to modified, not back to untracked. Any file that was untracked before generate_patch() is called is left in the index as "new file (staged)" afterward. If the agent calls create_pr more than once, or runs any git status check after the first call, it will see unexpected staged additions. The fix is to stash the untracked files before staging, or use git reset HEAD --mixed && git clean -fd after the reset — but most straightforward: use git stash --include-untracked / git stash pop as the bracket instead of synthetic commit.

• src/tools/create_pr.rs — deleted files can never be excluded via excluded-files
filter_excluded_files_from_patch identifies a diff block's file path from the +++ b/<path> line only. Deletions produce +++ /dev/null, so current_path is None for those blocks. A deleted *.lock file will never match an exclusion pattern; it will always appear in the patch and trigger a protection check. If intentional, it needs a code comment and docs; if not, --- a/<path> should be used as fallback when no +++ b/ line is found.

• src/main.rs / src/execute.rs — failure_count under-counts on warning-only runs
  results.iter().filter(|r| !r.success).count() is correct; warnings correctly have success: true. However the Total line in the summary prints results.len() which equals success_count + warning_count + failure_count. That arithmetic is fine — just double-checked the invariant holds. ✅ (Not a bug, just verifying.)

🔒 Security Concerns

• src/tools/create_pr.rs — raw ADO API response body embedded in fallback_description
  In the PR-creation-failure branch, truncate_error_body(&body, 500) is concatenated into fallback_description which is placed inside ExecutionResult::failure_with_data. The data field ends up in the NDJSON and is subsequently logged at Stage 2. If ADO ever returns a body containing ##vso[ task commands (possible if, e.g., the org name or PR title appears in the error), those commands would be emitted into the pipeline log. The existing sanitize.rs infrastructure should be applied here: strip or escape ##vso[ from body before embedding it, consistent with how the project handles other user-controlled strings written to logs.

• src/mcp.rs — git -c user.email=agent@ado-aw in synthetic commit
  The injected identity values are hardcoded constants, so there's no injection risk. But the synthetic commit object (with the fixed author) is briefly pushed into the repo history. If git reset HEAD~1 --mixed is skipped due to an earlier error path, a commit with this author would persist. The current code handles this: the reset is attempted even on format_patch failure. Looks okay, but depends on the cleanup always running — worth a defensive note.

⚠️ Suggestions

• src/tools/create_pr.rs — record-branch-on-failure vs. PR description fallback-as-work-item
  The PR description advertises fallback-as-work-item: true as the config key, but the actual serialized field name is record-branch-on-failure. The struct doc also says "No work item is created automatically" — so the PR description name is inaccurate and will confuse operators. Either rename the field to match the PR description, or update the description.

• src/mcp.rs — find_merge_base root-commit fallback produces unbounded patches
  When no remote tracking branch is found, the code falls back to git rev-list --max-parents=0 HEAD, which produces the entire repo history as the patch base. On a repo with a long history and no remote configured (e.g., a fresh clone that somehow lost its remote), the resulting format-patch output could be enormous. The existing 5 MB MAX_PATCH_SIZE_BYTES check in Stage 2 will catch it, but a warning message to the operator at this fallback point would help diagnose the situation faster.

• src/tools/create_pr.rs — extract_paths_from_patch strips quotes but not escape sequences
  rest.trim().trim_matches('"') handles simply-quoted paths. git also uses C-style octal escaping (\303\251 for é) inside quoted paths. For non-ASCII filenames this will produce garbled paths in the protection check, though the validate_patch_paths check would still catch traversal attempts in the actual application step. Low risk but worth noting for completeness.

✅ What Looks Good

• find_merge_base is well-structured with a clean fallback chain (symbolic-ref → common defaults → root commit) and correct error propagation.
• The filter_excluded_files_from_patch multi-commit format-patch parser correctly handles envelope stripping; the test coverage for multi-commit cases is thorough.
• read_file_change with UTF-8 → base64 fallback is the right approach for binary files and matches ADO API expectations.
• collect_changes_from_diff_tree correctly handles R<N> rename similarity scores and emits a follow-up edit when similarity < 100%.
• Exit code 2 → SucceededWithIssues wiring in base.yml is clean and idiomatic for ADO pipelines.
• Protected-files list is comprehensive with case-insensitive comparison; the ordering (exclusion before protection) is correctly documented.

Generated by Rust PR Reviewer for issue #155 · ● 972.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security thinking, but has a few real bugs and one breaking change that need addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/create_pr.rs:267-270 — agent_labels skipped by sanitize_fields()

  Sanitize::sanitize_fields() on CreatePrResult sanitizes title and description but not the new agent_labels: Vec<String> field. The defense-in-depth pattern used everywhere in this codebase requires all agent-controlled string fields to be sanitized before writing to NDJSON (Stage 2 applies sanitization again, but Stage 1 also calls result.sanitize_fields() before persisting). Labels are serialized as JSON (so no JSON injection), but the pattern break is inconsistent and could matter if the data is ever printed or logged in a context where ##vso[ gets processed:

  fn sanitize_fields(&mut self) {
      self.title = sanitize_text(&self.title);
      self.description = sanitize_text(&self.description);
      // ← agent_labels missing
  }

• src/mcp.rs — synthetic commit not always cleaned up

  If git reset HEAD~1 --mixed fails (e.g., due to a filesystem error), the function returns early:

  if !reset_output.status.success() {
      return Err(anyhow_to_mcp_error(...));  // synthetic commit persists
  }

  The agent's repository now has an unexpected commit (user.email=agent@ado-aw). The SHA is included in the error message which is good, but the agent can't easily recover. This path should at minimum log a warn! banner like "WARNING: synthetic commit {sha} was not cleaned up; run git reset HEAD~1 to restore state."

• src/tools/create_pr.rs — git apply --3way fallback silently pushes conflict markers to ADO

  When git am --3way fails and the fallback git apply --3way is used, a partial merge may succeed by inserting conflict markers (<<<<<<<, =======, >>>>>>>) into the file content. The code then proceeds to collect_changes_from_worktree, reads these files verbatim, and pushes them to ADO. The resulting PR branch has unresolved conflicts baked in. The fallback should either fail explicitly or scan for conflict markers before proceeding:

  // After git apply --3way succeeds, check for unresolved conflicts
  let conflict_check = Command::new("git")
      .args(["diff", "--check"])
      .current_dir(&worktree_path)
      .output().await?;
  if !conflict_check.status.success() {
      return Ok(ExecutionResult::failure("Patch applied with conflicts — resolve manually"));
  }

🔒 Security Concerns

• src/tools/create_pr.rs:1108-1130 — self.description embedded unsanitized in failure data

  In the fallback_as_work_item path, self.description is used raw in fallback_description before sanitize_fields() has been applied to it (the execute path runs after sanitization, so in practice it is already sanitized via execute_sanitized() — but the structure creates an implicit ordering dependency). Worth an explicit sanitize_text(&self.description) call at the construction site, mirroring how truncate_error_body sanitizes the ADO error response on the same code path.

⚠️ Suggestions

• src/tools/create_pr.rs — draft: true default is a silent breaking change

  Any existing pipeline that omits draft: will begin creating draft PRs after this update. This is a user-visible behavior change that warrants a note in the PR description (and ideally a CHANGELOG entry / release note). Consider defaulting to false and requiring explicit opt-in, or adding a compile-time warning when no draft: value is present.

• src/mcp.rs — find_merge_base root-commit fallback should fail, not warn

  For a repo with years of history, falling back to the root commit could produce a multi-GB patch before the 5 MB Stage 2 limit truncates it. The warning message is accurate but doesn't prevent the tool from running. A hard failure here ("cannot determine upstream branch; please push a tracking branch or specify a base") gives a better user experience than a cryptic Stage 2 size error.

• fallback_as_work_item naming is misleading

  The field is called fallback-as-work-item: true but no work item is created — only structured failure data with the branch name is recorded. Consider renaming to fallback-record-branch or updating the doc comment to make it explicit that this is not a work item creation.

✅ What Looks Good

• Crypto randomness for branch suffix — replacing the timestamp-derived suffix with rand::rng().random::<u32>() eliminates the predictability concern cleanly.
• Format-patch migration — the merge-base..HEAD + synthetic commit approach handles both committed and uncommitted changes correctly, and the cleanup code properly captures the SHA for diagnostics before resetting.
• Protected files list — comprehensive, case-insensitive, covers manifests + CI configs + agent configs + CODEOWNERS. The test coverage (test_find_protected_files_*) is thorough.
• Excluded files filtering — the segment-based parser for multi-commit format-patch output correctly strips empty commit envelopes, avoiding git am errors on some git versions.
• Label validation — the allowed_labels allowlist with case-insensitive comparison is the right design; merging operator labels unconditionally while validating agent labels is the correct security model.

Generated by Rust PR Reviewer for issue #155 · ● 2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation overall, but has one breaking default change and a handful of correctness/security issues worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs ~line 291 — git status exit code unchecked before has_uncommitted check

  let has_uncommitted = !String::from_utf8_lossy(&status_output.stdout)
      .trim()
      .is_empty();

  status_output.status.success() is never verified. If git status fails (detached HEAD, broken repo), has_uncommitted is silently false, the synthetic-commit step is skipped, and the subsequent git format-patch call may silently produce an empty or wrong patch — with no error surfaced to the agent. Should be:

  if !status_output.status.success() {
      return Err(anyhow_to_mcp_error(anyhow::anyhow!(...)));
  }

• src/mcp.rs ~cleanup block — second git reset HEAD --quiet is a no-op with a misleading comment

  After git reset --mixed HEAD~1, git moves HEAD back and resets the index to match the original HEAD tree. New files that were staged by git add -A are not in HEAD~1's tree, so they become untracked again — they're not left as staged. The subsequent git reset HEAD --quiet is therefore a no-op in the normal case. The comment ("leaves previously-untracked files as staged new files") describes a behaviour that doesn't actually occur with --mixed. Harmless but will confuse future readers.

• src/tools/result.rs — failure_count in execute.rs / main.rs will double-count if is_warning() can ever return true when success == false

  The invariant ("warning == true implies success == true") is documented but not enforced at the type level. The current failure_count is results.iter().filter(|r| !r.success).count(), which is disjoint from warnings only because of that undocumented invariant. A future contributor who calls ExecutionResult { success: false, warning: true, ... } directly (the field is public) would silently corrupt the summary counts. Consider making warning private or enforcing via constructor only.

🔒 Security Concerns

• src/compile/common.rs — git added to the default bash allowlist

  Adding the bare git command gives agents access to every git subcommand by default, including git push, git remote add, and git clone. Network access to GitHub and ADO is already allowed in the AWF container (per allowed_hosts.rs), so agents could push to arbitrary branches in any accessible remote. The intent (allow git add/git commit for the format-patch workflow) would be better served by a more narrowly-scoped default. At minimum, document the expanded attack surface in the front-matter reference.

• src/tools/create_pr.rs:extract_paths_from_patch — +++ /dev/null not explicitly guarded

  For deleted files, the +++ b/ prefix becomes +++ /dev/null. The current code only matches +++ b/, so /dev/null is never added to the protected-files list — that's the correct behaviour. But the --- a/ branch for deleted files does correctly capture the original path. Just flagging this for awareness: if the prefix logic ever changes, silent bypasses of the protected-files check are possible for deleted manifests.

⚠️ Suggestions

• src/tools/create_pr.rs:CreatePrConfig — draft: true default is a silent breaking change

  All existing pipelines that don't explicitly set draft: will switch from creating ready-for-review PRs to draft PRs after this change. This will surprise operators. Consider defaulting to false (matching the previous behaviour) and making opt-in explicit, or at minimum calling it out as a breaking change in the CHANGELOG and emitting a compile-time notice.

• PR description vs. code — fallback-as-work-item vs. fallback-record-branch

  The PR summary advertises fallback-as-work-item: true as the new config key (implying a work item is created), but the actual serde field rename is fallback-record-branch and no work item is created — only branch info is written to the failure payload. Update the PR description / docs to reflect the correct key name and actual semantics.

• src/tools/create_pr.rs:normalize_glob_pattern — dead condition

  pattern.starts_with("**/") is always true when pattern.contains('/'), so the || branch is unreachable. Minor but misleading:

  // Before
  if pattern.contains('/') || pattern.starts_with("**/") { ... }
  // After
  if pattern.contains('/') { ... }

• src/tools/create_pr.rs:collect_changes_from_diff_tree — no unit tests for path parsing

  collect_changes_from_diff_tree parses the tab-delimited diff-tree output format, including the R<similarity>\told\tnew rename case. The existing tests cover find_protected_files and filter_excluded_files_from_patch well, but there are no unit tests for the new diff-tree parser. Consider extracting the parsing logic and adding tests for M, A, D, R100, and R085 status codes.

✅ What Looks Good

• The synthetic-commit/format-patch approach is a meaningful correctness improvement over the old manual untracked-file reconstruction loop. The cleanup ordering (generate → clean up → check error) correctly avoids leaving the repo in a dirty state on format-patch failure.
• The protected-files system is well-structured with clear separation between manifest basenames, path-prefix checks, and exact-path checks. The case-insensitive comparison and the 7 mixed-case tests are solid.
• templates/base.yml correctly wires up exit code 2 → ##vso[task.complete result=SucceededWithIssues;], closing the loop on the warning path.
• ExecutionResult::failure_with_data is a clean addition; structuring fallback info as typed JSON in the result payload (rather than concatenating it into a message string) is the right design.
• The envelope-stripping logic in filter_excluded_files_from_patch for multi-commit format-patch output is subtle and the accompanying tests cover the edge cases well.

Generated by Rust PR Reviewer for issue #155 · ● 1.3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid feature set with good security design, but there's a correctness gap in binary-file path extraction that affects both the protected-files check and excluded-files filter.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/create_pr.rs:1584 — extract_paths_from_patch silently skips binary files
  The function only parses --- a/ and +++ b/ lines. Binary file entries in git format-patch output omit those lines; they only have the diff --git a/<path> b/<path> header. As a result, find_protected_files won't block modifications to binary files under .github/, .pipelines/, CODEOWNERS, etc. — and filter_excluded_files_from_patch also won't strip them via excluded-files patterns.
  The fix is to also parse diff --git a/<path> b/<path> headers for path extraction. Because these headers can have ambiguous paths with spaces, the validate_patch_paths workaround (taking --- a/ / +++ b/ lines for that function) was the right call there — but protection/exclusion need the diff --git header too.

🔒 Security Concerns

• src/tools/create_pr.rs:~1130 — raw ADO response body in failure_with_data message
  In the fallback_record_branch block, body is correctly sanitized when building fallback_description, but the outer failure_with_data message string passes it unsanitized:

  format!("Failed to create pull request: {} - {}. Branch ...", status, body, ...)

  The executor prints result.message to stdout. If an ADO error response ever contained ##vso[ strings, they'd be interpreted by the ADO agent runner. Low-probability (ADO API responses don't typically include pipeline logging commands), but inconsistent with how fallback_description handles it. sanitize_text(truncate_error_body(&body, 200)) would bring it in line.

⚠️ Suggestions

• src/mcp.rs:~325 — stale git stash comment
  The comment reads: "We capture the original index state via git stash to restore it exactly..." but the code uses git reset HEAD~1 --mixed. Looks like a leftover from an earlier iteration. The code itself is correct; the comment is just misleading.

• src/mcp.rs — git add -A silently clobbers .gitignore-excluded files
  git add -A stages all untracked files (including those normally gitignored). For an agent that creates temporary build artifacts, these could end up in the patch unexpectedly. Consider git add -u (only tracked changes) as a safer default, or at least document the intentional choice.

✅ What Looks Good

• is_warning() invariant (warning == true ⟹ success == true) is correctly enforced in ExecutionResult, and the three-way counting in execute.rs/main.rs is clean. The exit-code-2 → SucceededWithIssues loop through the template was already wired up — no template change needed.
• Exclusion-before-protection ordering is explicitly justified in the comment. The logic is sound: excluding a file prevents modification entirely, so there's nothing to protect against.
• find_merge_base handles the symbolic-ref refs/remotes/origin/HEAD detection cleanly and falls back gracefully to origin/main / origin/master, with a single-commit-repo escape hatch.
• Synthetic commit pattern in mcp.rs correctly stores the format_patch result before undoing the commit, then propagates errors only after cleanup — avoiding the TOCTOU-style issue where an error could leave the repo in a dirty state.
• rand::rng().random::<u32>() over the timestamp approach is the right call. 32-bit cryptographic randomness gives negligible collision probability and removes the predictability concern from branch name generation.
• Test coverage for find_protected_files (manifests, CI configs, agent configs, CODEOWNERS, case-insensitive) is comprehensive.

Generated by Rust PR Reviewer for issue #155 · ● 2.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Strong improvement over previous iterations — the format-patch migration, binary file handling, and security additions are well-implemented — but there is one correctness bug in the merge-base fallback, a security concern in the error-recovery path, and a misleading comment that will confuse future maintainers.

---

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs:find_merge_base — root-commit fallback fires on ALL normal repos, not just single-commit repos

  The comment says "Only use root commit fallback for repos with a single commit", but the guard is:

  let roots: Vec<&str> = output_str.trim().lines().collect();
  if roots.len() == 1 { ...use root SHA as merge base... }

  git rev-list --max-parents=0 HEAD prints the initial commit(s) of the repository. A normal, 1000-commit linear repo has exactly one root commit, so roots.len() == 1 is true regardless of history depth. This means: if origin/main and origin/master are both unreachable (fresh clone with no fetch, or a CI machine that only does a shallow clone of the feature branch), the fallback fires and generates a format-patch that covers the entire project history from its very first commit. That's an enormous patch — and it then hits the 5 MB hard limit in Stage 2 with no useful error.

  The intent is clear from the comment: this should only fire when HEAD itself has no parent (single-commit repo). Fix:

  // roots[0] is the root SHA; compare it to HEAD to confirm this is a 1-commit repo
  let head_sha = Command::new("git")
      .args(["rev-parse", "HEAD"])
      .current_dir(git_dir).output().await.ok()
      .and_then(|o| if o.status.success() {
          Some(String::from_utf8_lossy(&o.stdout).trim().to_string())
      } else { None });
  
  if roots.len() == 1 && head_sha.as_deref() == Some(roots[0]) {
      warn!("...single-commit repository...");
      return Ok(roots[0].to_string());
  }

🔒 Security Concerns

• src/safeoutputs/create_pr.rs — raw ADO API response body echoed to pipeline stdout

  When PR creation fails and fallback_record_branch is enabled, two messages are constructed. The fallback_description (stored in data) correctly sanitizes the body:

  sanitize_text(truncate_error_body(&body, 500))

  But the primary message in failure_with_data does not:

  format!(
      "Failed to create pull request: {} - {}. Branch '{}' was pushed...",
      status, body, self.source_branch,  // body is raw
  )

  result.message is printed to stdout via println! in both execute.rs and main.rs. In Azure Pipelines, stdout is scanned for ##vso[...] task commands. A crafted ADO API error response (or a MITM response) that includes ##vso[task.setvariable variable=SYSTEM_ACCESSTOKEN]... would be executed. Apply the same sanitize_text(truncate_error_body(&body, 500)) treatment to the message string.

⚠️ Suggestions

• src/mcp.rs — misleading comment contradicts the implementation

  The comment before the git reset HEAD~1 call reads:

  "We capture the original index state via git stash to restore it exactly, since git reset --mixed would leave previously-untracked files staged."

  The code does not use git stash; it uses git reset --mixed HEAD~1. Worse, the comment claims git reset --mixed is the wrong approach and that git stash is needed, yet that's exactly what the code does. The actual behavior of git reset --mixed HEAD~1 here is correct (un-commits, leaves previously-untracked files as untracked in the working tree), but the comment will send future maintainers on a false trail. Drop the stash reference.

• src/safeoutputs/create_pr.rs:read_file_change — unnecessary bytes.clone() for all text files

  let bytes = tokio::fs::read(full_path).await...?;
  match String::from_utf8(bytes.clone()) {  // clone allocated for every file
      Ok(content) => ...
      Err(_) => { ...base64::encode(&bytes)... }
  }

  String::from_utf8 takes Vec<u8> by value and returns it back via Utf8Error::into_bytes() on failure. Drop the clone:

  match String::from_utf8(bytes) {
      Ok(content) => ...,
      Err(e) => { let bytes = e.into_bytes(); ...base64::encode(&bytes)... }
  }

• draft: true default is a silent breaking change for existing pipelines

  Any existing pipeline that relies on auto-complete: true without explicitly setting draft: false will now have PRs created as drafts. In Azure DevOps, auto-complete is respected on draft PRs once they transition to "Ready for review", but if the agent never calls a "mark ready" action the PR will sit in draft indefinitely. Consider defaulting to draft: false to preserve backward compatibility, or at least call this out prominently in the changelog.

---

✅ What Looks Good

• The git format-patch + git am --3way / git apply --3way fallback chain is solid — handles renames, binary files, and multi-commit patches correctly.
• Binary file support via base64 (read_file_change) is a genuine gap-fill that the previous implementation was missing.
• PROTECTED_MANIFEST_BASENAMES / PROTECTED_PATH_PREFIXES are comprehensive and the case-insensitive matching is correct.
• count_patch_files (counting diff --git headers) correctly counts renames as 1 for max_files enforcement.
• The ExecutionResult::warning / exit-code-2 / SucceededWithIssues pipeline integration is clean end-to-end.

Generated by Rust PR Reviewer for issue #155 · ● 2.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security posture — has two real bugs and a documentation mismatch worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs — body not truncated in fallback failure message

  In execute_impl, when fallback_record_branch is true and PR creation fails, the raw ADO error body is embedded untrimmed in the failure_with_data message:

  return Ok(ExecutionResult::failure_with_data(
      format!(
          "Failed to create pull request: {} - {}. Branch '{}' was pushed...",
          status, body, source_branch,   // ← `body` can be arbitrarily large
      ),
      ...
  ));

  The data.description field inside the JSON correctly uses truncate_error_body(&body, 500), but the message field does not. Use truncate_error_body(&body, 500) in the message too, consistent with the rest of the file.

• src/safeoutputs/create_pr.rs — base_commit stores merge-base, not target-branch HEAD

  find_merge_base in Stage 1 returns the SHA of git merge-base HEAD origin/main, which is stored as CreatePrResult::base_commit. Stage 2 then uses this SHA as the parents field in the ADO Push API:

  "commits": [{ "parents": [base_commit], "changes": changes }]

  If the target branch has advanced past the merge-base (common case: other PRs landed while agent was running), the new source branch's git parent will be an older ancestor, not the current HEAD of main. The PR diff in ADO will be correct (it compares file states), but the branch history is disconnected from current main, which breaks fast-forward assumptions and can confuse git log --graph / rebase workflows for reviewers.

  The fallback (no base_commit field) correctly queries the ADO refs API for the live HEAD of the target branch. Consider always resolving from ADO in Stage 2 rather than trusting a potentially stale Stage 1 snapshot, or at minimum document this tradeoff explicitly in the field's doc comment.

⚠️ Suggestions

• src/main.rs — Exit code 2 has no corresponding template update

  } else if warning_count > 0 {
      // Exit code 2 signals "succeeded with issues" — the pipeline
      // step wraps this to emit ##vso[task.complete result=SucceededWithIssues;]
      std::process::exit(2);
  }

  templates/base.yml is not modified in this PR. If the ado-aw execute bash step doesn't already special-case exit 2, pipelines that produce only warnings will be treated as failures (exit(1) is the typical error indicator). Confirm the template already maps exit 2 to SucceededWithIssues, or add the template change to this PR.

• src/mcp.rs — PR description / config naming mismatch

  The PR description documents the new field as fallback-as-work-item: true and says it "records branch info in a work item". The actual config field is fallback-record-branch and no work item is created — just structured failure data. Update the PR description (or the AGENTS.md doc if it was written) to reflect the correct field name and behavior.

• src/mcp.rs — No RAII guard for synthetic commit cleanup

  If the process is killed (OOM, SIGKILL) between git commit (synthetic commit creation) and git reset HEAD~1, the synthetic commit persists in the agent's working repo. The current approach handles soft errors well (the warn + early return is correct), but a process kill bypasses it. Low probability in practice, but adding a SyntheticCommitGuard Drop impl similar to WorktreeGuard would make cleanup unconditional.

✅ What Looks Good

• Security validation is thorough: path traversal (validate_single_path), .git directory guard, absolute path rejection (including Windows C:\), protected files list with case-insensitive comparison, label allowlist validation — all solid.
• rename from/to with spaces in validate_patch_paths is correctly handled via splitn(3, ' ') and has a regression test.
• git am --3way → git apply --3way fallback with conflict marker detection (git diff --check) is the right approach and mirrors gh-aw behaviour.
• filter_excluded_files_from_patch correctly strips empty commit envelopes (format-patch multi-commit case) and has comprehensive tests including the all-excluded edge case.
• ExecutionResult::warning with the warning => success invariant is a clean addition; the summary math in execute.rs/main.rs is correct.
• Cryptographic branch salt (rand::rng().random()) is a clear improvement over the timestamp-based approach.

Generated by Rust PR Reviewer for issue #155 · ● 1.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured security improvements and feature parity — a few issues need attention before merge.

---

Findings

🐛 Bugs / Logic Issues

draft: true default silently breaks auto-complete: true

src/safeoutputs/create_pr.rs:1148 + 1286

The new draft: true default and auto-complete: true are mutually exclusive in ADO — you cannot enable auto-complete on a draft PR. When the two are combined, the PATCH to set autoCompleteSetBy will silently fail (the existing code only warns, never fails). Any existing config with auto-complete: true will now produce a draft PR that never auto-completes, with no diagnostic. At minimum, add a compile-time or runtime warning when draft: true && auto_complete: true are both set:

if config.draft && config.auto_complete {
    warn!("auto-complete cannot be set on a draft PR; set draft: false to enable auto-complete");
}

git add -A failure leaves a partially-staged index without cleanup

src/mcp.rs — generate_patch, after the git add -A block

When git add -A fails mid-operation, the code returns Err(...) immediately without resetting the index. The cleanup (git reset HEAD --quiet) is only placed after the git commit failure. Add the same reset guard to the add_output.status.success() failure branch so the agent's working tree is always left in a clean state.

🔒 Security Concerns

git added to the default bash allow-list without scoping discussion

src/compile/common.rs:387

git is an extremely broad command — it exposes subcommands like git push, git remote add, git fetch, git credential, git archive, and git bundle, all of which could be used to exfiltrate data or interact with unintended remotes even inside the AWF-firewalled container. The PR description justifies this by saying agents need git add and git commit before calling create-pull-request, but those are only needed inside the worktree at Stage 2 — the agent's sandbox runs at Stage 1. Consider limiting to a git:commit,add,diff,log,status allow-list or a separate tools.git config field rather than allowing the whole git binary by default. This is a high-risk default change.

base_commit from Stage 1 NDJSON used without format validation

src/safeoutputs/create_pr.rs:1007-1010

The base_commit SHA is written by the Stage 1 agent to NDJSON and then consumed at Stage 2 to set the parents field of the ADO push payload. There is no validation that the value is a well-formed 40-character hex SHA. A compromised agent could write an arbitrary string here. The ADO API would return an error (no RCE risk), but the failure path would embed the untrusted string in an error message. Add a simple format check:

let re = regex::Regex::new(r"^[0-9a-f]{40}$").unwrap();
if !re.is_match(recorded) { bail!("..."); }

Or since the project already uses regex-lite:

if !recorded.chars().all(|c| c.is_ascii_hexdigit()) || recorded.len() != 40 {
    bail!("Invalid base_commit SHA from Stage 1 NDJSON");
}

docs/CODEOWNERS not protected

src/safeoutputs/create_pr.rs:95

GitHub resolves CODEOWNERS from three locations: CODEOWNERS (root), .github/CODEOWNERS, and docs/CODEOWNERS. The current protection list covers the root path via PROTECTED_EXACT_PATHS and .github/ via PROTECTED_PATH_PREFIXES, but docs/CODEOWNERS has no coverage. Add "docs/CODEOWNERS" to PROTECTED_EXACT_PATHS or add "docs/" handling scoped to the CODEOWNERS filename.

⚠️ Suggestions

ExecutionResult::warning invariant not enforced during deserialization

src/safeoutputs/result.rs:115

The doc comment states "Invariant: warning == true implies success == true" but only skip_serializing_if is applied; the field is still fully deserialized. Deserializing {"success": false, "warning": true} would silently violate the invariant and cause incorrect exit-code counting. Either add a #[serde(skip_deserializing)] (warning is only set programmatically, never from NDJSON) or a custom deserializer that enforces the invariant.

**/*.lock glob relies on crate-specific zero-segment ** matching

src/safeoutputs/create_pr.rs — normalize_glob_pattern

**/*.lock matching Cargo.lock (no directory prefix) depends on glob-match treating ** as matching zero path segments including the separator. This is not universal glob behavior; the test passes with the chosen crate but the invariant isn't documented. Add a brief comment to normalize_glob_pattern explaining this assumption, or test explicitly that glob_match::glob_match("**/*.lock", "Cargo.lock") returns true in the test suite.

✅ What Looks Good

• Synthetic commit / reset pattern in generate_patch is carefully implemented — the reset happens before propagating any format-patch error, avoiding abandoned synthetic commits.
• git am --3way → git apply --3way fallback with conflict-marker detection is the right approach for handling patches that don't apply cleanly.
• Branch collision retry via random suffix regeneration is solid.
• File protection list is comprehensive — covering supply-chain manifests, CI configs, and agent instruction files.
• sanitize_text applied to ADO error bodies in fallback_record_branch and truncate_error_body — correct defence against ##vso[ injection in failure messages.
• ExecutionResult::failure_with_data and is_warning() are clean additions for structured error signalling.
• Exit code 2 → SucceededWithIssues pipeline integration in templates/base.yml is the right ADO pattern.
• Test coverage for protected-files, excluded-files, glob normalisation, multi-commit envelope stripping, and rename-with-spaces — all the tricky edge cases have tests.

Generated by Rust PR Reviewer for issue #155 · ● 2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good overall direction — the security additions (protected files, patch size cap, cryptographic branch suffix) are well-implemented and the git format-patch/git am upgrade is sound. A few correctness gaps need attention before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs — collect_changes_from_diff_tree() silently mishandles C (copy) status
  The function handles D, A, R*, and falls back to "edit" for everything else. git diff-tree --name-status can emit C100\told\tnew for copied files (when e.g. the patch contains a copy). The else branch emits changeType: "edit" with file_path = parts[1] (the source path), which will fail at the ADO Pushes API since that path doesn't exist as an edit target. In practice format-patch -M (no -C) shouldn't generate copies, but if git am --3way fallback produces a worktree state that the diff-tree path somehow reflects a copy it would silently produce a bad push payload. Consider adding an explicit C branch (treat like A on the destination path) and a warning for unexpected status codes.

• src/safeoutputs/create_pr.rs — extract_paths_from_patch() captures --- a/<path> for deleted files, causing protection check to block deletions
  For a patch that deletes package.json, the --- a/package.json line is captured (since +++ b/ is /dev/null). find_protected_files() then blocks the PR. Whether deleting a manifest is a security risk depends on the use case, but the PR description only mentions modifications to manifests are blocked. This is undocumented behavior that will surprise operators who legitimately need to remove a lockfile. Consider filtering out paths that appear only as --- a/ with a corresponding +++ /dev/null (i.e., pure deletes), or at minimum document this in the config field.

• src/safeoutputs/create_pr.rs — branch collision retry is single-shot
  Lines ~1055–1100: when a branch already exists the code generates one new hex suffix and continues. If that branch also exists (e.g. from a retry that itself partially failed) the subsequent push will fail with a confusing ADO error instead of a clean "branch already exists" message. With 32-bit entropy two collisions is effectively impossible, but wrapping the check in a for _ in 0..3 loop with a proper "exhausted retries" error would make the intent explicit.

---

🔒 Security Concerns

• src/safeoutputs/create_pr.rs — self.repository embedded unsanitized in fallback_description
  ~line 1230: self.repository is included directly in fallback_description without sanitize_text(). It's validated against the allowed-checkout list earlier so it can only be "self" or a clean alias — no realistic injection path — but it's inconsistent with how self.description and self.title are handled in the same block. One-liner fix: sanitize_text(&self.repository).

• src/safeoutputs/create_pr.rs — draft: true + auto_complete: true warns but still PATCHes autoCompleteSetBy
  The warning fires (~line 946) but execution continues. Further down (~line 1299), autoCompleteSetBy is unconditionally set when config.auto_complete == true, even when the PR is draft. ADO accepts this without error and the auto-complete will silently activate when the PR is published. For most operators this is unexpected — they set auto-complete: true to merge automatically, not to have it lurk and fire when someone manually publishes the draft. Consider returning ExecutionResult::failure(...) here rather than just warning, or at minimum documenting the deferred-merge behavior.

---

⚠️ Suggestions

• src/mcp.rs — generate_patch() leaves working tree in intermediate state if git reset HEAD~1 fails
  If the reset_output.status.success() check fails (~line 360 of mcp.rs diff), the function returns an error and the synthetic commit remains. The warning is helpful, but the error message should include the full current HEAD SHA (not just head_sha) and the exact reset command so operators can recover. The warn!() already prints the SHA — the error message just lacks the recovery command. Minor polish.

• src/safeoutputs/create_pr.rs — no opt-out for the provenance footer
  generate_pr_footer() is unconditionally appended to every PR description. Operators using PR templates (e.g., ADO's pull request description template) will find the footer breaks template validation. Consider a footer: true/false config field (defaulting to true for backward compat).

• src/safeoutputs/create_pr.rs — IfNoChanges::Warn returns ExecutionResult::warning() for the "excluded files" path but ExecutionResult::failure() in Default impl docs
  The IfNoChanges::Warn variant in the excluded-files block correctly emits warning(). The post-apply no-changes block also correctly emits warning(). Both consistent — just confirming this was intentional (exit code 2 = SucceededWithIssues is the right signal for "nothing to do").

• templates/base.yml — exit code 2 handling
  The ##vso[task.complete result=SucceededWithIssues;] emission followed by exit 0 is the correct ADO pattern. ✓

---

✅ What Looks Good

• The git format-patch → git am --3way → git apply --3way fallback chain is well-structured with proper cleanup of the synthetic commit in all exit paths.
• find_merge_base() with symbolic-ref refs/remotes/origin/HEAD detection (instead of hardcoding origin/main) is the right approach for non-standard default branches.
• The base64 binary file handling in read_file_change() is clean and correct.
• validate_patch_paths() fix for rename/copy paths with spaces (splitn(3, ' ').nth(2)) is exactly right and has a test.
• filter_excluded_files_from_patch() correctly handles multi-commit format-patch output including stripping empty commit envelopes.
• Protected-files system is comprehensive. Case-insensitive matching via to_lowercase() is correct for cross-platform compatibility.
• SHA validation on the recorded base_commit before trusting Stage 1 data is good defensive practice.

Generated by Rust PR Reviewer for issue #155 · ● 3.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid feature work with good security intent, but there are two correctness bugs worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs ~line 813 & ~line 851 — --exclude flags placed after the positional argument

  git am and git apply both follow POSIX-style option parsing where options after the mbox/patch file path are treated as additional file arguments, not flags. The current construction:

  vec!["am".into(), "--3way".into(), patch_path.to_string_lossy().into_owned()]
  // then: am_args.extend(exclude_args.iter().cloned())
  // result: git am --3way /path/to/patch.mbox --exclude=*.lock

  Git would try to open --exclude=*.lock as a second mbox file and fail. When excluded-files is configured, both git am and the git apply fallback would silently break. The fix is to insert exclude_args before the patch path:

  let mut am_args: Vec<String> = vec!["am".into(), "--3way".into()];
  am_args.extend(exclude_args.iter().cloned());
  am_args.push(patch_path.to_string_lossy().into_owned());

• src/safeoutputs/create_pr.rs ~line 985 — protected-files check fires before exclusions are applied, contradicting the comment

  The comment on line ~955 says: "Exclusion happens during patch application (before the protection check). If a protected file matches an excluded-files pattern, it is silently dropped rather than triggering a protection error."

  But the code extracts patch_paths from the raw patch content and runs find_protected_files before git am ever runs. So excluded-files: ["Cargo.lock"] + a patch that touches Cargo.lock will still fail with a protected-file error, even though Cargo.lock would have been excluded. The fix is to filter patch_paths against excluded_files before the protection check:

  let patch_paths: Vec<String> = extract_paths_from_patch(&patch_content)
      .into_iter()
      .filter(|p| !config.excluded_files.iter().any(|pat| glob_matches(pat, p)))
      .collect();

  (or alternatively, document clearly that exclusions do not bypass the protection check.)

⚠️ Suggestions

• src/safeoutputs/create_pr.rs ~line 971 — dead code rebinding

  let patch_content = patch_content; is a no-op that adds noise. Remove it.

• src/safeoutputs/create_pr.rs ~line 569 — misleading comment on commit_count <= 1 path

  In a single-commit repo the root commit IS HEAD, so git format-patch <root>..HEAD produces zero output. This hits "no changes detected" with a confusing error. Worth adding a targeted error here: "Cannot capture changes: repository has only one commit and no upstream tracking branch."

✅ What Looks Good

• Synthetic commit cleanup ordering is correct — format_patch_result is awaited before the reset, so cleanup always runs even on I/O failure.
• The base_commit SHA is validated (len == 40 && all hex) before trusting Stage 1 NDJSON data — good defense against tampered records.
• sanitize_text() applied consistently to all ADO error response bodies included in fallback failure messages.
• ExecutionResult::warning invariant (warning ⟹ success) is enforced by construction and the three summary counters are correctly disjoint.
• Cryptographic randomness for branch suffix is a meaningful improvement over the millisecond-timestamp approach.

Generated by Rust PR Reviewer for issue #155 · ● 1.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Substantial improvement — well-structured, good test coverage, solid security hardening. A few real issues worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs:glob_match_simple — **/ patterns are broken
  The **/ strip-prefix removes the prefix but then exact-matches the remaining pattern against the full path. **/Cargo.lock strips to Cargo.lock, then "subdir/Cargo.lock" == "Cargo.lock" → false. Anyone specifying excluded-files: ["**/Cargo.lock"] will find their exclusion silently ignored. Basename-only patterns (*.lock, package.json) work correctly because of the separate basename branch. Fix: after stripping **/, try matching against every path segment suffix, or use path.ends_with(&format!("/{}", rest)) || path == rest.

• src/safeoutputs/create_pr.rs:extract_paths_from_patch — false positives from commit messages
  git format-patch --stdout produces email-formatted output that includes the commit message before the --- diff separator. A commit message that happens to contain a line like --- a/Cargo.toml (e.g., quoting a patch or documenting a change) would be extracted as a path and potentially trigger the protected-files check, causing a spurious failure. count_patch_files has the same exposure for diff --git lines in commit messages. Mitigation: skip lines before the first diff --git header in each patch email.

• src/safeoutputs/create_pr.rs:~line 1050 — branch collision retry is one-shot
  After checking ADO refs and finding a collision, a new random suffix is generated once. If that new branch also collides (low probability but possible under retries or parallel runs), the push will fail with a confusing error. A short loop (e.g., up to 3 attempts) would be more robust.

⚠️ Suggestions

• src/safeoutputs/create_pr.rs — draft: true default silently breaks existing auto-complete: true configs
  This is a semantic breaking change. Any existing pipeline with auto-complete: true that upgrades will silently have auto-complete ignored because draft: true takes precedence (and the code only warn!s, not bail!s). Since auto-complete and draft are mutually exclusive, consider either: (a) defaulting draft: false (consistent with the old implicit behavior), or (b) returning ExecutionResult::failure if both are set. At minimum, the warning should be much louder — perhaps a compile-time validation error.

• src/safeoutputs/create_pr.rs:fallback_description — original PR description sanitization coverage
  sanitize_text(&self.description) in the fallback string is correct, but self.description was already sanitized via Sanitize::sanitize_fields before execute() is called. The double-sanitize is harmless but worth noting. More importantly, source_branch and target_branch in the fallback description aren't wrapped in sanitize_text — they're safe here since source_branch is derived from slugify_title output, but the pattern is inconsistent.

• src/mcp.rs:generate_patch — synthetic commit not cleaned up on git reset IO error
  If the git reset HEAD~1 command itself fails to spawn (IO error), the ? on map_err(...)? returns early without printing the "synthetic commit may remain" warning. The error message "Failed to run git reset" is returned, which is sufficient, but adding the commit SHA to that error message would help operators recover manually.

• src/safeoutputs/create_pr.rs:patch_content redundant rebinding
  let patch_content = patch_content; at ~line 660 is a no-op. Looks like a refactor artifact.

✅ What Looks Good

• The synthetic commit cleanup-before-error-check pattern is well-structured — format_patch_result is awaited first, cleanup always runs, then the result is inspected. Correct ordering.
• find_merge_base fallback hierarchy (symbolic-ref → common defaults → root commit guard) is sensible.
• Protected-files check correctly handles case-insensitive basename matching, and the pre-filter of excluded-files before the protection check (so excluded_files: ["*.lock"] prevents Cargo.lock from tripping the manifest check) is a good design decision.
• SHA validation (len != 40 || !chars().all(|c| c.is_ascii_hexdigit())) on base_commit from NDJSON before trusting Stage 1 data is a good security practice. Propagating as anyhow::Err is fine here — execute_safe_output catches it at the outer match and converts it to ExecutionResult::failure.
• Binary file support via base64 fallback is a nice addition.
• The warning invariant in ExecutionResult (warning == true implies success == true) is correctly maintained through private field + constructor discipline.
• Exit code 2 → SucceededWithIssues pipeline integration in base.yml is clean.

Generated by Rust PR Reviewer for issue #155 · ● 1.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good overall direction — the security additions (file protection, crypto branch IDs, patch format upgrade) are solid, but there are 3 correctness issues worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

1. count_patch_files double-counts files in multi-commit format-patch output (src/safeoutputs/create_pr.rs)

git format-patch emits one diff --git header per file per commit. If an agent has two commits each touching src/main.rs, the counter reports 2, potentially triggering the max_files rejection for a PR that only changes one logical file. The comment says "each header represents exactly one file change" — true per-commit, but not unique across the whole patch series. Consider deduplicating on path instead:

fn count_patch_files(patch_content: &str) -> usize {
    patch_content
        .lines()
        .filter_map(|l| l.strip_prefix("diff --git a/"))
        .collect::<std::collections::HashSet<_>>()
        .len()
}

2. ExecutionResult::warning missing #[serde(default)] (src/safeoutputs/result.rs)

The field has skip_serializing_if (omits false from output) but no serde(default). Deserializing any ExecutionResult JSON that was serialized before this PR (i.e., without the warning key) will fail. ExecutionResult isn't currently persisted to disk, so it's latent — but it's a footgun for anyone who adds persistence or caching later, and the attribute pattern is inconsistent with how the rest of the codebase handles additive fields:

+    #[serde(default, skip_serializing_if = "std::ops::Not::not")]
     warning: bool,

3. collect_changes_from_diff_tree silently drops copy operations (src/safeoutputs/create_pr.rs)

The function handles A, D, R<score>, and falls through for everything else (treated as edit). git diff-tree --name-status also emits C<score>\told\tnew for copies. The fallback else branch reads file_path (= parts[1] = the old path) as an edit — it will try to read a file that may no longer exist at that path, or push a content update to the wrong ADO path. Copies are uncommon but they can appear when agents use cp + modify. Should handle C analogously to R.

---

⚠️ Suggestions

4. draft: true + auto_complete: true emits a warning but still proceeds (src/safeoutputs/create_pr.rs, line ~314–318)

ADO's API silently rejects auto-complete on draft PRs — the PATCH call will either error or be a no-op, but the operator gets no feedback except the Stage 2 log. Since this is an operator configuration error (both fields in front matter), it's cleaner to return ExecutionResult::failure here rather than a warn! that most operators won't see until they investigate logs.

5. Combined title length not validated after prepending title_prefix (src/safeoutputs/create_pr.rs, ~line 321–325)

CreatePrParams::validate() checks title is ≤ 200 chars, but effective_title = format!("{}{}", prefix, self.title) can exceed ADO's 400-char PR title limit. Worth adding a post-prefix length check before the push:

if effective_title.len() > 400 {
    return Ok(ExecutionResult::failure(format!(
        "PR title too long after applying title-prefix ({} chars, max 400)",
        effective_title.len()
    )));
}

6. find_merge_base fragile under shallow clones (src/mcp.rs)

ADO pipelines frequently use --depth=1 or fetchDepth. With a shallow fetch, git merge-base HEAD origin/main may fail if the common ancestor isn't in the local history (depth truncation). The function then falls through to the commit_count <= 1 single-commit guard, which will fail for any real repo — returning a confusing "Cannot determine diff base" error. Consider adding a deepen-or-log path, or at minimum making the error message mention shallow clones as a possible cause.

---

✅ What Looks Good

• Crypto random for branch suffix — clear improvement over timestamp & 0xFFFFFF which was predictable within a millisecond window
• Synthetic commit cleanup is correct — format_patch_result is stored before the cleanup block runs, so the cleanup executes even when format-patch fails; no resource leak
• truncate_error_body uses char_indices — correct UTF-8 boundary handling
• sanitize_text applied to ADO error responses in the fallback description — prevents ##vso[ injection from a malicious ADO server response
• base_commit SHA format validation — right to validate length/charset before trusting Stage 1 NDJSON data
• Rename/copy path parsing with spaces (splitn(3, ' ').nth(2)) — the old split_whitespace().last() was a genuine path traversal bypass for paths with embedded spaces; good catch with a regression test
• Binary file support via base64 fallback in read_file_change is a clean addition

Generated by Rust PR Reviewer for issue #155 · ● 2.5M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good overall direction with some important concerns — the draft: true default is a breaking behavioral change, and the conflict marker regex has a false-positive pattern worth fixing.

---

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs — draft: true is a silent breaking default

  fn default_draft() -> bool { true }

  Every existing pipeline that doesn't explicitly set draft: false will start creating draft PRs instead of ready-to-review PRs. This will silently change the behavior of every deployed agent. The combination with auto-complete: true is caught by a warn! but not an error — auto_complete is also default: false, so enabling auto-complete already requires an explicit config change. That means the practical impact is real: pipelines currently creating ready PRs will switch to draft-only.

  Consider defaulting to false (matching the pre-existing behavior) or emitting a compilation warning asking operators to set an explicit value.

• src/safeoutputs/create_pr.rs:541 — conflict marker regex false positives

  .args(["grep", "-l", "-E", r"^(<<<<<<<|=======|>>>>>>>)"])

  The ^======= pattern matches any line consisting solely of 7 equals signs at the start — this appears legitimately in reStructuredText section dividers and markdown. A file with ======= as a title underline (e.g., a 7-char heading) would falsely fail the conflict check, causing a valid patch application to be rejected.

  Use the more specific git conflict marker patterns that include surrounding context: ^<<<<<<< (note trailing space — git always writes <<<<<<< HEAD or similar) and ^>>>>>>> to reduce false positives. The ======= separator line alone is too common to be reliable as a standalone check.

• src/safeoutputs/create_pr.rs:328 — effective_title.len() uses byte count for a character-based limit

  if effective_title.len() > 400 {

  .len() on a Rust String returns byte length, not Unicode character count. A title with 200 CJK characters (600 bytes) would be rejected even though it's well within ADO's 400-character limit. Use effective_title.chars().count() > 400 instead.

• src/safeoutputs/create_pr.rs:1293 — count_patch_files misparses filenames containing b/

  .and_then(|rest| rest.rsplit_once(" b/"))

  For a diff --git header like diff --git a/src b/dir b/src b/dir, rsplit_once(" b/") finds the last occurrence, yielding path dir instead of src b/dir. This causes count_patch_files to under-count distinct files, potentially allowing oversized patches past the max_files check.

  This affects the max-files security boundary. The existing validate_patch_paths / extract_paths_from_patch functions already handle quoted paths correctly — consider reusing their approach here, or parsing the quoted git header form.

⚠️ Suggestions

• src/safeoutputs/create_pr.rs:375-377 — ends_with(pat) is a dead check for glob patterns

  p.ends_with(pat) || glob_match_simple(pat, p)

  String::ends_with does literal string matching, so "Cargo.lock".ends_with("*.lock") returns false (it checks for the literal *). For patterns with *, only glob_match_simple ever matches. For literal patterns (e.g., Cargo.lock), the ends_with check short-circuits before glob_match_simple — but glob_match_simple handles literals too (via wildcard_match with no * parts). The ends_with call can be removed with no functional change.

• Branch collision TOCTOU — the check-then-push loop queries ADO refs before pushing, but another run could create the same branch between the check and the push. In that case the ADO push API will return a meaningful error, but the retry loop won't catch it (it only retries on the existence check). If branch collision is a real concern, catching and retrying on the specific ADO push error (HTTP 409 / "branch already exists") would be more reliable.

✅ What Looks Good

• Replacing timestamp-based branch suffixes with rand::rng().random() is the right call — SystemTime millis are predictable and not collision-safe in concurrent environments.
• The protected-files system is well thought out: case-insensitive comparison, prefix matching for CI dirs, and basename matching for manifests are all correct. The tests are thorough.
• failure_with_data with branch/target info for manual recovery is a solid UX improvement.
• Sanitizing ADO API error bodies via sanitize_text(truncate_error_body(&body, 500)) before embedding in results is good hygiene.
• The ##vso[task.complete result=SucceededWithIssues;] exit-code-2 mapping in base.yml is correctly handled.
• git am --exclude is confirmed to work correctly (unlike what I initially suspected — verified in the environment).

Generated by Rust PR Reviewer for issue #155 · ● 2.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Substantial improvement to create-pull-request — well-structured with good security layering. One confirmed bug in the git am/--exclude interaction; a few other concerns worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs — git am --exclude= will always fail when excluded_files is configured (confirmed with git am --unknown-flag 2>&1 → "error: unknown option")

  git am does not support --exclude. When excluded_files is non-empty, am_args contains --exclude=pattern arguments; git am rejects these and exits non-zero, triggering the fallback to git apply --3way --exclude=... (which does support --exclude). The end result is that excluded-files behaviour is correct only via the fallback path, never via git am. The PR description and comments ("Excluded files are handled via --exclude flags on git am / git apply") are therefore misleading. Fix: build exclude_args only for the git apply branch, not for am_args.

  // current — wrong, git am has no --exclude
  let mut am_args = vec!["am".into(), "--3way".into()];
  am_args.extend(exclude_args.iter().cloned());  // ← will always fail if non-empty
  
  // fix
  let mut am_args = vec!["am".into(), "--3way".into()];
  // exclude_args are only valid for git apply

• src/mcp.rs — synthetic commit left behind on git format-patch I/O error

  format_patch_result is stored before the made_synthetic_commit reset path, but the actual .map_err()? unwrap happens after cleanup. However, if Command::new("git").output().await itself returns Err (i.e., the git binary can't be spawned — the outer ? in format_patch_result.map_err(|e| ...)?), the reset does still run because the reset block is unconditional. This is actually handled correctly; just noting it as the flow is subtle enough to warrant the comment that's already there. ✓

⚠️ Suggestions

• src/safeoutputs/create_pr.rs:~1862 — glob_match_simple / wildcard_match have no unit tests

  These functions gate the excluded_files pre-protection filter — if a protected file (e.g. package.json) matches an exclusion pattern, it bypasses the protection check entirely. This is security-adjacent code that really should have tests. A few cases worth covering: *.lock basename match against a nested path, **/*.json depth match, patterns with no /, a pattern that should not match an adjacent protected file.

• src/safeoutputs/create_pr.rs — draft: true is a silent breaking default

  Flipping the default from false → true will silently convert all previously non-draft PRs to draft on next compilation, which may break auto-complete pipelines or auto-merge branch rules that require a "ready for review" state. This warrants a callout in the CHANGELOG / migration notes beyond just the PR body, and arguably a compile-time warning (like the existing workspace mismatch warn) when draft: true and auto-complete: true coexist (the code already warns, but not when users didn't set draft explicitly and it defaulted to true).

• src/safeoutputs/create_pr.rs:~1930 — extract_paths_from_patch resets in_diff on bare "From " lines

  The guard is correct for format-patch envelope headers, but the code comment says "A new commit envelope resets — skip until next diff --git". This means paths in subsequent commits are only collected after the next diff --git header, which is the right behaviour. However, this logic silently discards paths from a commit's message that happen to contain quoted patch fragments (unusual but possible). Low risk in practice; worth a defensive note.

• src/safeoutputs/create_pr.rs — max_files: 0 is silently accepted

  max-files: 0 would block every PR. No validation or warning is emitted. Consider a minimum of 1 (or emit a compile-time warning analogous to the max-turns deprecation path).

• src/safeoutputs/create_pr.rs — branch suffix replacement with rfind('-') is fragile

  source_branch = if let Some(pos) = source_branch.rfind('-') {
      format!("{}-{}", &source_branch[..pos], new_hex)
  } else {
      format!("{}-{}", source_branch, new_hex)
  };

  If the slugified title itself contains a hyphen (e.g. agent/fix-the-bug-00aa1122), rfind('-') strips the original random suffix correctly. But on a second retry (the TOCTOU path fires after the pre-check loop already ran 3 attempts), the same pattern runs again and produces agent/fix-the-bug-<new_hex> rather than keeping the previous new suffix. This is benign (still unique), but after two retries the branch name becomes agent/fix-<new-suffix> instead of agent/fix-the-bug-<new-suffix>, dropping slug segments. A helper that explicitly tracks the suffix position would be cleaner.

✅ What Looks Good

• The synthetic-commit / git reset HEAD~1 cleanup in generate_patch is carefully ordered: format-patch output is stored before the reset, cleanup runs unconditionally, then the stored result is checked. No window for a leaked commit on error paths.
• base_commit SHA validation (len == 40 && all hex) before trusting Stage 1 NDJSON data is good defensive practice.
• sanitize_text() applied to all agent-controlled strings in fallback_record_branch error output and Sanitize impl for agent_labels.
• Conflict-marker regex ^(<<<<<<<\s|>>>>>>>\s) requiring trailing whitespace correctly avoids false positives from reStructuredText/markdown horizontal rules.
• Exit-code-2 → SucceededWithIssues pipeline integration is clean: main.rs exits 2, base.yml wraps it without touching non-2 exit codes.

Generated by Rust PR Reviewer for issue #155 · ● 2.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large, well-structured improvement — mostly looks good with two correctness issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs:1723-1730 — validate_patch_paths still uses split_whitespace() for ---/+++ lines.
  The PR correctly fixes rename/copy path parsing (using splitn(3, ' ').nth(2)) and adds a test for it, but the ---/+++ lines still use split_whitespace().nth(1). For --- a/path with spaces/file.txt, only a/path is extracted and validated — the rest of the path is silently skipped. extract_paths_from_patch (added in the same PR) correctly uses strip_prefix("--- a/") and captures the full path. These two functions are now inconsistent and the fix should be applied to validate_patch_paths too. The same issue exists for the diff --git header parsing. In practice the security impact is minimal (git refuses to create objects with .. as path components), but the inconsistency undermines the test added for rename paths.

• src/safeoutputs/create_pr.rs:1713-1739 — validate_patch_paths has no in_diff guard, so it validates paths from commit message bodies in format-patch output.
  extract_paths_from_patch tracks an in_diff boolean to avoid false-positives from commit messages. validate_patch_paths doesn't. A commit message containing a line like --- a/path/to/file would trigger path validation on non-diff content, potentially causing false-positive rejections for patches whose commit messages happen to reference paths that fail validation (e.g., any mention of .git/... documentation). This was not an issue with the previous git diff format but is newly relevant with git format-patch.

⚠️ Suggestions

• src/safeoutputs/create_pr.rs — draft: true default is a silent breaking change. Existing configurations that rely on PR creation without specifying draft: will now get draft PRs. Worth calling out in the CHANGELOG or with a migration note, since auto_complete (which has been in production) is now silently incompatible with the default draft: true. The compile-time warning (if config.draft && config.auto_complete) helps operators discover the conflict, but only after they've already had an unexpected behavior.

• src/mcp.rs:1856-1895 — extract_paths_from_patch resets in_diff = false on "From " lines (the commit envelope) but in_diff is never set back to true until the next diff --git line. The logic is correct, but a multi-commit format-patch file resets in_diff for each commit envelope. There's no gap, but it's worth a comment clarifying that the next diff --git reactivates it.

• src/safeoutputs/create_pr.rs — No test for read_file_change with binary content. The base64 fallback path is new and important for binary files. A simple test with non-UTF-8 bytes would confirm the base64encoded content type is emitted correctly.

✅ What Looks Good

• CSPRNG branch suffix (rand::rng().random::<u32>()) is a solid improvement over the predictable millisecond timestamp. The collision retry logic (pre-check loop + TOCTOU fallback on 409/400) is well-thought-out.
• collect_changes_from_diff_tree correctly gates the extra edit emit on status_code != "R100" — only rename-with-modifications get the content update.
• Base commit SHA validation (len() != 40 || !all ascii_hexdigit) before trusting Stage 1 NDJSON data is exactly right.
• Synthetic commit cleanup is careful: the reset happens before propagating the format-patch error, and the HEAD SHA is captured for diagnostics in case the reset itself fails.
• validate_single_path is robust — .. traversal is detected at the component level (split on ['/','\\']), so even imprecisely-split paths from split_whitespace can't hide a .. component.
• Test coverage for protected files, glob matching, path extraction, and config defaults is comprehensive.
• sanitize_text(truncate_error_body(&body, 500)) for the fallback failure description correctly defends against ##vso[ injection in ADO error responses.

Generated by Rust PR Reviewer for issue #155 · ● 1.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Mostly solid — good security thinking throughout, but there's one correctness bug in the glob implementation and a misleading error message in the merge-base fallback worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• wildcard_match doesn't respect directory separators (src/safeoutputs/create_pr.rs, new wildcard_match fn)

  * is treated as matching any character sequence including /, so wildcard_match("src/*.rs", "src/nested/file.rs") returns true when standard glob semantics say it shouldn't. The ends_with check for the last segment doesn't enforce that * can't cross a /:

  } else if i == parts.len() - 1 {
      return remaining.ends_with(part);  // remaining = "nested/file.rs" → ".rs" suffix matches
  }

  This only affects excluded-files patterns that include a / (e.g. src/*.lock), and it makes exclusions more permissive (more files excluded) rather than fewer — so the blast radius is limited. But an operator who writes src/*.lock expecting only direct children to be excluded will silently get recursive exclusion, potentially bypassing the protected-file check for a file they didn't intend to exclude. Fix: guard against / in the middle of the matched range for single-* patterns.

  Note: the **/*.json and bare *.lock patterns work correctly because they take the ** or basename-only code paths before reaching wildcard_match.

• Misleading error in find_merge_base for diverged branches (src/mcp.rs, find_merge_base)

  When a repo has origin/main but HEAD shares no common ancestry (e.g. an orphan branch or a complete rebase-and-push), all git merge-base HEAD origin/main calls exit non-zero and are silently skipped. The function falls through to the commit_count <= 1 check, which fails for multi-commit repos, and returns:

  "Cannot determine diff base: no remote tracking branch found"

  But the tracking branch exists — it just has no common ancestor with HEAD. The error will confuse operators debugging this scenario. Consider distinguishing "tracking branch exists but no common ancestor" from "no tracking branch at all".

⚠️ Suggestions

• generate_patch synthetic-commit lifetime (src/mcp.rs, generate_patch)

  The synthetic commit is created, format-patch is spawned (.output().await captures the result), and then the reset runs. If the process crashes between commit and reset (e.g. OOM kill), the synthetic commit remains in the agent's repo. This is acknowledged in the warn! log, but since this runs inside the agent's real working directory (not a worktree), a stranded commit could confuse subsequent agent invocations. A lighter alternative that avoids committing: use git stash before the merge-base diff, or just pass --cached + unstaged diffs separately. Not a blocker, but worth a FIXME note if the synthetic-commit approach stays.

• base_commit from Stage 1 as ADO push parent (src/safeoutputs/create_pr.rs, execute)

  When self.base_commit is present, it's used as "parents": [base_commit] in the ADO Push API payload. The merge-base is by definition an ancestor of the target branch, so this is semantically correct. However, if the target branch was force-pushed after Stage 1 ran, the merge-base SHA might no longer be reachable in ADO's history — the push would fail with a cryptic 400. The existing fallback (resolving live target branch HEAD when base_commit is None) doesn't help here. Low probability but worth calling out: if you see unexpected 400s in practice, this is the first thing to check.

• auto-complete + draft warning doesn't prevent creating the PR (src/safeoutputs/create_pr.rs, ~line 533)

  if config.draft && config.auto_complete {
      warn!("auto-complete cannot be set on a draft PR; ...");
  }

  The warn fires but execution continues — config.auto_complete is still applied to the PR body, and ADO will silently ignore it for draft PRs. Consider returning an ExecutionResult::failure here to give operators a clear signal, or just skip the auto-complete call when draft: true. As-is, the operator gets a log warning but the PR is created silently without auto-complete, which may surprise them.

✅ What Looks Good

• CSPRNG upgrade is the right call — timestamp-based IDs were guessable. rand::rng().random::<u32>() gives 32 bits of entropy.
• Both apply paths check for conflict markers — I initially thought the use_git_apply branch missed this, but it has its own git grep check for <<<<<<< / >>>>>>> patterns. Good symmetry.
• sanitize_text on ADO error responses in fallback_description prevents ##vso[ injection from a malicious ADO server echoing back commands. Correct layering.
• base_sha capture before patch apply + diff-tree base_sha..HEAD for multi-commit patches is the right approach — avoids missing changes from earlier commits in a multi-patch series.
• Protected-file constants are lowercase + case-insensitive comparison — handles Cargo.toml / cargo.toml / CARGO.TOML uniformly.
• ExecutionResult::warning invariant (warning == true → success == true) is documented and the serde skip attribute #[serde(skip_serializing_if = "std::ops::Not::not")] is an unusual but correct idiom.

Generated by Rust PR Reviewer for issue #155 · ● 2.7M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security posture and test coverage — a few issues worth following up on.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/create_pr.rs:528 — draft: true + auto-complete: true warning-only, no enforcement

  When draft: true (the new default) and auto-complete: true are both configured, a warn! is emitted but the code still proceeds to PATCH autoCompleteSetBy on the draft PR. ADO's behavior varies — it may silently accept the setting or return a non-success status (which is only warn!-ed at line 1412). An operator who writes auto-complete: true without explicitly setting draft: false will get a draft PR with auto-complete silently missing. Suggested fix: skip the autoCompleteSetBy patch (or convert to ExecutionResult::warning) when config.draft && config.auto_complete.

• src/safeoutputs/create_pr.rs:1371 — pr_id silently defaults to 0

  let pr_id = pr_data["pullRequestId"].as_i64().unwrap_or(0);

  If the ADO response is malformed (missing pullRequestId), pr_id = 0. All subsequent calls (completion options PATCH, reviewer PUT) target pullrequests/0?api-version=7.1, fail silently with warn!, and the PR is created without the configured options. Suggest anyhow::bail! or context("missing pullRequestId from ADO response") here since an absent PR ID is a genuine API contract violation.

⚠️ Suggestions

• src/safeoutputs/create_pr.rs:879–956 — Duplicate conflict marker detection

  The conflict marker scan (git grep -l -E "^(<<<<<<<\s|>>>>>>>\s)") is copy-pasted verbatim into both branches of the if use_git_apply / else (git am fallback) block. This is ~15 lines × 2 = 30 lines that should be a helper function, especially since the conflict pattern itself would need to be updated in two places if it ever changes.

• src/mcp.rs — find_merge_base error message for multi-commit repos without remotes is surprising

  For a repo with multiple commits but no remote tracking ref (e.g., a fresh ADO repo where the agent checked out a branch but origin/HEAD isn't set), the fallback messages say "shallow clones" or "no remote tracking branch". The commit_count <= 1 guard correctly prevents using the root SHA for repos with history, but leaves multi-commit repos with no remote in a hard error with a message that may confuse operators who aren't doing shallow clones. Adding a branch like fetchDepth: 0 to the pipeline template checkout may be worth documenting.

• src/safeoutputs/create_pr.rs:1765–1779 — Doubled function-level doc comment

  glob_match_simple has two /// doc-comment blocks that say essentially the same thing. Only the second one (above the function signature) is rendered by rustdoc; the first is on the preceding function's closing }.

✅ What Looks Good

• Synthetic commit + cleanup pattern is carefully ordered: git format-patch is captured before git reset HEAD~1, cleanup happens even if format-patch fails, and the captured SHA is included in error messages for diagnostics. Clean.
• base_commit SHA validation (40 hex chars) before accepting Stage 1 NDJSON data is good defense-in-depth against corrupted records.
• sanitize_text(truncate_error_body(&body, 500)) consistently applied to all ADO API error responses prevents ##vso[ injection from propagating into error messages.
• validate_patch_paths for rename from with spaces is correctly fixed via splitn(3, ' ').nth(2) — the new regression test (test_validate_patch_paths_rename_with_spaces) covers the exact bypass scenario.
• Label validation (case-insensitive eq_ignore_ascii_case against allowed-labels) and dedup on merge with operator labels is correct.
• Branch collision retry correctly handles both pre-push (3 attempts) and TOCTOU during push (1 retry on 409/400 "already exists").

Generated by Rust PR Reviewer for issue #155 · ● 2M · ◷