Skip to main content

aptu_core/github/
pulls.rs

1// SPDX-License-Identifier: Apache-2.0
2
3//! Pull request fetching via Octocrab.
4//!
5//! Provides functions to parse PR references and fetch PR details
6//! including file diffs for AI review.
7
8use anyhow::{Context, Result};
9use octocrab::Octocrab;
10use tracing::{debug, instrument};
11
12use super::{ReferenceKind, parse_github_reference};
13use crate::ai::types::{PrDetails, PrFile, PrReviewComment, ReviewEvent};
14use crate::error::{AptuError, ResourceType};
15use crate::triage::render_pr_review_comment_body;
16
17/// Result from creating a pull request.
18#[derive(Debug, serde::Serialize)]
19pub struct PrCreateResult {
20    /// PR number.
21    pub pr_number: u64,
22    /// PR URL.
23    pub url: String,
24    /// Head branch.
25    pub branch: String,
26    /// Base branch.
27    pub base: String,
28    /// PR title.
29    pub title: String,
30    /// Whether the PR is a draft.
31    pub draft: bool,
32    /// Number of files changed.
33    pub files_changed: u32,
34    /// Number of additions.
35    pub additions: u64,
36    /// Number of deletions.
37    pub deletions: u64,
38}
39
40/// Parses a PR reference into (owner, repo, number).
41///
42/// Supports multiple formats:
43/// - Full URL: `https://github.com/owner/repo/pull/123`
44/// - Short form: `owner/repo#123`
45/// - Bare number: `123` (requires `repo_context`)
46///
47/// # Arguments
48///
49/// * `reference` - PR reference string
50/// * `repo_context` - Optional repository context for bare numbers (e.g., "owner/repo")
51///
52/// # Returns
53///
54/// Tuple of (owner, repo, number)
55///
56/// # Errors
57///
58/// Returns an error if the reference format is invalid or `repo_context` is missing for bare numbers.
59pub fn parse_pr_reference(
60    reference: &str,
61    repo_context: Option<&str>,
62) -> Result<(String, String, u64)> {
63    parse_github_reference(ReferenceKind::Pull, reference, repo_context)
64}
65
66/// Fetches PR details including file diffs from GitHub.
67///
68/// Uses Octocrab to fetch PR metadata and file changes.
69///
70/// # Arguments
71///
72/// * `client` - Authenticated Octocrab client
73/// * `owner` - Repository owner
74/// * `repo` - Repository name
75/// * `number` - PR number
76///
77/// # Returns
78///
79/// `PrDetails` struct with PR metadata and file diffs.
80///
81/// # Errors
82///
83/// Returns an error if the API call fails or PR is not found.
84#[instrument(skip(client), fields(owner = %owner, repo = %repo, number = number))]
85#[allow(clippy::too_many_lines)]
86pub async fn fetch_pr_details(
87    client: &Octocrab,
88    owner: &str,
89    repo: &str,
90    number: u64,
91    review_config: &crate::config::ReviewConfig,
92) -> Result<PrDetails> {
93    debug!("Fetching PR details");
94
95    // Fetch PR metadata
96    let pr = match client.pulls(owner, repo).get(number).await {
97        Ok(pr) => pr,
98        Err(e) => {
99            // Check if this is a 404 error and if an issue exists instead
100            if let octocrab::Error::GitHub { source, .. } = &e
101                && source.status_code == 404
102            {
103                // Try to fetch as an issue to provide a better error message
104                if (client.issues(owner, repo).get(number).await).is_ok() {
105                    return Err(AptuError::TypeMismatch {
106                        number,
107                        expected: ResourceType::PullRequest,
108                        actual: ResourceType::Issue,
109                    }
110                    .into());
111                }
112                // Issue check failed, fall back to original error
113            }
114            return Err(e)
115                .with_context(|| format!("Failed to fetch PR #{number} from {owner}/{repo}"));
116        }
117    };
118
119    // Fetch PR files (diffs) with pagination (per_page=100, max 300 files)
120    let mut pr_files: Vec<PrFile> = Vec::new();
121    let mut page = client
122        .pulls(owner, repo)
123        .list_files(number)
124        .await
125        .with_context(|| format!("Failed to fetch files for PR #{number}"))?;
126
127    loop {
128        pr_files.extend(page.items.into_iter().map(|f| PrFile {
129            filename: f.filename,
130            status: format!("{:?}", f.status),
131            additions: f.additions,
132            deletions: f.deletions,
133            patch: f.patch,
134            patch_truncated: false,
135            full_content: None,
136        }));
137
138        if pr_files.len() >= 300 {
139            tracing::warn!(
140                "PR #{} has reached 300-file cap; stopping pagination",
141                number
142            );
143            pr_files.truncate(300);
144            break;
145        }
146
147        match client
148            .get_page::<octocrab::models::repos::DiffEntry>(&page.next)
149            .await
150        {
151            Ok(Some(next_page)) => page = next_page,
152            Ok(None) => break,
153            Err(e) => {
154                tracing::warn!("Error fetching next page of files: {}", e);
155                break;
156            }
157        }
158    }
159
160    // Detect truncated patches and attempt Contents API fallback
161    for file in &mut pr_files {
162        #[allow(clippy::collapsible_if)]
163        if let Some(patch) = &file.patch {
164            if is_patch_truncated(patch) {
165                file.patch_truncated = true;
166                // Attempt Contents API fallback
167                if let Ok(Some(content)) = fetch_file_contents_single(
168                    client,
169                    owner,
170                    repo,
171                    &file.filename,
172                    &pr.head.sha,
173                    review_config.max_chars_per_file,
174                )
175                .await
176                {
177                    file.patch = Some(content);
178                }
179            }
180        }
181    }
182
183    // Fetch full file contents for eligible files (default: up to 10 files, max 4000 chars each)
184    let file_contents = fetch_file_contents(
185        client,
186        owner,
187        repo,
188        &pr_files,
189        &pr.head.sha,
190        review_config.max_full_content_files,
191        review_config.max_chars_per_file,
192    )
193    .await;
194
195    // Merge file contents back into pr_files
196    debug_assert_eq!(
197        pr_files.len(),
198        file_contents.len(),
199        "fetch_file_contents must return one entry per file"
200    );
201    let pr_files: Vec<PrFile> = pr_files
202        .into_iter()
203        .zip(file_contents)
204        .map(|(mut file, content)| {
205            file.full_content = content;
206            file
207        })
208        .collect();
209
210    let labels: Vec<String> = pr.labels.iter().map(|l| l.name.clone()).collect();
211
212    let details = PrDetails {
213        owner: owner.to_string(),
214        repo: repo.to_string(),
215        number,
216        title: pr.title.clone(),
217        body: pr.body.clone().unwrap_or_default(),
218        base_branch: pr.base.ref_field,
219        head_branch: pr.head.ref_field,
220        head_sha: pr.head.sha,
221        files: pr_files,
222        url: pr.html_url.to_string(),
223        labels,
224        review_comments: Vec::new(),
225        instructions: None,
226        dep_enrichments: Vec::new(),
227    };
228
229    debug!(
230        file_count = details.files.len(),
231        "PR details fetched successfully"
232    );
233
234    Ok(details)
235}
236
237/// Detects if a patch is truncated mid-hunk by GitHub API.
238///
239/// A patch is considered truncated if the last non-empty line starts with '+' or '-',
240/// indicating an incomplete hunk.
241fn is_patch_truncated(patch: &str) -> bool {
242    let lines: Vec<&str> = patch.lines().collect();
243
244    // Rule 1: Check if last non-empty line starts with '+' or '-' (mid-hunk cutoff)
245    if let Some(last_line) = lines.iter().rev().find(|line| !line.trim().is_empty())
246        && (last_line.starts_with('+') || last_line.starts_with('-'))
247    {
248        return true;
249    }
250
251    // Rule 2: Check if declared hunk size matches actual lines delivered
252    // Parse the last @@ -a,b +c,d @@ header and verify line count
253    if let Some(last_hunk_header) = lines.iter().rev().find(|line| line.contains("@@")) {
254        // Extract the +c,d part from the hunk header
255        if let Some(plus_part) = last_hunk_header.split('+').nth(1) {
256            // Extract the number after '+' and before the next space or @@
257            if let Some(size_str) = plus_part.split_whitespace().next() {
258                // Parse "c,d" format
259                if let Some(count_str) = size_str.split(',').nth(1)
260                    && let Ok(declared_count) = count_str.parse::<usize>()
261                {
262                    // Count actual lines after this hunk header (context + added lines)
263                    // Find the index of this hunk header
264                    if let Some(hunk_idx) = lines.iter().position(|&line| line == *last_hunk_header)
265                    {
266                        let lines_after_hunk = &lines[hunk_idx + 1..];
267                        // Count lines that are context (' '), additions ('+'), or deletions ('-')
268                        // Stop counting if we hit another hunk header
269                        let mut actual_count = 0;
270                        for line in lines_after_hunk {
271                            if line.starts_with("@@") {
272                                break;
273                            }
274                            if line.starts_with(' ')
275                                || line.starts_with('+')
276                                || line.starts_with('-')
277                            {
278                                actual_count += 1;
279                            }
280                        }
281                        // If actual count is less than declared, the hunk is truncated
282                        if actual_count < declared_count {
283                            return true;
284                        }
285                    }
286                }
287            }
288        }
289    }
290
291    false
292}
293
294/// Fetches a single file's content from GitHub Contents API as a fallback for truncated patches.
295///
296/// Returns the file content truncated to `max_chars`, or `None` if the file cannot be fetched.
297/// Non-fatal errors (404, rate limits) are logged as warnings.
298async fn fetch_file_contents_single(
299    client: &Octocrab,
300    owner: &str,
301    repo: &str,
302    filename: &str,
303    head_sha: &str,
304    max_chars: usize,
305) -> Result<Option<String>> {
306    match client
307        .repos(owner, repo)
308        .get_content()
309        .path(filename)
310        .r#ref(head_sha)
311        .send()
312        .await
313    {
314        Ok(content) => {
315            // Try to decode the first item (should be the file, not a directory listing)
316            if let Some(item) = content.items.first() {
317                if let Some(decoded) = item.decoded_content() {
318                    let truncated = if decoded.len() > max_chars {
319                        decoded.chars().take(max_chars).collect::<String>()
320                    } else {
321                        decoded
322                    };
323                    Ok(Some(truncated))
324                } else {
325                    tracing::warn!(
326                        "Failed to decode content for {}/{}/{} at {}",
327                        owner,
328                        repo,
329                        filename,
330                        head_sha
331                    );
332                    Ok(None)
333                }
334            } else {
335                tracing::warn!(
336                    "File content response was empty for {}/{}/{} at {}",
337                    owner,
338                    repo,
339                    filename,
340                    head_sha
341                );
342                Ok(None)
343            }
344        }
345        Err(e) => {
346            tracing::warn!(
347                "Failed to fetch content for {}/{}/{} at {}: {}",
348                owner,
349                repo,
350                filename,
351                head_sha,
352                e
353            );
354            Ok(None)
355        }
356    }
357}
358
359/// Fetches full file contents for PR files from GitHub Contents API.
360///
361/// Fetches content for eligible files up to a specified limit and truncates each to a character limit.
362/// Skips deleted files and files with empty patches. Per-file errors are non-fatal: they produce
363/// `None` entries and log warnings.
364///
365/// # Arguments
366///
367/// * `client` - Authenticated Octocrab client
368/// * `owner` - Repository owner
369/// * `repo` - Repository name
370/// * `files` - Slice of PR files to fetch
371/// * `head_sha` - PR head commit SHA to fetch from
372/// * `max_files` - Maximum number of files to fetch content for
373/// * `max_chars_per_file` - Truncate each file's content at this character limit
374///
375/// # Returns
376///
377/// Vector of `Option<String>` with one entry per input file (in order):
378/// - `Some(content)` if fetch succeeded
379/// - `None` if fetch failed, file was skipped, or file index exceeded `max_files`
380#[instrument(skip(client, files), fields(owner = %owner, repo = %repo, max_files = max_files))]
381async fn fetch_file_contents(
382    client: &Octocrab,
383    owner: &str,
384    repo: &str,
385    files: &[PrFile],
386    head_sha: &str,
387    max_files: usize,
388    max_chars_per_file: usize,
389) -> Vec<Option<String>> {
390    let mut results = Vec::with_capacity(files.len());
391    let mut fetched_count = 0usize;
392
393    for file in files {
394        if should_skip_file(&file.filename, &file.status, file.patch.as_ref()) {
395            results.push(None);
396            continue;
397        }
398
399        // Skip if beyond max_files cap (count only successfully-fetched files)
400        if fetched_count >= max_files {
401            debug!(
402                file = %file.filename,
403                fetched_count = fetched_count,
404                max_files = max_files,
405                "Fetched file count exceeds max_files cap"
406            );
407            results.push(None);
408            continue;
409        }
410
411        // Attempt to fetch file content
412        match client
413            .repos(owner, repo)
414            .get_content()
415            .path(&file.filename)
416            .r#ref(head_sha)
417            .send()
418            .await
419        {
420            Ok(content) => {
421                // Try to decode the first item (should be the file, not a directory listing)
422                if let Some(item) = content.items.first() {
423                    if let Some(decoded) = item.decoded_content() {
424                        let truncated = if decoded.len() > max_chars_per_file {
425                            decoded.chars().take(max_chars_per_file).collect::<String>()
426                        } else {
427                            decoded
428                        };
429                        debug!(
430                            file = %file.filename,
431                            content_len = truncated.len(),
432                            "File content fetched and truncated"
433                        );
434                        results.push(Some(truncated));
435                        fetched_count += 1;
436                    } else {
437                        tracing::warn!(
438                            file = %file.filename,
439                            "Failed to decode file content; skipping"
440                        );
441                        results.push(None);
442                    }
443                } else {
444                    tracing::warn!(
445                        file = %file.filename,
446                        "File content response was empty; skipping"
447                    );
448                    results.push(None);
449                }
450            }
451            Err(e) => {
452                tracing::warn!(
453                    file = %file.filename,
454                    err = %e,
455                    "Failed to fetch file content; skipping"
456                );
457                results.push(None);
458            }
459        }
460    }
461
462    results
463}
464
465/// Posts a PR review to GitHub.
466///
467/// Uses Octocrab's custom HTTP POST to create a review with the specified event type.
468/// Requires write access to the repository.
469///
470/// # Arguments
471///
472/// * `client` - Authenticated Octocrab client
473/// * `owner` - Repository owner
474/// * `repo` - Repository name
475/// * `number` - PR number
476/// * `body` - Review comment text
477/// * `event` - Review event type (Comment, Approve, or `RequestChanges`)
478/// * `comments` - Inline review comments to attach; entries with `line = None` are silently skipped
479/// * `commit_id` - Head commit SHA to associate with the review; omitted from payload if empty
480///
481/// # Returns
482///
483/// Review ID on success.
484///
485/// # Errors
486///
487/// Returns an error if the API call fails, user lacks write access, or PR is not found.
488#[allow(clippy::too_many_arguments)]
489#[instrument(skip(client, comments), fields(owner = %owner, repo = %repo, number = number, event = %event))]
490pub async fn post_pr_review(
491    client: &Octocrab,
492    owner: &str,
493    repo: &str,
494    number: u64,
495    body: &str,
496    event: ReviewEvent,
497    comments: &[PrReviewComment],
498    commit_id: &str,
499) -> Result<u64> {
500    debug!("Posting PR review");
501
502    let route = format!("/repos/{owner}/{repo}/pulls/{number}/reviews");
503
504    // Build inline comments array; skip entries without a line number.
505    let inline_comments: Vec<serde_json::Value> = comments
506        .iter()
507        // Comments without a line number cannot be anchored to the diff; skip silently.
508        .filter_map(|c| {
509            c.line.map(|line| {
510                serde_json::json!({
511                    "path": c.file,
512                    "line": line,
513                    // RIGHT = new version of the file (added/changed lines).
514                    // Use line (file line number) rather than the deprecated
515                    // position (diff hunk offset) so no hunk parsing is needed.
516                    "side": "RIGHT",
517                    "body": render_pr_review_comment_body(c),
518                })
519            })
520        })
521        .collect();
522
523    let mut payload = serde_json::json!({
524        "body": body,
525        "event": event.to_string(),
526        "comments": inline_comments,
527    });
528
529    // commit_id is optional; include only when non-empty.
530    if !commit_id.is_empty() {
531        payload["commit_id"] = serde_json::Value::String(commit_id.to_string());
532    }
533
534    #[derive(serde::Deserialize)]
535    struct ReviewResponse {
536        id: u64,
537    }
538
539    let response: ReviewResponse = client.post(route, Some(&payload)).await.with_context(|| {
540        format!(
541            "Failed to post review to PR #{number} in {owner}/{repo}. \
542                 Check that you have write access to the repository."
543        )
544    })?;
545
546    debug!(review_id = response.id, "PR review posted successfully");
547
548    Ok(response.id)
549}
550
551/// Deletes a PR review comment.
552///
553/// # Errors
554///
555/// Returns an error if the API request fails. 404 errors (comment not found)
556/// are treated as success (idempotent).
557#[instrument(skip(client), fields(owner = %owner, repo = %repo, comment_id = comment_id))]
558pub async fn delete_pr_review_comment(
559    client: &Octocrab,
560    owner: &str,
561    repo: &str,
562    comment_id: u64,
563) -> Result<()> {
564    debug!("Deleting PR review comment");
565
566    let route = format!("/repos/{owner}/{repo}/pulls/comments/{comment_id}");
567
568    // Use generic delete method; needs explicit empty object body type
569    let empty_body = serde_json::json!({});
570    let result: std::result::Result<serde_json::Value, _> =
571        client.delete(&route, Some(&empty_body)).await;
572
573    match result {
574        Ok(_) => {
575            debug!("PR review comment deleted successfully");
576            Ok(())
577        }
578        Err(e)
579            if let octocrab::Error::GitHub { source, .. } = &e
580                && source.status_code.as_u16() == 404 =>
581        {
582            debug!("PR review comment already deleted (404); treating as success");
583            Ok(())
584        }
585        Err(e) => {
586            Err(e).with_context(|| format!("Failed to delete PR review comment #{comment_id}"))
587        }
588    }
589}
590
591/// Extract labels from PR metadata (title and file paths).
592///
593/// Parses conventional commit prefix from PR title and maps file paths to scope labels.
594/// Returns a vector of label names to apply to the PR.
595///
596/// # Arguments
597/// * `title` - PR title (may contain conventional commit prefix)
598/// * `file_paths` - List of file paths changed in the PR
599///
600/// # Returns
601/// Vector of label names to apply
602#[must_use]
603pub fn labels_from_pr_metadata(title: &str, file_paths: &[String]) -> Vec<String> {
604    let mut labels = std::collections::HashSet::new();
605
606    // Extract conventional commit prefix from title
607    // Handle both "feat: ..." and "feat(scope): ..." formats
608    let prefix = title
609        .split(':')
610        .next()
611        .unwrap_or("")
612        .split('(')
613        .next()
614        .unwrap_or("")
615        .trim();
616
617    // Map conventional commit type to label
618    let type_label = match prefix {
619        "feat" | "perf" => Some("enhancement"),
620        "fix" => Some("bug"),
621        "docs" => Some("documentation"),
622        "refactor" => Some("refactor"),
623        _ => None,
624    };
625
626    if let Some(label) = type_label {
627        labels.insert(label.to_string());
628    }
629
630    // Map file paths to scope labels
631    for path in file_paths {
632        let scope = if path.starts_with("crates/aptu-cli/") {
633            Some("cli")
634        } else if path.starts_with("docs/") {
635            Some("documentation")
636        } else {
637            None
638        };
639
640        if let Some(label) = scope {
641            labels.insert(label.to_string());
642        }
643    }
644
645    labels.into_iter().collect()
646}
647
648/// Creates a pull request on GitHub.
649///
650/// # Arguments
651///
652/// * `client` - Authenticated Octocrab client
653/// * `owner` - Repository owner
654/// * `repo` - Repository name
655/// * `title` - PR title
656/// * `head_branch` - Head branch (the branch with changes)
657/// * `base_branch` - Base branch (the branch to merge into)
658/// * `body` - Optional PR body text
659///
660/// # Returns
661///
662/// `PrCreateResult` with PR metadata.
663///
664/// # Errors
665///
666/// Returns an error if the API call fails or the user lacks write access.
667#[instrument(skip(client), fields(owner = %owner, repo = %repo, head = %head_branch, base = %base_branch))]
668#[allow(clippy::too_many_arguments)]
669pub async fn create_pull_request(
670    client: &Octocrab,
671    owner: &str,
672    repo: &str,
673    title: &str,
674    head_branch: &str,
675    base_branch: &str,
676    body: Option<&str>,
677    draft: bool,
678) -> anyhow::Result<PrCreateResult> {
679    debug!("Creating pull request");
680
681    let pr = client
682        .pulls(owner, repo)
683        .create(title, head_branch, base_branch)
684        .body(body.unwrap_or_default())
685        .draft(draft)
686        .send()
687        .await
688        .with_context(|| {
689            format!("Failed to create PR in {owner}/{repo} ({head_branch} -> {base_branch})")
690        })?;
691
692    let result = PrCreateResult {
693        pr_number: pr.number,
694        url: pr.html_url.to_string(),
695        branch: pr.head.ref_field,
696        base: pr.base.ref_field,
697        title: pr.title.clone(),
698        draft: pr.draft.unwrap_or(false),
699        files_changed: u32::try_from(pr.changed_files).unwrap_or(u32::MAX),
700        additions: pr.additions,
701        deletions: pr.deletions,
702    };
703
704    debug!(
705        pr_number = result.pr_number,
706        "Pull request created successfully"
707    );
708
709    Ok(result)
710}
711
712/// Determines whether a file should be skipped during fetch based on status and patch.
713/// Emits a debug log with the skip reason. Returns true if the file should be skipped
714/// (removed status or no patch), false otherwise.
715fn should_skip_file(filename: &str, status: &str, patch: Option<&String>) -> bool {
716    if status.to_lowercase().contains("removed") {
717        debug!(file = %filename, "Skipping removed file");
718        return true;
719    }
720    if patch.is_none_or(String::is_empty) {
721        debug!(file = %filename, "Skipping file with empty patch");
722        return true;
723    }
724    false
725}
726
727#[cfg(test)]
728mod tests {
729    use super::*;
730    use crate::ai::types::CommentSeverity;
731
732    fn decode_content(encoded: &str, max_chars: usize) -> Option<String> {
733        use base64::Engine;
734        let engine = base64::engine::general_purpose::STANDARD;
735        let decoded_bytes = engine.decode(encoded).ok()?;
736        let decoded_str = String::from_utf8(decoded_bytes).ok()?;
737
738        if decoded_str.len() <= max_chars {
739            Some(decoded_str)
740        } else {
741            Some(decoded_str.chars().take(max_chars).collect::<String>())
742        }
743    }
744
745    #[test]
746    fn test_pr_create_result_fields() {
747        // Arrange / Act: construct directly (no network call needed)
748        let result = PrCreateResult {
749            pr_number: 42,
750            url: "https://github.com/owner/repo/pull/42".to_string(),
751            branch: "feat/my-feature".to_string(),
752            base: "main".to_string(),
753            title: "feat: add feature".to_string(),
754            draft: false,
755            files_changed: 3,
756            additions: 100,
757            deletions: 10,
758        };
759
760        // Assert
761        assert_eq!(result.pr_number, 42);
762        assert_eq!(result.url, "https://github.com/owner/repo/pull/42");
763        assert_eq!(result.branch, "feat/my-feature");
764        assert_eq!(result.base, "main");
765        assert_eq!(result.title, "feat: add feature");
766        assert!(!result.draft);
767        assert_eq!(result.files_changed, 3);
768        assert_eq!(result.additions, 100);
769        assert_eq!(result.deletions, 10);
770    }
771
772    // ---------------------------------------------------------------------------
773    // post_pr_review payload construction
774    // ---------------------------------------------------------------------------
775
776    /// Helper: build the inline comments JSON array using the same logic as
777    /// `post_pr_review`, without making a live HTTP call.
778    fn build_inline_comments(comments: &[PrReviewComment]) -> Vec<serde_json::Value> {
779        comments
780            .iter()
781            .filter_map(|c| {
782                c.line.map(|line| {
783                    serde_json::json!({
784                        "path": c.file,
785                        "line": line,
786                        "side": "RIGHT",
787                        "body": render_pr_review_comment_body(c),
788                    })
789                })
790            })
791            .collect()
792    }
793
794    #[test]
795    fn test_post_pr_review_payload_with_comments() {
796        // Arrange
797        let comments = vec![PrReviewComment {
798            file: "src/main.rs".to_string(),
799            line: Some(42),
800            comment: "Consider using a match here.".to_string(),
801            severity: CommentSeverity::Suggestion,
802            suggested_code: None,
803        }];
804
805        // Act
806        let inline = build_inline_comments(&comments);
807
808        // Assert
809        assert_eq!(inline.len(), 1);
810        assert_eq!(inline[0]["path"], "src/main.rs");
811        assert_eq!(inline[0]["line"], 42);
812        assert_eq!(inline[0]["side"], "RIGHT");
813        assert_eq!(inline[0]["body"], "Consider using a match here.");
814    }
815
816    #[test]
817    fn test_post_pr_review_skips_none_line_comments() {
818        // Arrange: one comment with a line, one without.
819        let comments = vec![
820            PrReviewComment {
821                file: "src/lib.rs".to_string(),
822                line: None,
823                comment: "General file comment.".to_string(),
824                severity: CommentSeverity::Info,
825                suggested_code: None,
826            },
827            PrReviewComment {
828                file: "src/lib.rs".to_string(),
829                line: Some(10),
830                comment: "Inline comment.".to_string(),
831                severity: CommentSeverity::Warning,
832                suggested_code: None,
833            },
834        ];
835
836        // Act
837        let inline = build_inline_comments(&comments);
838
839        // Assert: only the comment with a line is included.
840        assert_eq!(inline.len(), 1);
841        assert_eq!(inline[0]["line"], 10);
842    }
843
844    #[test]
845    fn test_post_pr_review_empty_comments() {
846        // Arrange
847        let comments: Vec<PrReviewComment> = vec![];
848
849        // Act
850        let inline = build_inline_comments(&comments);
851
852        // Assert: empty slice produces empty array, which serializes as [].
853        assert!(inline.is_empty());
854        let serialized = serde_json::to_string(&inline).unwrap();
855        assert_eq!(serialized, "[]");
856    }
857
858    // ---------------------------------------------------------------------------
859    // Existing tests
860    // ---------------------------------------------------------------------------
861
862    // Smoke test to verify parse_pr_reference delegates correctly.
863    // Comprehensive parsing tests are in github/mod.rs.
864    #[test]
865    fn test_parse_pr_reference_delegates_to_shared() {
866        let (owner, repo, number) =
867            parse_pr_reference("https://github.com/block/goose/pull/123", None).unwrap();
868        assert_eq!(owner, "block");
869        assert_eq!(repo, "goose");
870        assert_eq!(number, 123);
871    }
872
873    #[test]
874    fn test_title_prefix_to_label_mapping() {
875        let cases = vec![
876            (
877                "feat: add new feature",
878                vec!["enhancement"],
879                "feat should map to enhancement",
880            ),
881            ("fix: resolve bug", vec!["bug"], "fix should map to bug"),
882            (
883                "docs: update readme",
884                vec!["documentation"],
885                "docs should map to documentation",
886            ),
887            (
888                "refactor: improve code",
889                vec!["refactor"],
890                "refactor should map to refactor",
891            ),
892            (
893                "perf: optimize",
894                vec!["enhancement"],
895                "perf should map to enhancement",
896            ),
897            (
898                "chore: update deps",
899                vec![],
900                "chore should produce no labels",
901            ),
902        ];
903
904        for (title, expected_labels, msg) in cases {
905            let labels = labels_from_pr_metadata(title, &[]);
906            for expected in &expected_labels {
907                assert!(
908                    labels.contains(&expected.to_string()),
909                    "{msg}: expected '{expected}' in {labels:?}",
910                );
911            }
912            if expected_labels.is_empty() {
913                assert!(labels.is_empty(), "{msg}: expected empty, got {labels:?}");
914            }
915        }
916    }
917
918    #[test]
919    fn test_file_path_to_scope_mapping() {
920        let cases = vec![
921            (
922                "feat: cli",
923                vec!["crates/aptu-cli/src/main.rs"],
924                vec!["enhancement", "cli"],
925                "cli path should map to cli scope",
926            ),
927            (
928                "feat: docs",
929                vec!["docs/GITHUB_ACTION.md"],
930                vec!["enhancement", "documentation"],
931                "docs path should map to documentation scope",
932            ),
933            (
934                "feat: workflow",
935                vec![".github/workflows/test.yml"],
936                vec!["enhancement"],
937                "workflow path should be ignored",
938            ),
939        ];
940
941        for (title, paths, expected_labels, msg) in cases {
942            let labels = labels_from_pr_metadata(
943                title,
944                &paths
945                    .iter()
946                    .map(std::string::ToString::to_string)
947                    .collect::<Vec<_>>(),
948            );
949            for expected in expected_labels {
950                assert!(
951                    labels.contains(&expected.to_string()),
952                    "{msg}: expected '{expected}' in {labels:?}",
953                );
954            }
955        }
956    }
957
958    #[test]
959    fn test_combined_title_and_paths() {
960        let labels = labels_from_pr_metadata(
961            "feat: multi",
962            &[
963                "crates/aptu-cli/src/main.rs".to_string(),
964                "docs/README.md".to_string(),
965            ],
966        );
967        assert!(
968            labels.contains(&"enhancement".to_string()),
969            "should include enhancement from feat prefix"
970        );
971        assert!(
972            labels.contains(&"cli".to_string()),
973            "should include cli from path"
974        );
975        assert!(
976            labels.contains(&"documentation".to_string()),
977            "should include documentation from path"
978        );
979    }
980
981    #[test]
982    fn test_no_match_returns_empty() {
983        let cases = vec![
984            (
985                "Random title",
986                vec![],
987                "unrecognized prefix should return empty",
988            ),
989            (
990                "chore: update",
991                vec![],
992                "ignored prefix should return empty",
993            ),
994        ];
995
996        for (title, paths, msg) in cases {
997            let labels = labels_from_pr_metadata(title, &paths);
998            assert!(labels.is_empty(), "{msg}: got {labels:?}");
999        }
1000    }
1001
1002    #[test]
1003    fn test_scoped_prefix_extracts_type() {
1004        let labels = labels_from_pr_metadata("feat(cli): add new feature", &[]);
1005        assert!(
1006            labels.contains(&"enhancement".to_string()),
1007            "scoped prefix should extract type from feat(cli)"
1008        );
1009    }
1010
1011    #[test]
1012    fn test_duplicate_labels_deduplicated() {
1013        let labels = labels_from_pr_metadata("docs: update", &["docs/README.md".to_string()]);
1014        assert_eq!(
1015            labels.len(),
1016            1,
1017            "should have exactly one label when title and path both map to documentation"
1018        );
1019        assert!(
1020            labels.contains(&"documentation".to_string()),
1021            "should contain documentation label"
1022        );
1023    }
1024
1025    #[test]
1026    fn test_should_skip_file_respects_fetched_count_cap() {
1027        // Test that should_skip_file correctly identifies files to skip.
1028        // Files with removed status or no patch should be skipped.
1029        let removed_file = PrFile {
1030            filename: "removed.rs".to_string(),
1031            status: "removed".to_string(),
1032            additions: 0,
1033            deletions: 5,
1034            patch: None,
1035            patch_truncated: false,
1036            full_content: None,
1037        };
1038        let modified_file = PrFile {
1039            filename: "file_0.rs".to_string(),
1040            status: "modified".to_string(),
1041            additions: 1,
1042            deletions: 0,
1043            patch: Some("+ new code".to_string()),
1044            patch_truncated: false,
1045            full_content: None,
1046        };
1047        let no_patch_file = PrFile {
1048            filename: "file_1.rs".to_string(),
1049            status: "modified".to_string(),
1050            additions: 1,
1051            deletions: 0,
1052            patch: None,
1053            patch_truncated: false,
1054            full_content: None,
1055        };
1056
1057        // Assert: removed files are skipped
1058        assert!(
1059            should_skip_file(
1060                &removed_file.filename,
1061                &removed_file.status,
1062                removed_file.patch.as_ref()
1063            ),
1064            "removed files should be skipped"
1065        );
1066
1067        // Assert: modified files with patch are not skipped
1068        assert!(
1069            !should_skip_file(
1070                &modified_file.filename,
1071                &modified_file.status,
1072                modified_file.patch.as_ref()
1073            ),
1074            "modified files with patch should not be skipped"
1075        );
1076
1077        // Assert: files without patch are skipped
1078        assert!(
1079            should_skip_file(
1080                &no_patch_file.filename,
1081                &no_patch_file.status,
1082                no_patch_file.patch.as_ref()
1083            ),
1084            "files without patch should be skipped"
1085        );
1086    }
1087
1088    #[test]
1089    fn test_decode_content_valid_base64() {
1090        // Arrange: valid base64-encoded string
1091        use base64::Engine;
1092        let engine = base64::engine::general_purpose::STANDARD;
1093        let original = "Hello, World!";
1094        let encoded = engine.encode(original);
1095
1096        // Act: decode with sufficient max_chars
1097        let result = decode_content(&encoded, 1000);
1098
1099        // Assert: decoding succeeds and matches original
1100        assert_eq!(
1101            result,
1102            Some(original.to_string()),
1103            "valid base64 should decode successfully"
1104        );
1105    }
1106
1107    #[test]
1108    fn test_decode_content_invalid_base64() {
1109        // Arrange: invalid base64 string
1110        let invalid_base64 = "!!!invalid!!!";
1111
1112        // Act: attempt to decode
1113        let result = decode_content(invalid_base64, 1000);
1114
1115        // Assert: decoding fails gracefully
1116        assert_eq!(result, None, "invalid base64 should return None");
1117    }
1118
1119    #[test]
1120    fn test_decode_content_truncates_at_max_chars() {
1121        // Arrange: multi-byte UTF-8 string (Japanese characters)
1122        use base64::Engine;
1123        let engine = base64::engine::general_purpose::STANDARD;
1124        let original = "こんにちは".repeat(10); // 50 characters total
1125        let encoded = engine.encode(&original);
1126        let max_chars = 10;
1127
1128        // Act: decode with max_chars limit
1129        let result = decode_content(&encoded, max_chars);
1130
1131        // Assert: result is truncated to max_chars on character boundary
1132        assert!(result.is_some(), "decoding should succeed");
1133        let decoded = result.unwrap();
1134        assert_eq!(
1135            decoded.chars().count(),
1136            max_chars,
1137            "output should be truncated to max_chars on character boundary"
1138        );
1139        assert!(
1140            decoded.is_char_boundary(decoded.len()),
1141            "output should be valid UTF-8 (truncated on char boundary)"
1142        );
1143    }
1144
1145    #[test]
1146    fn test_list_files_pagination_collects_all_pages() {
1147        // Arrange: simulate pagination with two pages
1148        // Page 1: 100 items with next_link set
1149        let mut page1_items = Vec::new();
1150        for i in 0..100 {
1151            page1_items.push(PrFile {
1152                filename: format!("file{}.rs", i),
1153                status: "modified".to_string(),
1154                additions: 1,
1155                deletions: 0,
1156                patch: Some("@@ -1,1 +1,1 @@\n-old\n+new".to_string()),
1157                patch_truncated: false,
1158                full_content: None,
1159            });
1160        }
1161
1162        // Page 2: 50 items with no next_link
1163        let mut page2_items = Vec::new();
1164        for i in 100..150 {
1165            page2_items.push(PrFile {
1166                filename: format!("file{}.rs", i),
1167                status: "modified".to_string(),
1168                additions: 1,
1169                deletions: 0,
1170                patch: Some("@@ -1,1 +1,1 @@\n-old\n+new".to_string()),
1171                patch_truncated: false,
1172                full_content: None,
1173            });
1174        }
1175
1176        // Act: collect all items (simulating pagination loop)
1177        let mut all_files = Vec::new();
1178        all_files.extend(page1_items);
1179        all_files.extend(page2_items);
1180
1181        // Assert: total collected == 150
1182        assert_eq!(
1183            all_files.len(),
1184            150,
1185            "pagination should collect all items from both pages"
1186        );
1187    }
1188
1189    #[test]
1190    fn test_list_files_pagination_respects_300_file_cap() {
1191        // Arrange: build a Vec of 301 PrFile items
1192        let mut files = Vec::new();
1193        for i in 0..301 {
1194            files.push(PrFile {
1195                filename: format!("file{}.rs", i),
1196                status: "modified".to_string(),
1197                additions: 1,
1198                deletions: 0,
1199                patch: Some("@@ -1,1 +1,1 @@\n-old\n+new".to_string()),
1200                patch_truncated: false,
1201                full_content: None,
1202            });
1203        }
1204
1205        // Act: apply the 300-file cap (simulating the truncate logic)
1206        if files.len() >= 300 {
1207            files.truncate(300);
1208        }
1209
1210        // Assert: result.len() == 300
1211        assert_eq!(files.len(), 300, "pagination should enforce 300-file cap");
1212    }
1213
1214    #[test]
1215    fn test_is_patch_truncated_detects_mid_hunk_plus() {
1216        // Test: patch ending with '+' (mid-hunk truncation)
1217        let truncated_patch = "@@ -1,3 +1,4 @@\n line1\n line2\n+";
1218        assert!(
1219            is_patch_truncated(truncated_patch),
1220            "patch ending with + should be detected as truncated"
1221        );
1222    }
1223
1224    #[test]
1225    fn test_is_patch_truncated_detects_mid_hunk_minus() {
1226        // Test: patch ending with '-' (mid-hunk truncation)
1227        let truncated_patch = "@@ -1,3 +1,4 @@\n line1\n line2\n-";
1228        assert!(
1229            is_patch_truncated(truncated_patch),
1230            "patch ending with - should be detected as truncated"
1231        );
1232    }
1233
1234    #[test]
1235    fn test_is_patch_truncated_clean_patch_context_line() {
1236        // Test: patch ending with ' ' (context line, not truncated)
1237        let clean_patch = "@@ -1,3 +1,3 @@\n line1\n line2\n line3";
1238        assert!(
1239            !is_patch_truncated(clean_patch),
1240            "patch ending with context line should not be detected as truncated"
1241        );
1242    }
1243
1244    #[test]
1245    fn test_is_patch_truncated_correct_hunk_line_count() {
1246        // Test: patch with correct hunk line count (declared 3, actual 3)
1247        let clean_patch = "@@ -1,3 +1,3 @@\n line1\n line2\n line3";
1248        assert!(
1249            !is_patch_truncated(clean_patch),
1250            "patch with correct hunk line count should not be detected as truncated"
1251        );
1252    }
1253
1254    #[test]
1255    fn test_is_patch_truncated_declared_hunk_size_larger_than_delivered() {
1256        // Test: patch with declared hunk size larger than delivered lines
1257        // Declared: +1,4 (4 lines in new file), Actual: only 2 lines delivered
1258        let truncated_patch = "@@ -1,3 +1,4 @@\n line1\n line2";
1259        assert!(
1260            is_patch_truncated(truncated_patch),
1261            "patch with declared hunk size larger than delivered should be detected as truncated"
1262        );
1263    }
1264
1265    #[test]
1266    fn test_is_patch_truncated_no_hunk_header_but_last_line_plus() {
1267        // Test: patch with no @@ header but last line is '+'
1268        let truncated_patch = "line1\nline2\n+";
1269        assert!(
1270            is_patch_truncated(truncated_patch),
1271            "patch with no @@ header but ending with + should be detected as truncated"
1272        );
1273    }
1274
1275    #[test]
1276    fn test_is_patch_truncated_empty_patch() {
1277        // Test: empty patch
1278        let empty_patch = "";
1279        assert!(
1280            !is_patch_truncated(empty_patch),
1281            "empty patch should not be detected as truncated"
1282        );
1283    }
1284
1285    #[test]
1286    fn test_is_patch_truncated_multiple_hunks_last_hunk_truncated() {
1287        // Test: multiple hunks where the last hunk is truncated
1288        let truncated_patch = "@@ -1,2 +1,2 @@\n line1\n line2\n@@ -5,3 +5,4 @@\n line5\n line6";
1289        assert!(
1290            is_patch_truncated(truncated_patch),
1291            "patch with last hunk truncated should be detected as truncated"
1292        );
1293    }
1294
1295    #[test]
1296    fn test_fetch_file_contents_fallback_on_truncated_patch() {
1297        // Note: The Contents API network call cannot be unit-tested without a mock.
1298        // The fallback is exercised in integration tests via the full fetch_pr_details flow.
1299        // Unit tests for is_patch_truncated are above.
1300    }
1301}