From 2a9119802685845cc83db359e5700ce5c82ad0f7 Mon Sep 17 00:00:00 2001 From: Hesham Salman Date: Wed, 20 May 2026 16:18:23 -0700 Subject: [PATCH] fix: separate PR fetch source from review engine --- README.md | 6 +- crates/inspect-cli/src/commands/mod.rs | 1 + crates/inspect-cli/src/commands/pr.rs | 140 ++++------ crates/inspect-cli/src/commands/pr_source.rs | 264 +++++++++++++++++++ crates/inspect-cli/src/commands/review.rs | 191 ++++++++++++-- docs/src/app/docs/page.tsx | 26 ++ site/src/app/docs/page.tsx | 8 + 7 files changed, 519 insertions(+), 117 deletions(-) create mode 100644 crates/inspect-cli/src/commands/pr_source.rs diff --git a/README.md b/README.md index 8165d36..90b62e8 100644 --- a/README.md +++ b/README.md @@ -107,10 +107,12 @@ inspect diff HEAD~1 --format markdown # markdown output (for agents) ### `inspect pr ` -Review all changes in a GitHub pull request. Uses `gh` CLI to resolve base/head refs. +Review all changes in a GitHub pull request. By default, uses `gh` CLI to resolve base/head commit OIDs and local git objects. Use `--fetch github` to fetch PR files through the GitHub API without local refs. ```bash inspect pr 42 +inspect pr 42 --fetch github --remote owner/repo +inspect pr 42 --base abc123 --head def456 inspect pr 42 --min-risk medium inspect pr 42 --format json ``` @@ -132,6 +134,8 @@ Triage + LLM review. Triages entities by risk, sends the highest-risk ones to an inspect review HEAD~1 # Anthropic (default) inspect review HEAD~1 --provider ollama --model llama3 # local Ollama inspect review HEAD~1 --api-base http://localhost:8000/v1 --model my-model # any OpenAI-compatible server +inspect review 42 --fetch github --remote owner/repo --provider anthropic # review a GitHub PR locally +inspect review 42 --engine hosted --remote owner/repo # submit to hosted inspect API inspect review HEAD~1 --min-risk medium # review more entities inspect review HEAD~1 --max-entities 20 # send more to LLM ``` diff --git a/crates/inspect-cli/src/commands/mod.rs b/crates/inspect-cli/src/commands/mod.rs index 620b6f1..cc82d79 100644 --- a/crates/inspect-cli/src/commands/mod.rs +++ b/crates/inspect-cli/src/commands/mod.rs @@ -6,6 +6,7 @@ pub mod grep; pub mod login; pub mod logout; pub mod pr; +pub mod pr_source; pub mod predict; pub mod review; pub mod whoami; diff --git a/crates/inspect-cli/src/commands/pr.rs b/crates/inspect-cli/src/commands/pr.rs index 27a7870..144e417 100644 --- a/crates/inspect-cli/src/commands/pr.rs +++ b/crates/inspect-cli/src/commands/pr.rs @@ -1,14 +1,13 @@ use std::path::PathBuf; -use std::process::Command; use clap::Args; -use sem_core::git::types::DiffScope; +use crate::commands::pr_source::{ + analyze_explicit_range, analyze_github_pr, analyze_local_pr, FetchSource, +}; use crate::formatters; use crate::OutputFormat; -use inspect_core::analyze::{analyze, analyze_remote, retain_entity_reviews}; -use inspect_core::github::GitHubClient; -use inspect_core::noise::is_noise_file; +use inspect_core::analyze::retain_entity_reviews; use inspect_core::types::RiskLevel; #[derive(Args)] @@ -28,116 +27,65 @@ pub struct PrArgs { #[arg(long)] pub context: bool, - /// Remote repository (owner/repo). If set, fetches from GitHub API instead of local git. + /// PR content source + #[arg(long, value_enum, default_value = "local")] + pub fetch: FetchSource, + + /// Remote repository (owner/repo), required with --fetch github. #[arg(long)] pub remote: Option, + /// Explicit base commit/ref to compare instead of looking up PR refs. + #[arg(long, requires = "head")] + pub base: Option, + + /// Explicit head commit/ref to compare instead of looking up PR refs. + #[arg(long, requires = "base")] + pub head: Option, + /// Repository path (for local mode) #[arg(short = 'C', long, default_value = ".")] pub repo: PathBuf, } pub async fn run(args: PrArgs) { - if let Some(ref remote_repo) = args.remote { - run_remote(&args, remote_repo).await; - } else { - run_local(&args); - } -} - -fn run_local(args: &PrArgs) { let repo = args.repo.canonicalize().unwrap_or(args.repo.clone()); - let output = Command::new("gh") - .args([ - "pr", - "view", - &args.number.to_string(), - "--json", - "baseRefName,headRefName", - ]) - .current_dir(&repo) - .output(); - - let output = match output { - Ok(o) if o.status.success() => o, - Ok(o) => { - eprintln!( - "error: gh pr view failed: {}", - String::from_utf8_lossy(&o.stderr) - ); - std::process::exit(1); - } - Err(e) => { - eprintln!("error: could not run gh CLI: {}", e); - std::process::exit(1); - } - }; - - let json: serde_json::Value = - serde_json::from_slice(&output.stdout).expect("invalid gh output"); - let base = json["baseRefName"].as_str().unwrap_or("main"); - let head = json["headRefName"].as_str().unwrap_or("HEAD"); - - let scope = DiffScope::Range { - from: base.to_string(), - to: head.to_string(), - }; - - match analyze(&repo, scope) { - Ok(mut result) => { - apply_filters_and_print(&mut result, args); - } - Err(e) => { - eprintln!("error: {}", e); + let result = if args.base.is_some() || args.head.is_some() { + if args.fetch == FetchSource::Github { + eprintln!("error: --base/--head cannot be combined with --fetch github"); std::process::exit(1); } - } -} - -async fn run_remote(args: &PrArgs, remote_repo: &str) { - let client = match GitHubClient::new() { - Ok(c) => c, - Err(e) => { - eprintln!("error: {}", e); + if args.remote.is_some() { + eprintln!("error: --remote is only used with --fetch github"); std::process::exit(1); } - }; - - eprintln!("Fetching PR #{} from {}...", args.number, remote_repo); - - let pr = match client.get_pr(remote_repo, args.number).await { - Ok(pr) => pr, - Err(e) => { - eprintln!("error: {}", e); - std::process::exit(1); + analyze_explicit_range(&repo, args.base.as_deref(), args.head.as_deref()) + .and_then(|result| result.ok_or_else(|| "missing --base/--head".to_string())) + } else { + match args.fetch { + FetchSource::Local => { + if args.remote.is_some() { + eprintln!( + "error: --remote names a GitHub repository; use --fetch github --remote owner/repo" + ); + std::process::exit(1); + } + analyze_local_pr(&repo, args.number) + } + FetchSource::Github => { + let Some(remote_repo) = args.remote.as_deref() else { + eprintln!("error: --fetch github requires --remote owner/repo"); + std::process::exit(1); + }; + analyze_github_pr(remote_repo, args.number).await + } } }; - let visible_files: Vec<_> = pr - .files - .iter() - .filter(|f| !is_noise_file(&f.filename)) - .cloned() - .collect(); - - let noise_count = pr.files.len() - visible_files.len(); - if noise_count > 0 { - eprintln!("({} noise files hidden)", noise_count); - } - - eprintln!("Fetching {} file contents...", visible_files.len()); - - // Use head_sha (commit SHA) instead of head_ref (branch name) for fetching - // after content. For fork PRs, the branch name doesn't exist on the base repo, - // but the commit SHA is accessible via GitHub's merge refs. - let file_pairs = client - .get_file_pairs(remote_repo, &visible_files, &pr.base_sha, &pr.head_sha) - .await; - - match analyze_remote(&file_pairs) { + match result { Ok(mut result) => { - apply_filters_and_print(&mut result, args); + apply_filters_and_print(&mut result, &args); } Err(e) => { eprintln!("error: {}", e); diff --git a/crates/inspect-cli/src/commands/pr_source.rs b/crates/inspect-cli/src/commands/pr_source.rs new file mode 100644 index 0000000..8bff701 --- /dev/null +++ b/crates/inspect-cli/src/commands/pr_source.rs @@ -0,0 +1,264 @@ +use std::path::Path; +use std::process::Command; + +use clap::ValueEnum; +use sem_core::git::types::DiffScope; +use serde::Deserialize; + +use inspect_core::analyze::{analyze, analyze_remote}; +use inspect_core::github::GitHubClient; +use inspect_core::noise::is_noise_file; +use inspect_core::types::ReviewResult; + +#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)] +pub enum FetchSource { + /// Read changes from local git refs. + Local, + /// Fetch PR files through the GitHub API. + Github, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct GhPrView { + base_ref_name: Option, + head_ref_name: Option, + base_ref_oid: Option, + head_ref_oid: Option, +} + +pub fn analyze_explicit_range( + repo: &Path, + base: Option<&str>, + head: Option<&str>, +) -> Result, String> { + let Some(base) = base else { + if head.is_some() { + return Err("--head requires --base".to_string()); + } + return Ok(None); + }; + + let Some(head) = head else { + return Err("--base requires --head".to_string()); + }; + + analyze( + repo, + DiffScope::Range { + from: base.to_string(), + to: head.to_string(), + }, + ) + .map(Some) + .map_err(|e| e.to_string()) +} + +pub fn analyze_local_pr(repo: &Path, number: u64) -> Result { + let pr = load_gh_pr_view(repo, number)?; + let base = pr + .base_ref_oid + .as_deref() + .or(pr.base_ref_name.as_deref()) + .unwrap_or("main"); + let head = pr + .head_ref_oid + .as_deref() + .or(pr.head_ref_name.as_deref()) + .unwrap_or("HEAD"); + + ensure_local_pr_commits(repo, number, &pr, base, head)?; + + analyze( + repo, + DiffScope::Range { + from: base.to_string(), + to: head.to_string(), + }, + ) + .map_err(|e| { + format!( + "{}\nTry `inspect pr {} --fetch github --remote owner/repo` to fetch PR files through the GitHub API.", + e, number + ) + }) +} + +pub async fn analyze_github_pr(remote_repo: &str, number: u64) -> Result { + let client = GitHubClient::new().map_err(|e| e.to_string())?; + + eprintln!( + "Fetching PR #{} from {} via GitHub API...", + number, remote_repo + ); + + let pr = client + .get_pr(remote_repo, number) + .await + .map_err(|e| e.to_string())?; + + let visible_files: Vec<_> = pr + .files + .iter() + .filter(|f| !is_noise_file(&f.filename)) + .cloned() + .collect(); + + let noise_count = pr.files.len() - visible_files.len(); + if noise_count > 0 { + eprintln!("({} noise files hidden)", noise_count); + } + + eprintln!("Fetching {} file contents...", visible_files.len()); + + // Use commit SHAs instead of branch names so fork PRs work even when the + // head branch does not exist on the base repository. + let file_pairs = client + .get_file_pairs(remote_repo, &visible_files, &pr.base_sha, &pr.head_sha) + .await; + + analyze_remote(&file_pairs).map_err(|e| e.to_string()) +} + +fn load_gh_pr_view(repo: &Path, number: u64) -> Result { + let output = Command::new("gh") + .args([ + "pr", + "view", + &number.to_string(), + "--json", + "baseRefName,headRefName,baseRefOid,headRefOid", + ]) + .current_dir(repo) + .output() + .map_err(|e| format!("could not run gh CLI: {e}"))?; + + if !output.status.success() { + return Err(format!( + "gh pr view failed: {}", + String::from_utf8_lossy(&output.stderr) + )); + } + + serde_json::from_slice(&output.stdout).map_err(|e| format!("invalid gh output: {e}")) +} + +fn ensure_local_pr_commits( + repo: &Path, + number: u64, + pr: &GhPrView, + base: &str, + head: &str, +) -> Result<(), String> { + if commit_exists(repo, base) && commit_exists(repo, head) { + return Ok(()); + } + + let mut fetch_errors = Vec::new(); + + if !commit_exists(repo, base) { + if let Some(base_ref) = pr.base_ref_name.as_deref() { + if let Err(e) = fetch_ref(repo, base_ref) { + fetch_errors.push(e); + } + } + } + + let pr_fetch_ref = pr_head_refspec(number); + if !commit_exists(repo, head) { + if let Err(e) = fetch_ref(repo, &pr_fetch_ref) { + fetch_errors.push(e); + } + } + + if commit_exists(repo, base) && commit_exists(repo, head) { + return Ok(()); + } + + let mut commands = Vec::new(); + if !commit_exists(repo, base) { + if let Some(base_ref) = pr.base_ref_name.as_deref() { + commands.push(format!("git fetch origin {base_ref}")); + } + } + if !commit_exists(repo, head) { + commands.push(format!("git fetch origin {pr_fetch_ref}")); + } + + Err(missing_commits_error(number, &commands, &fetch_errors)) +} + +fn pr_head_refspec(number: u64) -> String { + format!("+pull/{number}/head:refs/remotes/inspect/pr-{number}") +} + +fn missing_commits_error(number: u64, commands: &[String], fetch_errors: &[String]) -> String { + let command_hint = if commands.is_empty() { + "fetch the missing PR commits locally".to_string() + } else { + commands.join("\n ") + }; + + let fetch_error_hint = if fetch_errors.is_empty() { + String::new() + } else { + format!("\nFetch attempts failed:\n {}", fetch_errors.join("\n ")) + }; + + format!( + "PR #{number} commits are not available locally.\nRun:\n {command_hint}{fetch_error_hint}\nOr use `--fetch github --remote owner/repo` to fetch file contents through GitHub." + ) +} + +fn commit_exists(repo: &Path, rev: &str) -> bool { + Command::new("git") + .args(["cat-file", "-e", &format!("{rev}^{{commit}}")]) + .current_dir(repo) + .status() + .map(|status| status.success()) + .unwrap_or(false) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pr_head_refspec_forces_updates() { + assert_eq!( + pr_head_refspec(42), + "+pull/42/head:refs/remotes/inspect/pr-42" + ); + } + + #[test] + fn missing_commits_error_includes_fetch_failures() { + let message = missing_commits_error( + 42, + &["git fetch origin +pull/42/head:refs/remotes/inspect/pr-42".to_string()], + &["git fetch origin failed: permission denied".to_string()], + ); + + assert!(message.contains("git fetch origin +pull/42/head:refs/remotes/inspect/pr-42")); + assert!(message.contains("Fetch attempts failed")); + assert!(message.contains("permission denied")); + assert!(message.contains("--fetch github --remote owner/repo")); + } +} + +fn fetch_ref(repo: &Path, refspec: &str) -> Result<(), String> { + let output = Command::new("git") + .args(["fetch", "origin", refspec]) + .current_dir(repo) + .output() + .map_err(|e| format!("could not run git fetch: {e}"))?; + + if output.status.success() { + Ok(()) + } else { + Err(format!( + "git fetch origin {refspec} failed: {}", + String::from_utf8_lossy(&output.stderr) + )) + } +} diff --git a/crates/inspect-cli/src/commands/review.rs b/crates/inspect-cli/src/commands/review.rs index e81545a..9a0435f 100644 --- a/crates/inspect-cli/src/commands/review.rs +++ b/crates/inspect-cli/src/commands/review.rs @@ -1,21 +1,37 @@ use std::io::{self, Write}; use std::path::PathBuf; -use clap::Args; +use clap::{Args, ValueEnum}; use colored::Colorize; use sem_core::git::types::DiffScope; +use crate::commands::pr_source::{analyze_github_pr, FetchSource}; use crate::OutputFormat; use inspect_core::analyze::analyze; use inspect_core::llm::{ estimate_entity_input_tokens, AnthropicClient, EntityLlmReview, LlmProvider, LlmReviewOptions, LlmReviewStatus, LlmVerdict, OpenAIClient, }; -use inspect_core::types::RiskLevel; +use inspect_core::types::{ReviewResult, RiskLevel}; + +#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)] +pub enum ReviewEngine { + /// Run the review locally using the selected LLM provider. + Local, + /// Submit the review to the hosted inspect API. + Hosted, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ReviewMode { + LocalDiff, + GithubPr, + Hosted, +} #[derive(Args)] pub struct ReviewArgs { - /// Commit ref, range, or PR number (with --remote) + /// Commit ref, range, or PR number (with --fetch github or --engine hosted) pub target: String, /// Output format @@ -50,7 +66,15 @@ pub struct ReviewArgs { #[arg(long)] pub api_key: Option, - /// Remote repo (e.g. owner/repo). Target becomes PR number. + /// PR content source for local review + #[arg(long, value_enum, default_value = "local")] + pub fetch: FetchSource, + + /// Review engine + #[arg(long, value_enum, default_value = "local")] + pub engine: ReviewEngine, + + /// Remote repo (owner/repo), required with --fetch github or --engine hosted. #[arg(long)] pub remote: Option, @@ -118,14 +142,44 @@ fn build_provider(args: &ReviewArgs) -> Result, String> { } pub async fn run(args: ReviewArgs) { - if args.remote.is_some() { - return run_remote(args).await; + let mode = match validate_review_mode( + args.engine, + args.fetch, + args.remote.is_some(), + args.strategy.is_some(), + ) { + Ok(mode) => mode, + Err(e) => { + eprintln!("{}", format!("error: {e}").red()); + std::process::exit(1); + } + }; + + if mode == ReviewMode::Hosted { + return run_hosted(args).await; } - let scope = parse_scope(&args.target); - let repo = args.repo.canonicalize().unwrap_or(args.repo.clone()); + let result = match mode { + ReviewMode::LocalDiff => { + let scope = parse_scope(&args.target); + let repo = args.repo.canonicalize().unwrap_or(args.repo.clone()); + analyze(&repo, scope).map_err(|e| e.to_string()) + } + ReviewMode::GithubPr => { + let Some(remote_repo) = args.remote.as_deref() else { + eprintln!( + "{}", + "error: --fetch github requires --remote owner/repo".red() + ); + std::process::exit(1); + }; + let pr_number = parse_pr_number(&args.target, "--fetch github"); + analyze_github_pr(remote_repo, pr_number).await + } + ReviewMode::Hosted => unreachable!("hosted mode returned before local review"), + }; - let mut result = match analyze(&repo, scope) { + let result = match result { Ok(r) => r, Err(e) => { eprintln!("error: {}", e); @@ -133,6 +187,52 @@ pub async fn run(args: ReviewArgs) { } }; + run_local_review(&args, result).await; +} + +fn validate_review_mode( + engine: ReviewEngine, + fetch: FetchSource, + has_remote: bool, + has_strategy: bool, +) -> Result { + if engine == ReviewEngine::Hosted { + if fetch != FetchSource::Local { + return Err( + "hosted reviews fetch PR content through the inspect API; omit --fetch github" + .to_string(), + ); + } + if !has_remote { + return Err("--engine hosted requires --remote owner/repo".to_string()); + } + return Ok(ReviewMode::Hosted); + } + + if has_remote && fetch == FetchSource::Local { + return Err( + "--remote only names a GitHub repository; choose --fetch github for local review or --engine hosted" + .to_string(), + ); + } + + if has_strategy { + return Err("--strategy is only supported with --engine hosted".to_string()); + } + + match fetch { + FetchSource::Local => Ok(ReviewMode::LocalDiff), + FetchSource::Github => { + if has_remote { + Ok(ReviewMode::GithubPr) + } else { + Err("--fetch github requires --remote owner/repo".to_string()) + } + } + } +} + +async fn run_local_review(args: &ReviewArgs, mut result: ReviewResult) { let total_entities = result.entity_reviews.len(); let min_level = parse_risk_level(&args.min_risk); @@ -431,18 +531,15 @@ fn summarize_reviews(reviews: &[EntityLlmReview]) -> ReviewSummary { summary } -async fn run_remote(args: ReviewArgs) { - let remote = args.remote.as_ref().unwrap(); - let pr_number: u64 = match args.target.parse() { - Ok(n) => n, - Err(_) => { - eprintln!( - "{}", - "error: target must be a PR number when using --remote".red() - ); - std::process::exit(1); - } +async fn run_hosted(args: ReviewArgs) { + let Some(remote) = args.remote.as_ref() else { + eprintln!( + "{}", + "error: --engine hosted requires --remote owner/repo".red() + ); + std::process::exit(1); }; + let pr_number = parse_pr_number(&args.target, "--engine hosted"); let creds = match crate::config::require_credentials() { Ok(c) => c, @@ -613,6 +710,19 @@ fn parse_scope(target: &str) -> DiffScope { } } +fn parse_pr_number(target: &str, mode: &str) -> u64 { + match target.parse() { + Ok(n) => n, + Err(_) => { + eprintln!( + "{}", + format!("error: target must be a PR number when using {mode}").red() + ); + std::process::exit(1); + } + } +} + fn parse_risk_level(s: &str) -> RiskLevel { match s.to_lowercase().as_str() { "critical" => RiskLevel::Critical, @@ -673,4 +783,45 @@ mod tests { assert_eq!(format_markdown_status(&skipped), "Skipped"); assert_eq!(format_markdown_status(&reviewed), "Changes Requested"); } + + #[test] + fn github_fetch_with_remote_runs_local_github_pr_review() { + let mode = + validate_review_mode(ReviewEngine::Local, FetchSource::Github, true, false).unwrap(); + + assert_eq!(mode, ReviewMode::GithubPr); + } + + #[test] + fn hosted_engine_requires_remote() { + let error = validate_review_mode(ReviewEngine::Hosted, FetchSource::Local, false, false) + .unwrap_err(); + + assert!(error.contains("--engine hosted requires --remote")); + } + + #[test] + fn remote_without_fetch_or_engine_is_rejected() { + let error = + validate_review_mode(ReviewEngine::Local, FetchSource::Local, true, false).unwrap_err(); + + assert!(error.contains("choose --fetch github")); + assert!(error.contains("--engine hosted")); + } + + #[test] + fn hosted_engine_rejects_github_fetch() { + let error = validate_review_mode(ReviewEngine::Hosted, FetchSource::Github, true, false) + .unwrap_err(); + + assert!(error.contains("omit --fetch github")); + } + + #[test] + fn local_review_rejects_hosted_strategy() { + let error = + validate_review_mode(ReviewEngine::Local, FetchSource::Local, false, true).unwrap_err(); + + assert!(error.contains("--strategy is only supported")); + } } diff --git a/docs/src/app/docs/page.tsx b/docs/src/app/docs/page.tsx index 9adfea0..f9f8dde 100644 --- a/docs/src/app/docs/page.tsx +++ b/docs/src/app/docs/page.tsx @@ -99,6 +99,20 @@ export default function DocsPage() { Review all changes in a GitHub pull request +
+
+ --fetch <source>{" "} + local (default) or github +
+
+ --remote <owner/repo>{" "} + Required with --fetch github +
+
+ --base <ref> --head <ref>{" "} + Compare explicit refs instead of PR refs +
+
@@ -150,6 +164,18 @@ export default function DocsPage() { --api-key <key>{" "} API key (overrides env var)
+
+ --fetch <source>{" "} + local (default) or github. Use github to review a PR locally from the GitHub API. +
+
+ --engine <engine>{" "} + local (default) or hosted. Hosted requires inspect login. +
+
+ --remote <owner/repo>{" "} + Required with --fetch github or --engine hosted +
--min-risk <level>{" "} diff --git a/site/src/app/docs/page.tsx b/site/src/app/docs/page.tsx index d8cb3ab..1006ed3 100644 --- a/site/src/app/docs/page.tsx +++ b/site/src/app/docs/page.tsx @@ -80,6 +80,11 @@ export default function DocsPage() { inspect pr <number> Review all changes in a GitHub pull request
+
+
--fetch <source> local (default) or github
+
--remote <owner/repo> Required with --fetch github
+
--base <ref> --head <ref> Compare explicit refs instead of PR refs
+
@@ -106,6 +111,9 @@ export default function DocsPage() {
--model <model> Model name (e.g. claude-sonnet-4-5-20250929, gpt-4o, llama3)
--api-base <url> Custom endpoint URL. Automatically uses the OpenAI-compatible client.
--api-key <key> API key (overrides env var)
+
--fetch <source> local (default) or github. Use github to review a PR locally from the GitHub API.
+
--engine <engine> local (default) or hosted. Hosted requires inspect login.
+
--remote <owner/repo> Required with --fetch github or --engine hosted
--min-risk <level> Minimum risk to review (default: high)
--max-entities <n> Cap on entities sent to LLM (default: 10)