From e6896ac1429c76ee4911a0dd987470fd2cf27d92 Mon Sep 17 00:00:00 2001 From: thedoublejay Date: Wed, 13 May 2026 12:00:44 +0800 Subject: [PATCH 1/5] feat: add pr-review PR-set manifest schema --- crates/gather-step-cli/src/pr_review/mod.rs | 1 + .../src/pr_review/multi_pr/manifest.rs | 393 ++++++++++++++++++ .../src/pr_review/multi_pr/mod.rs | 6 + examples/pr-set/divergent-bases.yaml | 17 + examples/pr-set/reg-13863.yaml | 25 ++ examples/pr-set/stacked-in-one-repo.yaml | 17 + 6 files changed, 459 insertions(+) create mode 100644 crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs create mode 100644 crates/gather-step-cli/src/pr_review/multi_pr/mod.rs create mode 100644 examples/pr-set/divergent-bases.yaml create mode 100644 examples/pr-set/reg-13863.yaml create mode 100644 examples/pr-set/stacked-in-one-repo.yaml diff --git a/crates/gather-step-cli/src/pr_review/mod.rs b/crates/gather-step-cli/src/pr_review/mod.rs index c623548e..7486b552 100644 --- a/crates/gather-step-cli/src/pr_review/mod.rs +++ b/crates/gather-step-cli/src/pr_review/mod.rs @@ -12,6 +12,7 @@ pub mod delta_report; pub mod engine; pub mod extract; pub mod index_runner; +pub mod multi_pr; pub mod overlay; pub mod parity; #[cfg(test)] diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs b/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs new file mode 100644 index 00000000..7871e5f5 --- /dev/null +++ b/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs @@ -0,0 +1,393 @@ +//! PR-set manifest schema for multi-PR `pr-review`. +//! +//! The manifest is intentionally small: it names a review set and lists each +//! repo/ref pair that belongs to it. Later phases use this validated contract to +//! coordinate per-PR review runs and cross-PR analysis. + +use std::{ + collections::{BTreeMap, BTreeSet}, + path::{Path, PathBuf}, +}; + +use serde::{Deserialize, Serialize}; + +pub const PR_SET_MANIFEST_VERSION: u32 = 0; + +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub struct PrSetManifest { + pub version: u32, + pub id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub title: Option, + pub prs: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub struct PrEntry { + pub id: String, + pub repo: String, + pub base: String, + pub head: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pr: Option, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub depends_on: Vec, +} + +#[derive(Debug, thiserror::Error)] +pub enum ManifestError { + #[error("Failed to read PR-set manifest `{}`: {source}", path.display())] + Read { + path: PathBuf, + #[source] + source: std::io::Error, + }, + #[error("Failed to parse PR-set manifest YAML: {source}")] + Parse { + #[source] + source: serde_norway::Error, + }, + #[error("Unsupported PR-set manifest version {found}; expected {expected}.")] + UnsupportedVersion { found: u32, expected: u32 }, + #[error("PR-set manifest id must not be empty.")] + EmptyManifestId, + #[error("PR-set manifest must contain at least one PR entry.")] + EmptyEntries, + #[error("PR entry at index {index} has an empty `{field}` field.")] + EmptyEntryField { index: usize, field: &'static str }, + #[error("Duplicate PR entry id `{id}`.")] + DuplicateEntryId { id: String }, + #[error("Duplicate PR entry repo/head pair `{repo}` / `{head}`.")] + DuplicateRepoHead { repo: String, head: String }, + #[error("Duplicate PR entry repo/pr pair `{repo}` / #{pr}.")] + DuplicateRepoPr { repo: String, pr: u64 }, + #[error("PR entry `{id}` depends on unknown entry `{depends_on}`.")] + UnknownDependency { id: String, depends_on: String }, + #[error("PR entry `{id}` depends on itself.")] + SelfDependency { id: String }, + #[error("PR-set manifest contains a dependency cycle involving `{id}`.")] + DependencyCycle { id: String }, +} + +impl PrSetManifest { + pub fn from_path(path: impl AsRef) -> Result { + let path = path.as_ref(); + let raw = std::fs::read_to_string(path).map_err(|source| ManifestError::Read { + path: path.to_path_buf(), + source, + })?; + Self::from_yaml_str(&raw) + } + + pub fn from_yaml_str(raw: &str) -> Result { + let manifest: Self = + serde_norway::from_str(raw).map_err(|source| ManifestError::Parse { source })?; + manifest.validate()?; + Ok(manifest) + } + + pub fn to_yaml_string(&self) -> Result { + serde_norway::to_string(self).map_err(|source| ManifestError::Parse { source }) + } + + pub fn validate(&self) -> Result<(), ManifestError> { + if self.version != PR_SET_MANIFEST_VERSION { + return Err(ManifestError::UnsupportedVersion { + found: self.version, + expected: PR_SET_MANIFEST_VERSION, + }); + } + if self.id.trim().is_empty() { + return Err(ManifestError::EmptyManifestId); + } + if self.prs.is_empty() { + return Err(ManifestError::EmptyEntries); + } + + let mut ids = BTreeSet::new(); + let mut repo_heads = BTreeSet::new(); + let mut repo_prs = BTreeSet::new(); + + for (index, entry) in self.prs.iter().enumerate() { + validate_required_entry_field(index, "id", &entry.id)?; + validate_required_entry_field(index, "repo", &entry.repo)?; + validate_required_entry_field(index, "base", &entry.base)?; + validate_required_entry_field(index, "head", &entry.head)?; + + if !ids.insert(entry.id.clone()) { + return Err(ManifestError::DuplicateEntryId { + id: entry.id.clone(), + }); + } + + let repo_head = (entry.repo.clone(), entry.head.clone()); + if !repo_heads.insert(repo_head) { + return Err(ManifestError::DuplicateRepoHead { + repo: entry.repo.clone(), + head: entry.head.clone(), + }); + } + + if let Some(pr) = entry.pr { + let repo_pr = (entry.repo.clone(), pr); + if !repo_prs.insert(repo_pr) { + return Err(ManifestError::DuplicateRepoPr { + repo: entry.repo.clone(), + pr, + }); + } + } + } + + self.validate_dependencies() + } + + fn validate_dependencies(&self) -> Result<(), ManifestError> { + let entries: BTreeMap<&str, &PrEntry> = self + .prs + .iter() + .map(|entry| (entry.id.as_str(), entry)) + .collect(); + + for entry in &self.prs { + for depends_on in &entry.depends_on { + if depends_on.trim().is_empty() { + return Err(ManifestError::UnknownDependency { + id: entry.id.clone(), + depends_on: depends_on.clone(), + }); + } + if depends_on == &entry.id { + return Err(ManifestError::SelfDependency { + id: entry.id.clone(), + }); + } + if !entries.contains_key(depends_on.as_str()) { + return Err(ManifestError::UnknownDependency { + id: entry.id.clone(), + depends_on: depends_on.clone(), + }); + } + } + } + + let mut visiting = BTreeSet::new(); + let mut visited = BTreeSet::new(); + for entry in &self.prs { + detect_dependency_cycle(entry.id.as_str(), &entries, &mut visiting, &mut visited)?; + } + + Ok(()) + } +} + +fn validate_required_entry_field( + index: usize, + field: &'static str, + value: &str, +) -> Result<(), ManifestError> { + if value.trim().is_empty() { + return Err(ManifestError::EmptyEntryField { index, field }); + } + Ok(()) +} + +fn detect_dependency_cycle<'a>( + id: &'a str, + entries: &BTreeMap<&'a str, &'a PrEntry>, + visiting: &mut BTreeSet<&'a str>, + visited: &mut BTreeSet<&'a str>, +) -> Result<(), ManifestError> { + if visited.contains(id) { + return Ok(()); + } + if !visiting.insert(id) { + return Err(ManifestError::DependencyCycle { id: id.to_owned() }); + } + + let Some(entry) = entries.get(id) else { + return Ok(()); + }; + + for depends_on in &entry.depends_on { + detect_dependency_cycle(depends_on, entries, visiting, visited)?; + } + + visiting.remove(id); + visited.insert(id); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::{ManifestError, PR_SET_MANIFEST_VERSION, PrSetManifest}; + + const REG_13863_EXAMPLE: &str = include_str!("../../../../../examples/pr-set/reg-13863.yaml"); + const STACKED_EXAMPLE: &str = + include_str!("../../../../../examples/pr-set/stacked-in-one-repo.yaml"); + const DIVERGENT_BASES_EXAMPLE: &str = + include_str!("../../../../../examples/pr-set/divergent-bases.yaml"); + + #[test] + fn pr_set_manifest_parses_reg_13863_example() { + let manifest = PrSetManifest::from_yaml_str(REG_13863_EXAMPLE) + .expect("REG-13863 example should parse"); + + assert_eq!(manifest.version, PR_SET_MANIFEST_VERSION); + assert_eq!(manifest.id, "REG-13863"); + assert_eq!(manifest.prs.len(), 3); + assert!( + manifest + .prs + .iter() + .any(|entry| entry.id == "label-review-502" + && entry.repo == "label-review" + && entry.pr == Some(502)) + ); + + let round_tripped = manifest + .to_yaml_string() + .expect("manifest should serialize to YAML"); + let reparsed = PrSetManifest::from_yaml_str(&round_tripped) + .expect("serialized manifest should parse again"); + assert_eq!(reparsed, manifest); + } + + #[test] + fn pr_set_manifest_examples_validate_cleanly() { + for raw in [REG_13863_EXAMPLE, STACKED_EXAMPLE, DIVERGENT_BASES_EXAMPLE] { + PrSetManifest::from_yaml_str(raw).expect("example manifest should validate cleanly"); + } + } + + #[test] + fn manifest_parse_rejects_duplicate_entry_ids() { + let err = PrSetManifest::from_yaml_str( + " +version: 0 +id: duplicate-ids +prs: + - id: duplicate + repo: api + base: main + head: feature/a + - id: duplicate + repo: web + base: main + head: feature/b +", + ) + .expect_err("duplicate ids should be rejected"); + + assert!(matches!(err, ManifestError::DuplicateEntryId { .. })); + } + + #[test] + fn manifest_parse_rejects_duplicate_repo_head_pairs() { + let err = PrSetManifest::from_yaml_str( + " +version: 0 +id: duplicate-repo-head +prs: + - id: api-1 + repo: api + base: main + head: feature/a + - id: api-2 + repo: api + base: release + head: feature/a +", + ) + .expect_err("duplicate repo/head pairs should be rejected"); + + assert!(matches!(err, ManifestError::DuplicateRepoHead { .. })); + } + + #[test] + fn manifest_parse_rejects_duplicate_repo_pr_pairs() { + let err = PrSetManifest::from_yaml_str( + " +version: 0 +id: duplicate-repo-pr +prs: + - id: api-1 + repo: api + base: main + head: feature/a + pr: 42 + - id: api-2 + repo: api + base: main + head: feature/b + pr: 42 +", + ) + .expect_err("duplicate repo/pr pairs should be rejected"); + + assert!(matches!(err, ManifestError::DuplicateRepoPr { .. })); + } + + #[test] + fn manifest_parse_rejects_dependency_cycle() { + let err = PrSetManifest::from_yaml_str( + " +version: 0 +id: cycle +prs: + - id: api + repo: api + base: main + head: feature/a + depends_on: [web] + - id: web + repo: web + base: main + head: feature/b + depends_on: [api] +", + ) + .expect_err("dependency cycles should be rejected"); + + assert!(matches!(err, ManifestError::DependencyCycle { .. })); + } + + #[test] + fn manifest_parse_rejects_unknown_dependency() { + let err = PrSetManifest::from_yaml_str( + " +version: 0 +id: unknown-dependency +prs: + - id: api + repo: api + base: main + head: feature/a + depends_on: [web] +", + ) + .expect_err("unknown dependencies should be rejected"); + + assert!(matches!(err, ManifestError::UnknownDependency { .. })); + } + + #[test] + fn manifest_parse_rejects_unknown_version() { + let err = PrSetManifest::from_yaml_str( + " +version: 1 +id: unknown-version +prs: + - id: api + repo: api + base: main + head: feature/a +", + ) + .expect_err("unknown manifest versions should be rejected"); + + assert!(matches!(err, ManifestError::UnsupportedVersion { .. })); + } +} diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs b/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs new file mode 100644 index 00000000..1a22b34d --- /dev/null +++ b/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs @@ -0,0 +1,6 @@ +//! Multi-PR `pr-review` support. +//! +//! This module owns the PR-set manifest contract first. Later phases add cache, +//! runner, cross-PR analysis, and output rendering on top of this schema. + +pub mod manifest; diff --git a/examples/pr-set/divergent-bases.yaml b/examples/pr-set/divergent-bases.yaml new file mode 100644 index 00000000..6f37d258 --- /dev/null +++ b/examples/pr-set/divergent-bases.yaml @@ -0,0 +1,17 @@ +version: 0 +id: divergent-bases +title: Divergent base branches +prs: + - id: backend-contract + repo: label-review + base: main + head: feature/backend-contract + pr: 1201 + depends_on: [] + - id: frontend-release + repo: web-frontend + base: release/2026.05 + head: feature/frontend-release-hookup + pr: 1202 + depends_on: + - backend-contract diff --git a/examples/pr-set/reg-13863.yaml b/examples/pr-set/reg-13863.yaml new file mode 100644 index 00000000..15ac7825 --- /dev/null +++ b/examples/pr-set/reg-13863.yaml @@ -0,0 +1,25 @@ +version: 0 +id: REG-13863 +title: Edit Label Project cross-stack +prs: + - id: label-review-502 + repo: label-review + base: main + head: REG-13863-edit-label-project-details + pr: 502 + depends_on: [] + - id: web-api-gateway-1090 + repo: web-api-gateway + base: main + head: REG-13863-edit-label-project-details + pr: 1090 + depends_on: + - label-review-502 + - id: web-frontend-8035 + repo: web-frontend + base: main + head: REG-13863-edit-label-project-details + pr: 8035 + depends_on: + - label-review-502 + - web-api-gateway-1090 diff --git a/examples/pr-set/stacked-in-one-repo.yaml b/examples/pr-set/stacked-in-one-repo.yaml new file mode 100644 index 00000000..b560d2f1 --- /dev/null +++ b/examples/pr-set/stacked-in-one-repo.yaml @@ -0,0 +1,17 @@ +version: 0 +id: stacked-in-one-repo +title: Stacked API cleanup +prs: + - id: api-foundation + repo: web-api-gateway + base: main + head: feature/api-foundation + pr: 1101 + depends_on: [] + - id: api-ui-hookup + repo: web-api-gateway + base: feature/api-foundation + head: feature/api-ui-hookup + pr: 1102 + depends_on: + - api-foundation From 33fd4d7d5cfdaa44c23bf4e946b7b4437ae07ecd Mon Sep 17 00:00:00 2001 From: thedoublejay Date: Wed, 13 May 2026 13:50:16 +0800 Subject: [PATCH 2/5] feat: add multi-pr review sets --- .../gather-step-cli/src/commands/pr_review.rs | 410 ++++++++++- .../src/pr_review/index_runner.rs | 12 +- .../src/pr_review/multi_pr/cache_key.rs | 139 ++++ .../src/pr_review/multi_pr/coordinator.rs | 383 +++++++++++ .../src/pr_review/multi_pr/delta_report.rs | 649 ++++++++++++++++++ .../src/pr_review/multi_pr/gh.rs | 323 +++++++++ .../src/pr_review/multi_pr/manifest.rs | 23 +- .../src/pr_review/multi_pr/mod.rs | 8 +- crates/gather-step-mcp/src/catalog.rs | 4 + crates/gather-step-mcp/src/server.rs | 25 +- crates/gather-step-mcp/src/tools/pr_review.rs | 129 +++- examples/pr-set/cross-repo-feature.yaml | 25 + examples/pr-set/divergent-bases.yaml | 4 +- examples/pr-set/reg-13863.yaml | 25 - examples/pr-set/stacked-in-one-repo.yaml | 4 +- website/src/content/docs/guides/pr-review.md | 78 +++ website/src/content/docs/reference/cli.md | 38 +- .../src/content/docs/reference/mcp-tools.md | 27 + 18 files changed, 2240 insertions(+), 66 deletions(-) create mode 100644 crates/gather-step-cli/src/pr_review/multi_pr/cache_key.rs create mode 100644 crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs create mode 100644 crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs create mode 100644 crates/gather-step-cli/src/pr_review/multi_pr/gh.rs create mode 100644 examples/pr-set/cross-repo-feature.yaml delete mode 100644 examples/pr-set/reg-13863.yaml diff --git a/crates/gather-step-cli/src/commands/pr_review.rs b/crates/gather-step-cli/src/commands/pr_review.rs index f76ee835..d9192066 100644 --- a/crates/gather-step-cli/src/commands/pr_review.rs +++ b/crates/gather-step-cli/src/commands/pr_review.rs @@ -7,6 +7,8 @@ //! # Sub-commands //! //! - `pr-review` (no subcommand): run a review. Requires `--base` and `--head`. +//! - `pr-review --pr-set `: run a coordinated review set. +//! - `pr-review init-set`: generate a PR-set manifest from GitHub search. //! - `pr-review clean ...`: clean up stale review artifacts (Phase 1 Task 6). use std::{ @@ -25,7 +27,7 @@ use gather_step_storage::IndexingOptions; use serde::Serialize; use crate::{ - app::AppContext, + app::{AppContext, WorkspacePaths}, pr_review::{ affected::{AffectedRepos, compute_affected_repos}, artifact_root::{ @@ -56,6 +58,11 @@ use crate::{ routes::{extract_route_deltas, find_route_node_id}, symbols::{extract_symbol_deltas, find_symbol_node_id}, }, + multi_pr::{ + coordinator::{PrSetRunArgs, run_pr_set}, + gh::{GhResolveOptions, resolve_pr_set_from_gh, slugify_set_id}, + manifest::PrSetManifest, + }, }, storage_context::StorageContext, }; @@ -75,14 +82,35 @@ pub struct PrReviewArgs { /// Base ref (branch, tag, SHA, or any git rev). /// Required when no subcommand is given (i.e., when running a review). - #[arg(long, value_name = "REF")] + #[arg(long, value_name = "REF", conflicts_with = "pr_set")] pub base: Option, /// Head ref (branch, tag, SHA, "HEAD", …). /// Required when no subcommand is given (i.e., when running a review). - #[arg(long, value_name = "REF")] + #[arg(long, value_name = "REF", conflicts_with = "pr_set")] pub head: Option, + /// Path to a PR-set manifest for coordinated multi-PR review. + #[arg(long = "pr-set", value_name = "PATH", conflicts_with_all = ["base", "head", "from_gh"])] + pub pr_set: Option, + + /// Resolve a PR-set manifest from GitHub search results and run it. + #[arg(long = "from-gh", value_name = "QUERY", conflicts_with_all = ["base", "head", "pr_set"])] + pub from_gh: Option, + + /// Override the manifest set id in the emitted PR-set report. + #[arg(long, value_name = "ID")] + pub set_id: Option, + + /// Number of independent PR-set entries to review in parallel. + #[arg(long, value_name = "N", default_value_t = 1)] + pub parallelism: usize, + + /// Include PRs whose GitHub repo is not listed in the workspace config + /// when resolving `--from-gh`. + #[arg(long, requires = "from_gh")] + pub allow_unknown_repos: bool, + /// Engine to use for the review. Only `temp-index` is supported in this MVP. #[arg(long, value_enum, default_value_t = ReviewEngine::TempIndex)] pub engine: ReviewEngine, @@ -102,8 +130,10 @@ pub struct PrReviewArgs { /// The review temp-index requires a config at the worktree root. By /// default the worktree is checked out at `--head`, so a config that /// is committed in that ref is naturally present. Pass this flag when - /// the workspace does not have a checked-in config (e.g. during - /// bootstrap), or to override the committed one for a single run. + /// the reviewed git repo does not commit its own config, or to override + /// the committed one for a run. When `--workspace` points at a child repo + /// and `--config` points at the parent workspace config, the matching repo + /// entry is rewritten to `path: "."` inside the temporary worktree. #[arg(long, value_name = "PATH")] pub config: Option, @@ -145,6 +175,27 @@ pub struct PrReviewArgs { pub enum PrReviewSubcommand { /// Clean up stale review artifact roots for this workspace. Clean(CleanArgs), + /// Generate a PR-set manifest from GitHub search results. + InitSet(InitSetArgs), +} + +#[derive(Args, Debug, Clone)] +pub struct InitSetArgs { + /// GitHub PR search query used with `gh pr list --search`. + #[arg(long, value_name = "QUERY")] + pub query: String, + + /// Output manifest path. Defaults to `-pr-set.yaml`. + #[arg(long, value_name = "PATH")] + pub output: Option, + + /// Override the generated manifest id. + #[arg(long, value_name = "ID")] + pub set_id: Option, + + /// Include PRs whose GitHub repo is not listed in the workspace config. + #[arg(long)] + pub allow_unknown_repos: bool, } #[derive(Args, Debug, Clone)] @@ -285,7 +336,100 @@ pub fn run(app: &AppContext, args: PrReviewArgs) -> Result { Some(PrReviewSubcommand::Clean(ref clean_args)) => { run_clean(app, &args, clean_args).map(|()| 0) } + Some(PrReviewSubcommand::InitSet(ref init_args)) => { + run_init_set(app, &args, init_args).map(|()| 0) + } None => { + if args.parallelism == 0 { + anyhow::bail!("--parallelism must be at least 1."); + } + if let Some(pr_set) = args.pr_set.as_ref() { + let set_args = PrSetRunArgs { + manifest_path: pr_set.clone(), + set_id: args.set_id.clone(), + parallelism: args.parallelism, + engine: args.engine, + keep_cache: args.keep_cache, + cache_root: args.cache_root.clone(), + config: args.config.clone(), + severity: args.severity, + format: if app.json_output { + OutputFormat::Json + } else { + args.format + }, + github_comment_file: args.github_comment_file.clone(), + no_baseline_check: args.no_baseline_check, + }; + let (report, exceeded) = run_pr_set(app, &set_args)?; + #[expect( + clippy::print_stdout, + reason = "pr-review is the sole caller of this path; structured output goes here" + )] + { + println!("{report}"); + } + let _ = ::flush(&mut std::io::stdout()); + return Ok(if exceeded { 2 } else { 0 }); + } + + if let Some(query) = args.from_gh.as_ref() { + let config_path = args + .config + .clone() + .unwrap_or_else(|| app.workspace_paths().config_path); + let manifest = resolve_pr_set_from_gh( + &app.workspace_path, + &config_path, + &GhResolveOptions { + query: query.clone(), + set_id: args.set_id.clone(), + allow_unknown_repos: args.allow_unknown_repos, + }, + ) + .with_context(|| "Resolving PR-set manifest from GitHub.")?; + let manifest_path = + write_generated_pr_set_manifest(app, &args, &manifest, "from-gh")?; + tracing::info!( + path = %manifest_path.display(), + "Wrote GitHub-resolved PR-set manifest." + ); + let set_args = PrSetRunArgs { + manifest_path, + set_id: args.set_id.clone(), + parallelism: args.parallelism, + engine: args.engine, + keep_cache: args.keep_cache, + cache_root: args.cache_root.clone(), + config: args.config.clone(), + severity: args.severity, + format: if app.json_output { + OutputFormat::Json + } else { + args.format + }, + github_comment_file: args.github_comment_file.clone(), + no_baseline_check: args.no_baseline_check, + }; + let (report, exceeded) = run_pr_set(app, &set_args)?; + #[expect( + clippy::print_stdout, + reason = "pr-review is the sole caller of this path; structured output goes here" + )] + { + println!("{report}"); + } + let _ = ::flush(&mut std::io::stdout()); + return Ok(if exceeded { 2 } else { 0 }); + } + + if args.set_id.is_some() { + anyhow::bail!("--set-id requires --pr-set or --from-gh."); + } + if args.parallelism != 1 { + anyhow::bail!("--parallelism requires --pr-set or --from-gh."); + } + // Default path: run a review. --base and --head are required here. let base = args .base @@ -343,6 +487,80 @@ pub fn run(app: &AppContext, args: PrReviewArgs) -> Result { } } +fn run_init_set(app: &AppContext, top: &PrReviewArgs, args: &InitSetArgs) -> Result<()> { + let config_path = top + .config + .clone() + .unwrap_or_else(|| app.workspace_paths().config_path); + let manifest = resolve_pr_set_from_gh( + &app.workspace_path, + &config_path, + &GhResolveOptions { + query: args.query.clone(), + set_id: args.set_id.clone(), + allow_unknown_repos: args.allow_unknown_repos, + }, + ) + .with_context(|| "Resolving PR-set manifest from GitHub.")?; + + let output = args.output.clone().unwrap_or_else(|| { + app.workspace_path + .join(format!("{}-pr-set.yaml", slugify_set_id(&manifest.id))) + }); + if output.exists() { + anyhow::bail!( + "Refusing to overwrite existing PR-set manifest `{}`.", + output.display() + ); + } + write_manifest_yaml(&output, &manifest)?; + + #[expect( + clippy::print_stdout, + reason = "init-set reports the generated manifest path for shell use" + )] + { + println!("{}", output.display()); + } + Ok(()) +} + +fn write_generated_pr_set_manifest( + app: &AppContext, + args: &PrReviewArgs, + manifest: &PrSetManifest, + source: &str, +) -> Result { + let cache_root = args + .cache_root + .clone() + .unwrap_or_else(|| default_cache_root(&app.workspace_path)); + let set_dir = cache_root + .join(workspace_hash(&app.workspace_path)) + .join(format!("set-{}-{}", source, slugify_set_id(&manifest.id))); + std::fs::create_dir_all(&set_dir).with_context(|| { + format!( + "Creating PR-set manifest cache dir `{}`.", + set_dir.display() + ) + })?; + let path = set_dir.join("manifest.yaml"); + write_manifest_yaml(&path, manifest)?; + Ok(path) +} + +fn write_manifest_yaml(path: &Path, manifest: &PrSetManifest) -> Result<()> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("Creating manifest output dir `{}`.", parent.display()))?; + } + let yaml = manifest + .to_yaml_string() + .with_context(|| "Serializing PR-set manifest YAML.")?; + std::fs::write(path, yaml) + .with_context(|| format!("Writing PR-set manifest `{}`.", path.display())) +} + // ─── Validated run-review args ───────────────────────────────────────────── /// Validated args for the "run a review" path (no subcommand). @@ -617,6 +835,15 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo ); } } + let baseline_workspace_path = config_override_path + .as_ref() + .and_then(|path| path.parent().map(PathBuf::from)) + .map_or_else( + || app.workspace_path.clone(), + |path| std::fs::canonicalize(&path).unwrap_or(path), + ); + let baseline_workspace_paths = workspace_paths_for_root(&baseline_workspace_path); + let config_override_bytes: Option> = if let Some(path) = config_override_path.as_ref() { Some( std::fs::read(path) @@ -625,6 +852,17 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo } else { None }; + let config_worktree_bytes: Option> = match ( + config_override_path.as_ref(), + config_override_bytes.as_ref(), + ) { + (Some(path), Some(bytes)) => Some(prepare_review_config_override( + &app.workspace_path, + path, + bytes, + )?), + _ => None, + }; let config_bytes: Vec = if let Some(bytes) = config_override_bytes.as_ref() { bytes.clone() } else { @@ -741,7 +979,7 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo // rather than racing with `git worktree add`. if let (Some(src), Some(bytes)) = ( config_override_path.as_ref(), - config_override_bytes.as_ref(), + config_worktree_bytes.as_ref(), ) { let dst = artifact_root.worktree_root.join("gather-step.config.yaml"); if let Err(e) = std::fs::write(&dst, bytes) { @@ -760,7 +998,7 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo // If the workspace has a normal index with a matching config hash, // copy it into the review artifact root so the indexer only needs to // update changed repos rather than rebuild from scratch. - match pick_seed_source(&app.workspace_path, &cache_key_struct.config_hash) { + match pick_seed_source(&baseline_workspace_path, &cache_key_struct.config_hash) { Ok(Some(seed)) => { tracing::info!("Seeding the review artifact from the baseline workspace index."); if let Err(e) = seed_artifact_root(&seed, &artifact_root) { @@ -795,7 +1033,7 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo // if the baseline doesn't exist yet (user hasn't indexed), pass None and // fall back to direct-change-only affected set. let affected_baseline_coord = { - let baseline_storage = app.workspace_paths().storage_root; + let baseline_storage = baseline_workspace_paths.storage_root.clone(); if baseline_storage.join("graph.redb").exists() { match gather_step_storage::StorageCoordinator::open_read_only(&baseline_storage) { Ok(coord) => Some(coord), @@ -899,7 +1137,7 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo let changed_repos = map_changed_repos_from_config(head_config.as_ref(), &all_changed_paths); // ── 8. Build report ──────────────────────────────────────────────────── - let ws_paths = app.workspace_paths(); + let ws_paths = baseline_workspace_paths.clone(); let ws_hash = workspace_hash(&app.workspace_path); let cache_key = format!("{ws_hash}:{base_sha}:{head_sha}"); @@ -1396,14 +1634,14 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo // ── Phase 3 Task 5: synthesize targeted pack commands ────────────────── let mut suggested_followups = build_suggested_followups( - &app.workspace_path, + &baseline_workspace_path, &artifact_root.registry_path, &artifact_root.storage_root, &route_deltas, &symbol_deltas, ); let pack_cmds = synthesize_review_pack_commands( - &app.workspace_path, + &baseline_workspace_path, &artifact_root.registry_path, &artifact_root.storage_root, &route_deltas, @@ -1557,6 +1795,63 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo // ─── Helpers ────────────────────────────────────────────────────────────────── +fn workspace_paths_for_root(workspace_path: &Path) -> WorkspacePaths { + let config_path = workspace_path.join("gather-step.config.yaml"); + let registry_path = workspace_path.join(".gather-step").join("registry.json"); + let storage_root = workspace_path.join(".gather-step").join("storage"); + let graph_path = storage_root.join("graph.redb"); + + WorkspacePaths { + config_path, + registry_path, + storage_root, + graph_path, + } +} + +fn prepare_review_config_override( + git_workspace: &Path, + config_path: &Path, + bytes: &[u8], +) -> Result> { + let Some(config_root) = config_path.parent() else { + return Ok(bytes.to_vec()); + }; + let config_root = + std::fs::canonicalize(config_root).unwrap_or_else(|_| config_root.to_path_buf()); + let git_workspace = + std::fs::canonicalize(git_workspace).unwrap_or_else(|_| git_workspace.to_path_buf()); + if config_root == git_workspace { + return Ok(bytes.to_vec()); + } + + let raw = std::str::from_utf8(bytes) + .with_context(|| format!("Reading --config `{}` as UTF-8.", config_path.display()))?; + let mut config = GatherStepConfig::from_yaml_str(raw) + .with_context(|| format!("Parsing --config `{}`.", config_path.display()))?; + + let Some(index) = config.repos.iter().position(|repo| { + let repo_root = config_root.join(&repo.path); + std::fs::canonicalize(repo_root).is_ok_and(|path| path == git_workspace) + }) else { + return Ok(bytes.to_vec()); + }; + + let mut selected_repo = config.repos[index].clone(); + ".".clone_into(&mut selected_repo.path); + let selected_name = selected_repo.name.clone(); + config.repos = vec![selected_repo]; + config + .allow_listed_repos + .retain(|repo| repo == &selected_name); + + serde_norway::to_string(&config) + .map(std::string::String::into_bytes) + .with_context(|| { + format!("Serializing a worktree-local PR-review config for repo `{selected_name}`.") + }) +} + /// Map changed file paths to configured repo names using longest-prefix /// matching against the supplied head-worktree config. Files that do not match /// any configured repo are grouped under the synthetic `""` entry. @@ -2820,6 +3115,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -2881,6 +3181,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -2946,6 +3251,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -3010,6 +3320,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -3074,6 +3389,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -3140,6 +3460,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -3733,6 +4058,39 @@ mod tests { ); } + #[test] + fn config_override_rewrites_parent_config_for_child_repo_worktree() { + let root = TempDir::new("pr-review-parent-config"); + let parent = root.path(); + let api = parent.join("api-service"); + let web = parent.join("web-client"); + fs::create_dir_all(&api).expect("api repo dir"); + fs::create_dir_all(&web).expect("web repo dir"); + let config_path = parent.join("gather-step.config.yaml"); + let raw = b" +allow_listed_repos: + - api-service + - web-client +repos: + - name: api-service + path: api-service + - name: web-client + path: web-client +indexing: + workspace_concurrency: 1 +"; + + let rewritten = + prepare_review_config_override(&web, &config_path, raw).expect("rewrite config"); + let config = GatherStepConfig::from_yaml_str(std::str::from_utf8(&rewritten).unwrap()) + .expect("rewritten config should parse"); + + assert_eq!(config.repos.len(), 1); + assert_eq!(config.repos[0].name, "web-client"); + assert_eq!(config.repos[0].path, "."); + assert_eq!(config.allow_listed_repos, vec!["web-client"]); + } + // ───────────────────────────────────────────────────────────────────────── // Finding 1: clean_older_than_skips_in_progress // ───────────────────────────────────────────────────────────────────────── @@ -3774,6 +4132,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -3838,6 +4201,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -3895,6 +4263,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -4327,6 +4700,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -4421,6 +4799,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), @@ -4493,6 +4876,11 @@ mod tests { command: None, base: None, head: None, + pr_set: None, + from_gh: None, + set_id: None, + parallelism: 1, + allow_unknown_repos: false, engine: ReviewEngine::TempIndex, keep_cache: false, cache_root: Some(cache.to_path_buf()), diff --git a/crates/gather-step-cli/src/pr_review/index_runner.rs b/crates/gather-step-cli/src/pr_review/index_runner.rs index c4c62c21..955adc4b 100644 --- a/crates/gather-step-cli/src/pr_review/index_runner.rs +++ b/crates/gather-step-cli/src/pr_review/index_runner.rs @@ -8,8 +8,8 @@ //! //! A copy of `gather-step.config.yaml` must exist at //! `artifact_root.worktree_root.join("gather-step.config.yaml")`. The review -//! worktree was created from a SHA in the user's repo, so the config file is -//! naturally present if the user has it checked in. +//! worktree may provide that file from the checked-out ref, or the caller may +//! copy a workspace-level config into the temporary worktree before indexing. //! //! Phase 1 Task 4 of the PR review mode plan. @@ -45,8 +45,8 @@ const CONFIG_FILENAME: &str = "gather-step.config.yaml"; /// /// A copy of `gather-step.config.yaml` must exist at /// `artifact_root.worktree_root.join("gather-step.config.yaml")`. The -/// review worktree was created from a SHA in the user's repo, so the -/// config file is naturally present if the user has it checked in. +/// review worktree may provide that file from the checked-out ref, or the +/// caller may copy a workspace-level config into the temporary worktree. /// /// # Errors /// @@ -64,8 +64,8 @@ pub fn run_review_index( if !config_path.exists() { bail!( "review index pre-condition violated: `{}` not found in worktree at `{}`.\n\ - The config file must be committed to the repository so it is present \ - in the detached worktree.", + Pass --config with an existing workspace config when the reviewed \ + git repository does not commit its own config file.", CONFIG_FILENAME, artifact_root.worktree_root.display(), ); diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/cache_key.rs b/crates/gather-step-cli/src/pr_review/multi_pr/cache_key.rs new file mode 100644 index 00000000..af495118 --- /dev/null +++ b/crates/gather-step-cli/src/pr_review/multi_pr/cache_key.rs @@ -0,0 +1,139 @@ +//! Cache identity helpers for coordinated PR-set reviews. + +use serde::{Deserialize, Serialize}; + +use crate::pr_review::artifact_root::{CacheKey, MARKER_SCHEMA_VERSION}; + +/// Resolved cache identity for one PR-set entry. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ResolvedPrCacheEntry { + pub id: String, + pub repo: String, + pub workspace_hash: String, + pub base_sha: String, + pub head_sha: String, + pub config_hash: String, + pub gather_step_version: String, +} + +impl ResolvedPrCacheEntry { + #[must_use] + pub fn cache_key(&self) -> CacheKey { + CacheKey { + workspace_hash: self.workspace_hash.clone(), + base_sha: self.base_sha.clone(), + head_sha: self.head_sha.clone(), + config_hash: self.config_hash.clone(), + schema_version: MARKER_SCHEMA_VERSION, + gather_step_version: self.gather_step_version.clone(), + } + } +} + +/// Stable cache identity for a full PR set. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PrSetCacheKey { + pub manifest_id: String, + pub manifest_version: u32, + pub entries: Vec, +} + +impl PrSetCacheKey { + /// Stable fingerprint for the complete set. Entry order in the manifest is + /// not semantically meaningful for cache reuse, so entries are sorted before + /// hashing. + #[must_use] + pub fn fingerprint(&self) -> String { + set_fingerprint(self) + } +} + +/// Return the same fingerprint a single-PR run would use for this entry. +#[must_use] +pub fn entry_fingerprint(entry: &ResolvedPrCacheEntry) -> String { + entry.cache_key().fingerprint() +} + +/// Stable fingerprint for the full set. +#[must_use] +pub fn set_fingerprint(key: &PrSetCacheKey) -> String { + let mut entries = key.entries.clone(); + entries.sort_by(|a, b| { + (&a.repo, &a.base_sha, &a.head_sha, &a.id).cmp(&(&b.repo, &b.base_sha, &b.head_sha, &b.id)) + }); + let entry_json = serde_json::to_string(&entries).unwrap_or_default(); + let canonical = format!( + r#"{{"entries":{},"manifest_id":{},"manifest_version":{}}}"#, + entry_json, + serde_json::to_string(&key.manifest_id).unwrap_or_default(), + key.manifest_version + ); + let hash = blake3::hash(canonical.as_bytes()); + let hex = hash.to_hex(); + hex[..16].to_owned() +} + +#[cfg(test)] +mod tests { + use super::{PrSetCacheKey, ResolvedPrCacheEntry, entry_fingerprint}; + use crate::pr_review::artifact_root::{CacheKey, MARKER_SCHEMA_VERSION}; + + fn entry(id: &str, repo: &str) -> ResolvedPrCacheEntry { + ResolvedPrCacheEntry { + id: id.to_owned(), + repo: repo.to_owned(), + workspace_hash: "workspacehash123".to_owned(), + base_sha: "1111111111111111111111111111111111111111".to_owned(), + head_sha: "2222222222222222222222222222222222222222".to_owned(), + config_hash: "confighash123456".to_owned(), + gather_step_version: "4.1.0".to_owned(), + } + } + + #[test] + fn entry_fingerprint_matches_single_pr_cache_key() { + let entry = entry("api", "web-api"); + let single = CacheKey { + workspace_hash: entry.workspace_hash.clone(), + base_sha: entry.base_sha.clone(), + head_sha: entry.head_sha.clone(), + config_hash: entry.config_hash.clone(), + schema_version: MARKER_SCHEMA_VERSION, + gather_step_version: entry.gather_step_version.clone(), + }; + + assert_eq!(entry_fingerprint(&entry), single.fingerprint()); + } + + #[test] + fn set_fingerprint_is_manifest_order_independent() { + let a = PrSetCacheKey { + manifest_id: "checkout-refresh".to_owned(), + manifest_version: 0, + entries: vec![entry("api", "api"), entry("web", "web")], + }; + let b = PrSetCacheKey { + manifest_id: "checkout-refresh".to_owned(), + manifest_version: 0, + entries: vec![entry("web", "web"), entry("api", "api")], + }; + + assert_eq!(a.fingerprint(), b.fingerprint()); + } + + #[test] + fn set_fingerprint_changes_when_manifest_id_changes() { + let a = PrSetCacheKey { + manifest_id: "checkout-refresh".to_owned(), + manifest_version: 0, + entries: vec![entry("api", "api")], + }; + let b = PrSetCacheKey { + manifest_id: "checkout-refresh-v2".to_owned(), + manifest_version: 0, + entries: vec![entry("api", "api")], + }; + + assert_ne!(a.fingerprint(), b.fingerprint()); + } +} diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs b/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs new file mode 100644 index 00000000..025a9c66 --- /dev/null +++ b/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs @@ -0,0 +1,383 @@ +//! Coordinator for `gather-step pr-review --pr-set`. + +use std::{ + collections::{BTreeMap, BTreeSet}, + path::{Path, PathBuf}, + thread, +}; + +use anyhow::{Context, Result}; + +use crate::{ + app::AppContext, + commands::pr_review::{OutputFormat, PrReviewRunArgs, ReviewEngine, SeverityMode, run_inner}, + pr_review::{ + delta_report::GITHUB_COMMENT_LIMIT, + multi_pr::{ + delta_report::{ + ErroredPrReview, MultiPrDeltaReport, PerPrDeltaReport, PrReviewSetEntryStatus, + }, + manifest::{PrEntry, PrSetManifest}, + }, + }, +}; + +#[derive(Debug, Clone)] +pub struct PrSetRunArgs { + pub manifest_path: PathBuf, + pub set_id: Option, + pub parallelism: usize, + pub engine: ReviewEngine, + pub keep_cache: bool, + pub cache_root: Option, + pub config: Option, + pub severity: SeverityMode, + pub format: OutputFormat, + pub github_comment_file: Option, + pub no_baseline_check: bool, +} + +pub fn run_pr_set(app: &AppContext, args: &PrSetRunArgs) -> Result<(String, bool)> { + let mut manifest = PrSetManifest::from_path(&args.manifest_path).with_context(|| { + format!( + "Loading PR-set manifest `{}`.", + args.manifest_path.display() + ) + })?; + if let Some(set_id) = args.set_id.as_ref() { + manifest.id.clone_from(set_id); + manifest + .validate() + .with_context(|| "Validating the PR-set manifest after applying --set-id.")?; + } + + let levels = dependency_levels(&manifest); + let mut completed = Vec::new(); + let mut errors = Vec::new(); + let mut completed_ids = BTreeSet::new(); + let mut blocked_ids = BTreeSet::new(); + let mut threshold_exceeded = false; + let run_settings = PerPrRunSettings::from_args(args); + + for level in levels { + let mut runnable = Vec::new(); + for entry in level { + let blocked_by: Vec = entry + .depends_on + .iter() + .filter(|dependency| blocked_ids.contains(*dependency)) + .cloned() + .collect(); + if blocked_by.is_empty() + && entry + .depends_on + .iter() + .all(|dependency| completed_ids.contains(dependency)) + { + runnable.push(entry); + } else { + blocked_ids.insert(entry.id.clone()); + errors.push(skipped_entry( + &entry, + format!( + "Skipped because dependency review did not complete successfully: {}.", + blocked_by.join(", ") + ), + )); + } + } + + for result in run_level(app, &run_settings, runnable, args.parallelism.max(1)) { + match result { + Ok(report) => { + threshold_exceeded |= report.threshold_exceeded; + completed_ids.insert(report.id.clone()); + completed.push(report); + } + Err(error) => { + blocked_ids.insert(error.id.clone()); + errors.push(*error); + } + } + } + } + + let report = MultiPrDeltaReport::from_parts( + &manifest, + Some(args.manifest_path.clone()), + completed, + errors, + threshold_exceeded, + ); + let effective_format = if app.json_output { + OutputFormat::Json + } else { + args.format + }; + let rendered = match effective_format { + OutputFormat::Json => report.render_json()?, + OutputFormat::GithubComment => report.render_github_comment(GITHUB_COMMENT_LIMIT), + OutputFormat::Braingent => report.render_braingent(), + OutputFormat::Markdown => report.render_markdown(), + }; + + if let Some(path) = args.github_comment_file.as_ref() { + std::fs::write(path, report.render_github_comment(GITHUB_COMMENT_LIMIT)).with_context( + || format!("Writing the PR-set GitHub comment to `{}`.", path.display()), + )?; + } + + Ok((rendered, report.threshold_exceeded)) +} + +#[derive(Debug, Clone)] +struct PerPrRunSettings { + engine: ReviewEngine, + keep_cache: bool, + cache_root: Option, + config: Option, + severity: SeverityMode, + no_baseline_check: bool, +} + +impl PerPrRunSettings { + fn from_args(args: &PrSetRunArgs) -> Self { + Self { + engine: args.engine, + keep_cache: args.keep_cache, + cache_root: args.cache_root.clone(), + config: args.config.clone(), + severity: args.severity, + no_baseline_check: args.no_baseline_check, + } + } +} + +fn run_level( + app: &AppContext, + settings: &PerPrRunSettings, + entries: Vec, + parallelism: usize, +) -> Vec>> { + if entries.is_empty() { + return Vec::new(); + } + if parallelism <= 1 || entries.len() == 1 { + return entries + .into_iter() + .map(|entry| run_one(app, settings, entry)) + .collect(); + } + + let mut results = Vec::new(); + for chunk in entries.chunks(parallelism) { + let chunk_results = thread::scope(|scope| { + let handles: Vec<_> = chunk + .iter() + .cloned() + .map(|entry| { + let app = app.clone(); + let settings = settings.clone(); + scope.spawn(move || run_one(&app, &settings, entry)) + }) + .collect(); + + handles + .into_iter() + .map(|handle| match handle.join() { + Ok(result) => result, + Err(_) => Err(Box::new(ErroredPrReview { + id: "".to_owned(), + repo: "".to_owned(), + pr: None, + base: String::new(), + head: String::new(), + status: PrReviewSetEntryStatus::Failed, + message: "PR-set worker thread panicked.".to_owned(), + })), + }) + .collect::>() + }); + results.extend(chunk_results); + } + + results +} + +fn run_one( + app: &AppContext, + settings: &PerPrRunSettings, + entry: PrEntry, +) -> Result> { + let mut entry_app = app.clone(); + if let Some(repo_path) = resolve_entry_workspace(&app.workspace_path, &entry.repo) { + entry_app.workspace_path = repo_path; + } + let config = settings.config.clone().or_else(|| { + let parent_config = app.workspace_path.join("gather-step.config.yaml"); + (entry_app.workspace_path != app.workspace_path && parent_config.is_file()) + .then_some(parent_config) + }); + + let review_args = PrReviewRunArgs { + base: entry.base.clone(), + head: entry.head.clone(), + engine: settings.engine, + keep_cache: settings.keep_cache, + cache_root: settings.cache_root.clone(), + config, + severity: settings.severity, + format: OutputFormat::Json, + github_comment_file: None, + no_baseline_check: settings.no_baseline_check, + }; + + let (rendered, threshold_exceeded) = run_inner(&entry_app, &review_args) + .map_err(|error| failed_entry(&entry, error.to_string()))?; + let delta_report = serde_json::from_str(&rendered).map_err(|error| { + failed_entry( + &entry, + format!("Failed to parse child DeltaReport JSON: {error}."), + ) + })?; + + Ok(PerPrDeltaReport { + id: entry.id, + repo: entry.repo, + pr: entry.pr, + base: review_args.base, + head: review_args.head, + threshold_exceeded, + delta_report, + }) +} + +fn resolve_entry_workspace(parent: &Path, repo: &str) -> Option { + let repo_path = Path::new(repo); + let candidates = if repo_path.is_absolute() { + vec![repo_path.to_path_buf()] + } else { + vec![parent.join(repo_path)] + }; + + candidates.into_iter().find_map(|candidate| { + let git_dir = candidate.join(".git"); + if git_dir.exists() { + std::fs::canonicalize(candidate).ok() + } else { + None + } + }) +} + +fn failed_entry(entry: &PrEntry, message: String) -> Box { + Box::new(ErroredPrReview { + id: entry.id.clone(), + repo: entry.repo.clone(), + pr: entry.pr, + base: entry.base.clone(), + head: entry.head.clone(), + status: PrReviewSetEntryStatus::Failed, + message, + }) +} + +fn skipped_entry(entry: &PrEntry, message: String) -> ErroredPrReview { + ErroredPrReview { + id: entry.id.clone(), + repo: entry.repo.clone(), + pr: entry.pr, + base: entry.base.clone(), + head: entry.head.clone(), + status: PrReviewSetEntryStatus::Skipped, + message, + } +} + +fn dependency_levels(manifest: &PrSetManifest) -> Vec> { + let mut entries: BTreeMap = manifest + .prs + .iter() + .cloned() + .map(|entry| (entry.id.clone(), entry)) + .collect(); + let mut completed = BTreeSet::new(); + let mut levels = Vec::new(); + + while !entries.is_empty() { + let ready_ids: Vec = entries + .iter() + .filter(|(_, entry)| { + entry + .depends_on + .iter() + .all(|dependency| completed.contains(dependency)) + }) + .map(|(id, _)| id.clone()) + .collect(); + + if ready_ids.is_empty() { + break; + } + + let mut level = Vec::new(); + for id in ready_ids { + if let Some(entry) = entries.remove(&id) { + completed.insert(id); + level.push(entry); + } + } + levels.push(level); + } + + levels +} + +#[cfg(test)] +mod tests { + use super::{dependency_levels, resolve_entry_workspace}; + use crate::pr_review::multi_pr::manifest::{PrEntry, PrSetManifest}; + + fn entry(id: &str, depends_on: Vec<&str>) -> PrEntry { + PrEntry { + id: id.to_owned(), + repo: id.to_owned(), + base: "main".to_owned(), + head: format!("feature/{id}"), + pr: None, + depends_on: depends_on.into_iter().map(str::to_owned).collect(), + } + } + + #[test] + fn dependency_levels_group_independent_entries() { + let manifest = PrSetManifest { + version: 0, + id: "set".to_owned(), + title: None, + prs: vec![ + entry("api", vec![]), + entry("web", vec!["api"]), + entry("docs", vec![]), + ], + }; + + let levels = dependency_levels(&manifest); + + assert_eq!( + levels[0] + .iter() + .map(|entry| entry.id.as_str()) + .collect::>(), + vec!["api", "docs"] + ); + assert_eq!(levels[1][0].id, "web"); + } + + #[test] + fn resolve_entry_workspace_ignores_non_git_repo_name() { + let temp = tempfile::tempdir().expect("tempdir"); + + assert!(resolve_entry_workspace(temp.path(), "api").is_none()); + } +} diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs b/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs new file mode 100644 index 00000000..7ca0f587 --- /dev/null +++ b/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs @@ -0,0 +1,649 @@ +//! Output model for coordinated PR-set reviews. + +use std::{ + collections::{BTreeMap, BTreeSet}, + fmt::Write as _, + path::PathBuf, +}; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +use super::{ + cache_key::{PrSetCacheKey, ResolvedPrCacheEntry}, + manifest::PrSetManifest, +}; + +pub const MULTI_PR_DELTA_REPORT_SCHEMA_VERSION: u32 = 0; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct MultiPrDeltaReport { + pub schema_version: u32, + pub metadata: MultiPrMetadata, + pub prs: Vec, + pub errors: Vec, + pub cross_pr: CrossPrFindings, + pub threshold_exceeded: bool, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct MultiPrMetadata { + pub set_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub title: Option, + pub manifest_version: u32, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub manifest_path: Option, + pub total_prs: usize, + pub completed_prs: usize, + pub failed_prs: usize, + pub skipped_prs: usize, + pub set_fingerprint: String, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct PerPrDeltaReport { + pub id: String, + pub repo: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pr: Option, + pub base: String, + pub head: String, + pub threshold_exceeded: bool, + pub delta_report: Value, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ErroredPrReview { + pub id: String, + pub repo: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pr: Option, + pub base: String, + pub head: String, + pub status: PrReviewSetEntryStatus, + pub message: String, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum PrReviewSetEntryStatus { + Failed, + Skipped, +} + +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct CrossPrFindings { + pub contract_drifts: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct CrossPrContractDrift { + pub identity: String, + pub producer_prs: Vec, + pub consumer_prs: Vec, + pub severity: CrossPrSeverity, + pub reason: String, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] +#[serde(rename_all = "snake_case")] +pub enum CrossPrSeverity { + Low, + Medium, + High, +} + +impl CrossPrSeverity { + const fn as_str(self) -> &'static str { + match self { + Self::Low => "low", + Self::Medium => "medium", + Self::High => "high", + } + } +} + +impl MultiPrDeltaReport { + #[must_use] + pub fn from_parts( + manifest: &PrSetManifest, + manifest_path: Option, + prs: Vec, + errors: Vec, + threshold_exceeded: bool, + ) -> Self { + let set_fingerprint = set_fingerprint_for_report(manifest, &prs); + let skipped_prs = errors + .iter() + .filter(|error| error.status == PrReviewSetEntryStatus::Skipped) + .count(); + let failed_prs = errors.len().saturating_sub(skipped_prs); + let cross_pr = CrossPrFindings::from_prs(&prs); + + Self { + schema_version: MULTI_PR_DELTA_REPORT_SCHEMA_VERSION, + metadata: MultiPrMetadata { + set_id: manifest.id.clone(), + title: manifest.title.clone(), + manifest_version: manifest.version, + manifest_path, + total_prs: manifest.prs.len(), + completed_prs: prs.len(), + failed_prs, + skipped_prs, + set_fingerprint, + }, + prs, + errors, + cross_pr, + threshold_exceeded, + } + } + + pub fn render_json(&self) -> anyhow::Result { + Ok(serde_json::to_string(self)?) + } + + #[must_use] + pub fn render_markdown(&self) -> String { + let mut buf = String::new(); + let title = self + .metadata + .title + .as_deref() + .unwrap_or(self.metadata.set_id.as_str()); + let _ = writeln!(buf, "# Gather Step PR-set review: {title}\n"); + let _ = writeln!(buf, "- Set id: `{}`", self.metadata.set_id); + let _ = writeln!( + buf, + "- PRs: {} completed, {} failed, {} skipped, {} total", + self.metadata.completed_prs, + self.metadata.failed_prs, + self.metadata.skipped_prs, + self.metadata.total_prs + ); + let _ = writeln!( + buf, + "- Set fingerprint: `{}`", + self.metadata.set_fingerprint + ); + if self.threshold_exceeded { + let _ = writeln!(buf, "- Severity threshold: exceeded"); + } else { + let _ = writeln!(buf, "- Severity threshold: not exceeded"); + } + + self.render_cross_pr_section(&mut buf); + self.render_errors_section(&mut buf); + self.render_pr_summaries(&mut buf); + + buf + } + + #[must_use] + pub fn render_github_comment(&self, limit: usize) -> String { + truncate_to_limit(self.render_markdown(), limit) + } + + #[must_use] + pub fn render_braingent(&self) -> String { + let mut buf = String::new(); + let _ = writeln!(buf, "---"); + let _ = writeln!(buf, "type: pr-review-set"); + let _ = writeln!(buf, "set_id: {}", self.metadata.set_id); + let _ = writeln!(buf, "set_fingerprint: {}", self.metadata.set_fingerprint); + let _ = writeln!(buf, "---\n"); + buf.push_str(&self.render_markdown()); + buf + } + + fn render_cross_pr_section(&self, buf: &mut String) { + let _ = writeln!(buf, "\n## Cross-PR findings\n"); + if self.cross_pr.contract_drifts.is_empty() { + let _ = writeln!(buf, "No cross-PR payload contract drift found."); + return; + } + + let _ = writeln!( + buf, + "| Severity | Contract | Producer PRs | Consumer PRs | Reason |" + ); + let _ = writeln!(buf, "|---|---|---|---|---|"); + for drift in &self.cross_pr.contract_drifts { + let consumer_prs = if drift.consumer_prs.is_empty() { + "none".to_owned() + } else { + drift.consumer_prs.join(", ") + }; + let _ = writeln!( + buf, + "| {} | `{}` | {} | {} | {} |", + drift.severity.as_str(), + drift.identity, + drift.producer_prs.join(", "), + consumer_prs, + drift.reason + ); + } + } + + fn render_errors_section(&self, buf: &mut String) { + if self.errors.is_empty() { + return; + } + + let _ = writeln!(buf, "\n## Failed or skipped PRs\n"); + let _ = writeln!(buf, "| Status | PR | Repo | Range | Message |"); + let _ = writeln!(buf, "|---|---|---|---|---|"); + for error in &self.errors { + let pr = error + .pr + .map_or_else(|| error.id.clone(), |pr| format!("#{pr}")); + let _ = writeln!( + buf, + "| {:?} | {} | `{}` | `{}`..`{}` | {} |", + error.status, pr, error.repo, error.base, error.head, error.message + ); + } + } + + fn render_pr_summaries(&self, buf: &mut String) { + let _ = writeln!(buf, "\n## Per-PR summaries\n"); + if self.prs.is_empty() { + let _ = writeln!(buf, "No PRs completed successfully."); + return; + } + + let _ = writeln!( + buf, + "| PR | Repo | Range | Changed files | Routes | Symbols | Payload contracts | Events | Deployment |" + ); + let _ = writeln!(buf, "|---|---|---|---:|---:|---:|---:|---:|---:|"); + for pr in &self.prs { + let label = pr + .pr + .map_or_else(|| pr.id.clone(), |number| format!("#{number}")); + let counts = ReportCounts::from_report(&pr.delta_report); + let _ = writeln!( + buf, + "| {} | `{}` | `{}`..`{}` | {} | {} | {} | {} | {} | {} |", + label, + pr.repo, + pr.base, + pr.head, + counts.changed_files, + counts.routes, + counts.symbols, + counts.payload_contracts, + counts.events, + counts.deployment + ); + } + } +} + +impl CrossPrFindings { + #[must_use] + pub fn from_prs(prs: &[PerPrDeltaReport]) -> Self { + let mut producer_mentions: BTreeMap = BTreeMap::new(); + let mut consumer_mentions: BTreeMap> = BTreeMap::new(); + + for pr in prs { + for mention in contract_mentions_for_pr(pr) { + if mention.side == ContractSide::Producer { + let entry = producer_mentions + .entry(mention.identity.clone()) + .or_default(); + entry.pr_ids.insert(pr.id.clone()); + entry.severity = entry.severity.max(mention.severity); + } else { + consumer_mentions + .entry(mention.identity) + .or_default() + .insert(pr.id.clone()); + } + } + } + + let mut contract_drifts = Vec::new(); + for (identity, producer) in producer_mentions { + let consumer_prs: BTreeSet = consumer_mentions + .get(&identity) + .cloned() + .unwrap_or_default(); + let covered_elsewhere = producer.pr_ids.iter().any(|producer_pr| { + consumer_prs + .iter() + .any(|consumer_pr| consumer_pr != producer_pr) + }); + if covered_elsewhere { + continue; + } + + contract_drifts.push(CrossPrContractDrift { + identity: identity.clone(), + producer_prs: producer.pr_ids.into_iter().collect(), + consumer_prs: consumer_prs.into_iter().collect(), + severity: producer.severity, + reason: if producer.severity >= CrossPrSeverity::High { + "Producer payload contract changed in a breaking direction without a matching consumer PR in this set.".to_owned() + } else { + "Producer payload contract changed without a matching consumer PR in this set.".to_owned() + }, + }); + } + + Self { contract_drifts } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ContractSide { + Producer, + Consumer, +} + +#[derive(Debug, Clone)] +struct ContractMention { + identity: String, + side: ContractSide, + severity: CrossPrSeverity, +} + +#[derive(Debug, Clone)] +struct ContractMentionAccumulator { + pr_ids: BTreeSet, + severity: CrossPrSeverity, +} + +impl Default for ContractMentionAccumulator { + fn default() -> Self { + Self { + pr_ids: BTreeSet::new(), + severity: CrossPrSeverity::Low, + } + } +} + +#[derive(Debug, Clone, Copy)] +struct ReportCounts { + changed_files: usize, + routes: usize, + symbols: usize, + payload_contracts: usize, + events: usize, + deployment: usize, +} + +impl ReportCounts { + fn from_report(report: &Value) -> Self { + Self { + changed_files: value_array(report, &["changed_files"]).len(), + routes: surface_count(report, "routes"), + symbols: surface_count(report, "symbols"), + payload_contracts: surface_count(report, "payload_contracts"), + events: surface_count(report, "events"), + deployment: deployment_count(report), + } + } +} + +fn contract_mentions_for_pr(pr: &PerPrDeltaReport) -> Vec { + let mut mentions = Vec::new(); + for bucket in ["added", "removed"] { + for item in value_array(&pr.delta_report, &["payload_contracts", bucket]) { + if let Some(mention) = contract_mention_from_value(item, bucket) { + mentions.push(mention); + } + } + } + for item in value_array(&pr.delta_report, &["payload_contracts", "changed"]) { + if let Some(mention) = contract_mention_from_value(item, "changed") { + mentions.push(mention); + } + } + mentions +} + +fn contract_mention_from_value(item: &Value, bucket: &str) -> Option { + let identity = item.get("target_qualified_name")?.as_str()?.to_owned(); + let side = match item.get("side").and_then(Value::as_str) { + Some("producer") => ContractSide::Producer, + Some("consumer") => ContractSide::Consumer, + _ => return None, + }; + let severity = match side { + ContractSide::Consumer => CrossPrSeverity::Low, + ContractSide::Producer => contract_change_severity(item, bucket), + }; + Some(ContractMention { + identity, + side, + severity, + }) +} + +fn contract_change_severity(item: &Value, bucket: &str) -> CrossPrSeverity { + match bucket { + "removed" => CrossPrSeverity::High, + "added" => CrossPrSeverity::Medium, + "changed" => { + let breaking = !value_array(item, &["fields_removed"]).is_empty() + || !value_array(item, &["fields_optional_to_required"]).is_empty() + || !value_array(item, &["fields_type_changed"]).is_empty(); + if breaking { + CrossPrSeverity::High + } else if !value_array(item, &["fields_added"]).is_empty() + || !value_array(item, &["fields_required_to_optional"]).is_empty() + { + CrossPrSeverity::Medium + } else { + CrossPrSeverity::Low + } + } + _ => CrossPrSeverity::Low, + } +} + +fn surface_count(report: &Value, surface: &str) -> usize { + ["added", "removed", "changed"] + .into_iter() + .map(|bucket| value_array(report, &[surface, bucket]).len()) + .sum() +} + +fn deployment_count(report: &Value) -> usize { + let Some(object) = report.get("deployment").and_then(Value::as_object) else { + return 0; + }; + object + .iter() + .filter(|(key, _)| key.as_str() != "unavailable") + .map(|(_, value)| value.as_array().map_or(0, Vec::len)) + .sum() +} + +fn value_array<'a>(value: &'a Value, path: &[&str]) -> Vec<&'a Value> { + let mut cursor = value; + for segment in path { + let Some(next) = cursor.get(*segment) else { + return Vec::new(); + }; + cursor = next; + } + cursor + .as_array() + .map(|items| items.iter().collect()) + .unwrap_or_default() +} + +fn set_fingerprint_for_report(manifest: &PrSetManifest, prs: &[PerPrDeltaReport]) -> String { + let entries = prs + .iter() + .map(|pr| ResolvedPrCacheEntry { + id: pr.id.clone(), + repo: pr.repo.clone(), + workspace_hash: pr + .delta_report + .get("safety") + .and_then(|safety| safety.get("cache_key")) + .and_then(Value::as_str) + .and_then(|cache_key| cache_key.split(':').next()) + .unwrap_or_default() + .to_owned(), + base_sha: pr + .delta_report + .get("metadata") + .and_then(|metadata| metadata.get("base_sha")) + .and_then(Value::as_str) + .unwrap_or(pr.base.as_str()) + .to_owned(), + head_sha: pr + .delta_report + .get("metadata") + .and_then(|metadata| metadata.get("head_sha")) + .and_then(Value::as_str) + .unwrap_or(pr.head.as_str()) + .to_owned(), + config_hash: String::new(), + gather_step_version: env!("CARGO_PKG_VERSION").to_owned(), + }) + .collect(); + + PrSetCacheKey { + manifest_id: manifest.id.clone(), + manifest_version: manifest.version, + entries, + } + .fingerprint() +} + +fn truncate_to_limit(mut value: String, limit: usize) -> String { + if value.len() <= limit { + return value; + } + let suffix = "\n\n_Report truncated to fit the GitHub comment limit._"; + if limit <= suffix.len() { + value.truncate(limit); + return value; + } + let mut target = limit - suffix.len(); + while target > 0 && !value.is_char_boundary(target) { + target -= 1; + } + format!("{}{}", &value[..target], suffix) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::{ + CrossPrFindings, CrossPrSeverity, MultiPrDeltaReport, PerPrDeltaReport, + PrReviewSetEntryStatus, + }; + use crate::pr_review::multi_pr::manifest::{PrEntry, PrSetManifest}; + + fn pr(id: &str, side: &str) -> PerPrDeltaReport { + PerPrDeltaReport { + id: id.to_owned(), + repo: id.to_owned(), + pr: None, + base: "main".to_owned(), + head: format!("feature/{id}"), + threshold_exceeded: false, + delta_report: json!({ + "metadata": { + "base_sha": "1111111111111111111111111111111111111111", + "head_sha": "2222222222222222222222222222222222222222" + }, + "safety": { + "cache_key": "workspace:base:head" + }, + "changed_files": ["src/app.ts"], + "routes": { "added": [], "removed": [], "changed": [] }, + "symbols": { "added": [], "removed": [], "changed": [] }, + "payload_contracts": { + "added": [], + "removed": [], + "changed": [{ + "repo": id, + "file": "src/payload.ts", + "target_qualified_name": "UpdateLabelProject", + "side": side, + "fields_added": [], + "fields_removed": [{ "name": "name", "type_name": "string", "optional": false }], + "fields_optional_to_required": [], + "fields_required_to_optional": [], + "fields_type_changed": [] + }] + }, + "events": { "added": [], "removed": [], "changed": [] }, + "deployment": {} + }), + } + } + + #[test] + fn cross_pr_drift_flags_producer_without_consumer_pr() { + let findings = CrossPrFindings::from_prs(&[pr("api", "producer")]); + + assert_eq!(findings.contract_drifts.len(), 1); + assert_eq!(findings.contract_drifts[0].severity, CrossPrSeverity::High); + } + + #[test] + fn cross_pr_drift_is_covered_by_other_consumer_pr() { + let findings = CrossPrFindings::from_prs(&[pr("api", "producer"), pr("web", "consumer")]); + + assert!(findings.contract_drifts.is_empty()); + } + + #[test] + fn multi_pr_report_tracks_failed_and_skipped_counts() { + let manifest = PrSetManifest { + version: 0, + id: "checkout-refresh".to_owned(), + title: None, + prs: vec![ + PrEntry { + id: "api".to_owned(), + repo: "api".to_owned(), + base: "main".to_owned(), + head: "feature/api".to_owned(), + pr: Some(1), + depends_on: vec![], + }, + PrEntry { + id: "web".to_owned(), + repo: "web".to_owned(), + base: "main".to_owned(), + head: "feature/web".to_owned(), + pr: Some(2), + depends_on: vec!["api".to_owned()], + }, + ], + }; + let report = MultiPrDeltaReport::from_parts( + &manifest, + None, + vec![pr("api", "producer")], + vec![super::ErroredPrReview { + id: "web".to_owned(), + repo: "web".to_owned(), + pr: Some(2), + base: "main".to_owned(), + head: "feature/web".to_owned(), + status: PrReviewSetEntryStatus::Skipped, + message: "dependency failed".to_owned(), + }], + false, + ); + + assert_eq!(report.metadata.completed_prs, 1); + assert_eq!(report.metadata.failed_prs, 0); + assert_eq!(report.metadata.skipped_prs, 1); + assert!(report.render_markdown().contains("Cross-PR findings")); + } +} diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/gh.rs b/crates/gather-step-cli/src/pr_review/multi_pr/gh.rs new file mode 100644 index 00000000..d72df612 --- /dev/null +++ b/crates/gather-step-cli/src/pr_review/multi_pr/gh.rs @@ -0,0 +1,323 @@ +//! GitHub CLI resolver for PR-set manifests. + +use std::{ + collections::{BTreeMap, BTreeSet}, + path::Path, + process::Command, +}; + +use gather_step_core::GatherStepConfig; +use serde::Deserialize; + +use super::manifest::{PR_SET_MANIFEST_VERSION, PrEntry, PrSetManifest}; + +const GH_LIMIT_PER_REPO: &str = "100"; + +#[derive(Debug, Clone)] +pub struct GhResolveOptions { + pub query: String, + pub set_id: Option, + pub allow_unknown_repos: bool, +} + +pub fn resolve_pr_set_from_gh( + workspace: &Path, + config_path: &Path, + options: &GhResolveOptions, +) -> Result { + let config = GatherStepConfig::from_yaml_file(config_path) + .map_err(|source| GhResolverError::Config { source })?; + let lookup = RepoLookup::from_config(&config); + let owner = config.github.as_ref().map(|github| github.owner.as_str()); + + let responses = if let Some(owner) = owner { + let mut all = Vec::new(); + let mut first_error = None; + let mut successes = 0usize; + for repo_name in lookup.local_repo_names() { + let repo = format!("{owner}/{repo_name}"); + match gh_pr_list(workspace, Some(&repo), &options.query) { + Ok(mut prs) => { + successes += 1; + all.append(&mut prs); + } + Err(error) => { + tracing::debug!( + error = %error, + repo = %repo, + "GitHub PR-set resolver skipped a repo whose PR list could not be loaded." + ); + if first_error.is_none() { + first_error = Some(error); + } + } + } + } + if successes == 0 + && let Some(error) = first_error + { + return Err(error); + } + all + } else { + gh_pr_list(workspace, None, &options.query)? + }; + + manifest_from_gh_prs(&responses, &lookup, options) +} + +fn gh_pr_list( + workspace: &Path, + repo: Option<&str>, + query: &str, +) -> Result, GhResolverError> { + let mut cmd = Command::new("gh"); + cmd.arg("pr").arg("list"); + if let Some(repo) = repo { + cmd.arg("--repo").arg(repo); + } + cmd.arg("--search").arg(query); + cmd.arg("--json") + .arg("number,headRefName,baseRefName,repoNameWithOwner"); + cmd.arg("--limit").arg(GH_LIMIT_PER_REPO); + cmd.current_dir(workspace); + + let output = cmd + .output() + .map_err(|source| GhResolverError::Launch { source })?; + if !output.status.success() { + return Err(GhResolverError::Command { + status: output.status.code(), + }); + } + serde_json::from_slice(&output.stdout).map_err(|source| GhResolverError::Parse { source }) +} + +fn manifest_from_gh_prs( + prs: &[GhPr], + lookup: &RepoLookup, + options: &GhResolveOptions, +) -> Result { + let mut deduped = BTreeMap::new(); + for pr in prs { + deduped.insert((pr.repo_name_with_owner.clone(), pr.number), pr.clone()); + } + + let mut entries = Vec::new(); + for pr in deduped.into_values().take(200) { + let repo = lookup + .local_name_for_gh_repo(&pr.repo_name_with_owner) + .or_else(|| { + options + .allow_unknown_repos + .then(|| gh_repo_basename(&pr.repo_name_with_owner)) + }); + let Some(repo) = repo else { + continue; + }; + entries.push(PrEntry { + id: format!("{}-{}", slugify_set_id(&repo), pr.number), + repo, + base: pr.base_ref_name, + head: pr.head_ref_name, + pr: Some(pr.number), + depends_on: Vec::new(), + }); + } + + if entries.is_empty() { + return Err(GhResolverError::NoPullRequests { + query: options.query.clone(), + }); + } + + let manifest = PrSetManifest { + version: PR_SET_MANIFEST_VERSION, + id: options + .set_id + .clone() + .unwrap_or_else(|| slugify_set_id(&options.query)), + title: Some(format!("PR set for {}", options.query)), + prs: entries, + }; + manifest + .validate() + .map_err(|source| GhResolverError::Manifest { source })?; + Ok(manifest) +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(rename_all = "camelCase")] +struct GhPr { + number: u64, + head_ref_name: String, + base_ref_name: String, + repo_name_with_owner: String, +} + +#[derive(Debug, Clone)] +struct RepoLookup { + aliases: BTreeMap, +} + +impl RepoLookup { + fn from_config(config: &GatherStepConfig) -> Self { + let mut aliases = BTreeMap::new(); + for repo in &config.repos { + aliases.insert(repo.name.clone(), repo.name.clone()); + if let Some(name) = Path::new(&repo.path) + .file_name() + .and_then(|name| name.to_str()) + { + aliases.insert(name.to_owned(), repo.name.clone()); + } + if let Some(github) = config.github.as_ref() { + aliases.insert(format!("{}/{}", github.owner, repo.name), repo.name.clone()); + if let Some(name) = Path::new(&repo.path) + .file_name() + .and_then(|name| name.to_str()) + { + aliases.insert(format!("{}/{}", github.owner, name), repo.name.clone()); + } + } + } + Self { aliases } + } + + fn local_repo_names(&self) -> BTreeSet { + self.aliases.values().cloned().collect() + } + + fn local_name_for_gh_repo(&self, repo_name_with_owner: &str) -> Option { + self.aliases.get(repo_name_with_owner).cloned().or_else(|| { + self.aliases + .get(&gh_repo_basename(repo_name_with_owner)) + .cloned() + }) + } +} + +fn gh_repo_basename(repo_name_with_owner: &str) -> String { + repo_name_with_owner + .rsplit('/') + .next() + .unwrap_or(repo_name_with_owner) + .to_owned() +} + +#[must_use] +pub fn slugify_set_id(input: &str) -> String { + let mut out = String::new(); + let mut last_was_dash = false; + for ch in input.chars() { + if ch.is_ascii_alphanumeric() { + out.push(ch.to_ascii_lowercase()); + last_was_dash = false; + } else if !last_was_dash { + out.push('-'); + last_was_dash = true; + } + } + let trimmed = out.trim_matches('-').to_owned(); + if trimmed.is_empty() { + "pr-set".to_owned() + } else { + trimmed + } +} + +#[derive(Debug, thiserror::Error)] +pub enum GhResolverError { + #[error("Failed to load the workspace config for GitHub PR-set resolution: {source}")] + Config { + #[source] + source: gather_step_core::ConfigError, + }, + #[error("Failed to launch `gh pr list`: {source}")] + Launch { + #[source] + source: std::io::Error, + }, + #[error("`gh pr list` exited with status {status:?}.")] + Command { status: Option }, + #[error("Failed to parse `gh pr list` JSON: {source}")] + Parse { + #[source] + source: serde_json::Error, + }, + #[error("No pull requests matched query `{query}` after workspace repo filtering.")] + NoPullRequests { query: String }, + #[error("Resolved PR-set manifest is invalid: {source}")] + Manifest { + #[source] + source: super::manifest::ManifestError, + }, +} + +#[cfg(test)] +mod tests { + use super::{GhPr, GhResolveOptions, RepoLookup, manifest_from_gh_prs, slugify_set_id}; + use gather_step_core::{GatherStepConfig, RepoConfig}; + + fn config() -> GatherStepConfig { + GatherStepConfig { + allow_listed_repos: vec![], + repos: vec![ + RepoConfig { + name: "api-service".to_owned(), + path: "services/api-service".to_owned(), + depth: None, + }, + RepoConfig { + name: "web-client".to_owned(), + path: "apps/web-client".to_owned(), + depth: None, + }, + ], + github: Some(gather_step_core::GithubConfig { + owner: "acme".to_owned(), + api_base_url: None, + token_env: None, + }), + jira: None, + indexing: gather_step_core::IndexingConfig::default(), + deployment: gather_step_core::DeploymentConfig::default(), + } + } + + #[test] + fn manifest_from_gh_prs_filters_to_configured_repos() { + let prs = vec![ + GhPr { + number: 42, + head_ref_name: "feature/api".to_owned(), + base_ref_name: "main".to_owned(), + repo_name_with_owner: "acme/api-service".to_owned(), + }, + GhPr { + number: 7, + head_ref_name: "feature/unknown".to_owned(), + base_ref_name: "main".to_owned(), + repo_name_with_owner: "acme/unknown".to_owned(), + }, + ]; + let options = GhResolveOptions { + query: "checkout refresh".to_owned(), + set_id: Some("checkout-refresh".to_owned()), + allow_unknown_repos: false, + }; + let manifest = manifest_from_gh_prs(&prs, &RepoLookup::from_config(&config()), &options) + .expect("manifest"); + + assert_eq!(manifest.id, "checkout-refresh"); + assert_eq!(manifest.prs.len(), 1); + assert_eq!(manifest.prs[0].repo, "api-service"); + assert_eq!(manifest.prs[0].pr, Some(42)); + } + + #[test] + fn slugify_set_id_keeps_ids_neutral_and_filesystem_safe() { + assert_eq!(slugify_set_id("Checkout Refresh!"), "checkout-refresh"); + assert_eq!(slugify_set_id(" "), "pr-set"); + } +} diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs b/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs index 7871e5f5..a3a06679 100644 --- a/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs +++ b/crates/gather-step-cli/src/pr_review/multi_pr/manifest.rs @@ -224,27 +224,28 @@ fn detect_dependency_cycle<'a>( mod tests { use super::{ManifestError, PR_SET_MANIFEST_VERSION, PrSetManifest}; - const REG_13863_EXAMPLE: &str = include_str!("../../../../../examples/pr-set/reg-13863.yaml"); + const CROSS_REPO_FEATURE_EXAMPLE: &str = + include_str!("../../../../../examples/pr-set/cross-repo-feature.yaml"); const STACKED_EXAMPLE: &str = include_str!("../../../../../examples/pr-set/stacked-in-one-repo.yaml"); const DIVERGENT_BASES_EXAMPLE: &str = include_str!("../../../../../examples/pr-set/divergent-bases.yaml"); #[test] - fn pr_set_manifest_parses_reg_13863_example() { - let manifest = PrSetManifest::from_yaml_str(REG_13863_EXAMPLE) - .expect("REG-13863 example should parse"); + fn pr_set_manifest_parses_cross_repo_feature_example() { + let manifest = PrSetManifest::from_yaml_str(CROSS_REPO_FEATURE_EXAMPLE) + .expect("cross-repo feature example should parse"); assert_eq!(manifest.version, PR_SET_MANIFEST_VERSION); - assert_eq!(manifest.id, "REG-13863"); + assert_eq!(manifest.id, "checkout-refresh"); assert_eq!(manifest.prs.len(), 3); assert!( manifest .prs .iter() - .any(|entry| entry.id == "label-review-502" - && entry.repo == "label-review" - && entry.pr == Some(502)) + .any(|entry| entry.id == "api-contract-42" + && entry.repo == "api-service" + && entry.pr == Some(42)) ); let round_tripped = manifest @@ -257,7 +258,11 @@ mod tests { #[test] fn pr_set_manifest_examples_validate_cleanly() { - for raw in [REG_13863_EXAMPLE, STACKED_EXAMPLE, DIVERGENT_BASES_EXAMPLE] { + for raw in [ + CROSS_REPO_FEATURE_EXAMPLE, + STACKED_EXAMPLE, + DIVERGENT_BASES_EXAMPLE, + ] { PrSetManifest::from_yaml_str(raw).expect("example manifest should validate cleanly"); } } diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs b/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs index 1a22b34d..91a67cb4 100644 --- a/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs +++ b/crates/gather-step-cli/src/pr_review/multi_pr/mod.rs @@ -1,6 +1,10 @@ //! Multi-PR `pr-review` support. //! -//! This module owns the PR-set manifest contract first. Later phases add cache, -//! runner, cross-PR analysis, and output rendering on top of this schema. +//! This module owns the PR-set manifest contract, cache identity, runner, +//! cross-PR analysis, and output rendering for coordinated review sets. +pub mod cache_key; +pub mod coordinator; +pub mod delta_report; +pub mod gh; pub mod manifest; diff --git a/crates/gather-step-mcp/src/catalog.rs b/crates/gather-step-mcp/src/catalog.rs index 8b4c0b57..8d8845bd 100644 --- a/crates/gather-step-mcp/src/catalog.rs +++ b/crates/gather-step-mcp/src/catalog.rs @@ -108,4 +108,8 @@ pub const MCP_TOOLS: &[(&str, &str)] = &[ "pr_review", "Build a disposable review index for a PR branch and return the delta report", ), + ( + "pr_review_set", + "Build coordinated disposable review indexes for a PR-set manifest", + ), ]; diff --git a/crates/gather-step-mcp/src/server.rs b/crates/gather-step-mcp/src/server.rs index 146c2d00..848f2b51 100644 --- a/crates/gather-step-mcp/src/server.rs +++ b/crates/gather-step-mcp/src/server.rs @@ -56,7 +56,10 @@ use crate::{ fix_pack_tool as run_fix_pack, planning_pack_tool as run_planning_pack, review_pack_tool as run_review_pack, }, - pr_review::{PrReviewInput, PrReviewResponse, run_pr_review}, + pr_review::{ + PrReviewInput, PrReviewResponse, PrReviewSetInput, PrReviewSetResponse, run_pr_review, + run_pr_review_set, + }, projection_impact::{ ProjectionImpactRequest, ProjectionImpactResponse, projection_impact_tool as run_projection_impact, @@ -1033,4 +1036,24 @@ impl GatherStepMcpServer { }) .await } + + #[tool( + name = "pr_review_set", + description = "Use this tool when the user asks to review multiple related pull requests, \ + a PR stack, or a cross-repo PR set with gather-step. Accepts a PR-set manifest path, \ + builds one disposable review per manifest entry, preserves dependency ordering, and \ + returns a MultiPrDeltaReport with per-PR results and cross-PR payload-contract drift.", + annotations(read_only_hint = true) + )] + pub async fn pr_review_set_tool( + &self, + Parameters(request): Parameters, + ) -> Result, String> { + let args = serde_json::to_value(&request).unwrap_or_default(); + let workspace = self.ctx.config.workspace_root(); + self.traced_call("pr_review_set", &args, move || { + run_pr_review_set(&workspace, &request).map(Json) + }) + .await + } } diff --git a/crates/gather-step-mcp/src/tools/pr_review.rs b/crates/gather-step-mcp/src/tools/pr_review.rs index 868f4ed8..43fdf4ac 100644 --- a/crates/gather-step-mcp/src/tools/pr_review.rs +++ b/crates/gather-step-mcp/src/tools/pr_review.rs @@ -81,6 +81,29 @@ pub struct PrReviewInput { pub timeout_secs: Option, } +/// Input parameters for the `pr_review_set` MCP tool. +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)] +pub struct PrReviewSetInput { + /// Path to a PR-set manifest, absolute or relative to the workspace root. + pub pr_set: String, + /// Override the manifest set id in the emitted report. + #[serde(default)] + pub set_id: Option, + /// Number of independent entries to review in parallel. + #[serde(default)] + pub parallelism: Option, + /// Keep review artifacts after the run. + #[serde(default)] + pub keep_cache: Option, + /// Severity mode: `"warn"` (default) | `"strict"` | `"pedantic"`. + #[serde(default)] + pub severity: Option, + /// Override the wall-clock timeout in seconds. Capped at + /// [`PR_REVIEW_TIMEOUT_MAX_SECS`]. + #[serde(default)] + pub timeout_secs: Option, +} + /// Structured response returned by the `pr_review` MCP tool. #[derive(Debug, Clone, Serialize, JsonSchema)] pub struct PrReviewResponse { @@ -90,6 +113,15 @@ pub struct PrReviewResponse { pub threshold_exceeded: bool, } +/// Structured response returned by the `pr_review_set` MCP tool. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct PrReviewSetResponse { + /// The full `MultiPrDeltaReport` as a JSON value. + pub multi_pr_delta_report: serde_json::Value, + /// `true` when any completed child review exceeded the severity threshold. + pub threshold_exceeded: bool, +} + // ─── Tool function ──────────────────────────────────────────────────────────── /// Run the pr-review pipeline via the `gather-step` CLI binary and return @@ -117,12 +149,59 @@ pub fn run_pr_review( if let Some(sev) = &input.severity { cmd.arg("--severity").arg(sev); } + + let (delta_report, threshold_exceeded) = + run_pr_review_json_child(cmd, &binary, effective_timeout(input.timeout_secs))?; + + Ok(PrReviewResponse { + delta_report, + threshold_exceeded, + }) +} + +/// Run the coordinated PR-set review pipeline via the `gather-step` CLI. +pub fn run_pr_review_set( + workspace: &std::path::Path, + input: &PrReviewSetInput, +) -> Result { + let binary = resolve_binary(); + let manifest_path = manifest_path_for_workspace(workspace, &input.pr_set); + + let mut cmd = Command::new(&binary); + cmd.arg("--workspace").arg(workspace); + cmd.arg("pr-review"); + cmd.arg("--pr-set").arg(manifest_path); + cmd.arg("--format").arg("json"); + if let Some(set_id) = &input.set_id { + cmd.arg("--set-id").arg(set_id); + } + if let Some(parallelism) = input.parallelism { + cmd.arg("--parallelism").arg(parallelism.to_string()); + } + if input.keep_cache.unwrap_or(false) { + cmd.arg("--keep-cache"); + } + if let Some(sev) = &input.severity { + cmd.arg("--severity").arg(sev); + } + + let (multi_pr_delta_report, threshold_exceeded) = + run_pr_review_json_child(cmd, &binary, effective_timeout(input.timeout_secs))?; + + Ok(PrReviewSetResponse { + multi_pr_delta_report, + threshold_exceeded, + }) +} + +fn run_pr_review_json_child( + mut cmd: Command, + binary: &std::path::Path, + timeout: Duration, +) -> Result<(serde_json::Value, bool), String> { cmd.stdin(Stdio::null()); cmd.stdout(Stdio::piped()); cmd.stderr(Stdio::piped()); - - let timeout = effective_timeout(input.timeout_secs); - let mut child = cmd.spawn().map_err(|e| { format!( "Failed to launch the `gather-step` binary at `{}`: {e}. \ @@ -188,10 +267,7 @@ pub fn run_pr_review( "Failed to parse the pr-review JSON output; check the operator log for details.".to_owned() })?; - Ok(PrReviewResponse { - delta_report, - threshold_exceeded, - }) + Ok((delta_report, threshold_exceeded)) } /// Resolve the path to the `gather-step` binary. @@ -209,6 +285,15 @@ fn resolve_binary() -> std::path::PathBuf { std::path::PathBuf::from("gather-step") } +fn manifest_path_for_workspace(workspace: &std::path::Path, value: &str) -> std::path::PathBuf { + let path = std::path::PathBuf::from(value); + if path.is_absolute() { + path + } else { + workspace.join(path) + } +} + fn effective_timeout(requested: Option) -> Duration { let secs = requested .unwrap_or(PR_REVIEW_TIMEOUT_SECS) @@ -358,6 +443,36 @@ mod tests { assert!(json.get("threshold_exceeded").is_some()); } + #[test] + fn pr_review_set_input_deserialises_minimal() { + let json = r#"{"pr_set": "examples/pr-set/cross-repo-feature.yaml"}"#; + let input: PrReviewSetInput = serde_json::from_str(json).unwrap(); + assert_eq!(input.pr_set, "examples/pr-set/cross-repo-feature.yaml"); + assert!(input.parallelism.is_none()); + assert!(input.timeout_secs.is_none()); + } + + #[test] + fn pr_review_set_response_serialises() { + let resp = PrReviewSetResponse { + multi_pr_delta_report: serde_json::json!({"schema_version": 0}), + threshold_exceeded: true, + }; + let json = serde_json::to_value(&resp).unwrap(); + assert!(json.get("multi_pr_delta_report").is_some()); + assert_eq!(json.get("threshold_exceeded").unwrap(), true); + } + + #[test] + fn manifest_path_for_workspace_resolves_relative_paths() { + let workspace = std::path::Path::new("/tmp/workspace"); + + assert_eq!( + manifest_path_for_workspace(workspace, "sets/review.yaml"), + std::path::PathBuf::from("/tmp/workspace/sets/review.yaml") + ); + } + #[test] fn effective_timeout_applies_default_when_unspecified() { assert_eq!(effective_timeout(None).as_secs(), PR_REVIEW_TIMEOUT_SECS); diff --git a/examples/pr-set/cross-repo-feature.yaml b/examples/pr-set/cross-repo-feature.yaml new file mode 100644 index 00000000..c95b6c47 --- /dev/null +++ b/examples/pr-set/cross-repo-feature.yaml @@ -0,0 +1,25 @@ +version: 0 +id: checkout-refresh +title: Checkout refresh cross-repo set +prs: + - id: api-contract-42 + repo: api-service + base: main + head: feature/checkout-contract + pr: 42 + depends_on: [] + - id: gateway-adapter-77 + repo: edge-gateway + base: main + head: feature/checkout-adapter + pr: 77 + depends_on: + - api-contract-42 + - id: ui-hookup-108 + repo: web-client + base: main + head: feature/checkout-ui + pr: 108 + depends_on: + - api-contract-42 + - gateway-adapter-77 diff --git a/examples/pr-set/divergent-bases.yaml b/examples/pr-set/divergent-bases.yaml index 6f37d258..c3598b22 100644 --- a/examples/pr-set/divergent-bases.yaml +++ b/examples/pr-set/divergent-bases.yaml @@ -3,13 +3,13 @@ id: divergent-bases title: Divergent base branches prs: - id: backend-contract - repo: label-review + repo: api-service base: main head: feature/backend-contract pr: 1201 depends_on: [] - id: frontend-release - repo: web-frontend + repo: web-client base: release/2026.05 head: feature/frontend-release-hookup pr: 1202 diff --git a/examples/pr-set/reg-13863.yaml b/examples/pr-set/reg-13863.yaml deleted file mode 100644 index 15ac7825..00000000 --- a/examples/pr-set/reg-13863.yaml +++ /dev/null @@ -1,25 +0,0 @@ -version: 0 -id: REG-13863 -title: Edit Label Project cross-stack -prs: - - id: label-review-502 - repo: label-review - base: main - head: REG-13863-edit-label-project-details - pr: 502 - depends_on: [] - - id: web-api-gateway-1090 - repo: web-api-gateway - base: main - head: REG-13863-edit-label-project-details - pr: 1090 - depends_on: - - label-review-502 - - id: web-frontend-8035 - repo: web-frontend - base: main - head: REG-13863-edit-label-project-details - pr: 8035 - depends_on: - - label-review-502 - - web-api-gateway-1090 diff --git a/examples/pr-set/stacked-in-one-repo.yaml b/examples/pr-set/stacked-in-one-repo.yaml index b560d2f1..80e6ab34 100644 --- a/examples/pr-set/stacked-in-one-repo.yaml +++ b/examples/pr-set/stacked-in-one-repo.yaml @@ -3,13 +3,13 @@ id: stacked-in-one-repo title: Stacked API cleanup prs: - id: api-foundation - repo: web-api-gateway + repo: api-service base: main head: feature/api-foundation pr: 1101 depends_on: [] - id: api-ui-hookup - repo: web-api-gateway + repo: api-service base: feature/api-foundation head: feature/api-ui-hookup pr: 1102 diff --git a/website/src/content/docs/guides/pr-review.md b/website/src/content/docs/guides/pr-review.md index af864b56..c46c7e20 100644 --- a/website/src/content/docs/guides/pr-review.md +++ b/website/src/content/docs/guides/pr-review.md @@ -35,6 +35,80 @@ The command resolves both refs to SHAs, expands the set of affected repos from the changed files, indexes the head branch into a disposable storage location, and prints the delta report. +### Review a child repo with a parent workspace config + +When the Git branch belongs to a child repo but the `gather-step.config.yaml` +lives at the parent workspace, run from the parent and point `--workspace` at +the child repo while passing the parent config: + +```bash +gather-step --workspace /path/to/workspace/web-client pr-review \ + --base main \ + --head feature/checkout-ui \ + --config /path/to/workspace/gather-step.config.yaml +``` + +The review rewrites the matching repo entry inside the temporary worktree so +the child repo indexes as `.`. The parent workspace's `.gather-step` baseline +remains the comparison source; no duplicate config is required inside the child +repo. + +### Review a related PR set + +Create a manifest that names each PR and its dependency order: + +```yaml +version: 0 +id: checkout-refresh +title: Checkout refresh cross-repo set +prs: + - id: api-contract-42 + repo: api-service + base: main + head: feature/checkout-contract + pr: 42 + depends_on: [] + - id: ui-hookup-108 + repo: web-client + base: main + head: feature/checkout-ui + pr: 108 + depends_on: + - api-contract-42 +``` + +Then run: + +```bash +gather-step --workspace /path/to/workspace pr-review \ + --pr-set examples/pr-set/cross-repo-feature.yaml \ + --parallelism 2 +``` + +Entries with satisfied dependencies can run in parallel. If one entry fails, +dependent entries are skipped and the final `MultiPrDeltaReport` still includes +the completed PRs, failed/skipped entries, and cross-PR payload-contract drift. + +You can also generate a draft manifest from GitHub search results: + +```bash +gather-step --workspace /path/to/workspace pr-review init-set \ + --query "checkout refresh is:open" \ + --output checkout-refresh-pr-set.yaml +``` + +Or resolve and run immediately: + +```bash +gather-step --workspace /path/to/workspace pr-review \ + --from-gh "checkout refresh is:open" \ + --set-id checkout-refresh \ + --parallelism 2 +``` + +`--from-gh` writes the resolved manifest under the review cache before running, +so the exact PR set remains inspectable after the command completes. + ### With JSON output ```bash @@ -167,6 +241,10 @@ The tool accepts the same parameters as the CLI (`base`, `head`, `keep_cache`, that supports the stdio transport. See [Connect an MCP Client](/guides/mcp-clients/) for setup. +For coordinated sets, MCP clients can call `pr_review_set` with a `pr_set` +manifest path. It returns the same `MultiPrDeltaReport` JSON as the CLI +`--pr-set` mode. + **Clients known to work with `pr_review`:** - Claude Code (stdio MCP, any version with tool-call support) diff --git a/website/src/content/docs/reference/cli.md b/website/src/content/docs/reference/cli.md index 9a69e654..c8d959b9 100644 --- a/website/src/content/docs/reference/cli.md +++ b/website/src/content/docs/reference/cli.md @@ -850,14 +850,29 @@ The `deployment` surface captures changes to deployment topology: added, removed ```bash gather-step [GLOBAL FLAGS] pr-review --base --head [--engine temp-index] \ [--config ] [--severity ] [--format ] [--keep-cache] [--no-baseline-check] + +gather-step [GLOBAL FLAGS] pr-review --pr-set [--parallelism ] [--set-id ] \ + [--engine temp-index] [--config ] [--severity ] [--format ] [--keep-cache] + +gather-step [GLOBAL FLAGS] pr-review --from-gh [--parallelism ] [--set-id ] \ + [--allow-unknown-repos] [--engine temp-index] [--config ] [--severity ] \ + [--format ] [--keep-cache] + +gather-step [GLOBAL FLAGS] pr-review init-set --query [--output ] \ + [--set-id ] [--allow-unknown-repos] ``` | Flag | Type | Default | Description | |---|---|---|---| | `--base ` | string | required | Base ref (branch, tag, SHA, or any git rev). | | `--head ` | string | required | Head ref (branch, tag, SHA, `HEAD`, etc.). | +| `--pr-set ` | path | — | Run a coordinated multi-PR review from a PR-set manifest instead of a single `--base` / `--head` pair. Conflicts with `--base` and `--head`. | +| `--from-gh ` | string | — | Resolve a PR-set manifest from GitHub search results using `gh pr list`, write the manifest under the review cache, then run it. Conflicts with `--base`, `--head`, and `--pr-set`. | +| `--parallelism ` | integer | `1` | Number of independent PR-set entries to review in parallel. Dependency levels still run in order. | +| `--set-id ` | string | manifest id | Override the manifest id in the emitted `MultiPrDeltaReport`. | +| `--allow-unknown-repos` | bool flag | false | With `--from-gh`, include PRs whose GitHub repo is not listed in the workspace config. By default the resolver filters to configured repos. | | `--engine ` | enum | `temp-index` | Engine to use for the review index. `temp-index` builds a full isolated index. This flag is retained for forward-compatible engine selection; no alternate public engine is currently exposed. | -| `--config ` | path | — | Path to a `gather-step.config.yaml` to use for this review run. The temp-index requires a config at the worktree root; by default the worktree is checked out at `--head`, so a config that is committed in that ref is picked up automatically. Pass `--config` when the workspace does not have a checked-in config (e.g. during bootstrap), or to override the committed one for a single run. The bytes of this file feed into the cache-key fingerprint, so different `--config` values produce distinct cache entries. | +| `--config ` | path | — | Path to a `gather-step.config.yaml` to use for this review run. The temp-index requires a config at the worktree root; by default the worktree is checked out at `--head`, so a config that is committed in that ref is picked up automatically. Pass `--config` when the reviewed git repo does not commit its own config, or to override the committed one for a run. When `--workspace` points at a child repo and `--config` points at a parent workspace config, the matching repo entry is rewritten to `path: "."` inside the temporary worktree. The original config bytes feed into the cache-key fingerprint, so different `--config` values produce distinct cache entries. | | `--severity ` | enum | `warn` | `warn` always exits 0. `strict` exits 2 on High risks or incompatible payload type changes. `pedantic` exits 2 on any Medium+ risk, any payload change, or removed permission decorators. | | `--format ` | enum | `markdown` | `markdown` emits a human-readable Markdown report. `json` emits compact machine-readable JSON. `github-comment` emits Markdown truncated to GitHub's 65 536-char comment limit. `braingent` emits Markdown with YAML frontmatter for [Braingent](https://braingent.dev) archival. | | `--keep-cache` | bool flag | false | Keep the review artifact directory after the run, including failed runs (failed artifacts are marked `Quarantined` so `pr-review clean` can find them). Cache hits are available when a retained matching artifact already exists. | @@ -899,6 +914,27 @@ gather-step --workspace /path/to/workspace pr-review \ --base main --head feature/my-branch \ --json --keep-cache +# Review a child repo using the parent workspace config +gather-step --workspace /path/to/workspace/web-client pr-review \ + --base main --head feature/checkout-ui \ + --config /path/to/workspace/gather-step.config.yaml + +# Review a coordinated PR set +gather-step --workspace /path/to/workspace pr-review \ + --pr-set examples/pr-set/cross-repo-feature.yaml \ + --parallelism 2 + +# Generate a draft PR-set manifest from GitHub search results +gather-step --workspace /path/to/workspace pr-review init-set \ + --query "checkout refresh is:open" \ + --output checkout-refresh-pr-set.yaml + +# Resolve a GitHub PR set and run it immediately +gather-step --workspace /path/to/workspace pr-review \ + --from-gh "checkout refresh is:open" \ + --set-id checkout-refresh \ + --parallelism 2 + # List all kept artifacts (dry run) gather-step --workspace /path/to/workspace pr-review clean --dry-run diff --git a/website/src/content/docs/reference/mcp-tools.md b/website/src/content/docs/reference/mcp-tools.md index ccfed7b0..83056bde 100644 --- a/website/src/content/docs/reference/mcp-tools.md +++ b/website/src/content/docs/reference/mcp-tools.md @@ -194,6 +194,33 @@ Used automatically when the user asks to review a pull request, do a structural **Implementation note.** The MCP tool shells out to the `gather-step` binary's `pr-review` subcommand. The binary must be on PATH or in the same directory as the MCP server. +### `pr_review_set` + +> "Review this related PR set using gather-step." + +Used automatically when the user asks to review multiple related pull requests, +a PR stack, or a cross-repo PR set. It shells out to +`gather-step pr-review --pr-set --format json`. + +**Inputs.** + +| Field | Type | Required | Description | +| --- | --- | --- | --- | +| `pr_set` | string | yes | Path to a PR-set manifest, absolute or relative to the workspace root. | +| `set_id` | string | no | Override the manifest id in the emitted report. | +| `parallelism` | integer | no | Number of independent entries to review in parallel. Dependency levels still run in order. | +| `keep_cache` | bool | no | Preserve each child review artifact for follow-up queries. | +| `severity` | string | no | One of `warn` (default), `strict`, `pedantic`. | +| `timeout_secs` | integer | no | Child-process timeout in seconds, capped by the server. | + +**Returns.** A JSON `MultiPrDeltaReport` (`schema_version: 0`) with: + +- `metadata` — set id, manifest version/path, completed/failed/skipped counts, and set fingerprint. +- `prs` — each completed child `DeltaReport` with entry id, repo, PR number, and base/head. +- `errors` — failed and dependency-skipped entries. +- `cross_pr.contract_drifts` — producer payload-contract changes in the set that lack a matching consumer PR. +- `threshold_exceeded` — true when any completed child review crossed the requested severity mode. + ## Contracts ### `payload_schema` From 83737d5bd4dfcea9bd04d2f08d54fe065ba5a9aa Mon Sep 17 00:00:00 2001 From: thedoublejay Date: Wed, 13 May 2026 14:15:07 +0800 Subject: [PATCH 3/5] docs: prepare 4.1.0 pr-review docs --- .../gather-step-cli/src/commands/pr_review.rs | 2 +- crates/gather-step-mcp/src/catalog.rs | 2 +- crates/gather-step-mcp/src/server.rs | 11 +- crates/gather-step-mcp/src/tools/pr_review.rs | 204 ++++++++++++++++-- website/src/content/docs/changelog.md | 25 +++ website/src/content/docs/guides/pr-review.md | 19 +- website/src/content/docs/reference/cli.md | 11 +- .../src/content/docs/reference/mcp-tools.md | 16 +- 8 files changed, 251 insertions(+), 39 deletions(-) diff --git a/crates/gather-step-cli/src/commands/pr_review.rs b/crates/gather-step-cli/src/commands/pr_review.rs index d9192066..5d00ffa2 100644 --- a/crates/gather-step-cli/src/commands/pr_review.rs +++ b/crates/gather-step-cli/src/commands/pr_review.rs @@ -122,7 +122,7 @@ pub struct PrReviewArgs { /// Override the OS cache root used for review artifacts. /// Useful for CI and tests. - #[arg(long, value_name = "PATH", hide = true)] + #[arg(long, value_name = "PATH")] pub cache_root: Option, /// Path to a `gather-step.config.yaml` to use for the review run. diff --git a/crates/gather-step-mcp/src/catalog.rs b/crates/gather-step-mcp/src/catalog.rs index 8d8845bd..1ae4c72a 100644 --- a/crates/gather-step-mcp/src/catalog.rs +++ b/crates/gather-step-mcp/src/catalog.rs @@ -110,6 +110,6 @@ pub const MCP_TOOLS: &[(&str, &str)] = &[ ), ( "pr_review_set", - "Build coordinated disposable review indexes for a PR-set manifest", + "Build coordinated disposable review indexes from a PR-set manifest or GitHub query", ), ]; diff --git a/crates/gather-step-mcp/src/server.rs b/crates/gather-step-mcp/src/server.rs index 848f2b51..c93093cd 100644 --- a/crates/gather-step-mcp/src/server.rs +++ b/crates/gather-step-mcp/src/server.rs @@ -1019,6 +1019,8 @@ impl GatherStepMcpServer { Builds a disposable review index for the PR head, diffs it against the workspace baseline, \ and returns a structured DeltaReport (routes, symbols, payload contracts, events, decorators, \ contract alignments, removed-surface risks, deployment topology, and impact summaries). \ + Accepts the same review controls as the CLI for parent workspace config files, review cache roots, \ + cache retention, severity mode, and baseline mismatch suppression. \ The workspace storage is never mutated — the review runs in a separate disposable artifact root. \ Requires the `gather-step` binary to be on PATH or in the same directory as the MCP server. \ First runs take ~30-90 seconds because a fresh review index is built; cache-hit runs against \ @@ -1040,9 +1042,12 @@ impl GatherStepMcpServer { #[tool( name = "pr_review_set", description = "Use this tool when the user asks to review multiple related pull requests, \ - a PR stack, or a cross-repo PR set with gather-step. Accepts a PR-set manifest path, \ - builds one disposable review per manifest entry, preserves dependency ordering, and \ - returns a MultiPrDeltaReport with per-PR results and cross-PR payload-contract drift.", + a PR stack, or a cross-repo PR set with gather-step. Accepts either a PR-set manifest path \ + or a GitHub search query to resolve with `gh pr list`, builds one disposable review per entry, \ + preserves dependency ordering, and returns a MultiPrDeltaReport with per-PR results and \ + cross-PR payload-contract drift. Accepts the same review controls as the CLI for parent \ + workspace config files, review cache roots, cache retention, severity mode, baseline mismatch \ + suppression, set id override, parallelism, and unknown-repo handling for GitHub-resolved sets.", annotations(read_only_hint = true) )] pub async fn pr_review_set_tool( diff --git a/crates/gather-step-mcp/src/tools/pr_review.rs b/crates/gather-step-mcp/src/tools/pr_review.rs index 43fdf4ac..46f5b980 100644 --- a/crates/gather-step-mcp/src/tools/pr_review.rs +++ b/crates/gather-step-mcp/src/tools/pr_review.rs @@ -69,12 +69,22 @@ pub struct PrReviewInput { pub base: String, /// Head ref (branch, tag, SHA, "HEAD", …). pub head: String, + /// Path to a gather-step config file, absolute or relative to the workspace root. + #[serde(default)] + pub config: Option, + /// Override the OS cache root used for review artifacts, absolute or relative + /// to the workspace root. + #[serde(default)] + pub cache_root: Option, /// Keep the review artifact after the run. #[serde(default)] pub keep_cache: Option, /// Severity mode: `"warn"` (default) | `"strict"` | `"pedantic"`. #[serde(default)] pub severity: Option, + /// Skip the warning when the workspace HEAD does not match `base`. + #[serde(default)] + pub no_baseline_check: Option, /// Override the wall-clock timeout in seconds. Capped at /// [`PR_REVIEW_TIMEOUT_MAX_SECS`]. #[serde(default)] @@ -85,19 +95,39 @@ pub struct PrReviewInput { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)] pub struct PrReviewSetInput { /// Path to a PR-set manifest, absolute or relative to the workspace root. - pub pr_set: String, + /// Required unless `from_gh` is set. + #[serde(default)] + pub pr_set: Option, + /// Resolve a PR-set manifest from GitHub search results and run it. + /// Required unless `pr_set` is set. + #[serde(default)] + pub from_gh: Option, /// Override the manifest set id in the emitted report. #[serde(default)] pub set_id: Option, /// Number of independent entries to review in parallel. #[serde(default)] pub parallelism: Option, + /// Include GitHub PRs whose repo is not listed in the workspace config + /// when resolving `from_gh`. + #[serde(default)] + pub allow_unknown_repos: Option, + /// Path to a gather-step config file, absolute or relative to the workspace root. + #[serde(default)] + pub config: Option, + /// Override the OS cache root used for review artifacts, absolute or relative + /// to the workspace root. + #[serde(default)] + pub cache_root: Option, /// Keep review artifacts after the run. #[serde(default)] pub keep_cache: Option, /// Severity mode: `"warn"` (default) | `"strict"` | `"pedantic"`. #[serde(default)] pub severity: Option, + /// Skip the warning when the workspace HEAD does not match each entry's base. + #[serde(default)] + pub no_baseline_check: Option, /// Override the wall-clock timeout in seconds. Capped at /// [`PR_REVIEW_TIMEOUT_MAX_SECS`]. #[serde(default)] @@ -143,12 +173,15 @@ pub fn run_pr_review( cmd.arg("--base").arg(&input.base); cmd.arg("--head").arg(&input.head); cmd.arg("--format").arg("json"); - if input.keep_cache.unwrap_or(false) { - cmd.arg("--keep-cache"); - } - if let Some(sev) = &input.severity { - cmd.arg("--severity").arg(sev); - } + append_common_pr_review_flags( + &mut cmd, + workspace, + input.config.as_deref(), + input.cache_root.as_deref(), + input.keep_cache.unwrap_or(false), + input.severity.as_deref(), + input.no_baseline_check.unwrap_or(false), + ); let (delta_report, threshold_exceeded) = run_pr_review_json_child(cmd, &binary, effective_timeout(input.timeout_secs))?; @@ -159,18 +192,65 @@ pub fn run_pr_review( }) } +fn append_common_pr_review_flags( + cmd: &mut Command, + workspace: &std::path::Path, + config: Option<&str>, + cache_root: Option<&str>, + keep_cache: bool, + severity: Option<&str>, + no_baseline_check: bool, +) { + if let Some(config) = config { + cmd.arg("--config") + .arg(path_for_workspace(workspace, config)); + } + if let Some(cache_root) = cache_root { + cmd.arg("--cache-root") + .arg(path_for_workspace(workspace, cache_root)); + } + if keep_cache { + cmd.arg("--keep-cache"); + } + if let Some(sev) = severity { + cmd.arg("--severity").arg(sev); + } + if no_baseline_check { + cmd.arg("--no-baseline-check"); + } +} + /// Run the coordinated PR-set review pipeline via the `gather-step` CLI. pub fn run_pr_review_set( workspace: &std::path::Path, input: &PrReviewSetInput, ) -> Result { let binary = resolve_binary(); - let manifest_path = manifest_path_for_workspace(workspace, &input.pr_set); let mut cmd = Command::new(&binary); cmd.arg("--workspace").arg(workspace); cmd.arg("pr-review"); - cmd.arg("--pr-set").arg(manifest_path); + match (input.pr_set.as_deref(), input.from_gh.as_deref()) { + (Some(_), Some(_)) => { + return Err("pr_review_set accepts exactly one of `pr_set` or `from_gh`.".to_owned()); + } + (Some(pr_set), None) => { + if input.allow_unknown_repos.unwrap_or(false) { + return Err("`allow_unknown_repos` is only valid when `from_gh` is set.".to_owned()); + } + cmd.arg("--pr-set") + .arg(path_for_workspace(workspace, pr_set)); + } + (None, Some(from_gh)) => { + cmd.arg("--from-gh").arg(from_gh); + if input.allow_unknown_repos.unwrap_or(false) { + cmd.arg("--allow-unknown-repos"); + } + } + (None, None) => { + return Err("pr_review_set requires either `pr_set` or `from_gh`.".to_owned()); + } + } cmd.arg("--format").arg("json"); if let Some(set_id) = &input.set_id { cmd.arg("--set-id").arg(set_id); @@ -178,12 +258,15 @@ pub fn run_pr_review_set( if let Some(parallelism) = input.parallelism { cmd.arg("--parallelism").arg(parallelism.to_string()); } - if input.keep_cache.unwrap_or(false) { - cmd.arg("--keep-cache"); - } - if let Some(sev) = &input.severity { - cmd.arg("--severity").arg(sev); - } + append_common_pr_review_flags( + &mut cmd, + workspace, + input.config.as_deref(), + input.cache_root.as_deref(), + input.keep_cache.unwrap_or(false), + input.severity.as_deref(), + input.no_baseline_check.unwrap_or(false), + ); let (multi_pr_delta_report, threshold_exceeded) = run_pr_review_json_child(cmd, &binary, effective_timeout(input.timeout_secs))?; @@ -285,7 +368,7 @@ fn resolve_binary() -> std::path::PathBuf { std::path::PathBuf::from("gather-step") } -fn manifest_path_for_workspace(workspace: &std::path::Path, value: &str) -> std::path::PathBuf { +fn path_for_workspace(workspace: &std::path::Path, value: &str) -> std::path::PathBuf { let path = std::path::PathBuf::from(value); if path.is_absolute() { path @@ -407,8 +490,11 @@ mod tests { let input: PrReviewInput = serde_json::from_str(json).unwrap(); assert_eq!(input.base, "main"); assert_eq!(input.head, "HEAD"); + assert!(input.config.is_none()); + assert!(input.cache_root.is_none()); assert!(input.keep_cache.is_none()); assert!(input.severity.is_none()); + assert!(input.no_baseline_check.is_none()); assert!(input.timeout_secs.is_none()); } @@ -419,14 +505,20 @@ mod tests { let json = r#"{ "base": "v1.0.0", "head": "feat/my-feature", + "config": "gather-step.config.yaml", + "cache_root": ".cache/pr-review", "keep_cache": true, "severity": "strict", + "no_baseline_check": true, "timeout_secs": 120 }"#; let input: PrReviewInput = serde_json::from_str(json).unwrap(); assert_eq!(input.base, "v1.0.0"); + assert_eq!(input.config.as_deref(), Some("gather-step.config.yaml")); + assert_eq!(input.cache_root.as_deref(), Some(".cache/pr-review")); assert_eq!(input.severity.as_deref(), Some("strict")); assert_eq!(input.keep_cache, Some(true)); + assert_eq!(input.no_baseline_check, Some(true)); assert_eq!(input.timeout_secs, Some(120)); } @@ -447,11 +539,43 @@ mod tests { fn pr_review_set_input_deserialises_minimal() { let json = r#"{"pr_set": "examples/pr-set/cross-repo-feature.yaml"}"#; let input: PrReviewSetInput = serde_json::from_str(json).unwrap(); - assert_eq!(input.pr_set, "examples/pr-set/cross-repo-feature.yaml"); + assert_eq!( + input.pr_set.as_deref(), + Some("examples/pr-set/cross-repo-feature.yaml") + ); + assert!(input.from_gh.is_none()); assert!(input.parallelism.is_none()); assert!(input.timeout_secs.is_none()); } + #[test] + fn pr_review_set_input_deserialises_from_gh() { + let json = r#"{ + "from_gh": "checkout refresh is:open", + "set_id": "checkout-refresh", + "parallelism": 2, + "allow_unknown_repos": true, + "config": "gather-step.config.yaml", + "cache_root": ".cache/pr-review", + "keep_cache": true, + "severity": "pedantic", + "no_baseline_check": true, + "timeout_secs": 300 + }"#; + let input: PrReviewSetInput = serde_json::from_str(json).unwrap(); + assert!(input.pr_set.is_none()); + assert_eq!(input.from_gh.as_deref(), Some("checkout refresh is:open")); + assert_eq!(input.set_id.as_deref(), Some("checkout-refresh")); + assert_eq!(input.parallelism, Some(2)); + assert_eq!(input.allow_unknown_repos, Some(true)); + assert_eq!(input.config.as_deref(), Some("gather-step.config.yaml")); + assert_eq!(input.cache_root.as_deref(), Some(".cache/pr-review")); + assert_eq!(input.keep_cache, Some(true)); + assert_eq!(input.severity.as_deref(), Some("pedantic")); + assert_eq!(input.no_baseline_check, Some(true)); + assert_eq!(input.timeout_secs, Some(300)); + } + #[test] fn pr_review_set_response_serialises() { let resp = PrReviewSetResponse { @@ -464,15 +588,57 @@ mod tests { } #[test] - fn manifest_path_for_workspace_resolves_relative_paths() { + fn path_for_workspace_resolves_relative_paths() { let workspace = std::path::Path::new("/tmp/workspace"); assert_eq!( - manifest_path_for_workspace(workspace, "sets/review.yaml"), + path_for_workspace(workspace, "sets/review.yaml"), std::path::PathBuf::from("/tmp/workspace/sets/review.yaml") ); } + #[test] + fn pr_review_set_rejects_missing_source_before_launching_child() { + let input = PrReviewSetInput { + pr_set: None, + from_gh: None, + set_id: None, + parallelism: None, + allow_unknown_repos: None, + config: None, + cache_root: None, + keep_cache: None, + severity: None, + no_baseline_check: None, + timeout_secs: None, + }; + + let err = run_pr_review_set(std::path::Path::new("/tmp/workspace"), &input) + .expect_err("missing source should be rejected before spawning gather-step"); + assert!(err.contains("requires either `pr_set` or `from_gh`")); + } + + #[test] + fn pr_review_set_rejects_ambiguous_source_before_launching_child() { + let input = PrReviewSetInput { + pr_set: Some("set.yaml".to_owned()), + from_gh: Some("checkout refresh is:open".to_owned()), + set_id: None, + parallelism: None, + allow_unknown_repos: None, + config: None, + cache_root: None, + keep_cache: None, + severity: None, + no_baseline_check: None, + timeout_secs: None, + }; + + let err = run_pr_review_set(std::path::Path::new("/tmp/workspace"), &input) + .expect_err("ambiguous source should be rejected before spawning gather-step"); + assert!(err.contains("exactly one of `pr_set` or `from_gh`")); + } + #[test] fn effective_timeout_applies_default_when_unspecified() { assert_eq!(effective_timeout(None).as_secs(), PR_REVIEW_TIMEOUT_SECS); diff --git a/website/src/content/docs/changelog.md b/website/src/content/docs/changelog.md index dfc57883..eff7baba 100644 --- a/website/src/content/docs/changelog.md +++ b/website/src/content/docs/changelog.md @@ -5,6 +5,31 @@ description: "User-visible changes to gather-step, listed by release. Updated ma This changelog lists significant user-visible changes. The latest release is shown in full at the top; earlier releases are collapsed under [Earlier releases](#earlier-releases) at the bottom of the page. +## v4.1.0 (2026-05-13) + +Release status: **unreleased**. + +Minor release on top of v4.0.5. Adds coordinated multi-PR review support so related PRs, stacks, and cross-repo feature sets can be reviewed together instead of one branch at a time. + +### Added + +- `gather-step pr-review --pr-set ` runs a coordinated review from a manifest that lists each PR's repo, base, head, PR number, and dependencies. +- `gather-step pr-review init-set --query ` generates a draft PR-set manifest from GitHub search results, and `gather-step pr-review --from-gh ` resolves and runs that set in one command. +- PR-set reviews return a `MultiPrDeltaReport` with per-PR `DeltaReport` results, failed/skipped entries, dependency-aware execution status, and cross-PR payload-contract drift. +- `pr_review_set` is now available through MCP for assistant-driven review of related PR sets. + +### Changed + +- `pr-review` can use a parent workspace `gather-step.config.yaml` while reviewing a child repo. The matching repo entry is rewritten to `path: "."` inside the temporary worktree, so the child repo no longer needs a duplicate committed config. +- Review-set execution supports `--parallelism`, `--set-id`, `--allow-unknown-repos`, `--config`, `--cache-root`, `--keep-cache`, `--severity`, and `--no-baseline-check` at the CLI surface. +- MCP `pr_review` and `pr_review_set` now expose the same config, cache-root, cache-retention, severity, baseline-check, timeout, set-id, parallelism, and GitHub-query controls that automation users need to discover from the tool schema. +- `--cache-root` is now a visible CLI option for `pr-review`, instead of a hidden automation-only flag. + +### Docs + +- Added PR-set examples for cross-repo sets, stacked PRs in one repo, and divergent-base sets. +- Expanded the PR Review guide, CLI reference, and MCP tools reference with PR-set manifests, GitHub query resolution, child-repo parent-config usage, and MCP input fields. + ## v4.0.5 (2026-05-13) Release status: **released**. diff --git a/website/src/content/docs/guides/pr-review.md b/website/src/content/docs/guides/pr-review.md index c46c7e20..2b3eb7c5 100644 --- a/website/src/content/docs/guides/pr-review.md +++ b/website/src/content/docs/guides/pr-review.md @@ -236,14 +236,17 @@ Claude Code triggers it automatically when you ask: > "Check the cross-repo impact of feat/my-change." -The tool accepts the same parameters as the CLI (`base`, `head`, `keep_cache`, -`severity`) and returns the same `DeltaReport` JSON. It works with any client -that supports the stdio transport. See [Connect an MCP Client](/guides/mcp-clients/) -for setup. - -For coordinated sets, MCP clients can call `pr_review_set` with a `pr_set` -manifest path. It returns the same `MultiPrDeltaReport` JSON as the CLI -`--pr-set` mode. +The tool accepts the same parameters as the CLI (`base`, `head`, `config`, +`cache_root`, `keep_cache`, `severity`, `no_baseline_check`, `timeout_secs`) +and returns the same `DeltaReport` JSON. It works with any client that supports +the stdio transport. See [Connect an MCP Client](/guides/mcp-clients/) for +setup. + +For coordinated sets, MCP clients can call `pr_review_set` with either a +`pr_set` manifest path or a `from_gh` search query. It also exposes `set_id`, +`parallelism`, `allow_unknown_repos`, `config`, `cache_root`, `keep_cache`, +`severity`, `no_baseline_check`, and `timeout_secs`, and returns the same +`MultiPrDeltaReport` JSON as the CLI PR-set modes. **Clients known to work with `pr_review`:** diff --git a/website/src/content/docs/reference/cli.md b/website/src/content/docs/reference/cli.md index c8d959b9..5a1b1281 100644 --- a/website/src/content/docs/reference/cli.md +++ b/website/src/content/docs/reference/cli.md @@ -849,14 +849,16 @@ The `deployment` surface captures changes to deployment topology: added, removed ```bash gather-step [GLOBAL FLAGS] pr-review --base --head [--engine temp-index] \ - [--config ] [--severity ] [--format ] [--keep-cache] [--no-baseline-check] + [--config ] [--cache-root ] [--severity ] [--format ] \ + [--keep-cache] [--no-baseline-check] gather-step [GLOBAL FLAGS] pr-review --pr-set [--parallelism ] [--set-id ] \ - [--engine temp-index] [--config ] [--severity ] [--format ] [--keep-cache] + [--engine temp-index] [--config ] [--cache-root ] [--severity ] \ + [--format ] [--keep-cache] [--no-baseline-check] gather-step [GLOBAL FLAGS] pr-review --from-gh [--parallelism ] [--set-id ] \ - [--allow-unknown-repos] [--engine temp-index] [--config ] [--severity ] \ - [--format ] [--keep-cache] + [--allow-unknown-repos] [--engine temp-index] [--config ] [--cache-root ] \ + [--severity ] [--format ] [--keep-cache] [--no-baseline-check] gather-step [GLOBAL FLAGS] pr-review init-set --query [--output ] \ [--set-id ] [--allow-unknown-repos] @@ -873,6 +875,7 @@ gather-step [GLOBAL FLAGS] pr-review init-set --query [--output ] | `--allow-unknown-repos` | bool flag | false | With `--from-gh`, include PRs whose GitHub repo is not listed in the workspace config. By default the resolver filters to configured repos. | | `--engine ` | enum | `temp-index` | Engine to use for the review index. `temp-index` builds a full isolated index. This flag is retained for forward-compatible engine selection; no alternate public engine is currently exposed. | | `--config ` | path | — | Path to a `gather-step.config.yaml` to use for this review run. The temp-index requires a config at the worktree root; by default the worktree is checked out at `--head`, so a config that is committed in that ref is picked up automatically. Pass `--config` when the reviewed git repo does not commit its own config, or to override the committed one for a run. When `--workspace` points at a child repo and `--config` points at a parent workspace config, the matching repo entry is rewritten to `path: "."` inside the temporary worktree. The original config bytes feed into the cache-key fingerprint, so different `--config` values produce distinct cache entries. | +| `--cache-root ` | path | OS cache directory | Override the cache root used for review artifacts and generated `--from-gh` manifests. Use this in CI or automation that needs review artifacts under a known directory. | | `--severity ` | enum | `warn` | `warn` always exits 0. `strict` exits 2 on High risks or incompatible payload type changes. `pedantic` exits 2 on any Medium+ risk, any payload change, or removed permission decorators. | | `--format ` | enum | `markdown` | `markdown` emits a human-readable Markdown report. `json` emits compact machine-readable JSON. `github-comment` emits Markdown truncated to GitHub's 65 536-char comment limit. `braingent` emits Markdown with YAML frontmatter for [Braingent](https://braingent.dev) archival. | | `--keep-cache` | bool flag | false | Keep the review artifact directory after the run, including failed runs (failed artifacts are marked `Quarantined` so `pr-review clean` can find them). Cache hits are available when a retained matching artifact already exists. | diff --git a/website/src/content/docs/reference/mcp-tools.md b/website/src/content/docs/reference/mcp-tools.md index 83056bde..b68862c5 100644 --- a/website/src/content/docs/reference/mcp-tools.md +++ b/website/src/content/docs/reference/mcp-tools.md @@ -163,8 +163,12 @@ Used automatically when the user asks to review a pull request, do a structural | --- | --- | --- | --- | | `base` | string | yes | Base ref (branch name, tag, or full SHA). The PR's target branch — typically `main`. | | `head` | string | yes | Head ref (branch name, tag, or full SHA). The PR's source branch. | +| `config` | string | no | Path to a `gather-step.config.yaml`, absolute or relative to the workspace root. Use this when the reviewed repo does not commit its own config, or when reviewing a child repo with a parent workspace config. | +| `cache_root` | string | no | Override the OS cache root used for review artifacts, absolute or relative to the workspace root. | | `keep_cache` | bool | no | Preserve the review artifact for follow-up `impact`/`trace`/`pack` queries. Default: `false` — the artifact is deleted after the report is returned. | | `severity` | string | no | One of `warn` (default), `strict`, `pedantic`. `strict` and `pedantic` cause non-zero exit on threshold violations; `warn` always returns the report regardless. | +| `no_baseline_check` | bool | no | Suppress the warning emitted when the workspace HEAD does not match `base`. | +| `timeout_secs` | integer | no | Child-process timeout in seconds, capped by the server. | **Returns.** A JSON `DeltaReport` (`schema_version: 1`) with these top-level sections: @@ -199,18 +203,24 @@ Used automatically when the user asks to review a pull request, do a structural > "Review this related PR set using gather-step." Used automatically when the user asks to review multiple related pull requests, -a PR stack, or a cross-repo PR set. It shells out to -`gather-step pr-review --pr-set --format json`. +a PR stack, or a cross-repo PR set. It shells out to `gather-step pr-review` +with either `--pr-set ` or `--from-gh `, and always requests +JSON output. **Inputs.** | Field | Type | Required | Description | | --- | --- | --- | --- | -| `pr_set` | string | yes | Path to a PR-set manifest, absolute or relative to the workspace root. | +| `pr_set` | string | required unless `from_gh` is set | Path to a PR-set manifest, absolute or relative to the workspace root. Mutually exclusive with `from_gh`. | +| `from_gh` | string | required unless `pr_set` is set | GitHub search query to resolve into a temporary PR-set manifest using `gh pr list`. Mutually exclusive with `pr_set`. | | `set_id` | string | no | Override the manifest id in the emitted report. | | `parallelism` | integer | no | Number of independent entries to review in parallel. Dependency levels still run in order. | +| `allow_unknown_repos` | bool | no | With `from_gh`, include PRs whose GitHub repo is not listed in the workspace config. Default: `false`. | +| `config` | string | no | Path to a `gather-step.config.yaml`, absolute or relative to the workspace root. Use this for child repos that rely on a parent workspace config. | +| `cache_root` | string | no | Override the OS cache root used for review artifacts, absolute or relative to the workspace root. | | `keep_cache` | bool | no | Preserve each child review artifact for follow-up queries. | | `severity` | string | no | One of `warn` (default), `strict`, `pedantic`. | +| `no_baseline_check` | bool | no | Suppress baseline-vs-workspace HEAD mismatch warnings for each child review. | | `timeout_secs` | integer | no | Child-process timeout in seconds, capped by the server. | **Returns.** A JSON `MultiPrDeltaReport` (`schema_version: 0`) with: From b33593af1fb719c6025bf408baf2ba43f4d1d7b1 Mon Sep 17 00:00:00 2001 From: thedoublejay Date: Wed, 13 May 2026 14:30:00 +0800 Subject: [PATCH 4/5] fix: satisfy multi-pr clippy lint --- crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs b/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs index 025a9c66..f7fff95b 100644 --- a/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs +++ b/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs @@ -174,8 +174,8 @@ fn run_level( let chunk_results = thread::scope(|scope| { let handles: Vec<_> = chunk .iter() - .cloned() .map(|entry| { + let entry = entry.clone(); let app = app.clone(); let settings = settings.clone(); scope.spawn(move || run_one(&app, &settings, entry)) From bdf91efcb58b806d585fca230f83ccb19e19cc22 Mon Sep 17 00:00:00 2001 From: thedoublejay Date: Wed, 13 May 2026 15:07:37 +0800 Subject: [PATCH 5/5] fix: address pr-review set review findings --- crates/gather-step-cli/src/commands/mod.rs | 20 ++++++ .../gather-step-cli/src/commands/pr_review.rs | 18 +++-- .../src/pr_review/delta_report.rs | 5 ++ .../src/pr_review/multi_pr/coordinator.rs | 31 ++++++-- .../src/pr_review/multi_pr/delta_report.rs | 71 +++++++++++++++++-- .../gather-step-cli/src/pr_review/parity.rs | 1 + crates/gather-step-mcp/src/tools/pr_review.rs | 12 +--- website/src/content/docs/guides/pr-review.md | 2 +- .../src/content/docs/reference/mcp-tools.md | 2 +- 9 files changed, 136 insertions(+), 26 deletions(-) diff --git a/crates/gather-step-cli/src/commands/mod.rs b/crates/gather-step-cli/src/commands/mod.rs index a31f4fb6..cd790df8 100644 --- a/crates/gather-step-cli/src/commands/mod.rs +++ b/crates/gather-step-cli/src/commands/mod.rs @@ -396,6 +396,26 @@ mod tests { } } + #[test] + fn rejects_zero_pr_review_parallelism_during_parse() { + let error = Cli::try_parse_from([ + "gather-step", + "pr-review", + "--pr-set", + "examples/pr-set/cross-repo-feature.yaml", + "--parallelism", + "0", + ]) + .expect_err("zero parallelism should be rejected by clap"); + + assert!( + error + .to_string() + .contains("--parallelism must be an integer greater than or equal to 1"), + "unexpected error for zero parallelism: {error}" + ); + } + #[test] fn parses_top_level_serve_args() { let cli = Cli::parse_from([ diff --git a/crates/gather-step-cli/src/commands/pr_review.rs b/crates/gather-step-cli/src/commands/pr_review.rs index 5d00ffa2..926aa89d 100644 --- a/crates/gather-step-cli/src/commands/pr_review.rs +++ b/crates/gather-step-cli/src/commands/pr_review.rs @@ -103,7 +103,7 @@ pub struct PrReviewArgs { pub set_id: Option, /// Number of independent PR-set entries to review in parallel. - #[arg(long, value_name = "N", default_value_t = 1)] + #[arg(long, value_name = "N", default_value_t = 1, value_parser = parse_positive_usize)] pub parallelism: usize, /// Include PRs whose GitHub repo is not listed in the workspace config @@ -329,6 +329,17 @@ pub enum ReviewEngine { TempIndex, } +fn parse_positive_usize(value: &str) -> std::result::Result { + let parsed = value + .parse::() + .map_err(|_| "--parallelism must be an integer greater than or equal to 1.".to_owned())?; + if parsed == 0 { + Err("--parallelism must be an integer greater than or equal to 1.".to_owned()) + } else { + Ok(parsed) + } +} + // ─── Handler ────────────────────────────────────────────────────────────────── pub fn run(app: &AppContext, args: PrReviewArgs) -> Result { @@ -340,9 +351,6 @@ pub fn run(app: &AppContext, args: PrReviewArgs) -> Result { run_init_set(app, &args, init_args).map(|()| 0) } None => { - if args.parallelism == 0 { - anyhow::bail!("--parallelism must be at least 1."); - } if let Some(pr_set) = args.pr_set.as_ref() { let set_args = PrSetRunArgs { manifest_path: pr_set.clone(), @@ -1690,6 +1698,7 @@ pub fn run_inner(app: &AppContext, args: &PrReviewRunArgs) -> Result<(String, bo run_id: artifact_root.run_id.clone(), cleanup_policy, cache_key, + config_hash: cache_key_struct.config_hash.clone(), }, changed_files: changed_files_display, changed_files_truncated, @@ -4939,6 +4948,7 @@ indexing: run_id: "test-run".to_owned(), cleanup_policy: CleanupPolicy::RemoveOnExit, cache_key: "hash:aaa:bbb".to_owned(), + config_hash: "cfg".to_owned(), }, changed_files: vec![], changed_files_truncated: false, diff --git a/crates/gather-step-cli/src/pr_review/delta_report.rs b/crates/gather-step-cli/src/pr_review/delta_report.rs index 1cb9c713..1798f015 100644 --- a/crates/gather-step-cli/src/pr_review/delta_report.rs +++ b/crates/gather-step-cli/src/pr_review/delta_report.rs @@ -631,6 +631,9 @@ pub struct SafetyMetadata { /// Composite key that uniquely identifies this review state. /// Format: `"::"`. pub cache_key: String, + /// First 16 hex chars of blake3(`gather-step.config.yaml` bytes) used by + /// the review cache identity. + pub config_hash: String, } /// Whether the review artifact root is kept or removed after the run. @@ -2436,6 +2439,7 @@ mod tests { run_id: "test-run".to_owned(), cleanup_policy: CleanupPolicy::RemoveOnExit, cache_key: "hash:aaa:bbb".to_owned(), + config_hash: "cfg".to_owned(), }, changed_files: vec![], changed_files_truncated: false, @@ -2908,6 +2912,7 @@ mod tests { run_id: "test-run-full".to_owned(), cleanup_policy: CleanupPolicy::KeepCache, cache_key: "hash:aaa:bbb".to_owned(), + config_hash: "cfg".to_owned(), }, changed_files: vec!["backend/src/routes.ts".to_owned()], changed_files_truncated: false, diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs b/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs index f7fff95b..73d07749 100644 --- a/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs +++ b/crates/gather-step-cli/src/pr_review/multi_pr/coordinator.rs @@ -79,10 +79,7 @@ pub fn run_pr_set(app: &AppContext, args: &PrSetRunArgs) -> Result<(String, bool blocked_ids.insert(entry.id.clone()); errors.push(skipped_entry( &entry, - format!( - "Skipped because dependency review did not complete successfully: {}.", - blocked_by.join(", ") - ), + skipped_dependency_message(&entry, &blocked_by), )); } } @@ -294,6 +291,18 @@ fn skipped_entry(entry: &PrEntry, message: String) -> ErroredPrReview { } } +fn skipped_dependency_message(entry: &PrEntry, blocked_by: &[String]) -> String { + let dependency_ids = if blocked_by.is_empty() { + &entry.depends_on + } else { + blocked_by + }; + format!( + "Skipped because dependency review did not complete successfully: {}.", + dependency_ids.join(", ") + ) +} + fn dependency_levels(manifest: &PrSetManifest) -> Vec> { let mut entries: BTreeMap = manifest .prs @@ -335,7 +344,7 @@ fn dependency_levels(manifest: &PrSetManifest) -> Vec> { #[cfg(test)] mod tests { - use super::{dependency_levels, resolve_entry_workspace}; + use super::{dependency_levels, resolve_entry_workspace, skipped_dependency_message}; use crate::pr_review::multi_pr::manifest::{PrEntry, PrSetManifest}; fn entry(id: &str, depends_on: Vec<&str>) -> PrEntry { @@ -380,4 +389,16 @@ mod tests { assert!(resolve_entry_workspace(temp.path(), "api").is_none()); } + + #[test] + fn skipped_dependency_message_falls_back_to_all_dependencies() { + let entry = entry("web", vec!["api", "contracts"]); + + let message = skipped_dependency_message(&entry, &[]); + + assert_eq!( + message, + "Skipped because dependency review did not complete successfully: api, contracts." + ); + } } diff --git a/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs b/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs index 7ca0f587..72dfc674 100644 --- a/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs +++ b/crates/gather-step-cli/src/pr_review/multi_pr/delta_report.rs @@ -72,6 +72,15 @@ pub enum PrReviewSetEntryStatus { Skipped, } +impl PrReviewSetEntryStatus { + pub fn as_str(self) -> &'static str { + match self { + Self::Failed => "failed", + Self::Skipped => "skipped", + } + } +} + #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct CrossPrFindings { pub contract_drifts: Vec, @@ -242,8 +251,13 @@ impl MultiPrDeltaReport { .map_or_else(|| error.id.clone(), |pr| format!("#{pr}")); let _ = writeln!( buf, - "| {:?} | {} | `{}` | `{}`..`{}` | {} |", - error.status, pr, error.repo, error.base, error.head, error.message + "| {} | {} | `{}` | `{}`..`{}` | {} |", + error.status.as_str(), + pr, + error.repo, + error.base, + error.head, + error.message ); } } @@ -506,7 +520,13 @@ fn set_fingerprint_for_report(manifest: &PrSetManifest, prs: &[PerPrDeltaReport] .and_then(Value::as_str) .unwrap_or(pr.head.as_str()) .to_owned(), - config_hash: String::new(), + config_hash: pr + .delta_report + .get("safety") + .and_then(|safety| safety.get("config_hash")) + .and_then(Value::as_str) + .unwrap_or_default() + .to_owned(), gather_step_version: env!("CARGO_PKG_VERSION").to_owned(), }) .collect(); @@ -546,6 +566,10 @@ mod tests { use crate::pr_review::multi_pr::manifest::{PrEntry, PrSetManifest}; fn pr(id: &str, side: &str) -> PerPrDeltaReport { + pr_with_config_hash(id, side, "cfg") + } + + fn pr_with_config_hash(id: &str, side: &str, config_hash: &str) -> PerPrDeltaReport { PerPrDeltaReport { id: id.to_owned(), repo: id.to_owned(), @@ -559,7 +583,8 @@ mod tests { "head_sha": "2222222222222222222222222222222222222222" }, "safety": { - "cache_key": "workspace:base:head" + "cache_key": "workspace:base:head", + "config_hash": config_hash }, "changed_files": ["src/app.ts"], "routes": { "added": [], "removed": [], "changed": [] }, @@ -644,6 +669,42 @@ mod tests { assert_eq!(report.metadata.completed_prs, 1); assert_eq!(report.metadata.failed_prs, 0); assert_eq!(report.metadata.skipped_prs, 1); - assert!(report.render_markdown().contains("Cross-PR findings")); + let markdown = report.render_markdown(); + assert!(markdown.contains("Cross-PR findings")); + assert!( + markdown.contains("| skipped | #2 |"), + "markdown should use the stable serialized status label: {markdown}" + ); + } + + #[test] + fn set_fingerprint_includes_child_config_hash() { + let manifest = PrSetManifest { + version: 0, + id: "checkout-refresh".to_owned(), + title: None, + prs: vec![PrEntry { + id: "api".to_owned(), + repo: "api".to_owned(), + base: "main".to_owned(), + head: "feature/api".to_owned(), + pr: Some(1), + depends_on: vec![], + }], + }; + + let first = super::set_fingerprint_for_report( + &manifest, + &[pr_with_config_hash("api", "producer", "cfg-a")], + ); + let second = super::set_fingerprint_for_report( + &manifest, + &[pr_with_config_hash("api", "producer", "cfg-b")], + ); + + assert_ne!( + first, second, + "set fingerprint must change when a child review config hash changes" + ); } } diff --git a/crates/gather-step-cli/src/pr_review/parity.rs b/crates/gather-step-cli/src/pr_review/parity.rs index 99eead2b..2ded30a7 100644 --- a/crates/gather-step-cli/src/pr_review/parity.rs +++ b/crates/gather-step-cli/src/pr_review/parity.rs @@ -409,6 +409,7 @@ mod tests { run_id: "test-run".to_owned(), cleanup_policy: CleanupPolicy::RemoveOnExit, cache_key: "hash:aaa:bbb".to_owned(), + config_hash: "cfg".to_owned(), }, changed_files: vec![], changed_files_truncated: false, diff --git a/crates/gather-step-mcp/src/tools/pr_review.rs b/crates/gather-step-mcp/src/tools/pr_review.rs index 46f5b980..fb551791 100644 --- a/crates/gather-step-mcp/src/tools/pr_review.rs +++ b/crates/gather-step-mcp/src/tools/pr_review.rs @@ -34,7 +34,6 @@ use std::io::Read; use std::process::{Command, Stdio}; -use std::sync::mpsc; use std::thread; use std::time::{Duration, Instant}; @@ -428,19 +427,12 @@ fn wait_with_timeout( child: &mut std::process::Child, timeout: Duration, ) -> Result { - let (tx, rx) = mpsc::channel::>(); - // We cannot move `child` into the waiter thread (we still need its - // pipes and `kill` handle on this thread), so the waiter polls - // `try_wait` from its own thread. A short cadence keeps the timeout - // honest without busy-spinning. + // A short polling cadence keeps the timeout honest without busy-spinning. let pid = child.id(); let deadline = Instant::now() + timeout; loop { match child.try_wait() { - Ok(Some(status)) => { - let _ = (tx, rx, pid); - return Ok(status); - } + Ok(Some(status)) => return Ok(status), Ok(None) => { if Instant::now() >= deadline { let _ = child.kill(); diff --git a/website/src/content/docs/guides/pr-review.md b/website/src/content/docs/guides/pr-review.md index 2b3eb7c5..1380fdaa 100644 --- a/website/src/content/docs/guides/pr-review.md +++ b/website/src/content/docs/guides/pr-review.md @@ -162,7 +162,7 @@ in that surface category. | Section | What it shows | |---|---| | `metadata` | Base/head SHAs, checkout mode, indexed repos, elapsed time, warnings | -| `safety` | Review storage path, run ID, cleanup policy, cache key | +| `safety` | Review storage path, run ID, cleanup policy, cache key, config hash | | `changed_files` | Repo-relative paths changed in `merge_base..head` | | `evidence` | Canonical evidence rows with closed kind/source enums and structured citations | | `routes` | Added / removed / changed HTTP routes. Removed routes carry downstream impact summaries. | diff --git a/website/src/content/docs/reference/mcp-tools.md b/website/src/content/docs/reference/mcp-tools.md index b68862c5..7f349900 100644 --- a/website/src/content/docs/reference/mcp-tools.md +++ b/website/src/content/docs/reference/mcp-tools.md @@ -173,7 +173,7 @@ Used automatically when the user asks to review a pull request, do a structural **Returns.** A JSON `DeltaReport` (`schema_version: 1`) with these top-level sections: - `metadata` — base/head SHAs, checkout mode, indexed repos, elapsed time, warnings (e.g., baseline-vs-base mismatch). -- `safety` — review storage path, run id, cleanup policy, cache key. +- `safety` — review storage path, run id, cleanup policy, cache key, config hash. - `changed_files` — list of repo-relative paths changed in `merge_base..head`. - `evidence` — canonical evidence rows computed from the typed delta surfaces at query time. - `routes` — added / removed / changed HTTP routes by `(method, canonical_path)`. Carry handler info via `Serves` edges and downstream impact summaries.