fix: report-incomplete fails pipeline, percent-encode user_id, stage-1 status validation, merge_strategy validation, dead code removal

[jamesadevine]

Five correctness/security issues in the new safe output tools identified in review.

Bug: report-incomplete silently succeeded

execute.rs was returning ExecutionResult::success(...) for report-incomplete, meaning pipelines where the agent signalled infrastructure failure would show green. Now parses the entry and returns a failure with the agent's reason:

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

Security: user_id not percent-encoded in reviewer URL

submit_pr_review.rs and update_pr.rs interpolated the GUID from _apis/connectiondata directly into URLs. Now encoded via utf8_percent_encode(user_id, PATH_SEGMENT), consistent with encoded_repo/encoded_project.

Suggestion: status validated at Stage 1 (add_pr_comment.rs)

AddPrCommentParams::validate() was not checking status against VALID_STATUSES, deferring failure to Stage 2. Added ensure!(status_to_int(&self.status).is_some(), ...).

Suggestion: merge_strategy validated against ADO-accepted values (update_pr.rs)

Added VALID_MERGE_STRATEGIES: &[&str] = &["squash", "noFastForward", "rebase", "rebaseMerge"] and a guard in execute_set_auto_complete that returns a descriptive failure before touching the API, replacing the cryptic ADO 400.

Dead code removal (upload_attachment.rs)

Removed unreachable canonical.exists() check — canonicalize() already fails if the path doesn't exist.

[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 · ◷

[jamesadevine]

@copilot

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.

[Copilot]

@copilot

Bugs / Logic Issues...

All issues addressed in commits a80669a and 65ae3ca:

Bug fix — report-incomplete now parses the entry and returns ExecutionResult::failure(...) with the agent's reason, causing the pipeline to exit non-zero. Previously it called ExecutionResult::success(...), which meant a pipeline where the agent called report-incomplete would show green.

Security — user_id (identity GUID from _apis/connectiondata) is now percent-encoded via utf8_percent_encode(user_id, PATH_SEGMENT) in both submit_pr_review.rs and update_pr.rs, consistent with how encoded_repo and encoded_project are handled.

Validation — status at Stage 1 — Added ensure!(status_to_int(&self.status).is_some(), ...) to AddPrCommentParams::validate(), so invalid status values are rejected before writing to NDJSON rather than only failing at Stage 2.

Validation — merge_strategy — Added VALID_MERGE_STRATEGIES: &[&str] = &["squash", "noFastForward", "rebase", "rebaseMerge"] constant and a guard in execute_set_auto_complete that returns a descriptive failure before making the ADO API call if the configured value is not in that set.

Dead code — Removed the unreachable canonical.exists() check in upload_attachment.rs.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• api.github.com (HTTP Only)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0e1wrd/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0lesijv6ffhd8jays209wgj5n.1hy03bi.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0fvewg/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0n7g41hi1b73dr0nnfdhz8mb1.1hy03bi.rcgu.o 2regaihc7r5d18zliln5ku8z.106udhm.rcgu.o bo8h4r0ac505s02ct9e9jfja.106udhm.rcgu.o hn3pk8m2qs5yvgvz2lhp494g.106udhm.rcgu.o imo2xsvhz31piwzuu8maj71s.106udhm.rcgu.o jb6ukj1d9vltv4tvdb2aptbe.106udhm.rcgu.o k6bus1woc864lk0ypulsh3ye.106udhm.rcgu.o tbuk6o45y9vvg5cz9ct0lzry.106udhm.rcgu.o uqvlm05fjogoxbv0012u6xvl.106udhm.rcgu.o azmhriamtr91bvha3pxyqnz5.106udhm.rcgu.o clcp8wiv2dn2tyge8zo6sya2.106udhm.rcgu.o dtuuubbi1poec8h30b6t4tp8.106udhm.rcgu.o kowmyjxwwcclh6toq1dlrf6o.106udhm.rcgu.o lltr8u89xait5bf8u1j3czre.106udhm.rcgu.o pdyehqhj2fm71ur0cn11ungu.106udhm.rcgu.o (packet block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com g/de�� pl-d776f39dc20c6/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rmcp_macros-ec/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0r8aubml7wtig88uujy5d34v7.0dxecp6.rcgu.o eta.o bin/rustc e-c4448fe3a5c46ecc e-c4448fe3a5c46e-m64 e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcQVGKB0/symbols.o bin/rustc e-c4�� e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0cc e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0-m64 lib/rustlib/x86_64-REDACTED-linux-gnu/bin/rust-lld e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf e-c4448fe3a5c46eupload_attachment e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.04mt0neqqftmr5yjelvmaemg2.0dxecp6.rcgu.o lib/rustlib/x86_64-REDACTED-linux-gnu/bin/rust-lld (packet block)
• dev.azure.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --warn=rust_2018_idioms --warn=clippy::zero_sized_map_values --warn=clippy::verbose_file_reads --warn=unused_qualifications --warn=unused_macro_rules 0.th�� 0.thiserror_impl.730f44467961dadd-cgu.13.rcgu.o 0.thiserror_impl.730f44467961dadd-cgu.14.rcgu.o 0ec67f6b19510/bu-m 0ec67f6b19510/bu/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/bin/rustc 43e3-cgu.0.rcgu.--crate-name 43e3-cgu.1.rcgu.anyhow lib/rustlib/x86_--edition=2018 (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com hn3pk8m2qs5yvgvz2lhp494g.106udhm.rcgu.o imo2xsvhz31piwzuu8maj71s.106udhm.rcgu.o jb6ukj1d9vltv4tvdb2aptbe.106udhm.rcgu.o k6bus1woc864lk0ypulsh3ye.106udhm.rcgu.o tbuk6o45y9vvg5cz9ct0lzry.106udhm.rcgu.o uqvlm05fjogoxbv0012u6xvl.106udhm.rcgu.o azmhriamtr91bvha3pxyqnz5.106udhm.rcgu.o clcp8wiv2dn2tyge8zo6sya2.106udhm.rcgu.o dtuuubbi1poec8h30b6t4tp8.106udhm.rcgu.o kowmyjxwwcclh6toq1dlrf6o.106udhm.rcgu.o lltr8u89xait5bf8u1j3czre.106udhm.rcgu.o pdyehqhj2fm71ur0cn11ungu.106udhm.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf imo2xsvhz31piwzuu8maj71s.106udhm.rcgu.o jb6ukj1d9vltv4tvdb2aptbe.106udhm.rcgu.o k6bus1woc864lk0ypulsh3ye.106udhm.rcgu.o tbuk6o45y9vvg5cz9ct0lzry.106udhm.rcgu.o uqvlm05fjogoxbv0012u6xvl.106udhm.rcgu.o azmhriamtr91bvha3pxyqnz5.106udhm.rcgu.o clcp8wiv2dn2tyge8zo6sya2.106udhm.rcgu.o dtuuubbi1poec8h30b6t4tp8.106udhm.rcgu.o kowmyjxwwcclh6toq1dlrf6o.106udhm.rcgu.o lltr8u89xait5bf8u1j3czre.106udhm.rcgu.o pdyehqhj2fm71ur0cn11ungu.106udhm.rcgu.o x7sx�� zqly8rrorvx78en0vc4wihwr.106udhm.rcgu.o 1um8yha4vk78sjlqo3wa9uqz.106udhm.rcgu.o mbols.o 3fabaecd10b65a.rbash 3fabaecd10b65a.r--norc 3fabaecd10b65a.r--noprofile 3fabaecd10b65a.rmcp_macros.16c3b4e54bafbd15-cgu.3.rcgu.o (dns block)
• msazuresphere.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com hn3pk8m2qs5yvgvz2lhp494g.106udhm.rcgu.o imo2xsvhz31piwzuu8maj71s.106udhm.rcgu.o jb6ukj1d9vltv4tvdb2aptbe.106udhm.rcgu.o k6bus1woc864lk0ypulsh3ye.106udhm.rcgu.o tbuk6o45y9vvg5cz9ct0lzry.106udhm.rcgu.o uqvlm05fjogoxbv0012u6xvl.106udhm.rcgu.o azmhriamtr91bvha3pxyqnz5.106udhm.rcgu.o clcp8wiv2dn2tyge8zo6sya2.106udhm.rcgu.o dtuuubbi1poec8h30b6t4tp8.106udhm.rcgu.o kowmyjxwwcclh6toq1dlrf6o.106udhm.rcgu.o lltr8u89xait5bf8u1j3czre.106udhm.rcgu.o pdyehqhj2fm71ur0cn11ungu.106udhm.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com s/serde_derive-7cc 8aa98-cgu.0.rcgu-m64 8aa98-cgu.1.rcgu/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcQVGKB0/symbols.o bin/rustc 8e9a�� 136bc698ae7.rlib/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0cc -4f0f17ff7b9c793/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0-m64 173f842cf s/serde_derive-7/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.02fbmm3nf9mf3llfipq39tfpe.0dxecp6.rcgu.o c1063f.rlib known-linux-gnu//home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.04mt0neqqftmr5yjelvmaemg2.0dxecp6.rcgu.o 173f842cf (dns block)
• raw.github.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow *.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0e1wrd/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0lesijv6ffhd8jays209wgj5n.1hy03bi.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0fvewg/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0n7g41hi1b73dr0nnfdhz8mb1.1hy03bi.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0gw65z/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0puf2pegyf2bjavduout6f0k2.1hy03bi.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0ihpbs/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0s8akuniuk96o2ye0nwmzljby.1hy03bi.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0ivgj5/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0tjiynvj2war7u5lefjl8opjn.1hy03bi.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0koksvpjep�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0l9kvea8cqy53einsef4u6bs4.0h2k7zd.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0mpu7ocrhlpkjj5ojgmb6s6su.0h2k7zd.rcgu.o lib/�� lib/rustlib/x86_--cap-lints=allow lib/rustlib/x86_--emit=dep-info,metadata bin/rustc lib/rustlib/x86_cc lib/rustlib/x86_-m64 lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcVHMF7P/symbols.o bin/rustc (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow *.github.com du9j3xczjkclanjhnnj.1hy03bi.rcgu.o 37rz1yw28luopeai196.1hy03bi.rcgu.o bb1ptb2o0833kfsb209.1hy03bi.rcgu.o kt4eq9l93halxbc4t0n.1hy03bi.rcgu.o wy2wmyt41wcsnyhwd96.1hy03bi.rcgu.o ggd6dihfhwgwakedav9.1hy03bi.rcgu.o v6ffhd8jays209wgj5n.1hy03bi.rcgu.o hi1b73dr0nnfdhz8mb1.1hy03bi.rcgu.o egyf�� niuk96o2ye0nwmzljby.1hy03bi.rcgu.o vj2war7u5lefjl8opjn.1hy03bi.rcgu.o f/reqwest-0.12.2/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustck66Ocp/symbols.o e-c4448fe3a5c46etail e-c4448fe3a5c46e-20 e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.0/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.04mt0neqqftmr5yjelvmaemg2.0dxecp6.rcgu.o e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/proxy_tests-981260d2d2ff567f.1/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.09s12zce251st894orudi6t9e.0dxecp6.rcgu.o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: looks good — the five stated fixes are all correct; one misleading claim in the PR description and one overly-strict Stage 1 path check worth noting.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/update_pr.rs:357–389 — The PR description says execute_set_auto_complete returns a descriptive failure "before touching the API", but the VALID_MERGE_STRATEGIES guard (line 389) is placed after a GET /pullRequests/{id} call (line 357). On an invalid merge-strategy config value a needless round-trip is made before the early return. This is purely wasteful (the PATCH is still blocked), but the description is misleading. Consider moving the guard to the top of execute_set_auto_complete, before any network I/O.

⚠️ Suggestions

• src/tools/upload_attachment.rs:32–34 — Stage 1 path validation uses file_path.contains(".."), which rejects legitimate filenames like report..v2.pdf or v2..3/notes.md. The actual escape defence is the canonicalize() + starts_with() check at Stage 2 (line 178–190), so this is functionally safe, but Stage 1 is over-restrictive. The .git component check on lines 50–57 already uses the correct pattern (split('/').any(|c| c == ".git")); the same per-component equality check (c == "..") would be more precise here and is consistent with validate_file_path in add_pr_comment.rs.

• src/tools/submit_pr_review.rs:~107 — allowed-events defaults to an empty Vec, which causes a runtime failure with a helpful message. Other tools like comment-on-work-item enforce their required config field (target) at compile time with bail!. An empty allowed-events list is a misconfiguration that could be caught at cargo run -- compile time rather than deferred to Stage 2.

✅ What Looks Good

• report-incomplete fix is clean and complete — ExecutionResult::failure(...) now propagates to exit(1) via the failure_count > 0 guard in main.rs. Previous silent-success was a genuine correctness bug.
• user_id percent-encoding in both submit_pr_review.rs and update_pr.rs is correct. AAD GUIDs are safe URL characters, but encoding them is good defensive practice consistent with encoded_repo/encoded_project.
• Stage 1 status validation in add_pr_comment.rs (status_to_int(&self.status).is_some() in validate()) is a solid improvement — bad status values now fail at the MCP layer rather than deferring a cryptic ADO error to Stage 2.
• Defense-in-depth in upload_attachment.rs: Stage 1 structural checks + Stage 2 canonicalize()/starts_with() properly defends against symlink traversal.
• Budget tracking for all 11 new tools is wired up in register_budgets! before the dispatch loop — consistent with existing tools.
• Test coverage: validation tests cover the happy path and the key rejection cases (zero IDs, invalid events, missing required fields) for all new tools.

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

[jamesadevine]

@copilot

Bugs / Logic Issues

src/tools/update_pr.rs:357–389 — The PR description says execute_set_auto_complete returns a descriptive failure "before touching the API", but the VALID_MERGE_STRATEGIES guard (line 389) is placed after a GET /pullRequests/{id} call (line 357). On an invalid merge-strategy config value a needless round-trip is made before the early return. This is purely wasteful (the PATCH is still blocked), but the description is misleading. Consider moving the guard to the top of execute_set_auto_complete, before any network I/O.
⚠️ Suggestions

src/tools/upload_attachment.rs:32–34 — Stage 1 path validation uses file_path.contains(".."), which rejects legitimate filenames like report..v2.pdf or v2..3/notes.md. The actual escape defence is the canonicalize() + starts_with() check at Stage 2 (line 178–190), so this is functionally safe, but Stage 1 is over-restrictive. The .git component check on lines 50–57 already uses the correct pattern (split('/').any(|c| c == ".git")); the same per-component equality check (c == "..") would be more precise here and is consistent with validate_file_path in add_pr_comment.rs.

src/tools/submit_pr_review.rs:~107 — allowed-events defaults to an empty Vec, which causes a runtime failure with a helpful message. Other tools like comment-on-work-item enforce their required config field (target) at compile time with bail!. An empty allowed-events list is a misconfiguration that could be caught at cargo run -- compile time rather than deferred to Stage 2.

[Copilot]

@copilot

Bugs / Logic Issues...

All three items addressed in commit d9e9939:

Bug — VALID_MERGE_STRATEGIES guard is now the first thing in execute_set_auto_complete, before encoded_repo construction and any network I/O. An invalid merge-strategy config value now returns immediately without making the GET /pullRequests/{id} call.

Suggestion — .. check — file_path.contains("..") replaced with split('/').any(|c| c == ".."), consistent with the .git component check in the same function and validate_file_path in add_pr_comment.rs. report..v2.pdf and v2..3/notes.md are now accepted; ../etc/passwd and src/../secret are still rejected. Added tests for all four cases.

Suggestion — compile-time allowed-events — Added validate_submit_pr_review_events() to src/compile/common.rs (same pattern as validate_comment_target), wired into both standalone.rs and onees.rs. An empty or missing allowed-events list now fails compile with a descriptive error pointing to the example config. Added 5 unit tests in common.rs and 3 integration tests in compiler_tests.rs.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• api.github.com (HTTP Only)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1gkg93gwdysjiryikxtmf5b5x.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1nybvizmkcj78n5jmgma2zoto.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1un8r8v55sdiul2d0y2znzydg.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1vu695gx1q3k56gdddavja5yy.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1zsw604cvr6oq6wz7bl3lju99.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1z�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22aas3zwl88bdilps4bo7emkc.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22slv6xs1c1qzd96t06c83xtu.1exw61t.rcgu.o .o 50a5878ab0/rustc/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9tail 50a5878ab0/build/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9-30 50a5878ab0/build/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9-C 50a5878ab0/buildcc 8aa98-cgu.0.rcgu-m64 8aa98-cgu.1.rcgu/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcZ4LGpV/symbols.o lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.01urfnei93e50nnarjtfyqtto.1exw61t.rcgu.o (packet block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com lib/�� lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/thiserror_impl-a69babd28d2d3b2cc lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/thiserror_impl-a69babd28d2d3b2-m64 bin/rustc lib/rustlib/x86_cc lib/rustlib/x86_-m64 lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcqwzKxs/symbols.o bin/rustc lib/�� lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.028lzygrep lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.02nu48FAILED\|test result lib/rustlib/x86_64-REDACTED-linux-gnu/bin/gcc-ld/ld.lld 6e1dc71b.rlib ccc.rlib known-linux-gnu/--noprofile lib/rustlib/x86_64-REDACTED-linux-gnu/bin/gcc-ld/ld.lld (packet block)
• dev.azure.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.1ekjyo5nqxvca1mydu8svrgiv.0uq6sor.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.1j1h0src4spulfxhu5rz09flg.0uq6sor.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.1stciq3pbk406mfjlpr8xj6qx.0uq6sor.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.1ww07smrbg4w7eay6camk5skk.0uq6sor.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.20k0a4ab6yrhrfmmyomnjf5ss.0uq6sor.rcgu.o b.01urfnei93e50nnarjtfyqtto.1exw61t.rcgu.o b.02dfcfnyihkn53hd608es2pqe.1exw61t.rcgu.o b.0ksfdvo4fp4wqxws744z0xyuq.1exw61t.rcgu.o b.0ly7e9iyrmxd50yuxdae76frw.1exw61t.rcgu.o b.0o4hfn3d3g0bgoe68wp5k3rev.1exw61t.rcgu.o b.0qccr2r3nx2pr0vpgm2p703ni.1exw61t.rcgu.o b.0uq0m15jok1y1km26l9x2c1cs.1exw61t.rcgu.o b.10dtlyvsicg31iomchnk8uav8.1exw61t.rcgu.o b.1a5yf5zxguk3270196709fltc.1exw61t.rcgu.o b.1gkg93gwdysjiryikxtmf5b5x.1exw61t.rcgu.o b.1nybvizmkcj78n5jmgma2zoto.1exw61t.rcgu.o b.1un8r8v55sdiul2d0y2znzydg.1exw61t.rcgu.o b.1vu695gx1q3k56gdddavja5yy.1exw61t.rcgu.o b.1zsw604cvr6oq6wz7bl3lju99.1exw61t.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1zsw604cvr6oq6wz7bl3lju99.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1z�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22aas3zwl88bdilps4bo7emkc.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22slv6xs1c1qzd96t06c83xtu.1exw61t.rcgu.o .o 50a5878ab0/rustc/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9cargo 50a5878ab0/build/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9test lib/rustlib/x86_64-REDACTED-linux-gnu/bin/gcc-ld/ld.lld 50a5878ab0/buildcc 8aa98-cgu.0.rcgu-m64 8aa98-cgu.1.rcgu/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcZ4LGpV/symbols.o lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.01urfnei93e50nnarjtfyqtto.1exw61t.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0gauchfgbqnpyfj3m32f4w80l.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0gm7l6kt4eq9l93halxbc4t0n.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0kirpcjs1fpj8yj0dvocwnpsc.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0l8fztggd6dihfhwgwakedav9.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0lesijv6ffhd8jays209wgj5n.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0pbwvhvdea8s164jnhs00wuk5.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0puf2pegyf2bjavduout6f0k2.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0s8akuniuk96o2ye0nwmzljby.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0vhan8qtp6�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0xmxmiekvl8ls56jxjx6weubh.0g01p9t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0z4pkbide9o9hkouesb34brrj.0g01p9t.rcgu.o tsrt�� 0czva2baecxpoofsbr3.10eti7d.rcgu.o crhlpkjj5ojgmb6s6su.10eti7d.rcgu.o mbols.o -a69babd28d2d3b2cc -a69babd28d2d3b2-m64 -a69babd28d2d3b2/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcTMi2gV/symbols.o -a69babd28d2d3b2/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.02fbmm3nf9mf3llfipq39tfpe.0g01p9t.rcgu.o (dns block)
• msazuresphere.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1zsw604cvr6oq6wz7bl3lju99.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1z�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22aas3zwl88bdilps4bo7emkc.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22slv6xs1c1qzd96t06c83xtu.1exw61t.rcgu.o .o 50a5878ab0/rustc/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9cargo 50a5878ab0/build/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/paste-410ab7e90b9a77a3.paste.9test lib/rustlib/x86_64-REDACTED-linux-gnu/bin/gcc-ld/ld.lld 50a5878ab0/buildcc 8aa98-cgu.0.rcgu-m64 8aa98-cgu.1.rcgu/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcZ4LGpV/symbols.o lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.01urfnei93e50nnarjtfyqtto.1exw61t.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com lib/rustlib/x86_cc lib/rustlib/x86_-m64 lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcqwzKxs/symbols.o bin/rustc lib/�� lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.028lzycargo 78713 lib/rustlib/x86_64-REDACTED-linux-gnu/bin/rust-lld 6e1dc71b.rlib ccc.rlib known-linux-gnu/--noprofile lib/rustlib/x86_64-REDACTED-linux-gnu/bin/rust-lld (dns block)
• raw.github.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow *.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1gkg93gwdysjiryikxtmf5b5x.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1nybvizmkcj78n5jmgma2zoto.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1un8r8v55sdiul2d0y2znzydg.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1vu695gx1q3k56gdddavja5yy.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1zsw604cvr6oq6wz7bl3lju99.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1z�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22aas3zwl88bdilps4bo7emkc.1exw61t.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22slv6xs1c1qzd96t06c83xtu.1exw61t.rcgu.o .o 50a5878ab0/rustc--error-format=json 50a5878ab0/build--json=diagnostic-rendered-ansi,artifacts,future-incompat 50a5878ab0/build--crate-type 50a5878ab0/buildcc nt-0df214036274c-m64 lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcZ4LGpV/symbols.o lib/rustlib/x86_/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.01urfnei93e50nnarjtfyqtto.1exw61t.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow *.github.com 78713.1ekjyo5nqxvca1mydu8svrgiv.0uq6sor.rcgu.o 78713.1j1h0src4spulfxhu5rz09flg.0uq6sor.rcgu.o 78713.1stciq3pbk406mfjlpr8xj6qx.0uq6sor.rcgu.o 78713.1ww07smrbg4w7eay6camk5skk.0uq6sor.rcgu.o 78713.20k0a4ab6yrhrfmmyomnjf5ss.0uq6sor.rcgu.o 78713.24wceespwybzb4rah7oihjiuv.0uq6sor.rcgu.o 78713.29a387r89nmiehrr7mqoapxje.0uq6sor.rcgu.o 78713.2nuxd190hsjm4dw0kkz5tdz18.0uq6sor.rcgu.o 7871�� 78713.2un7iz8wkhmaa2k8kok8d85y2.0uq6sor.rcgu.o 78713.3962d9m4nt9zvxlvw8yghcwsj.0uq6sor.rcgu.o mbols.o fe0dd3cb6f860c.cbash fe0dd3cb6f860c.c--norc fe0dd3cb6f860c.c--noprofile fe0dd3cb6f860c.c/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f44fa01fbba89cb4.0e1wrd/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf.0pbwvhvdea8s164jnhs00wuk5.0g01p9t.rcgu.o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Good foundational work adding 11 new safe-output tools — structure is consistent throughout. Three issues need attention before merge.

Findings

🐛 Bugs / Logic Issues

• src/tools/submit_pr_review.rs:306 — "status": 4 is hardcoded for the review comment thread body. In the ADO API, status 4 = Closed/Resolved, not Active. A newly posted review comment should be 1 (Active) so reviewers can engage with it. Currently all review rationale comments are posted as pre-resolved/closed threads, defeating their purpose entirely. Fix: change "status": 4 to "status": 1.

• src/tools/add_pr_comment.rs:230-231 — allowed_statuses.contains(&self.status) is a case-sensitive exact match, but status_to_int() accepts both "active" and "Active". The documented canonical form is lowercase kebab-case ("active", "wont-fix"), but the test example config uses ["Active", "Closed"]. An operator who configures ["Active"] and an agent that sends "active" (the canonical form) gets a false denial at Stage 2, silently breaking a valid workflow.

  Fix: config.allowed_statuses.iter().any(|s| s.eq_ignore_ascii_case(&self.status))

⚠️ Suggestions

• src/tools/update_pr.rs:467 vs src/compile/common.rs:793 — The vote operation has a runtime-only allowed_votes.is_empty() guard (Stage 2 failure). Unlike submit-pr-review which has validate_submit_pr_review_events() as a compile-time check, a misconfigured update-pr with operation: vote and no allowed-votes silently compiles and only fails at Stage 2. Consider adding a validate_update_pr_votes() compile-time validator analogous to validate_submit_pr_review_events() and wiring it into both standalone.rs and onees.rs.

✅ What Looks Good

• The security model for submit-pr-review is solid: allowed-events is enforced at both compile time (validate_submit_pr_review_events) and runtime, with a helpful operator-facing error message.
• Path traversal + symlink-escape handling in upload_attachment.rs (canonicalize + starts_with check) is the correct approach.
• validate_file_path in add_pr_comment.rs correctly rejects both /absolute and \\absolute paths.
• Every new tool follows the established 4-layer pattern (Params → Validate → Result → Executor) with no silent unwrap()s in execution paths.
• WRITE_REQUIRING_SAFE_OUTPUTS in compile/common.rs is correctly updated for all 11 new tools.

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

[jamesadevine]

@copilot

🐛 Bugs / Logic Issues

src/tools/submit_pr_review.rs:306 — "status": 4 is hardcoded for the review comment thread body. In the ADO API, status 4 = Closed/Resolved, not Active. A newly posted review comment should be 1 (Active) so reviewers can engage with it. Currently all review rationale comments are posted as pre-resolved/closed threads, defeating their purpose entirely. Fix: change "status": 4 to "status": 1.

src/tools/add_pr_comment.rs:230-231 — allowed_statuses.contains(&self.status) is a case-sensitive exact match, but status_to_int() accepts both "active" and "Active". The documented canonical form is lowercase kebab-case ("active", "wont-fix"), but the test example config uses ["Active", "Closed"]. An operator who configures ["Active"] and an agent that sends "active" (the canonical form) gets a false denial at Stage 2, silently breaking a valid workflow.

Fix: config.allowed_statuses.iter().any(|s| s.eq_ignore_ascii_case(&self.status))

⚠️ Suggestions

src/tools/update_pr.rs:467 vs src/compile/common.rs:793 — The vote operation has a runtime-only allowed_votes.is_empty() guard (Stage 2 failure). Unlike submit-pr-review which has validate_submit_pr_review_events() as a compile-time check, a misconfigured update-pr with operation: vote and no allowed-votes silently compiles and only fails at Stage 2. Consider adding a validate_update_pr_votes() compile-time validator analogous to validate_submit_pr_review_events() and wiring it into both standalone.rs and onees.rs

[Copilot]

@copilot

🐛 Bugs / Logic Issues...

All three items addressed in commit 33da3b1:

Bug — "status": 4 → "status": 1 — The review comment thread is now posted with "status": 1 (Active), so reviewers can engage with it. Status 4 (Closed) was the wrong value for a newly posted thread.

Bug — case-insensitive allowed_statuses — Changed config.allowed_statuses.contains(&self.status) to config.allowed_statuses.iter().any(|s| s.eq_ignore_ascii_case(&self.status)). An operator who configures ["Active"] and an agent that sends "active" (the canonical form) now correctly passes the check. Added two unit tests for both directions of the mismatch.

Suggestion — compile-time allowed-votes check — Added validate_update_pr_votes() to src/compile/common.rs, wired into both standalone.rs and onees.rs. Fires when update-pr is configured and the vote operation is reachable (either allowed-operations is absent/empty, or explicitly contains "vote") but allowed-votes is empty — consistent with how validate_submit_pr_review_events handles the analogous case. Added 7 unit tests in common.rs and 3 integration tests in compiler_tests.rs.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• api.github.com (HTTP Only)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1gkg93gwdysjiryikxtmf5b5x.099ebps.rcgu.o /lib/php/sessionclean; fi /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1un8r8v55sdiul2d0y2znzydg.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1vu695gx1q3k56gdddavja5yy.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1zsw604cvr6oq6wz7bl3lju99.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1z�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22aas3zwl88bdilps4bo7emkc.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22slv6xs1c1qzd96t06c83xtu.099ebps.rcgu.o (packet block)
• dev.azure.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf --warn=unused_qualifications --warn=unused_macro_rules --warn=unused_lifetimes o-aw�� u/13/liblto_plug-Wl,--as-needed b.01urfnei93e50nnarjtfyqtto.099ebps.rcgu.o b.02dfcfnyihkn53hd608es2pqe.099ebps.rcgu.o b.0ksfdvo4fp4wqxws744z0xyuq.099ebps.rcgu.o b.0ly7e9iyrmxd50yuxdae76frw.099ebps.rcgu.o b.0o4hfn3d3g0bgoe68wp5k3rev.099ebps.rcgu.o b.0qccr2r3nx2pr0vpgm2p703ni.099ebps.rcgu.o b.0uq0m15jok1y1km26l9x2c1cs.099ebps.rcgu.o b.10dtlyvsicg31iomchnk8uav8.099ebps.rcgu.o b.1a5yf5zxguk3270196709fltc.099ebps.rcgu.o b.1gkg93gwdysjiryikxtmf5b5x.099ebps.rcgu.o b.1nybvizmkcj78n5jmgma2zoto.099ebps.rcgu.o b.1un8r8v55sdiul2d0y2znzydg.099ebps.rcgu.o b.1vu695gx1q3k56gdddavja5yy.099ebps.rcgu.o b.1zsw604cvr6oq6wz7bl3lju99.099ebps.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com 0cmbw9s1epj6u5lpg56.1xswm23.rcgu.o eqqftmr5yjelvmaemg2.1xswm23.rcgu.o 37rz1yw28luopeai196.1xswm23.rcgu.o ytz31t1cdjc9wyiok85.1xswm23.rcgu.o 5q1cogohpo1bhc9a2ck.1xswm23.rcgu.o kt4eq9l93halxbc4t0n.1xswm23.rcgu.o ggd6dihfhwgwakedav9.1xswm23.rcgu.o v6ffhd8jays209wgj5n.1xswm23.rcgu.o egyf2bjavduout6f0k2.1xswm23.rcgu.o niuk96o2ye0nwmzljby.1xswm23.rcgu.o 4m06ssmyly1gqj5po1y.1xswm23.rcgu.o 643zpbybutmalw6lni5.1xswm23.rcgu.o (dns block)
• msazuresphere.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com 0cmbw9s1epj6u5lpg56.1xswm23.rcgu.o eqqftmr5yjelvmaemg2.1xswm23.rcgu.o 37rz1yw28luopeai196.1xswm23.rcgu.o ytz31t1cdjc9wyiok85.1xswm23.rcgu.o 5q1cogohpo1bhc9a2ck.1xswm23.rcgu.o kt4eq9l93halxbc4t0n.1xswm23.rcgu.o ggd6dihfhwgwakedav9.1xswm23.rcgu.o v6ffhd8jays209wgj5n.1xswm23.rcgu.o egyf2bjavduout6f0k2.1xswm23.rcgu.o niuk96o2ye0nwmzljby.1xswm23.rcgu.o 4m06ssmyly1gqj5po1y.1xswm23.rcgu.o 643zpbybutmalw6lni5.1xswm23.rcgu.o (dns block)
• raw.github.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow *.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1gkg93gwdysjiryikxtmf5b5x.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1nybvizmkcj78n5jmgma2zoto.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1un8r8v55sdiul2d0y2znzydg.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1vu695gx1q3k56gdddavja5yy.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1zsw604cvr6oq6wz7bl3lju99.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.1z�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22aas3zwl88bdilps4bo7emkc.099ebps.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d7b205d2f6ee756b.22slv6xs1c1qzd96t06c83xtu.099ebps.rcgu.o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Mostly looks good — the five targeted fixes are well-executed. Found two bugs worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/tools/add_pr_comment.rs:185 — validate_file_path uses path.contains("..") (substring match) instead of a component-wise check. This rejects legitimate paths like "releases..notes/v1.md" or "v2..beta/file.txt" where .. appears within a path component name (not as a traversal). upload_attachment.rs correctly uses path.split(['/', '\\']).any(|component| component == "..") — add_pr_comment.rs should match. Since this file_path is API-bound (not disk-read), it's a correctness/usability issue rather than a security gap, but operators will hit confusing validation failures.

• src/compile/common.rs — validate_resolve_pr_thread_statuses error message — The compile-time bail message lists "pending" as a valid status:

  Valid statuses: active, fixed, wont-fix, closed, by-design, pending
  

  But VALID_STATUSES in resolve_pr_thread.rs is ["active", "fixed", "wont-fix", "closed", "by-design"] — no "pending". An operator who follows the error message and writes allowed-statuses: [pending] will produce a pipeline that compiles cleanly but fails every Stage 2 execution with a cryptic "Status 'pending' is not in the allowed-statuses list" error.

⚠️ Suggestions

• src/tools/resolve_pr_thread.rs:260–280 — Repository resolution is inlined rather than using the resolve_repo_name helper added in mod.rs. The inline code also special-cases effective_repo.is_empty() as "self" (the helper doesn't), so the two paths already diverge. Better to use resolve_repo_name consistently and, if the empty-string case is intentional, handle it there.

✅ What Looks Good

• report-incomplete correctly propagates failure to the pipeline instead of swallowing it as success — the original bug was a silent pipeline green on infra failure.
• user_id percent-encoding in both submit_pr_review.rs and update_pr.rs is consistent with the rest of the codebase.
• merge_strategy validation fires before any network I/O in execute_set_auto_complete — clean fail-fast.
• add_pr_comment.rs Stage 1 status validation now catches invalid statuses before they reach Stage 2 — exactly the right layer.
• ndjson.rs flush() prevents silent data loss under async I/O.
• sanitize.rs ##vso[/##[ neutralization is a solid addition; the backtick wrapping is the right approach.
• Self-approval guard in submit_pr_review.rs is a strong security property; the connection-data approach is correct for national clouds.
• Budget enforcement is properly extended to all 11 new tools.
• Compile-time guards for submit-pr-review/update-pr/resolve-pr-review-thread are consistent with the existing comment-on-work-item target pattern.
• Test coverage is thorough throughout.

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

[jamesadevine]

@copilot

🐛 Bugs / Logic Issues

src/tools/add_pr_comment.rs:185 — validate_file_path uses path.contains("..") (substring match) instead of a component-wise check. This rejects legitimate paths like "releases..notes/v1.md" or "v2..beta/file.txt" where .. appears within a path component name (not as a traversal). upload_attachment.rs correctly uses path.split(['/', '\']).any(|component| component == "..") — add_pr_comment.rs should match. Since this file_path is API-bound (not disk-read), it's a correctness/usability issue rather than a security gap, but operators will hit confusing validation failures.

src/compile/common.rs — validate_resolve_pr_thread_statuses error message — The compile-time bail message lists "pending" as a valid status:

Valid statuses: active, fixed, wont-fix, closed, by-design, pending
But VALID_STATUSES in resolve_pr_thread.rs is ["active", "fixed", "wont-fix", "closed", "by-design"] — no "pending". An operator who follows the error message and writes allowed-statuses: [pending] will produce a pipeline that compiles cleanly but fails every Stage 2 execution with a cryptic "Status 'pending' is not in the allowed-statuses list" error.

⚠️ Suggestions

src/tools/resolve_pr_thread.rs:260–280 — Repository resolution is inlined rather than using the resolve_repo_name helper added in mod.rs. The inline code also special-cases effective_repo.is_empty() as "self" (the helper doesn't), so the two paths already diverge. Better to use resolve_repo_name consistently and, if the empty-string case is intentional, handle it there.

[Copilot]

@copilot

🐛 Bugs / Logic Issues
...

All three items addressed in commit 858284b:

Bug — component-wise .. check in add_pr_comment.rs — validate_file_path now uses path.split(['/', '\\']).any(|component| component == ".."), consistent with upload_attachment.rs and validate_file_path in the same file's test context. releases..notes/v1.md and v2..beta/file.txt are now accepted; ../etc/passwd and src/../secret still rejected. Added the two new cases to test_validate_file_path_accepts_valid.

Bug — spurious "pending" in error messages — Removed "pending" from both the compile-time validate_resolve_pr_thread_statuses bail message in common.rs and the Stage 2 runtime failure message in resolve_pr_thread.rs. Both now list only the five valid statuses: active, fixed, wont-fix, closed, by-design.

Suggestion — resolve_repo_name helper — Replaced the inlined repo resolution block (lines 204–220) with the shared resolve_repo_name helper from mod.rs. An empty-string alias is normalized to None before delegating, so the existing is_empty() → "self" behaviour is preserved. The config.allowed_repositories guard (operator's front-matter allow-list) is kept as a separate step before resolution, consistent with other tools.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• api.github.com (HTTP Only)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.1ww07smrbg4w7eay6camk5skk.05ecx75.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_firewall_tests-6848ed8c1a278713.20k0a4ab6yrhrfmmyomnjf5ss.05ecx75.rcgu.o b.01urfnei93e50nnarjtfyqtto.0x94f5f.rcgu.o b.02dfcfnyihkn53hd608es2pqe.0x94f5f.rcgu.o b.0ksfdvo4fp4wqxws744z0xyuq.0x94f5f.rcgu.o b.0ly7e9iyrmxd50yuxdae76frw.0x94f5f.rcgu.o b.0o4hfn3d3g0bgoe68wp5k3rev.0x94f5f.rcgu.o b.0qccr2r3nx2pr0vpgm2p703ni.0x94f5f.rcgu.o b.0uq0m15jok1y1km26l9x2c1cs.0x94f5f.rcgu.o b.10dtlyvsicg31iomchnk8uav8.0x94f5f.rcgu.o b.1a5yf5zxguk3270196709fltc.0x94f5f.rcgu.o b.1gkg93gwdysjiryikxtmf5b5x.0x94f5f.rcgu.o b.1nybvizmkcj78n5jmgma2zoto.0x94f5f.rcgu.o b.1un8r8v55sdiul2d0y2znzydg.0x94f5f.rcgu.o b.1vu695gx1q3k56gdddavja5yy.0x94f5f.rcgu.o b.1zsw604cvr6oq6wz7bl3lju99.0x94f5f.rcgu.o (packet block)
• dev.azure.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-ec3615b173f842cf --hash-style=gnu --as-needed -shared -z relro 78713.0mizz1d78o628ut4xx7wgyc9t.05ecx75.rcgu.o 78713.0ngazdthsfhxly5w0qh4nq8um.05ecx75.rcgu.o 78713.0qrrfjjfld233fqf7wjvw4wly.05ecx75.rcgu.o 78713.10j9icf1tulf8y6i9s99smszw.05ecx75.rcgu.o 78713.12m2pdor12j5wla2dibp5y1w9.05ecx75.rcgu.o 78713.13cd5s948jmwh580h4c9949ls.05ecx75.rcgu.o 78713.1ekjyo5nqxvca1mydu8svrgiv.05ecx75.rcgu.o 78713.1j1h0src4spulfxhu5rz09flg.05ecx75.rcgu.o 78713.1stciq3pbk406mfjlpr8xj6qx.05ecx75.rcgu.o 78713.1ww07smrbg4w7eay6camk5skk.05ecx75.rcgu.o 78713.20k0a4ab6yrhrfmmyomnjf5ss.05ecx75.rcgu.o 78713.24wceespwybzb4rah7oihjiuv.05ecx75.rcgu.o 78713.29a387r89nmiehrr7mqoapxje.05ecx75.rcgu.o 78713.2nuxd190hsjm4dw0kkz5tdz18.05ecx75.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com b.0ksfdvo4fp4wqxws744z0xyuq.0x94f5f.rcgu.o b.0ly7e9iyrmxd50yuxdae76frw.0x94f5f.rcgu.o b.0o4hfn3d3g0bgoe68wp5k3rev.0x94f5f.rcgu.o b.0qccr2r3nx2pr0vpgm2p703ni.0x94f5f.rcgu.o b.0uq0m15jok1y1km26l9x2c1cs.0x94f5f.rcgu.o b.10dtlyvsicg31iomchnk8uav8.0x94f5f.rcgu.o b.1a5yf5zxguk3270196709fltc.0x94f5f.rcgu.o b.1gkg93gwdysjiryikxtmf5b5x.0x94f5f.rcgu.o b.1nybvizmkcj78n5jmgma2zoto.0x94f5f.rcgu.o b.1un8r8v55sdiul2d0y2znzydg.0x94f5f.rcgu.o b.1vu695gx1q3k56gdddavja5yy.0x94f5f.rcgu.o b.1zsw604cvr6oq6wz7bl3lju99.0x94f5f.rcgu.o (dns block)
• msazuresphere.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow api.github.com --allow dev.azure.com --allow *.visualstudio.com b.0ksfdvo4fp4wqxws744z0xyuq.0x94f5f.rcgu.o b.0ly7e9iyrmxd50yuxdae76frw.0x94f5f.rcgu.o b.0o4hfn3d3g0bgoe68wp5k3rev.0x94f5f.rcgu.o b.0qccr2r3nx2pr0vpgm2p703ni.0x94f5f.rcgu.o b.0uq0m15jok1y1km26l9x2c1cs.0x94f5f.rcgu.o b.10dtlyvsicg31iomchnk8uav8.0x94f5f.rcgu.o b.1a5yf5zxguk3270196709fltc.0x94f5f.rcgu.o b.1gkg93gwdysjiryikxtmf5b5x.0x94f5f.rcgu.o b.1nybvizmkcj78n5jmgma2zoto.0x94f5f.rcgu.o b.1un8r8v55sdiul2d0y2znzydg.0x94f5f.rcgu.o b.1vu695gx1q3k56gdddavja5yy.0x94f5f.rcgu.o b.1zsw604cvr6oq6wz7bl3lju99.0x94f5f.rcgu.o (dns block)
• raw.github.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw /home/REDACTED/work/ado-aw/ado-aw/target/debug/ado-aw proxy --allow *.github.com --test --check-cfg b.01urfnei93e50nnarjtfyqtto.0x94f5f.rcgu.o b.02dfcfnyihkn53hd608es2pqe.0x94f5f.rcgu.o b.0ksfdvo4fp4wqxws744z0xyuq.0x94f5f.rcgu.o b.0ly7e9iyrmxd50yuxdae76frw.0x94f5f.rcgu.o b.0o4hfn3d3g0bgoe68wp5k3rev.0x94f5f.rcgu.o b.0qccr2r3nx2pr0vpgm2p703ni.0x94f5f.rcgu.o b.0uq0m15jok1y1km26l9x2c1cs.0x94f5f.rcgu.o b.10dtlyvsicg31iomchnk8uav8.0x94f5f.rcgu.o b.1a5yf5zxguk3270196709fltc.0x94f5f.rcgu.o b.1gkg93gwdysjiryikxtmf5b5x.0x94f5f.rcgu.o b.1nybvizmkcj78n5jmgma2zoto.0x94f5f.rcgu.o b.1un8r8v55sdiul2d0y2znzydg.0x94f5f.rcgu.o b.1vu695gx1q3k56gdddavja5yy.0x94f5f.rcgu.o b.1zsw604cvr6oq6wz7bl3lju99.0x94f5f.rcgu.o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)