fix(safeoutputs): render applyable PR suggestions correctly

src/safeoutputs/add_pr_comment.rs

@@ -4,6 +4,7 @@ use log::{debug, info};
 use percent_encoding::utf8_percent_encode;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
+use std::path::{Path, PathBuf};
 
 use super::PATH_SEGMENT;
 use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate};
@@ -55,6 +56,14 @@ fn default_status() -> String {
     "active".to_string()
 }
 
+fn validate_repository_selector(repository: &str) -> anyhow::Result<()> {
+    reject_pipeline_injection(repository, "repository")?;
+    if !repository.is_empty() {
+        crate::validate::validate_relative_safe_path(repository, "repository")?;
+    }
+    Ok(())
+}
+
 impl Validate for AddPrCommentParams {
     fn validate(&self) -> anyhow::Result<()> {
         ensure!(self.pull_request_id > 0, "pull_request_id must be positive");
@@ -87,7 +96,7 @@ impl Validate for AddPrCommentParams {
         if let Some(fp) = &self.file_path {
             validate_file_path(fp)?;
         }
-        reject_pipeline_injection(&self.repository, "repository")?;
+        validate_repository_selector(&self.repository)?;
         Ok(())
     }
 }
@@ -199,6 +208,93 @@ fn validate_file_path(path: &str) -> anyhow::Result<()> {
     Ok(())
 }
 
+fn repository_checkout_dir(repository: &str, ctx: &ExecutionContext) -> anyhow::Result<PathBuf> {
+    if crate::safeoutputs::input_refers_to_self(repository, ctx) {
+        return Ok(ctx.source_directory.clone());
+    }
+
+    if let Some((alias, _)) = ctx.allowed_repositories.get_key_value(repository) {
+        return Ok(ctx.source_directory.join(alias));
+    }
+
+    if let Some((alias, _)) = ctx
+        .allowed_repositories
+        .iter()
+        .find(|(_, name)| name.eq_ignore_ascii_case(repository))
+    {
+        return Ok(ctx.source_directory.join(alias));
+    }
+
+    if let Some((alias, _)) = ctx.allowed_repositories.iter().find(|(_, name)| {
+        name.rsplit('/')
+            .next()
+            .unwrap_or(name.as_str())
+            .eq_ignore_ascii_case(repository)
+    }) {
+        return Ok(ctx.source_directory.join(alias));
+    }
+
+    anyhow::bail!(
+        "Repository alias '{}' not found in allowed repositories",
+        repository
+    )
+}
+
+fn build_inline_thread_context(
+    workspace_root: &Path,
+    repo_root: &Path,
+    file_path: &str,
+    start_line: i32,
+    end_line: i32,
+) -> anyhow::Result<serde_json::Value> {
+    ensure!(start_line > 0, "start_line must be positive");
+    ensure!(end_line > 0, "end_line must be positive");
+    ensure!(
+        start_line <= end_line,
+        "start_line ({start_line}) must be less than or equal to line ({end_line})"
+    );
+
+    let resolved_path = repo_root.join(file_path);
+    let canonical = resolved_path.canonicalize().with_context(|| {
+        format!(
+            "Failed to canonicalize inline comment file '{}' — file may not exist",
+            file_path
+        )
+    })?;
+    let canonical_root = repo_root
+        .canonicalize()
+        .context("Failed to canonicalize repository checkout root")?;
+    ensure!(
+        canonical.starts_with(&canonical_root),
+        "Inline comment file '{}' resolves outside the repository checkout",
+        file_path
+    );
+    let canonical_workspace = workspace_root
+        .canonicalize()
+        .context("Failed to canonicalize build workspace root")?;
+    ensure!(
+        canonical.starts_with(&canonical_workspace),
+        "Inline comment file '{}' resolves outside the build workspace",
+        file_path
+    );
+
+    let contents = std::fs::read_to_string(&canonical)
+        .with_context(|| format!("Failed to read inline comment file '{}'", file_path))?;
+    let target_line = contents
+        .lines()
+        .nth((end_line - 1) as usize)
+        .with_context(|| format!("Inline comment line {} is out of range", end_line))?;
+    // Azure DevOps threadContext offsets are 1-based, so the end offset must point
+    // one UTF-16 code unit past the final character to span the whole target line.
+    let end_offset = target_line.encode_utf16().count() as i32 + 1;
+
+    Ok(serde_json::json!({
+        "filePath": format!("/{}", file_path),
+        "rightFileStart": { "line": start_line, "offset": 1 },
+        "rightFileEnd": { "line": end_line, "offset": end_offset }
+    }))
+}
+
 #[async_trait::async_trait]
 impl Executor for AddPrCommentResult {
     fn dry_run_summary(&self) -> String {
@@ -334,11 +430,36 @@ impl Executor for AddPrCommentResult {
         if let Some(ref fp) = self.file_path {
             let end_line = self.line.unwrap_or(1);
             let start_line = self.start_line.unwrap_or(end_line);
-            thread_body["threadContext"] = serde_json::json!({
-                "filePath": format!("/{}", fp),
-                "rightFileStart": { "line": start_line, "offset": 1 },
-                "rightFileEnd": { "line": end_line, "offset": 1 }
-            });
+            let repo_root = match repository_checkout_dir(&self.repository, ctx).and_then(|path| {
+                crate::validate::ensure_path_within_base(
+                    &path,
+                    &ctx.source_directory,
+                    "Repository checkout root",
+                )
+            }) {
+                Ok(path) => path,
+                Err(err) => {
+                    return Ok(ExecutionResult::failure(format!(
+                        "Failed to resolve repository checkout for '{}': {}",
+                        self.repository, err
+                    )));
+                }
+            };
+            match build_inline_thread_context(
+                &ctx.source_directory,
+                &repo_root,
+                fp,
+                start_line,
+                end_line,
+            ) {
+                Ok(thread_context) => thread_body["threadContext"] = thread_context,
+                Err(err) => {
+                    return Ok(ExecutionResult::failure(format!(
+                        "Failed to anchor inline comment for '{}': {}",
+                        fp, err
+                    )));
+                }
+            }
         }
 
         let client = reqwest::Client::new();
@@ -398,6 +519,7 @@ impl Executor for AddPrCommentResult {
 mod tests {
     use super::*;
     use crate::safeoutputs::ToolResult;
+    use tempfile::tempdir;
 
     #[test]
     fn test_result_has_correct_name() {
@@ -478,6 +600,36 @@ mod tests {
         assert!(result.is_err());
     }
 
+    #[test]
+    fn test_validation_rejects_repository_traversal_selector() {
+        let params = AddPrCommentParams {
+            pull_request_id: 42,
+            content: "This is a valid comment body text.".to_string(),
+            repository: "../sibling-repo".to_string(),
+            file_path: None,
+            start_line: None,
+            line: None,
+            status: "active".to_string(),
+        };
+        let result: Result<AddPrCommentResult, _> = params.try_into();
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_validation_accepts_project_scoped_repository_selector() {
+        let params = AddPrCommentParams {
+            pull_request_id: 42,
+            content: "This is a valid comment body text.".to_string(),
+            repository: "4x4/sdk-FtdiDeviceControl".to_string(),
+            file_path: None,
+            start_line: None,
+            line: None,
+            status: "active".to_string(),
+        };
+        let result: Result<AddPrCommentResult, _> = params.try_into();
+        assert!(result.is_ok());
+    }
+
     #[test]
     fn test_validation_rejects_line_without_file_path() {
         let params = AddPrCommentParams {
@@ -664,4 +816,78 @@ allowed-statuses:
             result.repository
         );
     }
+
+    #[test]
+    fn test_build_inline_thread_context_uses_utf16_end_offset() {
+        let dir = tempdir().unwrap();
+        std::fs::write(dir.path().join("suggestion.rs"), "prefix\nab😀\n").unwrap();
+
+        let thread_context =
+            build_inline_thread_context(dir.path(), dir.path(), "suggestion.rs", 2, 2).unwrap();
+
+        assert_eq!(thread_context["rightFileStart"]["line"], 2);
+        assert_eq!(thread_context["rightFileStart"]["offset"], 1);
+        assert_eq!(thread_context["rightFileEnd"]["line"], 2);
+        assert_eq!(thread_context["rightFileEnd"]["offset"], 5);
+    }
+
+    #[test]
+    fn test_build_inline_thread_context_uses_last_line_for_multiline_span() {
+        let dir = tempdir().unwrap();
+        std::fs::write(
+            dir.path().join("suggestion.rs"),
+            "first line\nab😀\nthird\n",
+        )
+        .unwrap();
+
+        let thread_context =
+            build_inline_thread_context(dir.path(), dir.path(), "suggestion.rs", 1, 2).unwrap();
+
+        assert_eq!(thread_context["rightFileStart"]["line"], 1);
+        assert_eq!(thread_context["rightFileEnd"]["line"], 2);
+        assert_eq!(thread_context["rightFileEnd"]["offset"], 5);
+    }
+
+    #[test]
+    fn test_build_inline_thread_context_rejects_repo_root_outside_workspace() {
+        let workspace = tempdir().unwrap();
+        let outside_repo = tempdir().unwrap();
+        std::fs::write(outside_repo.path().join("suggestion.rs"), "line 1\n").unwrap();
+
+        let err = build_inline_thread_context(
+            workspace.path(),
+            outside_repo.path(),
+            "suggestion.rs",
+            1,
+            1,
+        )
+        .unwrap_err()
+        .to_string();
+
+        assert!(err.contains("outside the build workspace"), "got: {err}");
+    }
+
+    #[test]
+    fn test_repository_checkout_dir_resolves_full_repository_name_to_alias_path() {
+        let workspace = tempdir().unwrap();
+        let alias_dir = workspace.path().join("repo-sdk-ftdidevicecontrol");
+        std::fs::create_dir(&alias_dir).unwrap();
+
+        let mut allowed_repositories = std::collections::HashMap::new();
+        allowed_repositories.insert(
+            "repo-sdk-ftdidevicecontrol".to_string(),
+            "4x4/sdk-FtdiDeviceControl".to_string(),
+        );
+
+        let ctx = ExecutionContext {
+            source_directory: workspace.path().to_path_buf(),
+            allowed_repositories,
+            repository_name: Some("4x4/current-repo".to_string()),
+            ..Default::default()
+        };
+
+        let resolved = repository_checkout_dir("4x4/sdk-ftdidevicecontrol", &ctx).unwrap();
+
+        assert_eq!(resolved, alias_dir);
+    }
 }