fix: address pr-review set review findings

thedoublejay · thedoublejay · commit bdf91efcb58b · 2026-05-13T15:07:37.000+08:00
diff --git 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
@@ -103,7 +103,7 @@ pub struct PrReviewArgs {
     pub set_id: Option<String>,
 
     /// 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<usize, String> {
+    let parsed = value
+        .parse::<usize>()
+        .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<u8> {
@@ -340,9 +351,6 @@ pub fn run(app: &AppContext, args: PrReviewArgs) -> Result<u8> {
             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
@@ -631,6 +631,9 @@ pub struct SafetyMetadata {
     /// Composite key that uniquely identifies this review state.
     /// Format: `"<workspace_hash>:<base_sha>:<head_sha>"`.
     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
@@ -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<Vec<PrEntry>> {
     let mut entries: BTreeMap<String, PrEntry> = manifest
         .prs
@@ -335,7 +344,7 @@ fn dependency_levels(manifest: &PrSetManifest) -> Vec<Vec<PrEntry>> {
 
 #[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
@@ -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<CrossPrContractDrift>,
@@ -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
@@ -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
@@ -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<std::process::ExitStatus, String> {
-    let (tx, rx) = mpsc::channel::<Result<std::process::ExitStatus, std::io::Error>>();
-    // 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
@@ -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
@@ -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.
