feat: add 12 new safe output tools with gh-aw alignment (#72-#79)

[jamesadevine]

Summary

Implements all 8 remaining safe output feature requests (issues #72–#79), plus 4 additional tools and enhancements to align with gh-aw's PR review capabilities. Takes the project from 6 to 18 safe output tools.

---

New Tools — Feature Issues

Tool	Issue	Description
add-pr-comment	#72	Comment threads on ADO pull requests (general + inline + multi-line)
link-work-items	#73	Work item relationships (parent, child, related, predecessor, successor, duplicate)
queue-build	#74	Trigger ADO pipeline/build runs with branch and template parameters
create-git-tag	#75	Annotated git tags on commits
add-build-tag	#76	Classification tags on builds
create-branch	#77	Branch creation without PR
update-pr	#78	PR metadata (reviewers, labels, auto-complete, vote, description)
upload-attachment	#79	File attachments on work items

New Tools — gh-aw Alignment

Tool	gh-aw Equivalent	Description
submit-pr-review	submit_pull_request_review	Review with decision (approve/request-changes/comment) + body rationale
reply-to-pr-review-comment	reply_to_pull_request_review_comment	Reply to existing review comment threads
resolve-pr-review-thread	resolve_pull_request_review_thread	Resolve/reactivate review threads
report-incomplete	report_incomplete	Signal infrastructure failure prevented task completion

Enhancements (gh-aw parity)

• add-pr-comment: start_line param for multi-line code comments
• link-work-items: DEFAULT_MAX=5 (matches gh-aw)
• queue-build: DEFAULT_MAX=3 (prevents flooding)

---

Per-tool structure

Each tool includes:

• Params with Validate trait — input validation
• Config — front matter configuration with serde rename for kebab-case YAML
• Sanitize — text field sanitization
• Executor — full ADO REST API implementation with config policy enforcement
• Unit tests — ~110 new tests

Integration points updated

• src/tools/mod.rs — module registration + re-exports
• src/mcp.rs — 12 new MCP tool handlers with sanitization
• src/execute.rs — dispatch cases + budget registration
• src/compile/common.rs — WRITE_REQUIRING_SAFE_OUTPUTS list

Security highlights

• permissions.write required at compile time for all write tools
• Config-driven allow-lists (allowed-pipelines, allowed-repositories, allowed-operations, allowed-events, allowed-link-types, etc.)
• submit-pr-review + update-pr vote: require explicit allowed-events/allowed-votes — empty config rejects all (prevents accidental auto-approve)
• queue-build: require explicit allowed-pipelines — empty rejects all
• upload-attachment: symlink escape protection via canonicalize() + prefix check, ##vso[ scanning, size limits
• Path traversal protection, identity lookup via VSSPS for reviewer GUIDs
• All ADO URLs derived from org_url context (supports vanity domains, national clouds)

Test results

602 tests passing (559 unit + 29 integration + 14 other)
0 new clippy warnings

Closes #72, closes #73, closes #74, closes #75, closes #76, closes #77, closes #78, closes #79

[github-actions]

🔍 Rust PR Review

Summary: Needs changes — solid foundation with good patterns throughout, but two correctness bugs and one meaningful security gap.

---

Findings

🐛 Bugs / Logic Issues

1. create_git_tag.rs — resolve_head_commit hardcodes heads/main

// Line ~170
"{}/{}/_apis/git/repositories/{}/refs?filter=heads/main&api-version=7.1"

The filter is literally heads/main. Any repo whose default branch is master, develop, or anything else will always fail with "No refs found for heads/main — cannot resolve HEAD commit". The fix is to either:

• Accept the branch name as a parameter (look it up from CreateGitTagConfig)
• Fetch the repository object first (GET /repositories/{repo}) and use defaultBranch, or
• Add a default-branch field to CreateGitTagConfig (mirroring the pattern in QueueBuildConfig)

---

2. update_pr.rs — execute_add_reviewers puts email address as reviewer ID in the ADO URL

let reviewer_url = format!(
    "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1",
    base_url, encoded_repo, self.pull_request_id,
    utf8_percent_encode(reviewer, PATH_SEGMENT),  // ← reviewer is an email
);
```

The `PUT .../reviewers/{reviewerId}` ADO endpoint expects a user **GUID/descriptor** as `reviewerId`, not an email address. Passing an email will consistently return a `404` or `400`. In practice the `add-reviewers` operation will always silently fail — every reviewer ends up in the `failed` list and the operation returns partial-success.

The fix requires an identity lookup step first:
```
GET (vssps.dev.azure.com/redacted)

Then use the returned id field in the reviewer URL.

---

🔒 Security Concerns

3. update_pr.rs — Voting approve is unrestricted by default

The PR description highlights "vote restrictions to prevent unauthorized approvals," but looking at the actual default:

impl Default for UpdatePrConfig {
    fn default() -> Self {
        Self {
            allowed_votes: Vec::new(),   // ← empty = all votes allowed
            ...
        }
    }
}

And the enforcement:

if !config.allowed_votes.is_empty() && !config.allowed_votes.contains(&vote_str.to_string()) {

An empty allowed_votes list (the default) grants the agent permission to cast any vote, including approve. Most pipeline operators who don't read the fine print will expose this operation without realising the agent can auto-approve PRs. This inverts the expected safety posture.

Suggested fix: flip the default so that vote is only permitted when explicitly listed in allowed-votes:

if config.allowed_votes.is_empty() {
    return Ok(ExecutionResult::failure(
        "vote operation requires allowed-votes to be configured"
    ));
}

Or at minimum restrict approve / approve-with-suggestions by default unless explicitly opted in.

---

⚠️ Suggestions

4. update_pr.rs — set-auto-complete hardcodes merge options

"completionOptions": {
    "deleteSourceBranch": true,   // ← always true
    "mergeStrategy": "squash"     // ← always squash
}

These options are hardcoded and not configurable via front matter. An operator who wants mergeStrategy: rebase or deleteSourceBranch: false has no way to express this. UpdatePrConfig could add optional fields (delete-source-branch, merge-strategy) to match the existing CreatePrConfig pattern.

---

5. add_pr_comment.rs — inline file_path not validated in Validate::validate()

validate_file_path (path traversal/absolute-path check) is only called in execute_impl, not in AddPrCommentParams::validate(). Other tools (e.g., upload_attachment) validate all inputs upfront in validate(). Here, a malformed path gets serialised into the NDJSON safe-output file and only rejected at Stage 2, with no feedback to the agent at Stage 1.

---

6. queue_build.rs — parameters values not sanitized

impl Sanitize for QueueBuildResult {
    fn sanitize_fields(&mut self) {
        if let Some(reason) = &self.reason {
            self.reason = Some(sanitize_text(reason));
        }
        // ← parameters HashMap values are not sanitized
    }
}

Parameter values are user-supplied strings that end up in ADO build logs. Other tools sanitize all free-text fields; parameters should too to prevent ##vso[ sequences in values.

---

✅ What Looks Good

• Config-driven allow-lists on every new tool are well-structured and consistent.
• upload_attachment.rs has thorough path-traversal + ##vso[ validation, and the orphan-attachment scenario is clearly logged.
• queue_build.rs making allowed-pipelines required (empty list → failure) is the right security default for a trigger-pipeline tool.
• link_work_items.rs is clean and the relation-type mapping table is clearly commented.
• add_build_tag.rs prefix-before-allow-list ordering is correct.

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

[github-actions]

🔍 Rust PR Review

Summary: 8 new tools with solid overall structure, but two bugs in update_pr.rs that will cause runtime failures, and one security gap in upload_attachment.rs.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/update_pr.rs:509 — execute_vote hardcodes (dev.azure.com/redacted) instead of deriving the base URL from org_urlin context. Every other API call in the codebase usesorg_url.trim_end_matches('/')as the base, but this one ignores it entirely. If the pipeline'sAZURE_DEVOPS_ORG_URLis anything other than the canonical(dev.azure.com/redacted) (e.g. a vanity domain, a national cloud endpoint, or a test environment), the vote operation will always hit the wrong host and fail at runtime.

  // Bug: hardcoded to cloud ADO
  let connection_url = format!(
      "(dev.azure.com/redacted),
      utf8_percent_encode(organization, PATH_SEGMENT)
  );
  // Should be derived from ctx.ado_org_url, not org_url + /_apis/connectiondata

• src/tools/update_pr.rs:622 — Same issue for the VSSPS identity lookup in execute_add_reviewers. (vssps.dev.azure.com/redacted) is hardcoded. This is a separate service endpoint that's also environment-specific. The vsspsendpoint for cloud ADO is knowingly different fromorg_url, but the consequence is that add-reviewers` will always silently fail in non-standard environments. At minimum this deserves a comment explaining why it's intentionally cloud-only.

  // Bug: hardcoded, env-specific VSSPS endpoint
  let identity_url = format!(
      "(vssps.dev.azure.com/redacted),
      utf8_percent_encode(organization, PATH_SEGMENT),
      …
  );

🔒 Security Concerns

• src/tools/upload_attachment.rs:174 — The file path is resolved via ctx.source_directory.join(&self.file_path) but there is no subsequent canonicalize() + prefix check to verify the resolved path stays within source_directory. The .. check in Validate is a good first line of defense, but symlinks within the workspace (e.g., a build artifact that contains a symlink pointing to /etc/passwd) are not caught. Any symlink in the workspace that predates Stage 2 execution can be followed to read and upload arbitrary files.

  Recommended fix:

  let resolved_path = ctx.source_directory.join(&self.file_path);
  let canonical = resolved_path.canonicalize()
      .context("Failed to canonicalize file path")?;
  let canonical_base = ctx.source_directory.canonicalize()
      .context("Failed to canonicalize source directory")?;
  if !canonical.starts_with(&canonical_base) {
      return Ok(ExecutionResult::failure(format!(
          "File path '{}' resolves outside the workspace",
          self.file_path
      )));
  }

⚠️ Suggestions

• src/tools/add_pr_comment.rs — Sanitize impl only touches content: file_path, repository, and status are persisted to the NDJSON safe-output file unsanitized. While they're all validated and JSON-encoded before hitting the ADO API (so direct injection risk is low), this is inconsistent with the defensive sanitize patterns used in other tools and could confuse future maintainers. Worth applying at least a control-char strip to structural fields like repository and status, matching what create_git_tag.rs does.

• src/tools/queue_build.rs — Branch wildcard edge case: The release/* pattern unintentionally matches the bare branch name release (no suffix). The check is effective_branch.starts_with(prefix) && (effective_branch.len() == prefix.len() || ...). If an agent provides branch release where release/* is the only pattern, it passes. This is a minor policy gap but worth a targeted test.

✅ What Looks Good

• Fail-closed config gates are well applied: queue-build fails if allowed-pipelines is empty, and update-pr vote fails if allowed-votes is unconfigured. This correctly puts the security burden on operators to opt-in explicitly rather than defaulting to permissive.
• URL encoding via utf8_percent_encode(..., PATH_SEGMENT) is applied consistently on all user-controlled values embedded in ADO API URLs across all 8 tools.
• ##vso[ scanning in upload_attachment.rs for text files is correct and consistent with the memory tool's approach.
• The execute_vote flow for resolving the current user identity via connectiondata before PUTting the vote is the correct ADO pattern, and the required allowed-votes gate prevents agents from silently auto-approving.

Generated by Rust PR Reviewer for issue #141 · ● 1.8M · ◷

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Previous review findings are resolved — one lingering bug in queue_build.rs branch wildcards, everything else looks good.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/queue_build.rs:183 — The release/* wildcard still matches the bare branch release (no suffix). The effective_branch.len() == prefix.len() arm was flagged in the last review and is unchanged:

  effective_branch.starts_with(prefix)
      && (effective_branch.len() == prefix.len()   // ← true when branch == "release"
          || effective_branch[prefix.len()..].starts_with('/'))

  Drop the equality arm — the intent of /* is to match a separator-prefixed suffix, not the bare prefix itself:

  effective_branch.starts_with(prefix)
      && effective_branch[prefix.len()..].starts_with('/')

  With this fix, release/1.0 still matches release/*, but bare release does not.

---

⚠️ Suggestions

• src/tools/update_pr.rs:614 — The VSSPS URL derivation using str::replace("://dev.azure.com/", "://vssps.dev.azure.com/") silently produces an invalid URL for Visual Studio.com–hosted organisations ((myorg.visualstudio.com/redacted)). The replace call won't find the pattern, so vssps_base equals org_url, and the identity lookup silently fails for every reviewer. This was flagged previously as "partially fixed"; the comment helps, but operators on legacy VisualStudio.com orgs will hit confusing silent failures. A warn! log when the replacement produces no change (i.e. vssps_base == org_url.trim_end_matches('/')) would at least surface the issue at runtime.

---

✅ What Was Fixed Since Last Review

• create_git_tag.rs — resolve_head_commit now fetches repo metadata to discover defaultBranch dynamically instead of hardcoding heads/main. ✓
• update_pr.rs — execute_vote derives connection_url from ctx.ado_org_url, no longer hardcoded. ✓
• update_pr.rs — allowed_votes now fails closed (empty list → rejection with helpful error message). ✓
• update_pr.rs — set-auto-complete merge options (delete-source-branch, merge-strategy) are now configurable via front matter. ✓
• add_pr_comment.rs — validate_file_path is now called in Validate::validate(), not only in execute_impl. ✓
• queue_build.rs — Both parameter keys and values are now sanitized. ✓
• upload_attachment.rs — Symlink escape is blocked via canonicalize() + prefix check. ✓

Generated by Rust PR Reviewer for issue #141 · ● 2.1M · ◷

Generated by Rust PR Reviewer for issue #141 · ● 823.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Largely solid — good security model, consistent patterns — but one clear bug and a few concerns worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/tools/add_pr_comment.rs — WontFix mapped to ADO integer 6 (should be 3)

  The status_to_int function in add_pr_comment.rs maps "WontFix" => Some(6), but resolve_pr_thread.rs correctly documents the ADO thread status enum:

  // ADO thread status values:
  // 1 = Active, 2 = Fixed, 3 = WontFix, 4 = Closed, 5 = ByDesign
  

  So WontFix should be 3, not 6. Passing 6 to the ADO API will likely result in a rejection or silent no-op. Fix by aligning with resolve_pr_thread.rs's correct mapping.

  Related: the two files use different naming conventions for the same concept — add_pr_comment.rs uses CamelCase ("WontFix", "ByDesign") while resolve_pr_thread.rs uses kebab-case ("wont-fix", "by-design"). Agents setting a thread to "WontFix" via add-pr-comment and then querying it via resolve-pr-review-thread will encounter different string values for the same state. Consider unifying.

🔒 Security Concerns

• src/tools/update_pr.rs:execute_add_reviewers — VSSPS URL derivation is dev.azure.com-only

  let vssps_base = trimmed_org.replace("://dev.azure.com/", "://vssps.dev.azure.com/");
  if vssps_base == trimmed_org {
      warn!("Could not derive VSSPS endpoint from org URL ...");
  }

  For older *.visualstudio.com-style organizations, vssps_base == trimmed_org (the replace is a no-op), so the identity lookup is sent to the original org URL instead of the VSSPS endpoint. The warn! is there, but the call proceeds with a malformed URL. Identity lookups will fail for those orgs, and reviewers will silently not be added while the operation reports partial success. Either reject with a clear error for non-dev.azure.com URLs, or implement a proper VisualStudio.com path.

⚠️ Suggestions

• src/tools/queue_build.rs — branch field not sanitized

  QueueBuildResult::sanitize_fields() sanitizes reason and parameters but skips branch. The MCP handler also omits it. The branch value flows into the ADO API request body (through serde_json, so no JSON injection risk), but the pattern is inconsistent with every other text field in the PR. Worth adding sanitize_text for defense-in-depth.

• src/tools/update_pr.rs — Inconsistent semantics for empty allow-lists

  allowed-operations: [] means all operations permitted (permissive default), but allowed-votes: [] means all votes rejected (secure default). These are both intentional and documented, but the inconsistency could trip up operators configuring update-pr. Consider a brief comment in the UpdatePrConfig doc noting this asymmetry explicitly, or a compile-time warning when vote is in allowed-operations but allowed-votes is empty.

• src/tools/create_branch.rs — Repo is resolved before policy check

  The code resolves the repo name from ctx.allowed_repositories first, then validates against config.allowed_repositories. If the alias is missing from the system checkout list, the error is "not in the allowed checkout list" rather than a policy error. This isn't wrong but the error message can mislead operators. Consider doing the config policy check first.

✅ What Looks Good

• submit-pr-review and update-pr vote both require explicit allow-lists (empty = reject all) — this is the right default and prevents accidental auto-approval.
• upload_attachment symlink escape protection via canonicalize() + prefix check is solid. The ##vso[ scan before upload is a nice extra defense.
• queue-build required allowed-pipelines list (empty = reject all) — consistent with the security model.
• The consistent structure across all 12 new tools (Params → Validate → Result → Sanitize → Config → Executor) makes the codebase easy to audit and extend.
• Error messages throughout are actionable (HTTP status + body, field names, valid alternatives shown).

Generated by Rust PR Reviewer for issue #141 · ● 1.5M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security defaults, a couple of correctness bugs worth fixing before merge.

---

Findings

🐛 Bugs / Logic Issues

src/tools/create_git_tag.rs — Repo resolution happens before config policy check

The ctx.allowed_repositories.get(repo_alias) call with ? propagates an anyhow::Error instead of returning ExecutionResult::failure when the alias is unknown. More importantly, the config.allowed_repositories policy check (which would give a clear "not in allowed list" message) is unreachable for any alias that isn't already in ctx.allowed_repositories:

// BUG: this `?` fires BEFORE the config policy check below it
let repo_name = if repo_alias == "self" {
    ...
} else {
    ctx.allowed_repositories
        .get(repo_alias)
        .cloned()
        .context(format!("Repository alias '{}' not found...", repo_alias))?  // hard error
};

// This block is never reached for an unknown alias
if !config.allowed_repositories.is_empty()
    && !config.allowed_repositories.contains(&repo_alias.to_string())
{
    return Ok(ExecutionResult::failure(...));
}

create_branch.rs does this correctly — it validates the config policy before resolving. create_git_tag.rs has them inverted.

---

src/tools/update_pr.rs execute_add_reviewers — VSSPS URL derived by string replacement every iteration

The VSSPS URL is re-derived (and the equality check is re-evaluated) inside the for reviewer in reviewers loop. If the org URL isn't a dev.azure.com URL, the function returns early on the first iteration with a failure — but the derivation logic and early-return guard should be hoisted out of the loop to avoid repeating work (and to make the error fire before touching the ADO API at all):

// Currently inside the for loop
let vssps_base = trimmed_org.replace("://dev.azure.com/", "://vssps.dev.azure.com/");
if vssps_base == trimmed_org {
    return Ok(ExecutionResult::failure(...));
}

Move this to the top of execute_add_reviewers, before the loop.

---

🔒 Security Concerns

src/tools/submit_pr_review.rs + src/tools/update_pr.rs — Duplicate resolve_repo_name function

Both files define an identical resolve_repo_name private function. This isn't a security issue on its own, but a future bug fix applied to one would silently miss the other. Should be extracted to src/tools/mod.rs or result.rs.

---

src/tools/upload_attachment.rs — ##vso[ scan is UTF-8 only

The injection check is skipped for binary files (any file where from_utf8 fails). This is the correct behavior, but worth noting: a binary file with a valid UTF-8 preamble but malformed tail would still fail from_utf8 and bypass the scan. In practice this is low-risk since ADO's attachment viewer won't execute ##vso[ out of binary content, but the behavior should be documented.

---

⚠️ Suggestions

• src/tools/add_pr_comment.rs:79 — start_line < line is strict, so a single-line span using start_line is impossible (must use line alone). This is correct behavior but worth adding to the doc comment so agents know not to pass start_line == line.

• src/tools/reply_to_pr_comment.rs — Hardcoded "parentCommentId": 1 — This is fine for ADO (the first comment in a thread is always ID 1 unless deleted), but a brief comment explaining that this is intentional and targets the root comment would help future readers understand why it isn't 0.

• src/mcp.rs report_incomplete — let _ = self.write_safe_output_file(&result).await; silently discards write failures. This is intentional ("best-effort" signal), but a warn! on error would improve observability in pipelines where the safe-output directory is misconfigured.

---

✅ What Looks Good

• Security defaults: submit-pr-review requires explicit allowed-events, update-pr vote requires explicit allowed-votes, and queue-build requires allowed-pipelines. All three default to "reject all" rather than "allow all" — this is exactly right for high-privilege operations.
• Upload attachment path hardening: The canonicalize() + prefix check correctly prevents symlink escapes. The ##vso[ text scan, .git component rejection, and size limit are all appropriate defense-in-depth layers.
• Validation trait pattern: Consistent use of Validate → try_into() → Sanitize → execute_impl() pipeline is clean and easy to audit. All new tools follow the established pattern.
• Test coverage: Each tool has coverage for happy path, invalid IDs, path traversal, config deserialization, and serialization round-trips. The test for test_validation_rejects_branch_with_dotdot specifically covering ../../etc/passwd is a nice explicit signal of intent.
• create_branch.rs ref update response check: Inspecting the value[0].success field in the ADO refs update response to catch per-ref failures (which return HTTP 200 with success: false) is a subtle but important correctness detail that's easy to miss.

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

[github-actions]

🔍 Rust PR Review

Summary: Solid overall — the security model (config-driven allow-lists, required allowed-events/allowed-pipelines, canonicalize+prefix for upload-attachment) is well thought out. One correctness bug that defeats the purpose of a tool, plus a few smaller items.

---

Findings

🐛 Bugs / Logic Issues

• src/execute.rs:384–387 — report-incomplete silently succeeds instead of failing the stage

  This is the most significant issue. The tool's stated purpose is to "signal that the task could not be completed due to an infrastructure failure" and cause the pipeline to fail — directly analogous to gh-aw's report_incomplete. However it's handled identically to noop:

  "report-incomplete" => {
      debug!("Skipping report-incomplete entry");
      ExecutionResult::success("Skipped informational output: report-incomplete")  // ← wrong
  }

  Because main.rs:245 calls std::process::exit(1) only when failure_count > 0, and success results don't count as failures, a pipeline where the agent calls report-incomplete will exit 0 and show green in Azure DevOps. The fix is to return a failure result that surfaces the agent's reason:

  "report-incomplete" => {
      // Parse to extract the reason for the failure message
      let output: ReportIncompleteResult = serde_json::from_value(entry.clone())
          .map_err(|e| anyhow::anyhow!("Failed to parse report-incomplete: {}", e))?;
      debug!("report-incomplete: {}", output.reason);
      ExecutionResult::failure(format!("Agent reported task incomplete: {}", output.reason))
  }

  The ReportIncompleteResult type is already imported in the dispatch block and the struct is already in scope.

🔒 Security Concerns

• src/tools/submit_pr_review.rs:263, src/tools/update_pr.rs:527 — user_id from API response not percent-encoded in URL

  The user identity GUID returned by _apis/connectiondata is interpolated directly into the PUT URL without encoding:

  let vote_url = format!(
      "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1",
      base_url, encoded_repo, self.pull_request_id, user_id  // ← not encoded
  );

  ADO identity IDs are GUIDs (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx), so in practice only hex characters and dashes appear, making this unexploitable today. But it's inconsistent with how encoded_repo and encoded_project are handled — adding utf8_percent_encode(user_id, PATH_SEGMENT) would make this future-proof.

⚠️ Suggestions

• src/tools/add_pr_comment.rs — status field not validated at Stage 1

  AddPrCommentParams::validate() doesn't check status against VALID_STATUSES. An agent can write any value (e.g. "unknown") to the NDJSON file and it only fails at Stage 2 when status_to_int() returns None. This is inconsistent with the rest of the validation pattern (validate-at-source). A one-liner in validate() would fix it:

  ensure!(
      status_to_int(&self.status).is_some(),
      "status must be one of: {}", VALID_STATUSES.join(", ")
  );

• src/tools/update_pr.rs — merge_strategy in config is not validated

  UpdatePrConfig::merge_strategy defaults to "squash" but accepts any string from the operator's front matter. ADO's completionOptions.mergeStrategy only accepts "squash", "noFastForward", "rebase", "rebaseMerge". An invalid value causes a runtime ADO 400 with a cryptic message. Worth validating at Default construction or adding a note in the doc comment.

• src/tools/upload_attachment.rs — dead code after canonicalize()

  The canonical.exists() check after a successful canonicalize() call is unreachable — canonicalize() already fails if the path doesn't exist:

  // This branch can never execute — canonicalize() would have failed above
  if !canonical.exists() {
      return Ok(ExecutionResult::failure(format!(
          "File not found: {}",
          resolved_path.display(),  // shows the non-canonical path, which is confusing too
      )));
  }

  Safe to remove.

✅ What Looks Good

• The allowed-events/allowed-votes empty-means-reject pattern for approval operations is exactly right — prevents accidental auto-approve with minimal config.
• upload_attachment.rs: canonicalize + prefix check for symlink escape is the correct approach. The ##vso[ scan on text files (skipping binary) with the explanation comment is thoughtful.
• queue-build branch wildcard matching (release/* → strip /*, verify / at boundary) correctly rejects releasefoo/bar while allowing release/1.0.
• All unwrap() calls in the diff are inside #[cfg(test)] — none on user-facing paths.
• The VSSPS identity resolution in add-reviewers correctly detects and rejects legacy .visualstudio.com org URLs rather than silently sending to the wrong endpoint.

Generated by Rust PR Reviewer for issue #141 · ● 2.1M · ◷