fix(safeoutputs): render applyable PR suggestions correctly

[Copilot]

Summary

add-pr-comment was creating inline suggestion threads that Azure DevOps treated as zero-width insertion points, and shared sanitization was escaping </> inside markdown code. The result was non-applyable suggestions and HTML entities being written back into suggested code.

• Thread anchoring

  • add-pr-comment now anchors inline threads to the full selected line range instead of offset: 1 → offset: 1.
  • The end offset is computed from the last target line’s UTF-16 length, matching ADO’s thread API semantics for single-line and multi-line suggestions.

• Markdown-aware sanitization

  • Shared HTML escaping now skips matched inline code spans and fenced code blocks, including ```suggestion bodies.
  • Escaping still applies outside code spans/fences, and unmatched backticks do not disable escaping.

• Focused coverage

  • Added unit coverage for UTF-16 end-offset calculation, multiline thread spans, fenced suggestion blocks, inline code spans, and unmatched-backtick behavior.

"rightFileStart": { "line": start_line, "offset": 1 },
"rightFileEnd": { "line": end_line, "offset": target_line.encode_utf16().count() as i32 + 1 }

Test plan

Covered by focused unit tests for:

• inline thread end-offset computation
• multiline thread anchoring
• fenced suggestion preservation
• inline code preservation
• unmatched-backtick safety

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Mostly solid work — correctness and test coverage are good, one security gap worth fixing before merging.

Findings

🔒 Security Concerns

src/safeoutputs/add_pr_comment.rs:91 — repository not validated with is_safe_path_segment

AddPrCommentParams::validate() calls reject_pipeline_injection on repository, but that check only blocks ADO expressions/pipeline commands. It does not reject path traversal components like ../. repository_checkout_dir then passes this value directly to source_directory.join(repository), which can escape the build workspace:

// Current (line 91) — only blocks ADO injection, not traversal
reject_pipeline_injection(&self.repository, "repository")?;

// Fix — consistent with mcp.rs:228 which already uses this
if !self.repository.is_empty() {
    ensure!(
        crate::validate::is_safe_path_segment(&self.repository),
        "repository must be a safe path segment (no '..', '/', or '\\'), got: '{}'",
        self.repository
    );
}

There is a runtime defense at execute_impl line ~345 (lookup_allowed_repository fails on unknown aliases), so this isn't an immediate exploit in normal use. But: (a) the Validate check runs before execution and should be the first line of defense, (b) mcp.rs already uses is_safe_path_segment for the same field type for consistency, and (c) an operator-defined checkout alias containing .. (unusual but possible) would bypass the runtime gate while the path traversal guard in build_inline_thread_context anchors to repo_root (the already-escaped path) rather than ctx.source_directory.

---

⚠️ Suggestions

src/safeoutputs/add_pr_comment.rs:231–235 — path guard anchored to repo_root, not source_directory

The starts_with(&canonical_root) check (line 234) guards against traversal within repo_root, but canonical_root is derived from the (potentially traversed) repo_root itself, not from ctx.source_directory. As defense-in-depth, also checking against the workspace root would be stronger:

let canonical_workspace = ctx.source_directory.canonicalize()
    .context("Failed to canonicalize source directory")?;
ensure!(
    canonical.starts_with(&canonical_workspace),
    "Inline comment file resolves outside the build workspace"
);

---

✅ What Looks Good

• UTF-16 counting: Using str::lines() (which strips \r\n) before encode_utf16().count() correctly handles CRLF files — nice catch.
• Fenced block parser: find_matching_fence_end returning next_line_start (which includes the closing fence's \n) means the protected range is complete. The empty-iterator all() case for a bare ``` closer is handled correctly.
• Unmatched backtick safety: The test test_escape_html_tags_unmatched_inline_backtick_does_not_disable_escaping is exactly the right edge case to have.
• Emoji test: Using ab😀 (1 + 1 + 2 UTF-16 code units = 4, +1 = offset 5) is a well-chosen test — hits the surrogate pair path.
• Error handling: File read errors and line-out-of-range all go through ExecutionResult::failure — no panics on bad agent input.
• ensure! ordering in build_inline_thread_context: Input validation (start_line > 0, end_line > 0, start_line ≤ end_line) is checked before any I/O. ✓

Generated by Rust PR Reviewer for issue #1104 · 322.1 AIC · ⌖ 12.1 AIC · ⊞ 34.2K · ◷
_{Comment /rust-review to run again}

[jamesadevine]

@copilot adfress review comments

[Copilot]

@copilot adfress review comments

Addressed in a97ed85. The fix now rejects traversal-style repository selectors, resolves inline comment checkout roots through the allowed repository map, and enforces build-workspace containment for anchored files.