diff --git a/AGENTS.md b/AGENTS.md index 8cdfa01c..2c815c47 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,7 +8,7 @@ Three levels of semantics, each building on the last: 1. **Structural** (free, deterministic) — builds a symbol graph from tree-sitter ASTs, detects entrypoints (HTTP routes, CLI commands, queue consumers, Effect.ts services, etc.), clusters changed files into flow groups via forward reachability, traces data flow across call chains. 2. **Heuristic** (free, deterministic) — framework detection (Express, Next.js, FastAPI, Effect.ts, 30+ frameworks), risk scoring, review ordering by composite score (risk/centrality/surface-area/uncertainty). -3. **LLM refinement** (paid, optional) — Anthropic, OpenAI, or Gemini reads the actual diff content and refines groupings: split coincidental coupling, merge scattered refactors, re-rank by semantic review order, reclassify misplaced files. Evaluator-optimizer loop scores v1 vs v2, keeps whichever is better. +3. **LLM refinement** (paid, optional) — Anthropic, OpenAI, or Gemini reads the actual diff content and refines groupings: split coincidental coupling, merge scattered refactors, re-rank by semantic review order, reclassify misplaced files. Applied as structural patch operations over the deterministic grouping; on failure the deterministic groups are kept unchanged. ## Target Grouping Strategy diff --git a/README.md b/README.md index 7d17ac35..d578fbdf 100644 --- a/README.md +++ b/README.md @@ -164,7 +164,6 @@ model = "default" enabled = true provider = "claude" model = "default" -max_iterations = 1 ``` Example repo-local config: diff --git a/crates/diffcore-cli/src/main.rs b/crates/diffcore-cli/src/main.rs index f6281651..a76f798c 100644 --- a/crates/diffcore-cli/src/main.rs +++ b/crates/diffcore-cli/src/main.rs @@ -96,10 +96,14 @@ struct AnalyzeArgs { #[arg(short, long)] output: Option, - /// Enable LLM annotation (Pass 1: overview) + /// Enable LLM annotation (Pass 1: PR-level overview) #[arg(long)] annotate: bool, + /// Enable the LLM group metadata pass (description, risk, impact, invariant, ...) + #[arg(long)] + describe: bool, + /// Enable LLM refinement pass (overrides config) #[arg(long)] refine: bool, @@ -510,7 +514,12 @@ fn run_analyze(mut args: AnalyzeArgs) -> Result<(), Box> // or if --no-cache was passed). Cache key incorporates ignore patterns so any // ignore-config change (incl. auto-detected subtrees) invalidates the entry. let cache_key = cache::compute_cache_key(&diff_result, &config.ignore.paths); - if !args.annotate && !args.refine && args.refine_model.is_none() && !args.no_cache { + if !args.annotate + && !args.refine + && !args.describe + && args.refine_model.is_none() + && !args.no_cache + { if let Some(cached) = cache::load_cached(&workdir, &cache_key) { return write_output(&cached, args.output.as_deref()); } @@ -598,6 +607,32 @@ fn run_analyze(mut args: AnalyzeArgs) -> Result<(), Box> } } + // Refinement rewrites group composition without re-scoring, so the ranking + // score and the heuristic metadata derived from it are both stale by here. + // Both have to be recomputed before the metadata pass runs on top. + rank::rescore_groups(&mut analysis_output.groups, &file_centrality, &weights); + diffcore_core::group_metadata::apply_heuristic_metadata(&mut analysis_output.groups); + + // Apply the LLM group metadata pass if requested. Explicit flag only — + // `diffcore analyze` in CI must never start billing silently. + if args.describe { + let rt = tokio::runtime::Runtime::new()?; + match rt.block_on(run_metadata( + &config, + &workdir, + &mut analysis_output, + &diff_result.files, + )) { + Ok(()) => {} + Err(e) => { + warn!( + "LLM metadata pass failed, keeping existing group metadata: {}", + e + ); + } + } + } + // Apply LLM annotation if requested if args.annotate { let rt = tokio::runtime::Runtime::new()?; @@ -747,6 +782,54 @@ async fn run_refinement( Ok(()) } +async fn run_metadata( + config: &DiffcoreConfig, + workdir: &std::path::Path, + analysis_output: &mut AnalysisOutput, + diffs: &[diffcore_core::git::FileDiff], +) -> Result<(), Box> { + let metadata_llm_config = diffcore_core::config::LlmConfig { + provider: config + .llm + .metadata + .provider + .clone() + .or_else(|| config.llm.provider.clone()), + model: config + .llm + .metadata + .model + .clone() + .or_else(|| config.llm.model.clone()), + key_cmd: config + .llm + .metadata + .key_cmd + .clone() + .or_else(|| config.llm.key_cmd.clone()), + key: config + .llm + .metadata + .key + .clone() + .or_else(|| config.llm.key.clone()), + ..Default::default() + }; + + let provider: std::sync::Arc = + llm::create_provider_for_workdir(&metadata_llm_config, Some(workdir))?.into(); + + llm::metadata::run_metadata_pass( + provider, + &mut analysis_output.groups, + diffs, + config.llm.metadata.batch_size, + ) + .await?; + + Ok(()) +} + async fn run_annotation( config: &DiffcoreConfig, workdir: &std::path::Path, @@ -1132,6 +1215,7 @@ fn run_export_groups(args: ExportGroupsArgs) -> Result<(), Box Result<(), Box Vec String { } /// Check if a file is a test file — by filename pattern OR directory. -pub(super) fn is_test_file_name(path: &str) -> bool { +pub(crate) fn is_test_file_name(path: &str) -> bool { let lower = path.to_lowercase(); let filename = lower.rsplit('/').next().unwrap_or(&lower); diff --git a/crates/diffcore-core/src/config.rs b/crates/diffcore-core/src/config.rs index e31a21fe..26ec91be 100644 --- a/crates/diffcore-core/src/config.rs +++ b/crates/diffcore-core/src/config.rs @@ -163,14 +163,17 @@ pub struct LlmConfig { /// Optional LLM refinement pass configuration. #[serde(default)] pub refinement: RefinementConfig, + /// Optional LLM group metadata pass configuration. + #[serde(default)] + pub metadata: MetadataConfig, } /// Configuration for the optional LLM refinement pass. /// /// The refinement pass takes deterministic groups (v1) and asks an LLM to improve them: /// split coincidental groupings, merge scattered refactors, re-rank by semantic review -/// order, reclassify misplaced files. Uses an evaluator-optimizer loop: refine → score → -/// refine again if score improved, up to `max_iterations`. +/// order, reclassify misplaced files. It is a single pass — the response is either +/// applied or discarded, leaving the deterministic groups in place. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct RefinementConfig { /// Whether refinement is enabled (default: true). @@ -185,24 +188,64 @@ pub struct RefinementConfig { /// Shell command to retrieve the refinement API key. #[serde(default)] pub key_cmd: Option, - /// Maximum evaluator-optimizer loop iterations (default: 1). - /// 1 = single refinement pass, 2+ = iterative improvement. - #[serde(default = "default_max_iterations")] - pub max_iterations: u32, } -fn default_max_iterations() -> u32 { - 1 +impl Default for RefinementConfig { + fn default() -> Self { + Self { + enabled: true, + provider: None, + model: None, + key_cmd: None, + } + } } -impl Default for RefinementConfig { +/// Configuration for the optional LLM group metadata pass. +/// +/// The metadata pass runs on the *final* groups — after refinement, if refinement +/// ran — and fills in the review metadata a reviewer glances at: group type, risk +/// band, impact scope, review complexity, review focus, a one-line description, and +/// the invariant to verify. It never writes `risk_score`. +/// +/// It gets its own provider/model because writing a one-line invariant is a much +/// cheaper task than the structural reasoning refinement does. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct MetadataConfig { + /// Whether the metadata pass is enabled (default: true). + #[serde(default = "default_true")] + pub enabled: bool, + /// Provider for the metadata pass (can differ from the refinement provider). + #[serde(default)] + pub provider: Option, + /// Model for the metadata pass (user-selectable). + #[serde(default)] + pub model: Option, + /// Shell command to retrieve the metadata API key. + #[serde(default)] + pub key_cmd: Option, + /// API key stored directly in the config file. + /// Precedence: key_cmd > key > env vars. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub key: Option, + /// Number of groups sent per LLM call (default: 20). + #[serde(default = "default_metadata_batch_size")] + pub batch_size: usize, +} + +fn default_metadata_batch_size() -> usize { + 20 +} + +impl Default for MetadataConfig { fn default() -> Self { Self { enabled: true, provider: None, model: None, key_cmd: None, - max_iterations: 1, + key: None, + batch_size: default_metadata_batch_size(), } } } @@ -216,6 +259,7 @@ impl Default for LlmConfig { key: None, annotations_enabled: true, refinement: RefinementConfig::default(), + metadata: MetadataConfig::default(), } } } @@ -349,10 +393,22 @@ impl DiffcoreConfig { } } - // Validate max_iterations is at least 1 - if self.llm.refinement.max_iterations == 0 { + // Validate metadata provider if specified + if let Some(ref provider) = self.llm.metadata.provider { + let valid = ["anthropic", "openai", "gemini", "codex", "claude"]; + if !valid.contains(&provider.as_str()) { + return Err(ConfigError::Validation(format!( + "Unknown metadata provider '{}'. Valid providers: {}", + provider, + valid.join(", ") + ))); + } + } + + // Validate batch_size is at least 1 + if self.llm.metadata.batch_size == 0 { return Err(ConfigError::Validation( - "Refinement max_iterations must be at least 1".to_string(), + "Metadata batch_size must be at least 1".to_string(), )); } @@ -484,8 +540,34 @@ impl DiffcoreConfig { .key_cmd .clone() .or_else(|| global.llm.refinement.key_cmd.clone()); - if self.llm.refinement.max_iterations == default_max_iterations() { - self.llm.refinement.max_iterations = global.llm.refinement.max_iterations; + + self.llm.metadata.enabled = self.llm.metadata.enabled || global.llm.metadata.enabled; + self.llm.metadata.provider = self + .llm + .metadata + .provider + .clone() + .or_else(|| global.llm.metadata.provider.clone()); + self.llm.metadata.model = self + .llm + .metadata + .model + .clone() + .or_else(|| global.llm.metadata.model.clone()); + self.llm.metadata.key_cmd = self + .llm + .metadata + .key_cmd + .clone() + .or_else(|| global.llm.metadata.key_cmd.clone()); + self.llm.metadata.key = self + .llm + .metadata + .key + .clone() + .or_else(|| global.llm.metadata.key.clone()); + if self.llm.metadata.batch_size == default_metadata_batch_size() { + self.llm.metadata.batch_size = global.llm.metadata.batch_size; } } } @@ -1010,7 +1092,6 @@ events = ["src/handlers/events/**/*.ts"] assert_eq!(config.llm.refinement.provider, None); assert_eq!(config.llm.refinement.model, None); assert_eq!(config.llm.refinement.key_cmd, None); - assert_eq!(config.llm.refinement.max_iterations, 1); } #[test] @@ -1023,13 +1104,11 @@ provider = "anthropic" enabled = true provider = "openai" model = "gpt-4.1" -max_iterations = 3 "#; let config = DiffcoreConfig::from_str(toml_str).unwrap(); assert!(config.llm.refinement.enabled); assert_eq!(config.llm.refinement.provider, Some("openai".to_string())); assert_eq!(config.llm.refinement.model, Some("gpt-4.1".to_string())); - assert_eq!(config.llm.refinement.max_iterations, 3); } #[test] @@ -1040,7 +1119,6 @@ provider = "anthropic" "#; let config = DiffcoreConfig::from_str(toml_str).unwrap(); assert!(config.llm.refinement.enabled); - assert_eq!(config.llm.refinement.max_iterations, 1); } #[test] @@ -1061,23 +1139,133 @@ provider = "invalid" } } + // ── Metadata Config Tests ── + #[test] - fn test_refinement_zero_iterations_rejected() { + fn test_metadata_config_defaults() { + let config = DiffcoreConfig::default(); + assert!(config.llm.metadata.enabled); + assert_eq!(config.llm.metadata.provider, None); + assert_eq!(config.llm.metadata.model, None); + assert_eq!(config.llm.metadata.key_cmd, None); + assert_eq!(config.llm.metadata.key, None); + assert_eq!(config.llm.metadata.batch_size, 20); + } + + #[test] + fn test_parse_metadata_config() { let toml_str = r#" -[llm.refinement] +[llm] +provider = "anthropic" + +[llm.metadata] enabled = true -max_iterations = 0 +provider = "anthropic" +model = "claude-haiku-4-5-20251001" +key_cmd = "op read op://vault/item/field" +batch_size = 5 "#; - let result = DiffcoreConfig::from_str(toml_str); - assert!(result.is_err()); - match result.unwrap_err() { + let config = DiffcoreConfig::from_str(toml_str).unwrap(); + assert!(config.llm.metadata.enabled); + assert_eq!(config.llm.metadata.provider, Some("anthropic".to_string())); + assert_eq!( + config.llm.metadata.model, + Some("claude-haiku-4-5-20251001".to_string()) + ); + assert_eq!( + config.llm.metadata.key_cmd, + Some("op read op://vault/item/field".to_string()) + ); + assert_eq!(config.llm.metadata.batch_size, 5); + } + + #[test] + fn test_metadata_enabled_by_default() { + let config = DiffcoreConfig::from_str("[llm]\nprovider = \"anthropic\"\n").unwrap(); + assert!(config.llm.metadata.enabled); + assert_eq!(config.llm.metadata.batch_size, 20); + } + + #[test] + fn test_metadata_invalid_provider_rejected() { + let toml_str = r#" +[llm.metadata] +provider = "invalid" +"#; + match DiffcoreConfig::from_str(toml_str).unwrap_err() { + ConfigError::Validation(msg) => { + assert!(msg.contains("metadata provider")); + assert!(msg.contains("invalid")); + } + err => panic!("Expected validation error, got: {:?}", err), + } + } + + #[test] + fn test_metadata_zero_batch_size_rejected() { + let toml_str = r#" +[llm.metadata] +batch_size = 0 +"#; + match DiffcoreConfig::from_str(toml_str).unwrap_err() { ConfigError::Validation(msg) => { - assert!(msg.contains("max_iterations")); + assert!(msg.contains("batch_size")); } err => panic!("Expected validation error, got: {:?}", err), } } + #[test] + fn test_metadata_provider_independent_of_refinement() { + let toml_str = r#" +[llm] +provider = "anthropic" + +[llm.refinement] +provider = "openai" + +[llm.metadata] +provider = "gemini" +"#; + let config = DiffcoreConfig::from_str(toml_str).unwrap(); + assert_eq!(config.llm.provider, Some("anthropic".to_string())); + assert_eq!(config.llm.refinement.provider, Some("openai".to_string())); + assert_eq!(config.llm.metadata.provider, Some("gemini".to_string())); + } + + #[test] + fn test_metadata_merges_from_global_config() { + let mut local = DiffcoreConfig::from_str("[llm.metadata]\nbatch_size = 20\n").unwrap(); + let mut global = DiffcoreConfig::default(); + global.llm.metadata.provider = Some("gemini".to_string()); + global.llm.metadata.model = Some("gemini-2.5-flash".to_string()); + global.llm.metadata.batch_size = 7; + + local.apply_global_llm_defaults(&global); + + assert_eq!(local.llm.metadata.provider, Some("gemini".to_string())); + assert_eq!( + local.llm.metadata.model, + Some("gemini-2.5-flash".to_string()) + ); + assert_eq!(local.llm.metadata.batch_size, 7); + } + + #[test] + fn test_metadata_local_config_wins_over_global() { + let mut local = + DiffcoreConfig::from_str("[llm.metadata]\nprovider = \"openai\"\nbatch_size = 3\n") + .unwrap(); + let mut global = DiffcoreConfig::default(); + global.llm.metadata.provider = Some("gemini".to_string()); + global.llm.metadata.batch_size = 7; + + local.apply_global_llm_defaults(&global); + + assert_eq!(local.llm.metadata.provider, Some("openai".to_string())); + assert_eq!(local.llm.metadata.batch_size, 3); + } + #[test] fn test_refinement_different_provider_from_annotation() { let toml_str = r#" @@ -1089,7 +1277,6 @@ model = "claude-sonnet-4-6" enabled = true provider = "gemini" model = "gemini-2.5-pro" -max_iterations = 2 "#; let config = DiffcoreConfig::from_str(toml_str).unwrap(); assert_eq!(config.llm.provider, Some("anthropic".to_string())); diff --git a/crates/diffcore-core/src/eval/repos.rs b/crates/diffcore-core/src/eval/repos.rs index 98b615ea..be770086 100644 --- a/crates/diffcore-core/src/eval/repos.rs +++ b/crates/diffcore-core/src/eval/repos.rs @@ -1424,6 +1424,7 @@ mod tests { edges: vec![], risk_score: 0.0, review_order: 1, + ..Default::default() } } diff --git a/crates/diffcore-core/src/eval/scoring.rs b/crates/diffcore-core/src/eval/scoring.rs index 77b4afa0..a08b9066 100644 --- a/crates/diffcore-core/src/eval/scoring.rs +++ b/crates/diffcore-core/src/eval/scoring.rs @@ -401,6 +401,7 @@ mod tests { edges: vec![], risk_score: 0.5, review_order: 1, + ..Default::default() }], infrastructure_group: None, annotations: None, @@ -443,6 +444,7 @@ mod tests { edges: vec![], risk_score: 1.5, // Out of bounds review_order: 1, + ..Default::default() }], infrastructure_group: None, annotations: None, diff --git a/crates/diffcore-core/src/group_metadata.rs b/crates/diffcore-core/src/group_metadata.rs new file mode 100644 index 00000000..63a879fb --- /dev/null +++ b/crates/diffcore-core/src/group_metadata.rs @@ -0,0 +1,358 @@ +//! Deterministic review metadata — the heuristic floor from `specs/group-metadata.md` §3.1. +//! +//! Populates the three `FlowGroup` fields that can be derived honestly without a model: +//! `risk` (bucketed from `risk_score`), `group_type` (path conventions) and `impact` +//! (how far the group's files spread across the tree). `description`, `invariant`, +//! `review_focus` and `complexity` are deliberately left empty: a wrong value in any of +//! them misdirects a reviewer, and no free signal is good enough to fill them. + +use std::collections::HashSet; + +use crate::cluster::classify_by_convention; +use crate::cluster::stem::is_test_file_name; +use crate::types::{FlowGroup, GroupType, ImpactScope, InfraCategory, Risk}; + +/// Fill the deterministic metadata fields on every group. +/// +/// `risk_score` and `review_order` are read-only here — see spec §1.3. +pub fn apply_heuristic_metadata(groups: &mut [FlowGroup]) { + for group in groups.iter_mut() { + let paths: Vec<&str> = group.files.iter().map(|f| f.path.as_str()).collect(); + group.risk = Some(bucket_risk(group.risk_score)); + group.group_type = infer_group_type(&paths); + group.impact = Some(infer_impact(&paths)); + } +} + +fn bucket_risk(score: f64) -> Risk { + if score >= 0.75 { + Risk::Critical + } else if score >= 0.55 { + Risk::High + } else if score >= 0.35 { + Risk::Medium + } else { + Risk::Low + } +} + +fn infer_group_type(paths: &[&str]) -> Option { + if paths.is_empty() { + return None; + } + + for (matcher, group_type) in [ + (is_ci_path as fn(&str) -> bool, GroupType::Ci), + (is_build_path, GroupType::Build), + (is_doc_path, GroupType::Docs), + (is_test_file_name, GroupType::Test), + ] { + if paths.iter().all(|path| matcher(path)) { + return Some(group_type); + } + } + + None +} + +fn is_ci_path(path: &str) -> bool { + let lower = path.to_lowercase(); + let filename = lower.rsplit('/').next().unwrap_or(&lower); + + lower.contains(".github/workflows/") + || lower.contains(".github/actions/") + || lower.contains(".circleci/") + || lower.contains(".buildkite/") + || matches!( + filename, + ".gitlab-ci.yml" + | "jenkinsfile" + | ".travis.yml" + | "azure-pipelines.yml" + | "bitbucket-pipelines.yml" + | ".pre-commit-config.yaml" + ) +} + +fn is_build_path(path: &str) -> bool { + let lower = path.to_lowercase(); + let filename = lower.rsplit('/').next().unwrap_or(&lower); + + if matches!( + filename, + "package.json" + | "package-lock.json" + | "yarn.lock" + | "pnpm-lock.yaml" + | "pnpm-workspace.yaml" + | "cargo.toml" + | "cargo.lock" + | "go.mod" + | "go.sum" + | "requirements.txt" + | "pipfile" + | "pipfile.lock" + | "pyproject.toml" + | "poetry.lock" + | "setup.py" + | "setup.cfg" + | "gemfile" + | "gemfile.lock" + | "composer.json" + | "composer.lock" + | "pom.xml" + | "build.sbt" + | "package.swift" + | "makefile" + | "cmakelists.txt" + | "build.rs" + | "flake.nix" + | "flake.lock" + | "shell.nix" + | "default.nix" + | ".dockerignore" + ) { + return true; + } + + filename.starts_with("dockerfile") + || filename.starts_with("docker-compose") + || filename.starts_with("tsconfig") + || filename.starts_with("build.gradle") + || filename.starts_with("webpack.") + || filename.starts_with("vite.") + || filename.starts_with("rollup.") + || filename.starts_with("esbuild.") + || filename.starts_with("babel.") + || filename.ends_with(".mk") + || filename.ends_with(".csproj") +} + +fn is_doc_path(path: &str) -> bool { + classify_by_convention(path) == InfraCategory::Documentation +} + +/// Fan-out of the group's own files across the tree: one directory is `Local`, several +/// directories under one module root is `Module`, two module roots is `CrossCutting`, +/// three or more is `System`. +/// +/// `FlowGroup::edges` cannot answer this — `cluster::bfs::collect_internal_edges` keeps +/// only edges whose endpoints are both inside the group, so no group edge ever crosses a +/// group boundary, and the symbol graph is gone by the time groups are finalized. +fn infer_impact(paths: &[&str]) -> ImpactScope { + let dirs: HashSet<&str> = paths.iter().map(|p| parent_dir(p)).collect(); + let roots: HashSet<&str> = paths.iter().map(|p| module_root(p)).collect(); + + match (roots.len(), dirs.len()) { + (roots, _) if roots >= 3 => ImpactScope::System, + (roots, _) if roots >= 2 => ImpactScope::CrossCutting, + (_, dirs) if dirs >= 2 => ImpactScope::Module, + _ => ImpactScope::Local, + } +} + +fn parent_dir(path: &str) -> &str { + path.rsplit_once('/').map_or("", |(dir, _)| dir) +} + +/// The unit a change is "inside": the first path segment, or two segments deep for the +/// usual monorepo container directories. +fn module_root(path: &str) -> &str { + let mut parts = path.split('/'); + let Some(first) = parts.next().filter(|segment| !segment.is_empty()) else { + return ""; + }; + if !matches!( + first, + "apps" | "packages" | "services" | "workers" | "libs" | "modules" | "crates" + ) { + return first; + } + match parts.next() { + Some(second) if !second.is_empty() => &path[..first.len() + 1 + second.len()], + _ => first, + } +} + +#[cfg(test)] +#[allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::print_stdout, + clippy::print_stderr +)] +mod tests { + use super::*; + use crate::types::{ChangeStats, FileChange, FileRole}; + + fn group(id: &str, risk_score: f64, paths: &[&str]) -> FlowGroup { + FlowGroup { + id: id.to_string(), + name: id.to_string(), + risk_score, + files: paths + .iter() + .map(|path| FileChange { + path: (*path).to_string(), + flow_position: 0, + role: FileRole::Service, + changes: ChangeStats { + additions: 1, + deletions: 0, + }, + symbols_changed: vec![], + }) + .collect(), + ..Default::default() + } + } + + fn metadata_for(paths: &[&str]) -> FlowGroup { + let mut groups = vec![group("g1", 0.0, paths)]; + apply_heuristic_metadata(&mut groups); + groups.remove(0) + } + + // ── risk bucketing ── + + #[test] + fn risk_buckets_span_the_score_range() { + for (score, expected) in [ + (0.0, Risk::Low), + (0.34, Risk::Low), + (0.35, Risk::Medium), + (0.54, Risk::Medium), + (0.55, Risk::High), + (0.74, Risk::High), + (0.75, Risk::Critical), + (1.0, Risk::Critical), + ] { + let mut groups = vec![group("g1", score, &["src/a.ts"])]; + apply_heuristic_metadata(&mut groups); + assert_eq!(groups[0].risk, Some(expected), "score {score}"); + } + } + + #[test] + fn risk_score_and_review_order_are_untouched() { + let mut groups = vec![group("g1", 0.62, &["src/a.ts"])]; + groups[0].review_order = 7; + apply_heuristic_metadata(&mut groups); + assert_eq!(groups[0].risk_score, 0.62); + assert_eq!(groups[0].review_order, 7); + } + + // ── group_type ── + + #[test] + fn all_test_files_are_a_test_group() { + assert_eq!( + metadata_for(&["tests/auth.rs", "src/user.test.ts", "pkg/user_test.go"]).group_type, + Some(GroupType::Test) + ); + } + + #[test] + fn all_docs_are_a_docs_group() { + assert_eq!( + metadata_for(&["README.md", "docs/guide.mdx"]).group_type, + Some(GroupType::Docs) + ); + } + + #[test] + fn workflow_files_are_a_ci_group() { + assert_eq!( + metadata_for(&[".github/workflows/ci.yml", ".circleci/config.yml"]).group_type, + Some(GroupType::Ci) + ); + } + + #[test] + fn manifests_and_dockerfiles_are_a_build_group() { + assert_eq!( + metadata_for(&["Cargo.toml", "crates/core/Cargo.toml", "Dockerfile"]).group_type, + Some(GroupType::Build) + ); + } + + #[test] + fn mixed_files_have_no_group_type() { + assert_eq!(metadata_for(&["src/auth.ts", "tests/auth.rs"]).group_type, None); + assert_eq!(metadata_for(&["src/auth.ts", "src/user.ts"]).group_type, None); + } + + #[test] + fn empty_group_has_no_group_type() { + assert_eq!(metadata_for(&[]).group_type, None); + } + + // ── impact ── + + #[test] + fn single_directory_is_local() { + assert_eq!( + metadata_for(&["src/auth/login.ts", "src/auth/token.ts"]).impact, + Some(ImpactScope::Local) + ); + } + + #[test] + fn several_directories_in_one_root_are_module_scoped() { + assert_eq!( + metadata_for(&["src/auth/login.ts", "src/http/router.ts"]).impact, + Some(ImpactScope::Module) + ); + } + + #[test] + fn two_roots_are_cross_cutting() { + assert_eq!( + metadata_for(&["api/handler.ts", "web/page.tsx"]).impact, + Some(ImpactScope::CrossCutting) + ); + } + + #[test] + fn sibling_groups_in_one_directory_stay_local() { + let mut groups = vec![ + group("g1", 0.0, &["src/routes/health.ts"]), + group("g2", 0.0, &["src/routes/users.ts"]), + ]; + apply_heuristic_metadata(&mut groups); + assert_eq!(groups[0].impact, Some(ImpactScope::Local)); + assert_eq!(groups[1].impact, Some(ImpactScope::Local)); + } + + #[test] + fn three_roots_are_system_wide() { + assert_eq!( + metadata_for(&["api/a.ts", "web/b.ts", "worker/c.ts"]).impact, + Some(ImpactScope::System) + ); + } + + #[test] + fn monorepo_packages_are_separate_roots() { + assert_eq!( + metadata_for(&["packages/ui/button.tsx", "packages/core/index.ts"]).impact, + Some(ImpactScope::CrossCutting) + ); + assert_eq!( + metadata_for(&["packages/ui/button.tsx", "packages/ui/card.tsx"]).impact, + Some(ImpactScope::Local) + ); + } + + // ── fields the heuristic floor must not fill ── + + #[test] + fn llm_only_fields_stay_empty() { + let g = metadata_for(&["src/auth/login.ts"]); + assert_eq!(g.description, None); + assert_eq!(g.invariant, None); + assert_eq!(g.complexity, None); + assert!(g.review_focus.is_empty()); + } +} diff --git a/crates/diffcore-core/src/lib.rs b/crates/diffcore-core/src/lib.rs index 3fecfb16..3142bed1 100644 --- a/crates/diffcore-core/src/lib.rs +++ b/crates/diffcore-core/src/lib.rs @@ -17,6 +17,7 @@ pub mod eval; pub mod flow; pub mod git; pub mod graph; +pub mod group_metadata; pub mod ir; pub mod llm; #[cfg(feature = "logging")] diff --git a/crates/diffcore-core/src/llm/anthropic.rs b/crates/diffcore-core/src/llm/anthropic.rs index 77a6b93f..562d324a 100644 --- a/crates/diffcore-core/src/llm/anthropic.rs +++ b/crates/diffcore-core/src/llm/anthropic.rs @@ -10,15 +10,19 @@ use reqwest::Client; use serde::{Deserialize, Serialize}; use super::schema::{ - judge_json_schema, pass1_json_schema, pass2_json_schema, refinement_json_schema, JudgeResponse, - Pass1Response, Pass2Response, RefinementResponse, + judge_json_schema, metadata_json_schema, pass1_json_schema, pass2_json_schema, + refinement_json_schema, JudgeResponse, MetadataResponse, Pass1Response, Pass2Response, + RefinementResponse, }; use super::{ judge_system_prompt, judge_user_prompt, pass1_system_prompt, pass1_user_prompt, pass2_system_prompt, pass2_user_prompt, refinement_system_prompt, refinement_user_prompt, truncate_to_token_budget, LlmError, LlmProvider, }; -use crate::llm::schema::{JudgeRequest, Pass1Request, Pass2Request, RefinementRequest}; +use crate::llm::metadata::{metadata_system_prompt, metadata_user_prompt}; +use crate::llm::schema::{ + JudgeRequest, MetadataRequest, Pass1Request, Pass2Request, RefinementRequest, +}; const ANTHROPIC_API_URL: &str = "https://api.anthropic.com/v1/messages"; const ANTHROPIC_API_VERSION: &str = "2023-06-01"; @@ -266,6 +270,23 @@ impl LlmProvider for AnthropicProvider { .await?; parse_json_response::(&response_text) } + + async fn describe_groups( + &self, + request: &MetadataRequest, + ) -> Result { + let system = metadata_system_prompt(); + let user = metadata_user_prompt(request); + let response_text = self + .send_structured_message( + &system, + &user, + metadata_json_schema(), + "Return the review metadata for the flow groups", + ) + .await?; + parse_json_response::(&response_text) + } } /// Parse a JSON response, stripping any markdown fencing the LLM may add. @@ -489,8 +510,7 @@ mod tests { assert_eq!(name, "structured_output"); // The input should be directly deserializable let pass1: Pass1Response = serde_json::from_value(input.clone()).unwrap(); - assert_eq!(pass1.groups.len(), 1); - assert_eq!(pass1.groups[0].id, "g1"); + assert_eq!(pass1.suggested_review_order, vec!["g1".to_string()]); assert_eq!(pass1.overall_summary, "Auth changes"); } } @@ -563,8 +583,7 @@ mod tests { "suggested_review_order": ["group_1"] }"#; let result: Pass1Response = parse_json_response(json).unwrap(); - assert_eq!(result.groups.len(), 1); - assert_eq!(result.groups[0].id, "group_1"); + assert_eq!(result.suggested_review_order, vec!["group_1".to_string()]); assert_eq!(result.overall_summary, "Auth changes"); } @@ -631,7 +650,7 @@ mod tests { assert!(parsed["input_schema"].is_object()); // Schema should contain the response type properties let schema_str = serde_json::to_string(&parsed["input_schema"]).unwrap(); - assert!(schema_str.contains("groups")); + assert!(schema_str.contains("overall_summary")); } #[test] diff --git a/crates/diffcore-core/src/llm/claude_cli.rs b/crates/diffcore-core/src/llm/claude_cli.rs index c1b4e528..714a3d8d 100644 --- a/crates/diffcore-core/src/llm/claude_cli.rs +++ b/crates/diffcore-core/src/llm/claude_cli.rs @@ -10,8 +10,8 @@ use tokio::time::{sleep, Duration}; use super::schema; use super::{ redact_api_keys, truncate_to_token_budget, BackendStatus, JudgeRequest, JudgeResponse, - LlmError, LlmProvider, Pass1Request, Pass1Response, Pass2Request, Pass2Response, - RefinementRequest, RefinementResponse, + LlmError, LlmProvider, MetadataRequest, MetadataResponse, Pass1Request, Pass1Response, + Pass2Request, Pass2Response, RefinementRequest, RefinementResponse, }; const AGENT_ADDENDUM: &str = @@ -214,6 +214,18 @@ impl LlmProvider for ClaudeCliProvider { ) .await } + + async fn describe_groups( + &self, + request: &MetadataRequest, + ) -> Result { + self.run_structured_prompt( + super::metadata::metadata_system_prompt(), + super::metadata::metadata_user_prompt(request), + schema::metadata_json_schema(), + ) + .await + } } pub fn detect_status() -> BackendStatus { diff --git a/crates/diffcore-core/src/llm/codex_cli.rs b/crates/diffcore-core/src/llm/codex_cli.rs index 343a8c45..497db8cf 100644 --- a/crates/diffcore-core/src/llm/codex_cli.rs +++ b/crates/diffcore-core/src/llm/codex_cli.rs @@ -11,8 +11,8 @@ use tokio::time::{sleep, Duration}; use super::schema; use super::{ redact_api_keys, truncate_to_token_budget, BackendStatus, JudgeRequest, JudgeResponse, - LlmError, LlmProvider, Pass1Request, Pass1Response, Pass2Request, Pass2Response, - RefinementRequest, RefinementResponse, + LlmError, LlmProvider, MetadataRequest, MetadataResponse, Pass1Request, Pass1Response, + Pass2Request, Pass2Response, RefinementRequest, RefinementResponse, }; const AGENT_ADDENDUM: &str = @@ -228,6 +228,18 @@ impl LlmProvider for CodexCliProvider { ) .await } + + async fn describe_groups( + &self, + request: &MetadataRequest, + ) -> Result { + self.run_structured_prompt( + super::metadata::metadata_system_prompt(), + super::metadata::metadata_user_prompt(request), + schema::metadata_json_schema(), + ) + .await + } } pub fn detect_status() -> BackendStatus { diff --git a/crates/diffcore-core/src/llm/gemini.rs b/crates/diffcore-core/src/llm/gemini.rs index 69c05d9e..73637acd 100644 --- a/crates/diffcore-core/src/llm/gemini.rs +++ b/crates/diffcore-core/src/llm/gemini.rs @@ -12,14 +12,17 @@ use serde::{Deserialize, Serialize}; use super::schema::{ flatten_json_schema, judge_json_schema, pass1_json_schema, pass2_json_schema, - refinement_json_schema, JudgeResponse, Pass1Response, Pass2Response, RefinementResponse, + refinement_json_schema, JudgeResponse, MetadataResponse, Pass1Response, Pass2Response, + RefinementResponse, }; use super::{ judge_system_prompt, judge_user_prompt, pass1_system_prompt, pass1_user_prompt, pass2_system_prompt, pass2_user_prompt, refinement_system_prompt, refinement_user_prompt, truncate_to_token_budget, LlmError, LlmProvider, }; -use crate::llm::schema::{JudgeRequest, Pass1Request, Pass2Request, RefinementRequest}; +use crate::llm::schema::{ + JudgeRequest, MetadataRequest, Pass1Request, Pass2Request, RefinementRequest, +}; const GEMINI_API_BASE: &str = "https://generativelanguage.googleapis.com/v1beta/models"; @@ -260,6 +263,18 @@ impl LlmProvider for GeminiProvider { .await?; parse_json_response::(&response_text) } + + async fn describe_groups( + &self, + request: &MetadataRequest, + ) -> Result { + let system = crate::llm::metadata::metadata_system_prompt(); + let user = crate::llm::metadata::metadata_user_prompt(request); + let response_text = self + .send_structured_message(&system, &user, crate::llm::schema::metadata_json_schema()) + .await?; + parse_json_response::(&response_text) + } } /// Parse a JSON response, stripping any markdown fencing the LLM may add. @@ -450,7 +465,7 @@ mod tests { assert!(parsed["responseSchema"].is_object()); // Schema should reference Pass1Response properties let schema_str = serde_json::to_string(&parsed["responseSchema"]).unwrap(); - assert!(schema_str.contains("groups")); + assert!(schema_str.contains("overall_summary")); } #[test] @@ -555,8 +570,7 @@ mod tests { "suggested_review_order": ["group_1"] }"#; let result: Pass1Response = parse_json_response(json).unwrap(); - assert_eq!(result.groups.len(), 1); - assert_eq!(result.groups[0].id, "group_1"); + assert_eq!(result.suggested_review_order, vec!["group_1".to_string()]); assert_eq!(result.overall_summary, "Auth changes"); } diff --git a/crates/diffcore-core/src/llm/judge.rs b/crates/diffcore-core/src/llm/judge.rs index 3a8a296b..b705c9d3 100644 --- a/crates/diffcore-core/src/llm/judge.rs +++ b/crates/diffcore-core/src/llm/judge.rs @@ -352,6 +352,7 @@ mod tests { }], risk_score: 0.65, review_order: 1, + ..Default::default() }], infrastructure_group: Some(InfrastructureGroup { files: vec!["package.json".to_string()], diff --git a/crates/diffcore-core/src/llm/metadata.rs b/crates/diffcore-core/src/llm/metadata.rs new file mode 100644 index 00000000..36edd7bc --- /dev/null +++ b/crates/diffcore-core/src/llm/metadata.rs @@ -0,0 +1,753 @@ +//! LLM group metadata pass — fills in the review metadata carried on each group. +//! +//! Consumes the *final* groups, after any refinement ops have been applied, and +//! asks an LLM "how should I review this group" rather than "what changed". The +//! answer lands on the `FlowGroup` itself: type, risk band, impact scope, review +//! complexity, review focus, a one-line description, and the invariant a reviewer +//! should verify. +//! +//! `risk_score` is never written — it drives review ranking and stays +//! deterministic. See `specs/group-metadata.md`. + +use std::collections::HashMap; +use std::sync::Arc; + +use crate::git::FileDiff; +use crate::types::{FlowGroup, MAX_REVIEW_FOCUS, MAX_SUMMARY_BULLETS}; + +use super::schema; +use super::schema::{ + GroupMetadata, MetadataFileInput, MetadataGroupDetail, MetadataGroupIndexEntry, MetadataRequest, +}; +use super::{LlmError, LlmProvider}; + +/// Per-file cap on the changed-code excerpt sent to the model. +const MAX_FILE_EXCERPT_TOKENS: usize = 600; + +/// Run the metadata pass over `groups`, mutating them in place. +/// +/// Batches are dispatched concurrently and their results merged by group id, so +/// completion order cannot reach the output. Groups are left sorted by +/// `review_order`. +/// +/// A batch that fails is logged and skipped; the groups it covered keep whatever +/// the deterministic floor gave them. +pub async fn run_metadata_pass( + provider: Arc, + groups: &mut [FlowGroup], + diffs: &[FileDiff], + batch_size: usize, +) -> Result<(), LlmError> { + let requests = build_metadata_requests(groups, diffs, batch_size); + if requests.is_empty() { + return Ok(()); + } + + let handles: Vec<_> = requests + .into_iter() + .map(|request| { + let provider = Arc::clone(&provider); + tokio::spawn(async move { provider.describe_groups(&request).await }) + }) + .collect(); + + let mut merged: HashMap = HashMap::new(); + let mut succeeded = 0usize; + let total = handles.len(); + + for handle in handles { + match handle.await { + Ok(Ok(response)) => { + succeeded += 1; + for meta in response.groups { + merged.insert(meta.id.clone(), meta); + } + } + Ok(Err(e)) => log::warn!(target: "metadata", "batch failed: {}", e), + Err(e) => log::warn!(target: "metadata", "batch panicked: {}", e), + } + } + + if succeeded == 0 { + return Err(LlmError::ParseResponse(format!( + "all {} metadata batches failed", + total + ))); + } + + apply_metadata(groups, &merged); + groups.sort_by_key(|g| g.review_order); + Ok(()) +} + +/// Split `groups` into batches of at most `batch_size`, each carrying full detail +/// for its own groups plus a read-only index of every group in the analysis. +pub fn build_metadata_requests( + groups: &[FlowGroup], + diffs: &[FileDiff], + batch_size: usize, +) -> Vec { + if groups.is_empty() { + return Vec::new(); + } + + let excerpts = excerpts_by_path(diffs); + + let index: Vec = groups + .iter() + .map(|g| MetadataGroupIndexEntry { + id: g.id.clone(), + name: g.name.clone(), + files: g.files.iter().map(|f| f.path.clone()).collect(), + risk_score: g.risk_score, + }) + .collect(); + + groups + .chunks(batch_size.max(1)) + .map(|chunk| MetadataRequest { + groups: chunk + .iter() + .map(|g| MetadataGroupDetail { + id: g.id.clone(), + name: g.name.clone(), + entrypoint: g + .entrypoint + .as_ref() + .map(|ep| format!("{}::{}", ep.file, ep.symbol)), + risk_score: g.risk_score, + files: g + .files + .iter() + .map(|f| MetadataFileInput { + path: f.path.clone(), + role: format!("{:?}", f.role), + diff: excerpts.get(f.path.as_str()).cloned().unwrap_or_default(), + }) + .collect(), + }) + .collect(), + index: index.clone(), + }) + .collect() +} + +/// Write metadata onto the groups it names, clamping to the response constraints. +/// +/// Unknown ids are ignored, and a field the model omitted leaves whatever the +/// deterministic floor produced in place. +pub fn apply_metadata(groups: &mut [FlowGroup], metadata: &HashMap) { + for group in groups.iter_mut() { + let Some(meta) = metadata.get(&group.id) else { + continue; + }; + + if meta.group_type.is_some() { + group.group_type = meta.group_type; + } + if meta.risk.is_some() { + group.risk = meta.risk; + } + if meta.impact.is_some() { + group.impact = meta.impact; + } + if meta.complexity.is_some() { + group.complexity = meta.complexity; + } + if let Some(ref description) = meta.description { + if let Some(clamped) = clamp_line(description) { + group.description = Some(clamped); + } + } + if let Some(ref invariant) = meta.invariant { + if let Some(clamped) = clamp_sentence(invariant) { + group.invariant = Some(clamped); + } + } + if !meta.review_focus.is_empty() { + let mut focus = meta.review_focus.clone(); + focus.dedup(); + focus.truncate(MAX_REVIEW_FOCUS); + group.review_focus = focus; + } + let summary: Vec = meta + .summary + .iter() + .filter_map(|bullet| clamp_line(strip_bullet_marker(bullet))) + .take(MAX_SUMMARY_BULLETS) + .collect(); + if !summary.is_empty() { + group.summary = summary; + } + } +} + +/// Drop a leading `-`, `*` or `\u{2022}` marker. Models add them despite the +/// schema saying not to, and the UI supplies its own. +fn strip_bullet_marker(text: &str) -> &str { + text.trim_start() + .trim_start_matches(['-', '*', '\u{2022}']) + .trim_start() +} + +/// Collapse to a single plain-text line. Returns `None` for empty input. +fn clamp_line(text: &str) -> Option { + let joined = text.split_whitespace().collect::>().join(" "); + if joined.is_empty() { + None + } else { + Some(joined) + } +} + +/// Collapse to a single plain-text sentence, keeping only the first. +fn clamp_sentence(text: &str) -> Option { + let line = clamp_line(text)?; + let end = line + .char_indices() + .find(|(i, c)| { + matches!(c, '.' | '!' | '?') + && line[i + c.len_utf8()..] + .chars() + .next() + .is_none_or(|next| next == ' ') + }) + .map(|(i, c)| i + c.len_utf8()); + + match end { + Some(end) if end < line.len() => Some(line[..end].trim_end().to_string()), + _ => Some(line), + } +} + +fn excerpts_by_path(diffs: &[FileDiff]) -> HashMap<&str, String> { + diffs + .iter() + .map(|file| (file.path(), changed_code_excerpt(file))) + .collect() +} + +/// Render the changed regions of a file, hunk by hunk. +/// +/// There is no unified-diff text in the pipeline — `git.rs` keeps hunk line ranges +/// plus whole-file contents — so this slices the post-change lines each hunk covers. +fn changed_code_excerpt(file: &FileDiff) -> String { + if file.is_binary { + return "(binary file)".to_string(); + } + + let use_new = file.new_content.is_some(); + let Some(content) = file.new_content.as_deref().or(file.old_content.as_deref()) else { + return String::new(); + }; + let lines: Vec<&str> = content.lines().collect(); + + let mut out = String::new(); + for hunk in &file.hunks { + let (start, count) = if use_new { + (hunk.new_start, hunk.new_lines) + } else { + (hunk.old_start, hunk.old_lines) + }; + let from = (start.saturating_sub(1)) as usize; + let to = (from + count as usize).min(lines.len()); + if from >= to { + continue; + } + out.push_str(&format!("@@ line {} @@\n", start)); + for line in &lines[from..to] { + out.push_str(line); + out.push('\n'); + } + } + + super::truncate_to_token_budget(&out, MAX_FILE_EXCERPT_TOKENS) +} + +/// Build the system prompt for the group metadata pass. +pub fn metadata_system_prompt() -> String { + format!( + "You are a senior engineer triaging a code review. For each group you are given, \ + answer the question a reviewer actually has: HOW SHOULD I REVIEW THIS? Not what changed \ + line by line — what to watch for while reading it.\n\n\ + Field meanings:\n\ + - `group_type`: the kind of change this is.\n\ + - `risk`: how much damage a mistake here does. Judge the change, not its size.\n\ + - `impact`: how far the blast radius reaches. Use the group index to see whether other \ + groups touch the same surface; CrossCutting means the change reaches beyond this group's \ + own files.\n\ + - `complexity`: how much effort reading this group carefully will take.\n\ + - `review_focus`: at most {max_focus} concerns, most important first. Pick only concerns \ + actually at stake in this change. Fewer is better than wrong.\n\ + - `description`: ONE line of plain text. What this group changes.\n\ + - `invariant`: ONE sentence of plain text naming the property that must still hold after \ + this change — the thing a reviewer should actively try to break. \ + Good: 'Two workers must never successfully claim the same job.' \ + Bad: 'The code should work correctly.'\n\n\ + Rules:\n\ + - `description` and `invariant` are PLAIN TEXT. No markdown, no bullet points, no code \ + fences, no backticks, no bold.\n\ + - `summary` is a LIST of plain-text entries. Size it to the change: one entry when a \ + single sentence covers what the group achieves, more only when it genuinely does \ + several things, at most {max_summary}. Each entry is a bare sentence — no leading \ + '-' or '*', no markdown. Convey the essence, do not restate `description`.\n\ + - Never restate line counts, file counts, or anything else mechanically visible from the \ + file list — the reviewer can already see it.\n\ + - Never invent an invariant you cannot support from the code shown. If nothing meaningful \ + is at stake, state the narrow property that is.\n\ + - Judge each group on its own merits. Do not compute it from the other groups' answers.\n\n\ + {}", + schema::metadata_schema_description(), + max_focus = MAX_REVIEW_FOCUS, + max_summary = MAX_SUMMARY_BULLETS, + ) +} + +/// Build the user prompt for one metadata batch. +pub fn metadata_user_prompt(request: &MetadataRequest) -> String { + let mut prompt = String::from("## Groups to describe\n"); + prompt.push_str( + "Produce exactly one metadata entry for each group in this section, keyed by its id.\n", + ); + + for group in &request.groups { + prompt.push_str(&format!( + "\n### {} ({})\n- Entrypoint: {}\n- Risk score: {:.2}\n", + group.name, + group.id, + group.entrypoint.as_deref().unwrap_or("none"), + group.risk_score, + )); + for file in &group.files { + prompt.push_str(&format!("\n#### {} (role: {})\n", file.path, file.role)); + if file.diff.is_empty() { + prompt.push_str("(no changed content available)\n"); + } else { + prompt.push_str(&format!("```\n{}\n```\n", file.diff)); + } + } + } + + prompt.push_str( + "\n## All groups in this analysis (context only — do NOT describe these)\n\ + Use this list to judge how far each change reaches. It is read-only context.\n", + ); + for entry in &request.index { + prompt.push_str(&format!( + "- {} ({}, risk {:.2}): {}\n", + entry.name, + entry.id, + entry.risk_score, + entry.files.join(", "), + )); + } + + prompt +} + +#[cfg(test)] +#[allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::print_stdout, + clippy::print_stderr +)] +mod tests { + use super::*; + use crate::git::{DiffHunk, FileStatus}; + use crate::types::{ + ChangeStats, FileChange, FileRole, GroupType, ImpactScope, ReviewComplexity, ReviewFocus, + Risk, + }; + + fn make_group(id: &str, review_order: u32, paths: &[&str]) -> FlowGroup { + FlowGroup { + id: id.to_string(), + name: format!("Group {}", id), + files: paths + .iter() + .enumerate() + .map(|(i, p)| FileChange { + path: p.to_string(), + flow_position: i as u32, + role: FileRole::Service, + changes: ChangeStats { + additions: 1, + deletions: 0, + }, + symbols_changed: vec![], + }) + .collect(), + risk_score: 0.5, + review_order, + ..Default::default() + } + } + + fn make_diff(path: &str, content: &str) -> FileDiff { + FileDiff { + old_path: Some(path.to_string()), + new_path: Some(path.to_string()), + old_content: None, + new_content: Some(content.to_string()), + hunks: vec![DiffHunk { + old_start: 1, + old_lines: 0, + new_start: 2, + new_lines: 2, + }], + status: FileStatus::Modified, + additions: 2, + deletions: 0, + is_binary: false, + } + } + + #[test] + fn batches_cover_every_group_and_carry_the_full_index() { + let groups: Vec = (0..7) + .map(|i| make_group(&format!("g{}", i), i, &["src/a.ts"])) + .collect(); + + let requests = build_metadata_requests(&groups, &[], 3); + + assert_eq!(requests.len(), 3); + let described: Vec<&str> = requests + .iter() + .flat_map(|r| r.groups.iter().map(|g| g.id.as_str())) + .collect(); + assert_eq!(described.len(), 7); + for request in &requests { + assert_eq!(request.index.len(), 7, "every batch sees every group"); + } + } + + #[test] + fn batch_index_carries_no_diff_content() { + let groups = vec![make_group("g0", 0, &["src/a.ts"])]; + let diffs = vec![make_diff("src/a.ts", "one\ntwo\nthree\n")]; + + let requests = build_metadata_requests(&groups, &diffs, 20); + let prompt = metadata_user_prompt(&requests[0]); + + assert!(requests[0].groups[0].files[0].diff.contains("two")); + let index_section = prompt.split("All groups in this analysis").nth(1).unwrap(); + assert!(!index_section.contains("two")); + } + + #[test] + fn applying_metadata_is_independent_of_batch_completion_order() { + let mut a = vec![ + make_group("g0", 2, &["src/a.ts"]), + make_group("g1", 1, &["src/b.ts"]), + ]; + let mut b = a.clone(); + + let mut forward = HashMap::new(); + forward.insert( + "g0".to_string(), + GroupMetadata { + id: "g0".to_string(), + risk: Some(Risk::High), + ..Default::default() + }, + ); + forward.insert( + "g1".to_string(), + GroupMetadata { + id: "g1".to_string(), + risk: Some(Risk::Low), + ..Default::default() + }, + ); + + apply_metadata(&mut a, &forward); + a.sort_by_key(|g| g.review_order); + apply_metadata(&mut b, &forward); + b.sort_by_key(|g| g.review_order); + + assert_eq!(a, b); + assert_eq!(a[0].id, "g1"); + assert_eq!(a[0].risk, Some(Risk::Low)); + } + + #[test] + fn caps_are_reenforced_on_the_consuming_side() { + let mut groups = vec![make_group("g0", 0, &["src/a.ts"])]; + let mut metadata = HashMap::new(); + metadata.insert( + "g0".to_string(), + GroupMetadata { + id: "g0".to_string(), + description: Some("first line\nsecond line\nthird".to_string()), + invariant: Some( + "Two workers never claim the same job. Also the cache stays warm.".to_string(), + ), + review_focus: vec![ + ReviewFocus::Concurrency, + ReviewFocus::DataIntegrity, + ReviewFocus::Security, + ReviewFocus::Performance, + ], + ..Default::default() + }, + ); + + apply_metadata(&mut groups, &metadata); + + assert_eq!( + groups[0].description.as_deref(), + Some("first line second line third") + ); + assert_eq!( + groups[0].invariant.as_deref(), + Some("Two workers never claim the same job.") + ); + assert_eq!(groups[0].review_focus.len(), MAX_REVIEW_FOCUS); + } + + #[test] + fn metadata_never_touches_risk_score_or_review_order() { + let mut groups = vec![make_group("g0", 4, &["src/a.ts"])]; + let (score, order) = (groups[0].risk_score, groups[0].review_order); + + let mut metadata = HashMap::new(); + metadata.insert( + "g0".to_string(), + GroupMetadata { + id: "g0".to_string(), + risk: Some(Risk::Critical), + impact: Some(ImpactScope::System), + complexity: Some(ReviewComplexity::Complex), + group_type: Some(GroupType::Fix), + ..Default::default() + }, + ); + + apply_metadata(&mut groups, &metadata); + + assert_eq!(groups[0].risk_score, score); + assert_eq!(groups[0].review_order, order); + assert_eq!(groups[0].risk, Some(Risk::Critical)); + } + + #[test] + fn omitted_fields_leave_the_deterministic_floor_alone() { + let mut groups = vec![make_group("g0", 0, &["src/a.ts"])]; + groups[0].risk = Some(Risk::Medium); + groups[0].group_type = Some(GroupType::Chore); + + let mut metadata = HashMap::new(); + metadata.insert( + "g0".to_string(), + GroupMetadata { + id: "g0".to_string(), + description: Some("Adds a retry.".to_string()), + ..Default::default() + }, + ); + + apply_metadata(&mut groups, &metadata); + + assert_eq!(groups[0].risk, Some(Risk::Medium)); + assert_eq!(groups[0].group_type, Some(GroupType::Chore)); + assert_eq!(groups[0].description.as_deref(), Some("Adds a retry.")); + } + + #[test] + fn unknown_group_ids_are_ignored() { + let mut groups = vec![make_group("g0", 0, &["src/a.ts"])]; + let mut metadata = HashMap::new(); + metadata.insert( + "hallucinated".to_string(), + GroupMetadata { + id: "hallucinated".to_string(), + risk: Some(Risk::Critical), + ..Default::default() + }, + ); + + apply_metadata(&mut groups, &metadata); + + assert_eq!(groups[0].risk, None); + } + + #[test] + fn empty_groups_produce_no_requests() { + assert!(build_metadata_requests(&[], &[], 20).is_empty()); + } + + #[test] + fn zero_batch_size_does_not_divide_by_zero() { + let groups = vec![make_group("g0", 0, &["src/a.ts"])]; + assert_eq!(build_metadata_requests(&groups, &[], 0).len(), 1); + } + + #[test] + fn prompt_states_the_caps_and_the_plain_text_rule() { + let prompt = metadata_system_prompt(); + assert!(prompt.contains("at most 3")); + assert!(prompt.contains("PLAIN TEXT")); + assert!(prompt.contains("ONE sentence")); + assert!(prompt.contains("ONE line")); + } + + /// A provider that answers every batch, after a delay that inverts completion + /// order relative to dispatch order. + struct BatchingProvider { + batches_seen: Arc>>, + delay_ms: u64, + } + + #[async_trait::async_trait] + impl LlmProvider for BatchingProvider { + fn name(&self) -> &str { + "batching-mock" + } + fn model(&self) -> &str { + "batching-mock-v1" + } + fn max_context_tokens(&self) -> usize { + 100_000 + } + async fn annotate_overview( + &self, + _: &crate::llm::schema::Pass1Request, + ) -> Result { + unimplemented!() + } + async fn annotate_group( + &self, + _: &crate::llm::schema::Pass2Request, + ) -> Result { + unimplemented!() + } + async fn evaluate_quality( + &self, + _: &crate::llm::schema::JudgeRequest, + ) -> Result { + unimplemented!() + } + async fn refine_groups( + &self, + _: &crate::llm::schema::RefinementRequest, + ) -> Result { + unimplemented!() + } + async fn describe_groups( + &self, + request: &crate::llm::schema::MetadataRequest, + ) -> Result { + let first: usize = request.groups[0] + .id + .trim_start_matches('g') + .parse() + .unwrap(); + tokio::time::sleep(std::time::Duration::from_millis( + self.delay_ms * (10 - first as u64), + )) + .await; + if let Ok(mut seen) = self.batches_seen.lock() { + seen.push(first); + } + Ok(crate::llm::schema::MetadataResponse { + groups: request + .groups + .iter() + .map(|g| GroupMetadata { + id: g.id.clone(), + description: Some(format!("desc for {}", g.id)), + risk: Some(Risk::Medium), + ..Default::default() + }) + .collect(), + }) + } + } + + #[tokio::test] + async fn every_group_gets_metadata_across_concurrent_batches() { + let mut groups: Vec = (0..9) + .map(|i| make_group(&format!("g{}", i), 9 - i, &["src/a.ts"])) + .collect(); + let provider = Arc::new(BatchingProvider { + batches_seen: Arc::new(std::sync::Mutex::new(Vec::new())), + delay_ms: 5, + }); + + run_metadata_pass(provider.clone(), &mut groups, &[], 3) + .await + .unwrap(); + + assert_eq!(groups.len(), 9); + for group in &groups { + assert!( + group.description.is_some(), + "{} missing metadata", + group.id + ); + } + + let orders: Vec = groups.iter().map(|g| g.review_order).collect(); + let mut sorted = orders.clone(); + sorted.sort_unstable(); + assert_eq!(orders, sorted, "output must be sorted by review_order"); + + let seen = provider.batches_seen.lock().unwrap().clone(); + assert_eq!(seen, vec![6, 3, 0], "batches completed out of dispatch order"); + } + + struct FailingProvider; + + #[async_trait::async_trait] + impl LlmProvider for FailingProvider { + fn name(&self) -> &str { + "failing-mock" + } + fn model(&self) -> &str { + "failing-mock-v1" + } + fn max_context_tokens(&self) -> usize { + 100_000 + } + async fn annotate_overview( + &self, + _: &crate::llm::schema::Pass1Request, + ) -> Result { + unimplemented!() + } + async fn annotate_group( + &self, + _: &crate::llm::schema::Pass2Request, + ) -> Result { + unimplemented!() + } + async fn evaluate_quality( + &self, + _: &crate::llm::schema::JudgeRequest, + ) -> Result { + unimplemented!() + } + async fn refine_groups( + &self, + _: &crate::llm::schema::RefinementRequest, + ) -> Result { + unimplemented!() + } + } + + #[tokio::test] + async fn a_provider_that_cannot_describe_leaves_groups_untouched() { + let mut groups = vec![make_group("g0", 0, &["src/a.ts"])]; + groups[0].risk = Some(Risk::Medium); + + let err = run_metadata_pass(Arc::new(FailingProvider), &mut groups, &[], 20) + .await + .unwrap_err(); + + assert!(matches!(err, LlmError::ParseResponse(_))); + assert_eq!(groups[0].risk, Some(Risk::Medium)); + assert_eq!(groups[0].description, None); + } +} diff --git a/crates/diffcore-core/src/llm/mod.rs b/crates/diffcore-core/src/llm/mod.rs index f9383574..bda75073 100644 --- a/crates/diffcore-core/src/llm/mod.rs +++ b/crates/diffcore-core/src/llm/mod.rs @@ -13,6 +13,7 @@ pub mod claude_cli; pub mod codex_cli; pub mod gemini; pub mod judge; +pub mod metadata; pub mod openai; pub mod refinement; pub mod schema; @@ -27,8 +28,8 @@ use std::future::Future; use crate::config::LlmConfig; use schema::{ - JudgeRequest, JudgeResponse, Pass1Request, Pass1Response, Pass2Request, Pass2Response, - RefinementRequest, RefinementResponse, + JudgeRequest, JudgeResponse, MetadataRequest, MetadataResponse, Pass1Request, Pass1Response, + Pass2Request, Pass2Response, RefinementRequest, RefinementResponse, }; /// Errors that can occur during LLM operations. @@ -373,6 +374,17 @@ pub trait LlmProvider: Send + Sync { &self, request: &RefinementRequest, ) -> Result; + + /// Run the group metadata pass on one batch of final flow groups. + /// + /// Returns review metadata keyed by group id. Providers that cannot serve + /// this pass fall back to the default and report themselves unsupported. + async fn describe_groups( + &self, + _request: &MetadataRequest, + ) -> Result { + Err(LlmError::UnsupportedProvider(self.name().to_string())) + } } /// Resolve the API key for an LLM provider. @@ -686,10 +698,11 @@ pub fn pass1_system_prompt() -> String { "You are a senior software engineer reviewing a code diff. \ Your task is to analyze the semantic flow groups identified by static analysis \ and provide a high-level overview of the changes.\n\n\ - Write the overall summary and each group summary so they can be reused in a pull request \ - description or shared with a non-developer reviewer. Prefer concrete behavior changes, \ - user impact, and review order rationale over jargon.\n\n\ - For each group, explain what it does, assess its risk, and suggest a review order.\n\n\ + Write the overall summary so it can be reused in a pull request description or shared \ + with a non-developer reviewer. Prefer concrete behavior changes and user impact over \ + jargon.\n\n\ + Describe the pull request as a whole. Per-group review metadata is produced by a \ + separate pass — do not restate it here.\n\n\ {}", schema::pass1_schema_description() ) diff --git a/crates/diffcore-core/src/llm/openai.rs b/crates/diffcore-core/src/llm/openai.rs index 346aa377..1c0aceb2 100644 --- a/crates/diffcore-core/src/llm/openai.rs +++ b/crates/diffcore-core/src/llm/openai.rs @@ -11,14 +11,17 @@ use serde::{Deserialize, Serialize}; use super::schema::{ flatten_json_schema, judge_json_schema, pass1_json_schema, pass2_json_schema, - refinement_json_schema, JudgeResponse, Pass1Response, Pass2Response, RefinementResponse, + refinement_json_schema, JudgeResponse, MetadataResponse, Pass1Response, Pass2Response, + RefinementResponse, }; use super::{ judge_system_prompt, judge_user_prompt, pass1_system_prompt, pass1_user_prompt, pass2_system_prompt, pass2_user_prompt, refinement_system_prompt, refinement_user_prompt, truncate_to_token_budget, LlmError, LlmProvider, }; -use crate::llm::schema::{JudgeRequest, Pass1Request, Pass2Request, RefinementRequest}; +use crate::llm::schema::{ + JudgeRequest, MetadataRequest, Pass1Request, Pass2Request, RefinementRequest, +}; const OPENAI_API_URL: &str = "https://api.openai.com/v1/chat/completions"; @@ -292,6 +295,23 @@ impl LlmProvider for OpenAIProvider { .await?; parse_json_response::(&response_text) } + + async fn describe_groups( + &self, + request: &MetadataRequest, + ) -> Result { + let system = crate::llm::metadata::metadata_system_prompt(); + let user = crate::llm::metadata::metadata_user_prompt(request); + let response_text = self + .send_structured_message( + &system, + &user, + crate::llm::schema::metadata_json_schema(), + "metadata_response", + ) + .await?; + parse_json_response::(&response_text) + } } /// Parse a JSON response, stripping any markdown fencing the LLM may add. @@ -533,7 +553,7 @@ mod tests { "suggested_review_order": ["group_1"] }"#; let result: Pass1Response = parse_json_response(json).unwrap(); - assert_eq!(result.groups.len(), 1); + assert_eq!(result.suggested_review_order, vec!["group_1".to_string()]); assert_eq!(result.overall_summary, "Auth changes"); } diff --git a/crates/diffcore-core/src/llm/refinement.rs b/crates/diffcore-core/src/llm/refinement.rs index 47ea0a3a..a2f7a467 100644 --- a/crates/diffcore-core/src/llm/refinement.rs +++ b/crates/diffcore-core/src/llm/refinement.rs @@ -315,6 +315,7 @@ pub fn apply_refinement( } else { merged_order }, + ..Default::default() }; refined_groups.retain(|g| !merge_ids.contains(g.id.as_str())); @@ -866,6 +867,7 @@ fn apply_split(source: &FlowGroup, split: &RefinementSplit, offset: usize) -> Ve // Sub-groups are read where their source group was read; they stay // adjacent because the reading-order sort is stable. review_order: source.review_order, + ..Default::default() } }) .collect() @@ -909,6 +911,7 @@ mod tests { edges: vec![], risk_score: 0.5, review_order: 0, + ..Default::default() } } @@ -1659,6 +1662,7 @@ mod tests { }], risk_score: 0.82, review_order: 1, + ..Default::default() }]; let request = build_refinement_request(&groups, None, "{}", "10 files changed"); diff --git a/crates/diffcore-core/src/llm/schema.rs b/crates/diffcore-core/src/llm/schema.rs index 3614e1cc..7928eae5 100644 --- a/crates/diffcore-core/src/llm/schema.rs +++ b/crates/diffcore-core/src/llm/schema.rs @@ -6,6 +6,11 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use crate::types::{ + GroupType, ImpactScope, ReviewComplexity, ReviewFocus, Risk, MAX_REVIEW_FOCUS, + MAX_SUMMARY_BULLETS, +}; + // ── Pass 1: Overview ── /// Pass 1 request context sent to the LLM. @@ -30,31 +35,18 @@ pub struct Pass1GroupInput { pub edge_summary: String, } -/// Pass 1 structured output: overview annotation. +/// Pass 1 structured output: PR-level overview annotation. +/// +/// Per-group narrative lives on the group itself (see `MetadataResponse` and +/// `specs/group-metadata.md` §9.1), not in a side-car keyed object. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, JsonSchema)] pub struct Pass1Response { - /// Per-group annotations. - pub groups: Vec, /// Overall summary of the entire diff. pub overall_summary: String, /// Suggested review order (group IDs). pub suggested_review_order: Vec, } -/// Per-group annotation from Pass 1. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, JsonSchema)] -pub struct Pass1GroupAnnotation { - pub id: String, - /// Human-readable name (may differ from deterministic name). - pub name: String, - /// Narrative summary of what this group does. - pub summary: String, - /// Why the LLM suggests this review order position. - pub review_order_rationale: String, - /// Risk flags identified by the LLM. - pub risk_flags: Vec, -} - // ── Pass 2: Deep Analysis ── /// Pass 2 request context for a single group. @@ -265,6 +257,106 @@ pub struct RefinementReclassify { pub reason: String, } +// ── Group Review Metadata ── + +/// Request context for one batch of the group metadata pass. +/// +/// `groups` carries full detail for the batch's own groups. `index` is a +/// read-only listing of *every* final group in the analysis, which is what makes +/// cross-group judgement (`ImpactScope::CrossCutting`) possible from inside a +/// batch. See `specs/group-metadata.md` §4.1. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct MetadataRequest { + /// The groups this batch must produce metadata for. + pub groups: Vec, + /// Every final group in the analysis, without diff content. Read-only context. + pub index: Vec, +} + +/// Full detail for a group the batch is responsible for. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct MetadataGroupDetail { + pub id: String, + pub name: String, + pub entrypoint: Option, + pub risk_score: f64, + pub files: Vec, +} + +/// A changed file as presented to the metadata pass. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct MetadataFileInput { + pub path: String, + /// Role inferred by deterministic analysis. + pub role: String, + /// The changed regions of the file, hunk by hunk. + pub diff: String, +} + +/// A group as it appears in the read-only cross-batch index. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct MetadataGroupIndexEntry { + pub id: String, + pub name: String, + pub files: Vec, + pub risk_score: f64, +} + +/// Structured output of one metadata batch. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct MetadataResponse { + pub groups: Vec, +} + +/// Review metadata for a single group. +/// +/// Every field but `id` is optional so a partial response still applies what it +/// did produce instead of discarding the batch. There is deliberately no +/// `risk_score` — the score drives review ranking and stays deterministic +/// (`specs/group-metadata.md` §1.3). +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Default)] +pub struct GroupMetadata { + pub id: String, + #[serde(default)] + pub group_type: Option, + #[serde(default)] + pub description: Option, + #[serde(default)] + pub risk: Option, + #[serde(default)] + pub impact: Option, + #[serde(default)] + pub complexity: Option, + #[serde(default)] + pub review_focus: Vec, + #[serde(default)] + pub summary: Vec, + #[serde(default)] + pub invariant: Option, +} + +/// Enum variant names as they appear on the wire, for the hand-written schema below. +/// +/// The metadata enums live in `crate::types` and do not derive `JsonSchema`, so +/// the provider-native schema is written out rather than generated. The +/// `test_metadata_enum_variant_names_match_serde` test pins these to serde. +const GROUP_TYPE_VARIANTS: &[&str] = &[ + "Feat", "Fix", "Perf", "Refactor", "Test", "Docs", "Build", "Ci", "Chore", +]; +const RISK_VARIANTS: &[&str] = &["Low", "Medium", "High", "Critical"]; +const IMPACT_VARIANTS: &[&str] = &["Local", "Module", "CrossCutting", "System"]; +const COMPLEXITY_VARIANTS: &[&str] = &["Trivial", "Simple", "Moderate", "Complex"]; +const REVIEW_FOCUS_VARIANTS: &[&str] = &[ + "Correctness", + "Security", + "Concurrency", + "Performance", + "DataIntegrity", + "Compatibility", + "ErrorHandling", + "ApiContract", +]; + // ── JSON Schema Generation ── /// Generate the JSON schema description for Pass 1 structured output. @@ -272,15 +364,6 @@ pub struct RefinementReclassify { pub fn pass1_schema_description() -> &'static str { r#"Respond with a JSON object matching this exact schema: { - "groups": [ - { - "id": "string (group ID from input)", - "name": "string (human-readable name for this change group)", - "summary": "string (1-3 sentence summary of what this group changes)", - "review_order_rationale": "string (why review this group at this position)", - "risk_flags": ["string (risk flag, e.g. 'auth_change', 'breaking_api', 'schema_change')"] - } - ], "overall_summary": "string (1-3 sentence overall summary of the entire diff)", "suggested_review_order": ["string (group IDs in suggested review order)"] }"# @@ -409,6 +492,95 @@ pub fn refinement_json_schema() -> serde_json::Value { serde_json::to_value(schemars::schema_for!(RefinementResponse)).unwrap_or_default() } +/// Generate the JSON schema description for the group metadata pass. +pub fn metadata_schema_description() -> String { + format!( + r#"Respond with a JSON object matching this exact schema: +{{ + "groups": [ + {{ + "id": "string (the group ID from the input — must be one of the IDs you were asked about)", + "group_type": "string (one of: {group_type})", + "description": "string (ONE line, plain text, no markdown: what this group changes)", + "risk": "string (one of: {risk})", + "impact": "string (one of: {impact})", + "complexity": "string (one of: {complexity})", + "review_focus": ["string (at most {max_focus}, most important first; one of: {focus})"], + "summary": ["string (plain text, no markdown, no leading bullet character)"], + "invariant": "string (ONE sentence, plain text, no markdown: the property a reviewer must verify still holds)" + }} + ] +}} + +"summary" explains what the group achieves, sized to the change: return a +single entry when one sentence covers it, and only reach for multiple entries +when the group genuinely does several things. At most {max_summary} entries, +each one short enough to scan. Do not restate the description. + +Emit exactly one entry per group you were asked about, and no entries for any other group."#, + group_type = GROUP_TYPE_VARIANTS.join(", "), + risk = RISK_VARIANTS.join(", "), + impact = IMPACT_VARIANTS.join(", "), + complexity = COMPLEXITY_VARIANTS.join(", "), + focus = REVIEW_FOCUS_VARIANTS.join(", "), + max_focus = MAX_REVIEW_FOCUS, + max_summary = MAX_SUMMARY_BULLETS, + ) +} + +/// Generate the JSON Schema for MetadataResponse. +/// +/// Hand-written rather than derived: the metadata enums live in `crate::types` +/// and do not derive `JsonSchema`. +pub fn metadata_json_schema() -> serde_json::Value { + let enum_prop = |variants: &[&str]| serde_json::json!({ "type": "string", "enum": variants }); + + serde_json::json!({ + "type": "object", + "properties": { + "groups": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { "type": "string" }, + "group_type": enum_prop(GROUP_TYPE_VARIANTS), + "description": { + "type": "string", + "description": "One line, plain text, no markdown." + }, + "risk": enum_prop(RISK_VARIANTS), + "impact": enum_prop(IMPACT_VARIANTS), + "complexity": enum_prop(COMPLEXITY_VARIANTS), + "review_focus": { + "type": "array", + "maxItems": MAX_REVIEW_FOCUS, + "items": enum_prop(REVIEW_FOCUS_VARIANTS) + }, + "summary": { + "type": "array", + "maxItems": MAX_SUMMARY_BULLETS, + "items": { + "type": "string", + "description": "Plain text, no markdown, no leading bullet character." + } + }, + "invariant": { + "type": "string", + "description": "One sentence, plain text, no markdown." + } + }, + "required": [ + "id", "group_type", "description", "risk", + "impact", "complexity", "review_focus", "summary", "invariant" + ] + } + } + }, + "required": ["groups"] + }) +} + /// Flatten a schemars-generated JSON Schema for providers that don't support /// `$ref`, `definitions`, or `$schema` (OpenAI strict mode, Gemini). /// @@ -509,16 +681,6 @@ mod tests { #[test] fn test_pass1_response_roundtrip() { let response = Pass1Response { - groups: vec![Pass1GroupAnnotation { - id: "group_1".to_string(), - name: "User authentication token refresh".to_string(), - summary: "Changes the token refresh flow to use rotating refresh tokens" - .to_string(), - review_order_rationale: - "Review first — changes auth contract that downstream groups depend on" - .to_string(), - risk_flags: vec!["auth_change".to_string(), "breaking_api".to_string()], - }], overall_summary: "Implements rotating refresh tokens and updates downstream consumers" .to_string(), suggested_review_order: vec!["group_1".to_string()], @@ -554,7 +716,6 @@ mod tests { fn test_annotations_combined() { let annotations = Annotations { overview: Some(Pass1Response { - groups: vec![], overall_summary: "test".to_string(), suggested_review_order: vec![], }), @@ -612,7 +773,7 @@ mod tests { assert!(!pass1_schema_description().is_empty()); assert!(!pass2_schema_description().is_empty()); // Should contain JSON structure markers - assert!(pass1_schema_description().contains("groups")); + assert!(pass1_schema_description().contains("suggested_review_order")); assert!(pass1_schema_description().contains("overall_summary")); assert!(pass2_schema_description().contains("group_id")); assert!(pass2_schema_description().contains("file_annotations")); @@ -621,14 +782,13 @@ mod tests { #[test] fn test_empty_pass1_response() { let response = Pass1Response { - groups: vec![], overall_summary: String::new(), suggested_review_order: vec![], }; let json = serde_json::to_string(&response).unwrap(); let deserialized: Pass1Response = serde_json::from_str(&json).unwrap(); assert_eq!(response, deserialized); - assert!(deserialized.groups.is_empty()); + assert!(deserialized.suggested_review_order.is_empty()); } #[test] @@ -647,28 +807,11 @@ mod tests { #[test] fn test_pass1_multiple_groups() { let response = Pass1Response { - groups: vec![ - Pass1GroupAnnotation { - id: "g1".to_string(), - name: "Auth flow".to_string(), - summary: "Changes auth".to_string(), - review_order_rationale: "Review first".to_string(), - risk_flags: vec!["auth_change".to_string()], - }, - Pass1GroupAnnotation { - id: "g2".to_string(), - name: "DB migration".to_string(), - summary: "Schema update".to_string(), - review_order_rationale: "Review second".to_string(), - risk_flags: vec!["schema_change".to_string(), "breaking_api".to_string()], - }, - ], overall_summary: "Auth + DB changes".to_string(), suggested_review_order: vec!["g1".to_string(), "g2".to_string()], }; let json = serde_json::to_string(&response).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); - assert_eq!(parsed["groups"].as_array().unwrap().len(), 2); assert_eq!( parsed["suggested_review_order"].as_array().unwrap().len(), 2 @@ -1045,7 +1188,6 @@ mod tests { let schema = pass1_json_schema(); assert!(schema.is_object()); let schema_str = serde_json::to_string(&schema).unwrap(); - assert!(schema_str.contains("groups")); assert!(schema_str.contains("overall_summary")); assert!(schema_str.contains("suggested_review_order")); } @@ -1149,7 +1291,6 @@ mod tests { assert_eq!(obj.get("type").and_then(|t| t.as_str()), Some("object")); assert!(obj.contains_key("properties")); let props = obj["properties"].as_object().unwrap(); - assert!(props.contains_key("groups")); assert!(props.contains_key("overall_summary")); assert!(props.contains_key("suggested_review_order")); } @@ -1214,7 +1355,6 @@ mod tests { let schema = flatten_json_schema(pass1_json_schema()); let required = schema["required"].as_array().unwrap(); let required_strs: Vec<&str> = required.iter().filter_map(|v| v.as_str()).collect(); - assert!(required_strs.contains(&"groups")); assert!(required_strs.contains(&"overall_summary")); assert!(required_strs.contains(&"suggested_review_order")); } @@ -1232,4 +1372,70 @@ mod tests { assert_ne!(p2, r); assert_ne!(j, r); } + + #[test] + fn test_metadata_enum_variant_names_match_serde() { + for v in GROUP_TYPE_VARIANTS { + serde_json::from_value::(serde_json::json!(v)) + .unwrap_or_else(|e| panic!("GroupType variant {v}: {e}")); + } + for v in RISK_VARIANTS { + serde_json::from_value::(serde_json::json!(v)) + .unwrap_or_else(|e| panic!("Risk variant {v}: {e}")); + } + for v in IMPACT_VARIANTS { + serde_json::from_value::(serde_json::json!(v)) + .unwrap_or_else(|e| panic!("ImpactScope variant {v}: {e}")); + } + for v in COMPLEXITY_VARIANTS { + serde_json::from_value::(serde_json::json!(v)) + .unwrap_or_else(|e| panic!("ReviewComplexity variant {v}: {e}")); + } + for v in REVIEW_FOCUS_VARIANTS { + serde_json::from_value::(serde_json::json!(v)) + .unwrap_or_else(|e| panic!("ReviewFocus variant {v}: {e}")); + } + } + + #[test] + fn test_metadata_response_roundtrip() { + let response = MetadataResponse { + groups: vec![GroupMetadata { + id: "group_1".to_string(), + group_type: Some(GroupType::Fix), + description: Some("Move job claiming behind a Redis lock.".to_string()), + risk: Some(Risk::High), + impact: Some(ImpactScope::CrossCutting), + complexity: Some(ReviewComplexity::Complex), + review_focus: vec![ReviewFocus::Concurrency, ReviewFocus::DataIntegrity], + summary: vec![ + "Claiming now happens under a Redis lock held for the whole claim.".to_string(), + "Workers that lose the race back off instead of proceeding.".to_string(), + ], + invariant: Some( + "Two workers must never successfully claim the same job.".to_string(), + ), + }], + }; + let json = serde_json::to_string(&response).unwrap(); + let deserialized: MetadataResponse = serde_json::from_str(&json).unwrap(); + assert_eq!(response, deserialized); + } + + #[test] + fn test_metadata_response_tolerates_missing_fields() { + let parsed: MetadataResponse = + serde_json::from_str(r#"{"groups":[{"id":"g1","risk":"Low"}]}"#).unwrap(); + assert_eq!(parsed.groups[0].risk, Some(Risk::Low)); + assert_eq!(parsed.groups[0].description, None); + assert!(parsed.groups[0].review_focus.is_empty()); + } + + #[test] + fn test_metadata_schema_caps_review_focus() { + let schema = metadata_json_schema(); + let focus = &schema["properties"]["groups"]["items"]["properties"]["review_focus"]; + assert_eq!(focus["maxItems"].as_u64(), Some(MAX_REVIEW_FOCUS as u64)); + assert!(metadata_schema_description().contains("at most 3")); + } } diff --git a/crates/diffcore-core/src/llm/vcr.rs b/crates/diffcore-core/src/llm/vcr.rs index 22a207c7..a39857c9 100644 --- a/crates/diffcore-core/src/llm/vcr.rs +++ b/crates/diffcore-core/src/llm/vcr.rs @@ -11,9 +11,10 @@ use std::path::{Path, PathBuf}; use async_trait::async_trait; use sha2::{Digest, Sha256}; +use super::metadata::metadata_system_prompt; use super::schema::{ - JudgeRequest, JudgeResponse, Pass1Request, Pass1Response, Pass2Request, Pass2Response, - RefinementRequest, RefinementResponse, + JudgeRequest, JudgeResponse, MetadataRequest, MetadataResponse, Pass1Request, Pass1Response, + Pass2Request, Pass2Response, RefinementRequest, RefinementResponse, }; use super::{ judge_system_prompt, pass1_system_prompt, pass2_system_prompt, refinement_system_prompt, @@ -109,6 +110,11 @@ impl VcrProvider { Self::sha256_hex(refinement_system_prompt().as_bytes()) } + /// Get the current prompt template hash for the group metadata pass. + pub fn metadata_template_hash() -> String { + Self::sha256_hex(metadata_system_prompt().as_bytes()) + } + /// Build the cache file path for a given pass type and cache key. fn cache_path(&self, pass_type: &str, cache_key: &str) -> PathBuf { self.cache_dir @@ -362,6 +368,47 @@ impl LlmProvider for VcrProvider { } } } + + async fn describe_groups( + &self, + request: &MetadataRequest, + ) -> Result { + let request_json = serde_json::to_string(request).map_err(|e| { + LlmError::ParseResponse(format!("Failed to serialize request for VCR key: {}", e)) + })?; + let template_hash = Self::metadata_template_hash(); + let key = Self::cache_key( + self.inner.name(), + self.inner.model(), + &request_json, + &template_hash, + ); + let path = self.cache_path("metadata", &key); + + match self.mode { + VcrMode::Replay => self + .read_cache::(&path, &template_hash) + .ok_or_else(|| { + LlmError::ParseResponse(format!( + "VCR replay: no cached entry at {}", + path.display() + )) + }), + VcrMode::Record => { + let response = self.inner.describe_groups(request).await?; + self.write_cache(&path, &key, &template_hash, &response)?; + Ok(response) + } + VcrMode::Auto => { + if let Some(cached) = self.read_cache::(&path, &template_hash) { + return Ok(cached); + } + let response = self.inner.describe_groups(request).await?; + self.write_cache(&path, &key, &template_hash, &response)?; + Ok(response) + } + } + } } #[cfg(test)] @@ -375,8 +422,7 @@ impl LlmProvider for VcrProvider { mod tests { use super::*; use crate::llm::schema::{ - JudgeCriterionScore, JudgeSourceFile, Pass1GroupAnnotation, Pass1GroupInput, - Pass2FileAnnotation, Pass2FileInput, RefinementGroupInput, + JudgeCriterionScore, JudgeSourceFile, Pass1GroupInput, Pass2FileAnnotation, Pass2FileInput, }; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; @@ -411,13 +457,6 @@ mod tests { ) -> Result { self.call_count.fetch_add(1, Ordering::SeqCst); Ok(Pass1Response { - groups: vec![Pass1GroupAnnotation { - id: "g1".to_string(), - name: "Mock group".to_string(), - summary: "Mock summary".to_string(), - review_order_rationale: "Mock rationale".to_string(), - risk_flags: vec!["mock_flag".to_string()], - }], overall_summary: "Mock overall".to_string(), suggested_review_order: vec!["g1".to_string()], }) @@ -523,7 +562,7 @@ mod tests { // Record let vcr = VcrProvider::new(Box::new(mock), tmp.path().to_path_buf(), VcrMode::Record); let response = vcr.annotate_overview(&sample_pass1()).await.unwrap(); - assert_eq!(response.groups[0].id, "g1"); + assert_eq!(response.suggested_review_order, vec!["g1".to_string()]); assert_eq!(call_count.load(Ordering::SeqCst), 1); // Replay with a new mock (should not be called) @@ -531,7 +570,7 @@ mod tests { let mock2 = MockProvider::new(call_count2.clone()); let vcr2 = VcrProvider::new(Box::new(mock2), tmp.path().to_path_buf(), VcrMode::Replay); let replayed = vcr2.annotate_overview(&sample_pass1()).await.unwrap(); - assert_eq!(replayed.groups[0].id, "g1"); + assert_eq!(replayed.suggested_review_order, vec!["g1".to_string()]); assert_eq!(replayed.overall_summary, "Mock overall"); assert_eq!( call_count2.load(Ordering::SeqCst), @@ -856,7 +895,6 @@ mod tests { prompt_template_hash: "def456".to_string(), recorded_at: "2026-03-19T00:00:00Z".to_string(), response: Pass1Response { - groups: vec![], overall_summary: "test".to_string(), suggested_review_order: vec![], }, @@ -905,7 +943,6 @@ mod tests { prompt_template_hash: "old_template_hash".to_string(), recorded_at: "2026-01-01T00:00:00Z".to_string(), response: Pass1Response { - groups: vec![], overall_summary: "stale".to_string(), suggested_review_order: vec![], }, @@ -1015,7 +1052,6 @@ mod tests { prompt_template_hash: "tmpl".to_string(), recorded_at: "2026-01-01T00:00:00Z".to_string(), response: Pass1Response { - groups: vec![], overall_summary: summary.clone(), suggested_review_order: vec![], }, diff --git a/crates/diffcore-core/src/manifest.rs b/crates/diffcore-core/src/manifest.rs index 6b1cdd79..0e4e2356 100644 --- a/crates/diffcore-core/src/manifest.rs +++ b/crates/diffcore-core/src/manifest.rs @@ -168,6 +168,7 @@ pub fn import_manifest( edges, risk_score, review_order: mg.review_order, + ..Default::default() }); } @@ -274,6 +275,7 @@ mod tests { }], risk_score: 0.65, review_order: 1, + ..Default::default() }, FlowGroup { id: "group_2".to_string(), @@ -289,6 +291,7 @@ mod tests { edges: vec![], risk_score: 0.2, review_order: 2, + ..Default::default() }, ], infrastructure_group: Some(InfrastructureGroup { diff --git a/crates/diffcore-core/src/output.rs b/crates/diffcore-core/src/output.rs index 86ccc7ff..b82521d5 100644 --- a/crates/diffcore-core/src/output.rs +++ b/crates/diffcore-core/src/output.rs @@ -98,6 +98,8 @@ pub fn build_analysis_output( .then_with(|| crate::rank::natural_group_key(&a.id).cmp(&crate::rank::natural_group_key(&b.id))) }); + crate::group_metadata::apply_heuristic_metadata(&mut groups); + let frameworks_detected = crate::flow::detect_frameworks(parsed_files); let summary = AnalysisSummary { @@ -460,6 +462,7 @@ mod tests { ], risk_score: 0.0, review_order: 0, + ..Default::default() } } @@ -484,6 +487,7 @@ mod tests { edges: vec![], risk_score: 0.0, review_order: 0, + ..Default::default() }, ], infrastructure: Some(InfrastructureGroup { @@ -1017,6 +1021,7 @@ mod tests { edges: vec![], risk_score: 0.0, review_order: 0, + ..Default::default() }; let mermaid = generate_mermaid(&group); assert!(mermaid.contains("graph TD")); @@ -1065,6 +1070,7 @@ mod tests { ], risk_score: 0.0, review_order: 0, + ..Default::default() }; let mermaid = generate_mermaid(&group); // Both edges are between the same files, so only one Mermaid edge. @@ -1098,6 +1104,7 @@ mod tests { }], risk_score: 0.0, review_order: 0, + ..Default::default() }; let mermaid = generate_mermaid(&group); // Should not have any edges (self-edge on same file). @@ -1123,6 +1130,7 @@ mod tests { edges: vec![], risk_score: 0.0, review_order: 0, + ..Default::default() }; let mermaid = generate_mermaid(&group); // Label should be "handlers/auth.ts" not the full path. @@ -1176,6 +1184,7 @@ mod tests { }], risk_score: 0.0, review_order: 0, + ..Default::default() }; let mermaid = generate_mermaid(&group); assert!( diff --git a/crates/diffcore-core/src/rank.rs b/crates/diffcore-core/src/rank.rs index a24562d6..43fb1a8a 100644 --- a/crates/diffcore-core/src/rank.rs +++ b/crates/diffcore-core/src/rank.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use crate::cluster::ClusterResult; -use crate::types::{GroupRankInput, RankWeights, RankedGroup}; +use crate::types::{FlowGroup, GroupRankInput, RankWeights, RankedGroup}; /// Compute the composite ranking score for a single group. /// @@ -103,28 +103,53 @@ pub fn build_rank_inputs( cluster_result .groups .iter() - .map(|group| { - let paths: Vec<&str> = group.files.iter().map(|f| f.path.as_str()).collect(); - let risk_flags = crate::output::compute_group_risk_flags(&paths); - let total_add: u32 = group.files.iter().map(|f| f.changes.additions).sum(); - let total_del: u32 = group.files.iter().map(|f| f.changes.deletions).sum(); - - GroupRankInput { - group_id: group.id.clone(), - risk: compute_risk_score( - risk_flags.has_schema_change, - risk_flags.has_api_change, - risk_flags.has_auth_change, - false, - ), - centrality: compute_group_centrality(&paths, file_centrality), - surface_area: compute_surface_area(total_add, total_del, 1000), - uncertainty: if risk_flags.has_test_only { 0.1 } else { 0.5 }, - } - }) + .map(|group| rank_input_for_group(group, file_centrality)) .collect() } +/// Build the ranking input for a single group from its own files. +pub fn rank_input_for_group( + group: &FlowGroup, + file_centrality: &HashMap, +) -> GroupRankInput { + let paths: Vec<&str> = group.files.iter().map(|f| f.path.as_str()).collect(); + let risk_flags = crate::output::compute_group_risk_flags(&paths); + let total_add: u32 = group.files.iter().map(|f| f.changes.additions).sum(); + let total_del: u32 = group.files.iter().map(|f| f.changes.deletions).sum(); + + GroupRankInput { + group_id: group.id.clone(), + risk: compute_risk_score( + risk_flags.has_schema_change, + risk_flags.has_api_change, + risk_flags.has_auth_change, + false, + ), + centrality: compute_group_centrality(&paths, file_centrality), + surface_area: compute_surface_area(total_add, total_del, 1000), + uncertainty: if risk_flags.has_test_only { 0.1 } else { 0.5 }, + } +} + +/// Recompute `risk_score` in place for groups whose file composition changed +/// after the initial ranking. +/// +/// LLM refinement splits and merges groups without re-scoring: `apply_split` +/// makes sub-groups inherit their source's score and `apply_merge` leaves the +/// merged group at 0.0, so a merged group sorts as the least risky thing in the +/// diff. `review_order` is deliberately left alone — refinement may have +/// re-ranked on purpose, and that decision outranks the composite score. +pub fn rescore_groups( + groups: &mut [FlowGroup], + file_centrality: &HashMap, + weights: &RankWeights, +) { + for group in groups.iter_mut() { + let input = rank_input_for_group(group, file_centrality); + group.risk_score = composite_score(&input, weights); + } +} + /// Compute a risk score from file-level risk indicators. /// /// Each indicator contributes to the risk score: diff --git a/crates/diffcore-core/src/types.rs b/crates/diffcore-core/src/types.rs index 6d481ad1..38ea573c 100644 --- a/crates/diffcore-core/src/types.rs +++ b/crates/diffcore-core/src/types.rs @@ -90,8 +90,69 @@ pub enum EntrypointType { EffectService, } +/// The kind of change a group represents, in conventional-commit terms. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum GroupType { + Feat, + Fix, + Perf, + Refactor, + Test, + Docs, + Build, + Ci, + Chore, +} + +/// Coarse risk band. Derived from [`FlowGroup::risk_score`], never the reverse: +/// the score drives review ranking and stays deterministic. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum Risk { + Low, + Medium, + High, + Critical, +} + +/// How far the blast radius of a group's changes reaches. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum ImpactScope { + Local, + Module, + CrossCutting, + System, +} + +/// How much effort reviewing a group is expected to take. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum ReviewComplexity { + Trivial, + Simple, + Moderate, + Complex, +} + +/// What a reviewer should be looking for while reading a group. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum ReviewFocus { + Correctness, + Security, + Concurrency, + Performance, + DataIntegrity, + Compatibility, + ErrorHandling, + ApiContract, +} + +/// Maximum number of [`FlowGroup::summary`] bullets a group may carry. +pub const MAX_SUMMARY_BULLETS: usize = 5; + +/// Maximum number of [`ReviewFocus`] entries a group may carry. +pub const MAX_REVIEW_FOCUS: usize = 3; + /// A semantic flow group — a set of files participating in the same data flow. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] pub struct FlowGroup { pub id: String, pub name: String, @@ -100,6 +161,34 @@ pub struct FlowGroup { pub edges: Vec, pub risk_score: f64, pub review_order: u32, + + // ── Review metadata (see specs/group-metadata.md) ── + /// What kind of change this is. + #[serde(default)] + pub group_type: Option, + /// One line, plain text: what changed. + #[serde(default)] + pub description: Option, + /// Risk band derived from `risk_score`. + #[serde(default)] + pub risk: Option, + /// Blast radius of the change. + #[serde(default)] + pub impact: Option, + /// Expected review effort. + #[serde(default)] + pub complexity: Option, + /// At most [`MAX_REVIEW_FOCUS`] concerns to review against. + #[serde(default)] + pub review_focus: Vec, + /// What the group achieves, sized to the change: one entry when a single + /// sentence covers it, otherwise up to [`MAX_SUMMARY_BULLETS`] bullets. + /// Plain-text entries — the list is the structure, so nothing is markdown. + #[serde(default)] + pub summary: Vec, + /// One sentence, plain text: the property a reviewer should verify. + #[serde(default)] + pub invariant: Option, } /// Risk indicators detected in changed files. @@ -310,6 +399,7 @@ mod tests { edges: vec![sample_flow_edge()], risk_score: 0.82, review_order: 1, + ..Default::default() } } @@ -496,6 +586,7 @@ mod tests { edges: vec![], risk_score: 0.1, review_order: 5, + ..Default::default() }; let json = serde_json::to_string(&g).unwrap(); let back: FlowGroup = serde_json::from_str(&json).unwrap(); @@ -740,6 +831,7 @@ mod tests { edges: vec![], risk_score: 0.0, review_order: 0, + ..Default::default() }; let json = serde_json::to_string(&g).unwrap(); let back: FlowGroup = serde_json::from_str(&json).unwrap(); @@ -1173,4 +1265,30 @@ mod tests { "empty sub_groups should not be serialized" ); } + + /// Analysis JSON written before review metadata existed must still load. + #[test] + fn flow_group_without_metadata_fields_deserializes() { + let legacy = r#"{ + "id": "group_1", + "name": "Legacy group", + "entrypoint": null, + "files": [], + "edges": [], + "risk_score": 0.42, + "review_order": 1 + }"#; + + let group: FlowGroup = serde_json::from_str(legacy).unwrap(); + + assert_eq!(group.id, "group_1"); + assert!((group.risk_score - 0.42).abs() < f64::EPSILON); + assert!(group.group_type.is_none()); + assert!(group.risk.is_none()); + assert!(group.impact.is_none()); + assert!(group.complexity.is_none()); + assert!(group.description.is_none()); + assert!(group.invariant.is_none()); + assert!(group.review_focus.is_empty()); + } } diff --git a/crates/diffcore-core/tests/eval_suite.rs b/crates/diffcore-core/tests/eval_suite.rs index 7eea816a..74507795 100644 --- a/crates/diffcore-core/tests/eval_suite.rs +++ b/crates/diffcore-core/tests/eval_suite.rs @@ -244,6 +244,102 @@ fn test_eval_all_fixtures_risk_bounds() { } } +/// Structural contract for group review metadata across all fixtures — spec §7. +/// +/// Content quality of `description` / `invariant` is deliberately not asserted: +/// scoring prose against a golden string makes the corpus fail whenever a +/// provider ships a new checkpoint. Structure is asserted on every group of +/// every run. +#[test] +fn test_eval_all_fixtures_group_metadata_contract() { + use diffcore_core::types::{MAX_REVIEW_FOCUS, MAX_SUMMARY_BULLETS}; + + for &name in FIXTURE_NAMES { + let (rb, baseline) = build_fixture(name).unwrap(); + let branch = find_feature_branch(rb.path()); + let output = run_pipeline(rb.path(), "main", &branch); + + for group in &output.groups { + // The heuristic floor promises risk and impact on every group, + // with or without an LLM. + assert!( + group.risk.is_some(), + "[{}] group '{}' has no risk band", + baseline.name, + group.name, + ); + assert!( + group.impact.is_some(), + "[{}] group '{}' has no impact scope", + baseline.name, + group.name, + ); + + // The floor never invents the subjective fields. + assert!( + group.description.is_none() + && group.invariant.is_none() + && group.complexity.is_none() + && group.review_focus.is_empty() + && group.summary.is_empty(), + "[{}] group '{}' has LLM-only metadata on a deterministic run", + baseline.name, + group.name, + ); + + assert!( + group.review_focus.len() <= MAX_REVIEW_FOCUS, + "[{}] group '{}' has {} review_focus entries, cap is {}", + baseline.name, + group.name, + group.review_focus.len(), + MAX_REVIEW_FOCUS, + ); + + assert!( + group.summary.len() <= MAX_SUMMARY_BULLETS, + "[{}] group '{}' has {} summary bullets, cap is {}", + baseline.name, + group.name, + group.summary.len(), + MAX_SUMMARY_BULLETS, + ); + + for bullet in &group.summary { + assert!( + !bullet.starts_with('-') && !bullet.starts_with('*'), + "[{}] group '{}' summary bullet carries its own marker: {:?}", + baseline.name, + group.name, + bullet, + ); + } + + if let Some(description) = &group.description { + assert!( + !description.contains('\n'), + "[{}] group '{}' description is not a single line", + baseline.name, + group.name, + ); + } + + // The risk band must agree with the score it is derived from, + // so the label can never contradict the ranking. + let expected_band_is_low = group.risk_score < 0.35; + assert_eq!( + group.risk == Some(diffcore_core::types::Risk::Low), + expected_band_is_low, + "[{}] group '{}' risk band {:?} disagrees with risk_score {}", + baseline.name, + group.name, + group.risk, + group.risk_score, + ); + } + } +} + /// Mermaid diagrams should be generated for all groups across all fixtures. #[test] fn test_eval_all_fixtures_mermaid() { @@ -360,6 +456,7 @@ mod scoring_properties { edges: vec![], risk_score: risk_raw.min(1.0), review_order: order, + ..Default::default() }); (prop::collection::vec(arb_group, 0..5), 0u32..50).prop_map(|(groups, extra_files)| { @@ -505,6 +602,7 @@ mod scoring_properties { edges: vec![], risk_score: 0.5, review_order: (i + 1) as u32, + ..Default::default() } }).collect(); diff --git a/crates/diffcore-core/tests/llm_live.rs b/crates/diffcore-core/tests/llm_live.rs index 3765de26..75c83b85 100644 --- a/crates/diffcore-core/tests/llm_live.rs +++ b/crates/diffcore-core/tests/llm_live.rs @@ -40,7 +40,6 @@ async fn test_live_anthropic_pass1() { let response = provider.annotate_overview(&request).await.unwrap(); // Verify structured output - assert!(!response.groups.is_empty(), "Should have group annotations"); assert!( !response.overall_summary.is_empty(), "Should have overall summary" @@ -51,26 +50,17 @@ async fn test_live_anthropic_pass1() { ); // Verify group IDs match input - let response_ids: Vec<&str> = response.groups.iter().map(|g| g.id.as_str()).collect(); + let response_ids: Vec<&str> = response + .suggested_review_order + .iter() + .map(|s| s.as_str()) + .collect(); assert!( response_ids.contains(&"group_1"), - "Should annotate group_1, got: {:?}", + "Should order group_1, got: {:?}", response_ids ); - // Each group should have meaningful content - for group in &response.groups { - assert!(!group.name.is_empty(), "Group name should not be empty"); - assert!( - !group.summary.is_empty(), - "Group summary should not be empty" - ); - assert!( - !group.review_order_rationale.is_empty(), - "Review rationale should not be empty" - ); - } - eprintln!("Pass 1 response: {:?}", response); } @@ -136,7 +126,6 @@ async fn test_live_openai_pass1() { let request = sample_pass1_request(); let response = provider.annotate_overview(&request).await.unwrap(); - assert!(!response.groups.is_empty(), "Should have group annotations"); assert!( !response.overall_summary.is_empty(), "Should have overall summary" @@ -146,8 +135,12 @@ async fn test_live_openai_pass1() { "Should have review order" ); - let response_ids: Vec<&str> = response.groups.iter().map(|g| g.id.as_str()).collect(); - assert!(response_ids.contains(&"group_1"), "Should annotate group_1"); + let response_ids: Vec<&str> = response + .suggested_review_order + .iter() + .map(|s| s.as_str()) + .collect(); + assert!(response_ids.contains(&"group_1"), "Should order group_1"); eprintln!("OpenAI Pass 1 response: {:?}", response); } @@ -257,7 +250,7 @@ async fn test_live_end_to_end_pipeline() { // Pass 1: Overview let pass1_request = sample_pass1_request(); let pass1_response = provider.annotate_overview(&pass1_request).await.unwrap(); - assert!(!pass1_response.groups.is_empty()); + assert!(!pass1_response.overall_summary.is_empty()); // Pass 2: Deep analysis on the first group let pass2_request = sample_pass2_request(); @@ -348,7 +341,6 @@ async fn test_live_gemini_pass1() { let response = provider.annotate_overview(&request).await.unwrap(); // Verify structured output - assert!(!response.groups.is_empty(), "Should have group annotations"); assert!( !response.overall_summary.is_empty(), "Should have overall summary" @@ -359,26 +351,17 @@ async fn test_live_gemini_pass1() { ); // Verify group IDs match input - let response_ids: Vec<&str> = response.groups.iter().map(|g| g.id.as_str()).collect(); + let response_ids: Vec<&str> = response + .suggested_review_order + .iter() + .map(|s| s.as_str()) + .collect(); assert!( response_ids.contains(&"group_1"), - "Should annotate group_1, got: {:?}", + "Should order group_1, got: {:?}", response_ids ); - // Each group should have meaningful content - for group in &response.groups { - assert!(!group.name.is_empty(), "Group name should not be empty"); - assert!( - !group.summary.is_empty(), - "Group summary should not be empty" - ); - assert!( - !group.review_order_rationale.is_empty(), - "Review rationale should not be empty" - ); - } - eprintln!("Gemini Pass 1 response: {:?}", response); } @@ -479,7 +462,7 @@ async fn test_live_gemini_context_window_handling() { // A normal request should work fine within the context window let request = sample_pass1_request(); let response = provider.annotate_overview(&request).await.unwrap(); - assert!(!response.groups.is_empty()); + assert!(!response.overall_summary.is_empty()); } #[tokio::test] diff --git a/crates/diffcore-core/tests/llm_provider_audit.rs b/crates/diffcore-core/tests/llm_provider_audit.rs index 9bd6936d..5a821201 100644 --- a/crates/diffcore-core/tests/llm_provider_audit.rs +++ b/crates/diffcore-core/tests/llm_provider_audit.rs @@ -87,7 +87,6 @@ fn valid_anthropic_pass1_tool_use() -> String { "id": "toolu_mock", "name": "structured_output", "input": { - "groups": [{"id": "group_1", "name": "Auth flow", "summary": "Changes auth", "review_order_rationale": "Review first", "risk_flags": ["auth_change"]}], "overall_summary": "Auth changes", "suggested_review_order": ["group_1"] } @@ -102,7 +101,7 @@ fn valid_anthropic_pass1_tool_use() -> String { fn valid_openai_pass1() -> String { r#"{ "choices": [{ - "message": {"role": "assistant", "content": "{\"groups\": [{\"id\": \"group_1\", \"name\": \"Auth flow\", \"summary\": \"Changes auth\", \"review_order_rationale\": \"Review first\", \"risk_flags\": [\"auth_change\"]}], \"overall_summary\": \"Auth changes\", \"suggested_review_order\": [\"group_1\"]}"}, + "message": {"role": "assistant", "content": "{\"overall_summary\": \"Auth changes\", \"suggested_review_order\": [\"group_1\"]}"}, "finish_reason": "stop" }], "model": "gpt-4.1", @@ -116,7 +115,7 @@ fn valid_gemini_pass1() -> String { r#"{ "candidates": [{ "content": { - "parts": [{"text": "{\"groups\": [{\"id\": \"group_1\", \"name\": \"Auth flow\", \"summary\": \"Changes auth\", \"review_order_rationale\": \"Review first\", \"risk_flags\": [\"auth_change\"]}], \"overall_summary\": \"Auth changes\", \"suggested_review_order\": [\"group_1\"]}"}], + "parts": [{"text": "{\"overall_summary\": \"Auth changes\", \"suggested_review_order\": [\"group_1\"]}"}], "role": "model" }, "finishReason": "STOP" @@ -1181,8 +1180,7 @@ async fn test_anthropic_valid_pass1_response() { let result = provider.annotate_overview(&sample_pass1_request()).await; let response = result.unwrap(); - assert_eq!(response.groups.len(), 1); - assert_eq!(response.groups[0].id, "group_1"); + assert_eq!(response.suggested_review_order, vec!["group_1".to_string()]); assert_eq!(response.overall_summary, "Auth changes"); } @@ -1198,8 +1196,7 @@ async fn test_openai_valid_pass1_response() { let result = provider.annotate_overview(&sample_pass1_request()).await; let response = result.unwrap(); - assert_eq!(response.groups.len(), 1); - assert_eq!(response.groups[0].id, "group_1"); + assert_eq!(response.suggested_review_order, vec!["group_1".to_string()]); } #[tokio::test] @@ -1215,8 +1212,7 @@ async fn test_gemini_valid_pass1_response() { let result = provider.annotate_overview(&sample_pass1_request()).await; let response = result.unwrap(); - assert_eq!(response.groups.len(), 1); - assert_eq!(response.groups[0].id, "group_1"); + assert_eq!(response.suggested_review_order, vec!["group_1".to_string()]); } // ═══════════════════════════════════════════════════════════════ diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__cross_cutting_refactor.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__cross_cutting_refactor.snap index e66f7c2a..29602904 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__cross_cutting_refactor.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__cross_cutting_refactor.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [ { "edge_type": "Imports", @@ -128,10 +130,16 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_1", + "impact": "Module", + "invariant": null, "name": "services (connected)", + "review_focus": [], "review_order": 1, - "risk_score": 0.35 + "risk": "Medium", + "risk_score": 0.35, + "summary": [] } ], "infrastructure_group": null, diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__go_http_api.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__go_http_api.snap index cb4c45a3..087e7b71 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__go_http_api.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__go_http_api.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [], "entrypoint": { "entrypoint_type": "HttpRoute", @@ -51,10 +53,16 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_1", + "impact": "Module", + "invariant": null, "name": "CreateUser (user) route", + "review_focus": [], "review_order": 1, - "risk_score": 0.4375 + "risk": "Medium", + "risk_score": 0.4375, + "summary": [] } ], "infrastructure_group": null, diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__infrastructure_heavy.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__infrastructure_heavy.snap index fbc782dd..05acee82 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__infrastructure_heavy.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__infrastructure_heavy.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [], "entrypoint": null, "files": [ @@ -27,10 +29,16 @@ expression: v "symbols_changed": [] } ], + "group_type": "Docs", "id": "group_1", + "impact": "Local", + "invariant": null, "name": "docs (directory)", + "review_focus": [], "review_order": 1, - "risk_score": 0.35 + "risk": "Medium", + "risk_score": 0.35, + "summary": [] } ], "infrastructure_group": { diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__mixed_language.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__mixed_language.snap index 27c252c2..2efef93f 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__mixed_language.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__mixed_language.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [ { "edge_type": "Imports", @@ -52,12 +54,20 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_1", + "impact": "Module", + "invariant": null, "name": "getDashboard (dashboard) route", + "review_focus": [], "review_order": 1, - "risk_score": 0.4375 + "risk": "Medium", + "risk_score": 0.4375, + "summary": [] }, { + "complexity": null, + "description": null, "edges": [], "entrypoint": { "entrypoint_type": "HttpRoute", @@ -86,10 +96,16 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_2", + "impact": "Module", + "invariant": null, "name": "get_analytics (analytics) route", + "review_focus": [], "review_order": 2, - "risk_score": 0.3125 + "risk": "Low", + "risk_score": 0.3125, + "summary": [] } ], "infrastructure_group": null, diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__multi_entrypoint.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__multi_entrypoint.snap index 390d198e..9e5780a8 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__multi_entrypoint.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__multi_entrypoint.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [ { "edge_type": "Imports", @@ -92,10 +94,16 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_1", + "impact": "Module", + "invariant": null, "name": "createOrder (orders) route", + "review_focus": [], "review_order": 1, - "risk_score": 0.4375 + "risk": "Medium", + "risk_score": 0.4375, + "summary": [] } ], "infrastructure_group": { diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__python_fastapi.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__python_fastapi.snap index f590cb8e..848e2745 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__python_fastapi.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__python_fastapi.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [], "entrypoint": { "entrypoint_type": "HttpRoute", @@ -61,10 +63,16 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_1", + "impact": "Module", + "invariant": null, "name": "post_item (items) route", + "review_focus": [], "review_order": 1, - "risk_score": 0.5425 + "risk": "Medium", + "risk_score": 0.5425, + "summary": [] } ], "infrastructure_group": null, diff --git a/crates/diffcore-core/tests/snapshots/snapshot_tests__simple_express_app.snap b/crates/diffcore-core/tests/snapshots/snapshot_tests__simple_express_app.snap index e32e1693..4d246a7e 100644 --- a/crates/diffcore-core/tests/snapshots/snapshot_tests__simple_express_app.snap +++ b/crates/diffcore-core/tests/snapshots/snapshot_tests__simple_express_app.snap @@ -13,6 +13,8 @@ expression: v }, "groups": [ { + "complexity": null, + "description": null, "edges": [ { "edge_type": "Imports", @@ -72,12 +74,20 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_2", + "impact": "Module", + "invariant": null, "name": "postUser (users) route", + "review_focus": [], "review_order": 1, - "risk_score": 0.4375 + "risk": "Medium", + "risk_score": 0.4375, + "summary": [] }, { + "complexity": null, + "description": null, "edges": [], "entrypoint": { "entrypoint_type": "HttpRoute", @@ -96,10 +106,16 @@ expression: v "symbols_changed": [] } ], + "group_type": null, "id": "group_1", + "impact": "Local", + "invariant": null, "name": "healthCheck (health) route", + "review_focus": [], "review_order": 2, - "risk_score": 0.2924685094576832 + "risk": "Low", + "risk_score": 0.2924685094576832, + "summary": [] } ], "infrastructure_group": null, diff --git a/crates/diffcore-core/tests/vcr_integration.rs b/crates/diffcore-core/tests/vcr_integration.rs index 0f8f714a..6144c882 100644 --- a/crates/diffcore-core/tests/vcr_integration.rs +++ b/crates/diffcore-core/tests/vcr_integration.rs @@ -40,14 +40,6 @@ async fn test_replay_from_prerecorded_pass1_fixture() { // Write a pre-recorded fixture let fixture_response = Pass1Response { - groups: vec![diffcore_core::llm::schema::Pass1GroupAnnotation { - id: "group_1".to_string(), - name: "User registration flow".to_string(), - summary: "Adds a new user registration endpoint with validation and persistence." - .to_string(), - review_order_rationale: "Core feature change, review first.".to_string(), - risk_flags: vec!["new_endpoint".to_string()], - }], overall_summary: "New user registration flow with validation.".to_string(), suggested_review_order: vec!["group_1".to_string(), "group_2".to_string()], }; @@ -113,7 +105,7 @@ async fn test_replay_from_prerecorded_pass1_fixture() { let result = vcr.annotate_overview(&request).await.unwrap(); assert_eq!(result, fixture_response); - assert_eq!(result.groups[0].id, "group_1"); + assert_eq!(result.suggested_review_order[0], "group_1"); assert_eq!( result.overall_summary, "New user registration flow with validation." @@ -242,7 +234,6 @@ async fn test_auto_mode_records_on_first_call_replays_on_second() { ) -> Result { self.count.fetch_add(1, Ordering::SeqCst); Ok(Pass1Response { - groups: vec![], overall_summary: "counted".to_string(), suggested_review_order: vec![], }) @@ -340,7 +331,6 @@ async fn test_live_vcr_record_replay_anthropic() { let request = sample_pass1_request(); let recorded = vcr_record.annotate_overview(&request).await.unwrap(); - assert!(!recorded.groups.is_empty()); assert!(!recorded.overall_summary.is_empty()); // Verify cache file was written diff --git a/crates/diffcore-tauri/src/commands.rs b/crates/diffcore-tauri/src/commands.rs index 84381704..89fa27ef 100644 --- a/crates/diffcore-tauri/src/commands.rs +++ b/crates/diffcore-tauri/src/commands.rs @@ -36,7 +36,10 @@ use diffcore_core::types::AnalysisOutput; /// Application state shared across commands. pub struct AppState { /// The most recent analysis result, available for subsequent queries. - pub last_analysis: Mutex>, + /// + /// `Arc` because the streaming refinement job persists its result from a + /// spawned `'static` task, which cannot borrow `State<'_, AppState>`. + pub last_analysis: Arc>>, /// Cached diff result from the most recent analysis, for instant file diff lookups. pub last_diff: Mutex>, /// Background LLM job manager for live SSE activity streams. @@ -45,6 +48,10 @@ pub struct AppState { pub activity_stream_base_url: Mutex>, /// Cache key from the most recent analysis, for refinement cache lookups. pub last_cache_key: Mutex>, + /// Per-file centrality from the most recent analysis. Refinement rewrites + /// group composition and has to re-score, which needs these; the symbol + /// graph they come from is gone by then. + pub last_file_centrality: Mutex>>, /// Path to the currently watched manifest file. pub watched_manifest_path: Mutex>, /// In-flight refinement tasks keyed by job_id, so the user can cancel them. @@ -65,11 +72,12 @@ pub struct CachedDiff { impl AppState { pub fn new() -> Self { Self { - last_analysis: Mutex::new(None), + last_analysis: Arc::new(Mutex::new(None)), last_diff: Mutex::new(None), activity_manager: Arc::new(activity_stream::ActivityManager::new()), activity_stream_base_url: Mutex::new(None), last_cache_key: Mutex::new(None), + last_file_centrality: Mutex::new(None), watched_manifest_path: Mutex::new(None), refinement_jobs: Arc::new(Mutex::new(HashMap::new())), git_head_watch_generation: Arc::new(AtomicU64::new(0)), @@ -337,6 +345,10 @@ pub fn analyze( *key = Some(cache_key); } + if let Ok(mut centrality) = state.last_file_centrality.lock() { + *centrality = Some(file_centrality); + } + // Store for subsequent queries match state.last_analysis.lock() { Ok(mut last) => *last = Some(analysis_output.clone()), @@ -766,11 +778,35 @@ async fn run_group_with_activity( .map_err(|e| CommandError::Llm(format!("{}", e))) } +/// Bring refined groups back into a consistent state. +/// +/// `apply_refinement_lenient` rewrites group composition without re-scoring — +/// merged groups come back at `risk_score: 0.0` — and the heuristic metadata +/// derived from that score is stale for every group it touched. Both are +/// recomputed here, before any LLM metadata pass runs on top. +/// +/// Centrality needs the symbol graph, which is gone by refinement time, so it +/// comes from `AppState::last_file_centrality`. Without it the scores would be +/// wrong in a different way, so scoring is skipped rather than guessed. +fn finalize_refined_groups( + groups: &mut [diffcore_core::types::FlowGroup], + file_centrality: Option<&HashMap>, + weights: &diffcore_core::types::RankWeights, +) { + match file_centrality { + Some(centrality) => diffcore_core::rank::rescore_groups(groups, centrality, weights), + None => warn!("No cached centrality; refined groups keep their pre-refinement risk scores"), + } + diffcore_core::group_metadata::apply_heuristic_metadata(groups); +} + async fn run_refinement_with_activity( analysis: AnalysisOutput, refinement_llm_config: diffcore_core::config::LlmConfig, workdir: Option, job: JobHandle, + file_centrality: Option>, + weights: diffcore_core::types::RankWeights, ) -> Result { emit_diffcore_activity(&job, "Preparing refinement request").await; let provider = llm::create_provider_for_workdir(&refinement_llm_config, workdir.as_deref()) @@ -837,12 +873,14 @@ async fn run_refinement_with_activity( }); } - let (refined_groups, infra, warnings) = refinement::apply_refinement_lenient( + let (mut refined_groups, infra, warnings) = refinement::apply_refinement_lenient( &analysis.groups, analysis.infrastructure_group.as_ref(), &response, ); + finalize_refined_groups(&mut refined_groups, file_centrality.as_ref(), &weights); + for warning in &warnings { tracing::warn!(target: "refinement", "repair: {}", warning.message); job.emit(ActivityEntry::info( @@ -1023,6 +1061,7 @@ pub fn start_refine_groups( key: config.llm.key.clone(), annotations_enabled: config.llm.annotations_enabled, refinement: config.llm.refinement.clone(), + metadata: config.llm.metadata.clone(), }; let provider_name = refinement_llm_config @@ -1039,20 +1078,55 @@ pub fn start_refine_groups( let job_id = start.job_id.clone(); let job_id_for_cleanup = job_id.clone(); let jobs_for_cleanup = Arc::clone(&state.refinement_jobs); + let analysis_for_persist = Arc::clone(&state.last_analysis); + let file_centrality = state + .last_file_centrality + .lock() + .ok() + .and_then(|c| c.clone()); + let weights = config.ranking.clone(); let handle = crate::runtime::background().spawn(async move { - match run_refinement_with_activity(analysis, refinement_llm_config, workdir, job.clone()) - .await + match run_refinement_with_activity( + analysis, + refinement_llm_config, + workdir, + job.clone(), + file_centrality, + weights, + ) + .await { - Ok(response) => match serde_json::to_value(&response) { - Ok(value) => job.complete("refinement", value).await, - Err(error) => { - job.fail(format!( - "Failed to serialize refinement response: {}", - error - )) - .await + Ok(response) => { + // Persist before completing the job. Every backend command that + // reads `last_analysis` — describe_groups, annotate_overview — + // would otherwise keep answering about pre-refinement groups. + if response.had_changes { + match analysis_for_persist.lock() { + Ok(mut last) => { + if let Some(ref mut a) = *last { + a.groups = response.refined_groups.clone(); + a.infrastructure_group = response.infrastructure_group.clone(); + a.summary.total_groups = a.groups.len() as u32; + } + } + Err(error) => warn!( + "Failed to persist refined groups (lock poisoned): {}", + error + ), + } } - }, + + match serde_json::to_value(&response) { + Ok(value) => job.complete("refinement", value).await, + Err(error) => { + job.fail(format!( + "Failed to serialize refinement response: {}", + error + )) + .await + } + } + } Err(error) => job.fail(error.to_string()).await, } if let Ok(mut map) = jobs_for_cleanup.lock() { @@ -1300,6 +1374,94 @@ pub async fn annotate_group( /// /// Takes the deterministic groups (v1) and asks an LLM to suggest structural /// improvements: splits, merges, re-ranks, and reclassifications. Applies the +/// Run the LLM group metadata pass over the cached analysis. +/// +/// Fills in `description`, `invariant`, `review_focus` and friends on every +/// group, overriding the deterministic heuristic floor where the model has an +/// opinion. Returns the updated groups; the cached analysis is updated too, so +/// a later `get_last_analysis` sees them. +#[cfg_attr(feature = "desktop", tauri::command)] +pub async fn describe_groups( + repo_path: Option, + state: State<'_, AppState>, +) -> Result, CommandError> { + let mut groups = { + let last = state + .last_analysis + .lock() + .map_err(|e| CommandError::Analysis(format!("Lock poisoned: {}", e)))?; + last.as_ref() + .ok_or_else(|| { + CommandError::Analysis("No analysis available. Run analyze first.".into()) + })? + .groups + .clone() + }; + + let diffs = { + let last = state + .last_diff + .lock() + .map_err(|e| CommandError::Analysis(format!("Lock poisoned: {}", e)))?; + last.as_ref() + .map(|d| d.diff_result.files.clone()) + .unwrap_or_default() + }; + + let (config, workdir) = load_config_from_path(repo_path.as_deref()); + if !config.llm.metadata.enabled { + return Ok(groups); + } + + let metadata_llm_config = diffcore_core::config::LlmConfig { + provider: config + .llm + .metadata + .provider + .clone() + .or_else(|| config.llm.provider.clone()), + model: config + .llm + .metadata + .model + .clone() + .or_else(|| config.llm.model.clone()), + key_cmd: config + .llm + .metadata + .key_cmd + .clone() + .or_else(|| config.llm.key_cmd.clone()), + key: config.llm.metadata.key.clone().or_else(|| config.llm.key.clone()), + ..Default::default() + }; + + let provider: std::sync::Arc = + llm::create_provider_for_workdir(&metadata_llm_config, workdir.as_deref()) + .map_err(|e| CommandError::Llm(format!("{}", e)))? + .into(); + + llm::metadata::run_metadata_pass( + provider, + &mut groups, + &diffs, + config.llm.metadata.batch_size, + ) + .await + .map_err(|e| CommandError::Llm(format!("{}", e)))?; + + match state.last_analysis.lock() { + Ok(mut last) => { + if let Some(ref mut a) = *last { + a.groups = groups.clone(); + } + } + Err(e) => warn!("Failed to update last_analysis groups (lock poisoned): {}", e), + } + + Ok(groups) +} + /// refinement operations and returns the result containing both the refined /// groups and the raw refinement response (for change indicators in the UI). /// @@ -1362,6 +1524,7 @@ pub async fn refine_groups( key: config.llm.key.clone(), annotations_enabled: config.llm.annotations_enabled, refinement: config.llm.refinement.clone(), + metadata: config.llm.metadata.clone(), }; let provider = llm::create_provider_for_workdir(&refinement_llm_config, workdir.as_deref()) @@ -1409,12 +1572,23 @@ pub async fn refine_groups( // Apply the refinement leniently: repair what we can, drop what we can't, // surface warnings instead of erroring on individual hallucinated ops. - let (refined_groups, infra, warnings) = refinement::apply_refinement_lenient( + let (mut refined_groups, infra, warnings) = refinement::apply_refinement_lenient( &analysis.groups, analysis.infrastructure_group.as_ref(), &response, ); + finalize_refined_groups( + &mut refined_groups, + state + .last_file_centrality + .lock() + .ok() + .and_then(|c| c.clone()) + .as_ref(), + &config.ranking, + ); + for w in &warnings { warn!("Refinement repair: {}", w.message); } @@ -1703,7 +1877,7 @@ pub fn get_llm_settings(repo_path: Option) -> Result Result<() config.llm.model = Some(settings.model); // Don't overwrite key_cmd — that's managed manually config.llm.refinement.enabled = settings.refinement_enabled; + config.llm.metadata.enabled = settings.metadata_enabled; config.llm.refinement.provider = Some(settings.refinement_provider); config.llm.refinement.model = Some(settings.refinement_model); - config.llm.refinement.max_iterations = settings.refinement_max_iterations; config.llm.annotations_enabled = settings.annotations_enabled; // Update diff behavior @@ -2144,6 +2318,8 @@ pub struct LlmSettings { pub annotations_enabled: bool, /// Whether LLM refinement is enabled. pub refinement_enabled: bool, + /// Whether the group metadata pass runs automatically after analyze. + pub metadata_enabled: bool, /// Selected LLM backend: subscription-backed CLI or direct API provider. pub provider: String, /// Selected model identifier. @@ -2156,8 +2332,6 @@ pub struct LlmSettings { pub refinement_provider: String, /// Refinement model. pub refinement_model: String, - /// Maximum refinement iterations. - pub refinement_max_iterations: u32, /// Where shared LLM settings are stored. pub global_config_path: String, /// Whether Codex CLI is installed. @@ -3060,13 +3234,13 @@ mod tests { let settings = LlmSettings { annotations_enabled: true, refinement_enabled: false, + metadata_enabled: true, provider: "codex".to_string(), model: "default".to_string(), api_key_source: "Codex CLI login".to_string(), has_api_key: true, refinement_provider: "claude".to_string(), refinement_model: "default".to_string(), - refinement_max_iterations: 2, global_config_path: "~/.diffcore/config.toml".to_string(), codex_available: true, codex_authenticated: true, @@ -3083,7 +3257,6 @@ mod tests { assert!(back.has_api_key); assert_eq!(back.refinement_provider, "claude"); assert_eq!(back.refinement_model, "default"); - assert_eq!(back.refinement_max_iterations, 2); assert!(back.codex_available); assert!(back.claude_authenticated); } @@ -3225,6 +3398,7 @@ mod tests { edges: vec![], risk_score: 0.5, review_order: 1, + ..Default::default() }], infrastructure_group: None, refinement_response: RefinementResponse { @@ -3946,13 +4120,13 @@ mod tests { let settings = LlmSettings { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: true, provider: "codex".to_string(), model: "default".to_string(), api_key_source: "test".to_string(), has_api_key: true, refinement_provider: "codex".to_string(), refinement_model: "default".to_string(), - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml".to_string(), codex_available: false, codex_authenticated: false, @@ -3971,13 +4145,13 @@ mod tests { let settings = LlmSettings { annotations_enabled: false, refinement_enabled: false, + metadata_enabled: true, provider: "anthropic".to_string(), model: "claude-sonnet-4-6".to_string(), api_key_source: "env".to_string(), has_api_key: false, refinement_provider: "anthropic".to_string(), refinement_model: "claude-sonnet-4-6".to_string(), - refinement_max_iterations: 3, global_config_path: "/tmp/config.toml".to_string(), codex_available: true, codex_authenticated: true, @@ -3996,13 +4170,13 @@ mod tests { let settings = LlmSettings { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: true, provider: "openai".to_string(), model: "gpt-4.1".to_string(), api_key_source: "config".to_string(), has_api_key: true, refinement_provider: "gemini".to_string(), refinement_model: "gemini-2.5-flash".to_string(), - refinement_max_iterations: 2, global_config_path: "~/.diffcore/config.toml".to_string(), codex_available: true, codex_authenticated: false, @@ -4018,7 +4192,6 @@ mod tests { assert!(json.contains("has_api_key")); assert!(json.contains("refinement_provider")); assert!(json.contains("refinement_model")); - assert!(json.contains("refinement_max_iterations")); assert!(json.contains("global_config_path")); assert!(json.contains("include_uncommitted")); } diff --git a/crates/diffcore-tauri/src/main.rs b/crates/diffcore-tauri/src/main.rs index 74a892a1..54a4041a 100644 --- a/crates/diffcore-tauri/src/main.rs +++ b/crates/diffcore-tauri/src/main.rs @@ -80,6 +80,7 @@ fn main() { commands::save_api_key, commands::clear_api_key, commands::refine_groups, + commands::describe_groups, commands::open_in_editor, commands::check_editors_available, commands::save_comment, diff --git a/crates/diffcore-tauri/src/web_server.rs b/crates/diffcore-tauri/src/web_server.rs index d54abdad..76de9171 100644 --- a/crates/diffcore-tauri/src/web_server.rs +++ b/crates/diffcore-tauri/src/web_server.rs @@ -205,6 +205,10 @@ async fn invoke( let (a, b, c) = llm_args(&mut args)?; return ok(commands::refine_groups(a, b, c, State(&state.app)).await?).map(Json); } + "describe_groups" => { + let (a, _, _) = llm_args(&mut args)?; + return ok(commands::describe_groups(a, State(&state.app)).await?).map(Json); + } "annotate_group" => { let group_id = req(&mut args, "groupId")?; let d = diff_args(&mut args)?; diff --git a/crates/diffcore-tauri/ui/src/App.tsx b/crates/diffcore-tauri/ui/src/App.tsx index 67a43405..1c197bd2 100644 --- a/crates/diffcore-tauri/ui/src/App.tsx +++ b/crates/diffcore-tauri/ui/src/App.tsx @@ -4,7 +4,6 @@ import type { FlowGroup, FileDiffContent, Pass1Response, - Pass1GroupAnnotation, Pass2Response, RepoInfo, ResolvedPr, @@ -29,7 +28,7 @@ import SourceExplorer, { type SourceFocusRequest } from "./components/SourceExpl import ErrorBoundary from "./components/ErrorBoundary"; import { buildManifestPrompt } from "./buildManifestPrompt"; import { THEMES, applyTheme, getTheme, loadThemePrefs, saveThemePrefs, resolveThemeId, type ThemeMode, type ThemePrefs } from "./themes"; -import { MOCK_ANALYSIS, MOCK_DIFFS, MOCK_PASS1, MOCK_PASS2, MOCK_REPO_INFO, MOCK_LLM_SETTINGS, MOCK_REFINEMENT, MOCK_RESOLVED_PR } from "./mock"; +import { MOCK_ANALYSIS, MOCK_DIFFS, MOCK_PASS2, MOCK_REPO_INFO, MOCK_LLM_SETTINGS, MOCK_REFINEMENT, MOCK_RESOLVED_PR } from "./mock"; import { IS_TAURI, HAS_BACKEND, DEFAULT_REPO, invoke as tauriInvoke } from "./backend"; @@ -123,9 +122,104 @@ function TruncatedText({ ); } +function hyphenateVariant(value: string): string { + return value.replace(/(?!^)([A-Z])/g, "-$1"); +} + +/** + * What the active group achieves. Sized to the change by the model: one + * sentence renders as prose, several render as bullets. + * + * Deliberately outside the fixed-layout block — its height varies per group, + * and reserving space for the worst case would defeat the glance row above it. + */ +function GroupSummary({ group }: { group: FlowGroup }) { + const summary = group.summary ?? []; + if (summary.length === 0) return null; + + return ( +
+

Summary

+ {summary.length === 1 ? ( +

{summary[0]}

+ ) : ( +
    + {summary.map((point, i) => ( +
  • {point}
    • + ))} +
+ )} +
+ ); +} + +/** + * Fixed-layout review summary. Every row keeps its height for the whole + * analysis, so the block never reflows as fields come and go between groups. + * A row the analysis never fills is dropped rather than left as dead space. + */ +function GroupReviewSummary({ group, groups }: { group: FlowGroup; groups: FlowGroup[] }) { + const focus = group.review_focus ?? []; + const hasMetadata = Boolean( + group.group_type || group.risk || group.impact || group.complexity || + group.description || group.invariant || focus.length > 0, + ); + if (!hasMetadata) return null; + + const reservesFocus = groups.some((g) => (g.review_focus ?? []).length > 0); + const reservesDescription = groups.some((g) => g.description); + const reservesInvariant = groups.some((g) => g.invariant); + const focusText = focus.map((f) => hyphenateVariant(f).toLowerCase()).join(", "); + + return ( +
+
+ {group.group_type && ( + {group.group_type.toUpperCase()} + )} + {group.risk && ( + + {group.risk.toUpperCase()} RISK + + )} + {group.impact && ( + {hyphenateVariant(group.impact).toUpperCase()} + )} + {group.complexity && ( + {group.complexity.toUpperCase()} + )} +
+ {reservesFocus && ( +
+ {focus.length > 0 && ( + + Focus: {focusText} + + )} +
+ )} + {reservesDescription && ( +

+ {group.description} +

+ )} + {reservesInvariant && ( +

+ {group.invariant && ( + <> + Invariant: {group.invariant} + + )} +

+ )} +
+ ); +} + /** Three-panel layout: flow groups | diff viewer | annotations */ export default function App() { const [analysis, setAnalysis] = useState(null); + const [showPrOverview, setShowPrOverview] = useState(false); const [selectedGroup, setSelectedGroup] = useState(null); const [selectedFile, setSelectedFile] = useState(null); const [fileDiff, setFileDiff] = useState(null); @@ -137,7 +231,6 @@ export default function App() { // LLM annotation state const [overview, setOverview] = useState(null); const [deepAnalyses, setDeepAnalyses] = useState>({}); - const [annotating, setAnnotating] = useState(false); const [deepAnalyzing, setDeepAnalyzing] = useState(false); // Counter to track concurrent deep analysis requests — prevents premature loading state clear const deepAnalyzingCount = useRef(0); @@ -791,11 +884,51 @@ export default function App() { [analysis, openFileInTab, selectedFile], ); + /** Show a toast notification that auto-dismisses. */ + const showToast = useCallback((message: string) => { + if (toastTimer.current) clearTimeout(toastTimer.current); + setToast(message); + toastTimer.current = setTimeout(() => setToast(null), 3500); + }, []); + + /** + * Run the LLM group metadata pass and fold the result into the current + * analysis. Fired after analyze and again after refinement — refinement + * rebuilds groups from scratch, so their descriptions go with them. + * + * Fire-and-forget: a failed description must never break the grouping the + * user already has. The backend re-checks `llm.metadata.enabled` too, so a + * stale UI flag cannot force a paid call. + */ + const describeGroups = useCallback(async () => { + if (!HAS_BACKEND || !llmSettings?.metadata_enabled) return; + try { + const described = await tauriInvoke("describe_groups", { + repoPath: repoPath || null, + }); + const byId = new Map(described.map((g) => [g.id, g])); + setAnalysis((prev) => + prev ? { ...prev, groups: prev.groups.map((g) => byId.get(g.id) ?? g) } : prev, + ); + // The right panel renders `selectedGroup`, which is a snapshot taken when + // the user picked it — updating `analysis` alone leaves the description + // in state but off the screen. + setSelectedGroup((prev) => (prev ? byId.get(prev.id) ?? prev : prev)); + } catch (e) { + // Never break the grouping over a failed description, but never hide the + // failure either: a silent catch here is indistinguishable from the pass + // working and returning nothing. + showToast(`Group descriptions unavailable: ${String(e)}`); + } + }, [llmSettings, repoPath, showToast]); + const handleSelectGroup = useCallback( async (group: FlowGroup) => { // Cancel any pending debounced file nav from the previous group if (pendingFileNav.current) clearTimeout(pendingFileNav.current); setSelectedGroup(group); + // Picking a group is a request to read that group, not the PR blurb. + setShowPrOverview(false); // Exit replay mode when switching groups setReplayActive(false); setReplayStep(0); @@ -886,6 +1019,7 @@ export default function App() { } }).catch(() => {}); } + void describeGroups(); } catch (e) { setError(String(e)); // Re-focus the repo input so user can fix the path @@ -894,7 +1028,7 @@ export default function App() { } finally { setLoading(false); } - }, [repoPath, baseRef, headRef, handleSelectGroup, closeActivityStream]); + }, [repoPath, baseRef, headRef, handleSelectGroup, closeActivityStream, describeGroups]); /** Analyze whatever is in the repository field — a local path, or a PR/MR URL * that we first clone and resolve to a base/head pair. */ @@ -959,41 +1093,48 @@ export default function App() { resolvedRefinementProvider, ); const aiAccessReady = hasApiKey || !!recommendedSubscriptionProvider; - const annotationsEnabled = (llmSettings?.annotations_enabled ?? false) || !!recommendedSubscriptionProvider; - /** Run LLM Pass 1: overview annotation for all groups. */ - const runAnnotateOverview = useCallback(async () => { - setAnnotating(true); - setError(null); + /** + * Run Pass 1 and stash the PR-level overview without taking over the panel. + * + * Folded into the analyze path rather than sitting behind a button: Pass 1 is + * PR-level only now, so there is nothing left to decide about it. Uses the + * plain command instead of the streaming job on purpose — a streaming job + * switches the right panel to the activity tab, which is intrusive when + * nobody asked for it. The result waits behind the PR Overview toggle. + */ + const annotateOverviewInBackground = useCallback(async () => { + // Demo mode gets its overview from MOCK_ANALYSIS.annotations, in the same + // state update as the analysis itself, so nothing shifts after mount. + if (!HAS_BACKEND || !llmSettings?.annotations_enabled || !aiAccessReady) return; try { - if (HAS_BACKEND) { - await runStreamingJob("start_annotate_overview", { - repoPath: repoPath || null, - llmProvider: resolvedPrimaryProvider, - llmModel: resolvedPrimaryModel, - }, (result) => { - setOverview(result); - }); - } else { - await runMockActivityJob( - { - job_id: "mock-overview", - operation: "overview", - provider: resolvedPrimaryProvider ?? "codex", - model: resolvedPrimaryModel ?? "default", - title: "Summarizing PR", - }, - buildMockActivityEntries("overview", resolvedPrimaryProvider ?? "codex"), - MOCK_PASS1, - (result) => setOverview(result), - ); - } + const result = await tauriInvoke("annotate_overview", { + repoPath: repoPath || null, + llmProvider: resolvedPrimaryProvider, + llmModel: resolvedPrimaryModel, + }); + setOverview(result); } catch (e) { - setError(`Annotation failed: ${String(e)}`); - } finally { - setAnnotating(false); + // Never block on the PR blurb, but say so rather than leaving the user to + // wonder whether it is still coming. + showToast(`PR overview unavailable: ${String(e)}`); } - }, [repoPath, resolvedPrimaryModel, resolvedPrimaryProvider, runMockActivityJob, runStreamingJob]); + }, [aiAccessReady, llmSettings, repoPath, resolvedPrimaryModel, resolvedPrimaryProvider, showToast]); + + // Fire it once per analysis. `analysis` is also rewritten by the metadata + // pass and by refinement, so key off the diff rather than object identity. + const autoAnnotatedRef = useRef(null); + useEffect(() => { + if (!analysis) return; + const key = `${analysis.diff_source.base_sha ?? ""}:${analysis.diff_source.head_sha ?? ""}`; + if (autoAnnotatedRef.current === key) return; + autoAnnotatedRef.current = key; + void annotateOverviewInBackground(); + }, [analysis, annotateOverviewInBackground]); + + const annotationsEnabled = (llmSettings?.annotations_enabled ?? false) || !!recommendedSubscriptionProvider; + + /** Run LLM Pass 1: overview annotation for all groups. */ /** Run LLM Pass 2: deep analysis for the selected group. */ const runDeepAnalysis = useCallback(async () => { @@ -1047,13 +1188,6 @@ export default function App() { } }, [selectedGroup, repoPath, baseRef, resolvedPrimaryModel, resolvedPrimaryProvider, runMockActivityJob, runStreamingJob]); - /** Show a toast notification that auto-dismisses. */ - const showToast = useCallback((message: string) => { - if (toastTimer.current) clearTimeout(toastTimer.current); - setToast(message); - toastTimer.current = setTimeout(() => setToast(null), 3500); - }, []); - const applyRefinementResult = useCallback((result: RefinementResult, opts?: { fromCache?: boolean }) => { if (!analysis) return; @@ -1090,6 +1224,9 @@ export default function App() { if (sorted.length > 0) { handleSelectGroup(sorted[0]); } + // Refinement rebuilds groups, so their descriptions went with them. + // Re-describe rather than making the user hunt for another button. + void describeGroups(); } else { setShowRefined(false); if (!opts?.fromCache) { @@ -1101,7 +1238,7 @@ export default function App() { if (HAS_BACKEND && !opts?.fromCache) { tauriInvoke("store_refinement_cache", { result, repoPath: repoPath || null }).catch(() => {}); } - }, [analysis, originalGroups, handleSelectGroup, showToast]); + }, [analysis, originalGroups, handleSelectGroup, showToast, describeGroups, repoPath]); /** Run LLM refinement pass on the current analysis groups. */ const runRefinement = useCallback(async () => { @@ -1464,11 +1601,6 @@ export default function App() { ); sortedGroupsRef.current = sortedGroups; - // Get the Pass 1 annotation for the currently selected group - const groupAnnotation: Pass1GroupAnnotation | undefined = overview?.groups.find( - (g) => g.id === selectedGroup?.id, - ); - // Get the Pass 2 deep analysis for the currently selected group const groupDeepAnalysis: Pass2Response | undefined = selectedGroup ? deepAnalyses[selectedGroup.id] @@ -1674,10 +1806,11 @@ export default function App() { return; } + const allGroups = analysis?.groups ?? []; const orderedGroups = overview.suggested_review_order - .map((id) => overview.groups.find((group) => group.id === id)) - .filter((group): group is Pass1GroupAnnotation => Boolean(group)); - const fallbackGroups = overview.groups.filter( + .map((id) => allGroups.find((group) => group.id === id)) + .filter((group): group is FlowGroup => Boolean(group)); + const fallbackGroups = allGroups.filter( (group) => !orderedGroups.some((ordered) => ordered.id === group.id), ); @@ -1688,9 +1821,9 @@ export default function App() { "", "# Review Flow", "", - ...[...orderedGroups, ...fallbackGroups].flatMap((group) => [ - `- ${group.name}: ${group.summary}`, - ]), + ...[...orderedGroups, ...fallbackGroups].map((group) => + group.description ? `- ${group.name}: ${group.description}` : `- ${group.name}`, + ), ]; try { @@ -1699,7 +1832,7 @@ export default function App() { } catch { showToast("Failed to copy PR description"); } - }, [overview, showToast]); + }, [analysis, overview, showToast]); /** Compute a simple hash of the analysis for comment scoping. */ const analysisHash = analysis @@ -2455,6 +2588,21 @@ export default function App() { {annotationSubTab === "info" && ( <> + {showPrOverview && overview ? ( +
+

PR Overview

+

{overview.overall_summary}

+ +
+ ) : ( + <> + + + + )} +

Flow Group

{selectedGroup.name}

@@ -2497,42 +2645,14 @@ export default function App() { {PROVIDER_LABELS[refinementVerdict.provider as LlmProvider] ?? refinementVerdict.provider}/{refinementVerdict.model}

{refinementVerdict.reasoning && ( -

{refinementVerdict.reasoning}

- )} -
- )} - - {overview && !groupAnnotation && ( -
-

LLM Overview

-

{overview.overall_summary}

-
- )} - - {groupAnnotation && ( -
-

LLM Summary

-

{groupAnnotation.summary}

-

- Review rationale: {groupAnnotation.review_order_rationale} -

- {groupAnnotation.risk_flags.length > 0 && ( -
- {groupAnnotation.risk_flags.map((flag, i) => ( - {flag} - ))} -
+
+ Why these groups +

{refinementVerdict.reasoning}

+
)}
)} - {overview && groupAnnotation && ( -
-

Overall Summary

-

{overview.overall_summary}

-
- )} - {groupDeepAnalysis && ( <>
@@ -3574,6 +3694,24 @@ export default function App() {

+ {/* Review Metadata Section */} +
+

Review Metadata

+ +

+ After each analysis, asks the model how each group should be reviewed — what kind of change it + is, what to look for, and what property to verify. Turn this off to keep the deterministic labels + only and avoid the extra call. +

+
+ {/* Refinement Section */}

Refinement

@@ -3622,19 +3760,6 @@ export default function App() { )}
-
- - - updateSetting("refinement_max_iterations", Math.max(1, parseInt(e.target.value) || 1)) - } - /> -
)} @@ -3912,6 +4037,22 @@ export default function App() { {group.risk_score.toFixed(2)} + {(group.group_type || group.risk) && ( +
+ {group.group_type && ( + {group.group_type.toUpperCase()} + )} + {group.risk && ( + + {group.risk.toUpperCase()} + + )} +
+ )} {changeIndicator && (
@@ -4522,14 +4663,10 @@ export default function App() { ? commentsTabContent : annotationsTabContent} - {(annotating || deepAnalyzing || refining) && rightPanelTab === "activity" && ( + {(deepAnalyzing || refining) && rightPanelTab === "activity" && (
- {annotating - ? "Generating overview..." - : deepAnalyzing - ? "Analyzing flow group..." - : "Refining groups..."} + {deepAnalyzing ? "Analyzing flow group..." : "Refining groups..."}
)} @@ -4544,7 +4681,7 @@ export default function App() { {selectedGroup && rightPanelTab === "annotations" && (
- {overview && !annotating && ( + {overview && ( )} {!groupDeepAnalysis && !deepAnalyzing && ( @@ -4805,7 +4937,7 @@ function resolveInteractiveModel( } function buildMockActivityEntries( - operation: "overview" | "group" | "refinement", + operation: "group" | "refinement", provider: string, ): Array> { const toolBacked = providerSupportsToolActivity(provider); @@ -4846,38 +4978,6 @@ function buildMockActivityEntries( ]; } - if (operation === "overview") { - return [ - ...sharedStart, - { - source, - level: "info", - message: `${providerName} is running rg --files crates/diffcore-tauri/ui/src`, - event_type: "stdout.command_execution", - payload: { - command: "rg --files crates/diffcore-tauri/ui/src", - cwd: "crates/diffcore-tauri/ui/src", - }, - }, - { - source, - level: "info", - message: `${providerName} is running sed -n '1,240p' crates/diffcore-tauri/ui/src/App.tsx`, - event_type: "stdout.command_execution", - payload: { - command: "sed -n '1,240p' crates/diffcore-tauri/ui/src/App.tsx", - path: "crates/diffcore-tauri/ui/src/App.tsx", - }, - }, - { - source, - level: "info", - message: "Writing PR-ready summary", - event_type: "provider.summary", - }, - ]; - } - if (operation === "group") { return [ ...sharedStart, diff --git a/crates/diffcore-tauri/ui/src/mock.ts b/crates/diffcore-tauri/ui/src/mock.ts index 8f249ab7..f0868d55 100644 --- a/crates/diffcore-tauri/ui/src/mock.ts +++ b/crates/diffcore-tauri/ui/src/mock.ts @@ -68,6 +68,18 @@ export const MOCK_ANALYSIS: AnalysisOutput = { ], risk_score: 0.82, review_order: 1, + group_type: "Feat", + description: "Add user creation with validation and a persisted audit trail.", + risk: "High", + impact: "CrossCutting", + complexity: "Complex", + review_focus: ["Correctness", "Security", "DataIntegrity"], + summary: [ + "Validates the POST /api/users payload before it reaches the service layer.", + "Rejects duplicate emails with a 409 instead of surfacing a Prisma constraint error.", + "Writes the user row and its audit entry together so neither can land alone.", + ], + invariant: "A user row and its audit entry must be written in the same transaction.", }, { id: "group_2", @@ -107,6 +119,16 @@ export const MOCK_ANALYSIS: AnalysisOutput = { ], risk_score: 0.74, review_order: 2, + group_type: "Fix", + description: "Move refresh-token rotation behind the rate limiter.", + risk: "Critical", + impact: "Module", + complexity: "Moderate", + review_focus: ["Security", "Concurrency"], + summary: [ + "Rotates refresh tokens on use and rate-limits the refresh endpoint.", + ], + invariant: "A refresh token must never be accepted twice after rotation.", }, { id: "group_3", @@ -138,6 +160,10 @@ export const MOCK_ANALYSIS: AnalysisOutput = { ], risk_score: 0.35, review_order: 3, + group_type: "Chore", + risk: "Low", + impact: "Local", + review_focus: [], }, ], infrastructure_group: { @@ -156,7 +182,7 @@ export const MOCK_ANALYSIS: AnalysisOutput = { ], reason: "Not reachable from any detected entrypoint", }, - annotations: null, + annotations: null, // set below, once MOCK_PASS1 is declared }; export const MOCK_DIFFS: Record = { @@ -485,35 +511,6 @@ export class EmailService { }; export const MOCK_PASS1: Pass1Response = { - groups: [ - { - id: "group_1", - name: "User creation API with validation and persistence", - summary: - "Adds input validation middleware, typed DTOs, and duplicate-email checking to the POST /api/users creation flow. The route handler now validates input before passing to the service layer, which hashes passwords before persisting via Prisma.", - review_order_rationale: - "Review first \u2014 this group changes the public API contract and touches the persistence layer (schema-adjacent). Downstream auth and email flows may depend on user creation succeeding correctly.", - risk_flags: ["schema_change", "auth_adjacent", "public_api_change"], - }, - { - id: "group_2", - name: "Auth token refresh with rotation and rate limiting", - summary: - "Implements rotating refresh tokens: old tokens are revoked on refresh, and a new refresh token is issued alongside the access token. Adds rate limiting middleware to prevent brute-force attacks on the refresh endpoint.", - review_order_rationale: - "Review second \u2014 auth token rotation is a security-critical change. A bug here could lock users out or allow token reuse after revocation.", - risk_flags: ["auth_change", "security_critical", "breaking_api"], - }, - { - id: "group_3", - name: "Email worker typed interface and priority routing", - summary: - "Adds TypeScript interfaces to the email worker queue consumer and introduces priority-based routing (high-priority emails are sent immediately). The email service now uses named templates with variable substitution.", - review_order_rationale: - "Review last \u2014 lowest risk. Changes are additive (new types, new feature) and isolated to the background worker pipeline.", - risk_flags: [], - }, - ], overall_summary: "This PR strengthens the user-facing API layer with input validation and auth hardening (rotating refresh tokens + rate limiting), then adds typed email templates to the background worker. The highest-risk changes are in auth token rotation \u2014 review the transaction logic carefully.", suggested_review_order: ["group_1", "group_2", "group_3"], @@ -658,6 +655,18 @@ export const MOCK_REFINEMENT: RefinementResult = { ], risk_score: 0.82, review_order: 1, + group_type: "Feat", + description: "Add user creation with validation and a persisted audit trail.", + risk: "High", + impact: "CrossCutting", + complexity: "Complex", + review_focus: ["Correctness", "Security", "DataIntegrity"], + summary: [ + "Validates the POST /api/users payload before it reaches the service layer.", + "Rejects duplicate emails with a 409 instead of surfacing a Prisma constraint error.", + "Writes the user row and its audit entry together so neither can land alone.", + ], + invariant: "A user row and its audit entry must be written in the same transaction.", }, { id: "group_refined_1", @@ -675,6 +684,10 @@ export const MOCK_REFINEMENT: RefinementResult = { edges: [], risk_score: 0.3, review_order: 2, + group_type: "Refactor", + risk: "Low", + impact: "Local", + review_focus: [], }, { id: "group_2", @@ -712,6 +725,16 @@ export const MOCK_REFINEMENT: RefinementResult = { ], risk_score: 0.74, review_order: 3, + group_type: "Fix", + description: "Move refresh-token rotation behind the rate limiter.", + risk: "Critical", + impact: "Module", + complexity: "Moderate", + review_focus: ["Security", "Concurrency"], + summary: [ + "Rotates refresh tokens on use and rate-limits the refresh endpoint.", + ], + invariant: "A refresh token must never be accepted twice after rotation.", }, { id: "group_3", @@ -742,6 +765,10 @@ export const MOCK_REFINEMENT: RefinementResult = { ], risk_score: 0.35, review_order: 4, + group_type: "Chore", + risk: "Low", + impact: "Local", + review_focus: [], }, ], infrastructure_group: { @@ -786,13 +813,13 @@ export const MOCK_REFINEMENT: RefinementResult = { export const MOCK_LLM_SETTINGS: LlmSettings = { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: true, provider: "codex", model: "default", api_key_source: "Codex CLI login", has_api_key: true, refinement_provider: "claude", refinement_model: "default", - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml", codex_available: true, codex_authenticated: true, @@ -833,3 +860,9 @@ export const MOCK_REPO_INFO: RepoInfo = { }, is_worktree: false, }; + +// Demo mode ships the overview with the analysis, exactly as `--annotate` does. +// Firing it as a separate async pass instead made every demo-mode render change +// shape on a microtask after mount, which is a race for any test measuring +// layout. +MOCK_ANALYSIS.annotations = MOCK_PASS1; diff --git a/crates/diffcore-tauri/ui/src/styles.css b/crates/diffcore-tauri/ui/src/styles.css index 2375b5ca..874186d0 100644 --- a/crates/diffcore-tauri/ui/src/styles.css +++ b/crates/diffcore-tauri/ui/src/styles.css @@ -918,6 +918,21 @@ select:focus-visible { font-size: 11px; } +.refinement-verdict-details { + margin-top: 8px; +} + +.refinement-verdict-details > summary { + cursor: pointer; + font-size: 11px; + color: var(--text-secondary); + user-select: none; +} + +.refinement-verdict-details > summary:hover { + color: var(--text-primary); +} + .refinement-verdict-reasoning { margin-top: 8px; color: var(--text-secondary); @@ -1467,6 +1482,47 @@ body { color: var(--risk-low); } +.group-meta-row { + display: flex; + align-items: center; + gap: 6px; + padding: 0 12px 8px 46px; + margin-top: -6px; +} + +.group-meta-chip { + font-size: 9px; + font-weight: 700; + font-family: var(--font-mono); + letter-spacing: 0.07em; + padding: 1px 5px; + border-radius: 3px; + flex-shrink: 0; + color: var(--text-secondary); + background: rgba(var(--text-secondary-rgb), 0.1); +} + +.group-meta-chip-risk[data-risk="low"] { + color: var(--risk-low); + background: rgba(var(--risk-low-rgb), 0.16); +} + +.group-meta-chip-risk[data-risk="medium"] { + color: var(--risk-medium); + background: rgba(var(--risk-medium-rgb), 0.16); +} + +.group-meta-chip-risk[data-risk="high"] { + color: var(--risk-high); + background: rgba(var(--risk-high-rgb), 0.16); +} + +.group-meta-chip-risk[data-risk="critical"] { + color: var(--risk-high); + background: rgba(var(--risk-high-rgb), 0.22); + box-shadow: inset 0 0 0 1px rgba(var(--risk-high-rgb), 0.65); +} + /* ── Copy Flow Paths Button ── */ .copy-flow-btn { @@ -1766,6 +1822,110 @@ body { border-bottom: 1px solid var(--border); } +/* ── Group Review Metadata (fixed-height glance block) ── */ + +.review-meta { + display: flex; + flex-direction: column; + gap: 10px; + background: rgba(var(--accent-rgb), 0.03); + border-left: 2px solid rgba(var(--accent-rgb), 0.5); +} + +.review-meta-verdict, +.review-meta-focus { + font-family: var(--font-mono); + height: 14px; + line-height: 14px; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} + +.review-meta-item { + font-size: 10px; + font-weight: 700; + letter-spacing: 0.08em; + color: var(--text-primary); +} + +.review-meta-item + .review-meta-item::before { + content: "\00b7"; + margin: 0 7px; + font-weight: 400; + letter-spacing: 0; + color: var(--text-muted); +} + +.review-meta-item-risk[data-risk="low"] { color: var(--risk-low); } +.review-meta-item-risk[data-risk="medium"] { color: var(--risk-medium); } +.review-meta-item-risk[data-risk="high"] { color: var(--risk-high); } + +.review-meta-item-risk[data-risk="critical"] { + color: var(--risk-high); + text-shadow: 0 0 12px rgba(var(--risk-high-rgb), 0.55); +} + +.review-meta-focus-text { + font-size: 10px; + color: var(--text-secondary); +} + +.review-meta-label { + text-transform: uppercase; + letter-spacing: 0.08em; + color: var(--text-muted); +} + +.review-meta .review-meta-description { + font-size: 12px; + height: 34px; + line-height: 17px; + color: var(--text-primary); + overflow: hidden; + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 2; + line-clamp: 2; +} + +.review-meta .review-meta-invariant { + font-size: 11px; + height: 32px; + line-height: 16px; + color: var(--text-secondary); + overflow: hidden; + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 2; + line-clamp: 2; +} + +.review-meta-invariant .review-meta-label { + font-family: var(--font-mono); + font-size: 10px; +} + +.group-summary .group-summary-text, +.group-summary .group-summary-list { + font-size: 12px; + line-height: 18px; + color: var(--text-primary); + margin: 0; +} + +.group-summary .group-summary-list { + padding-left: 16px; +} + +.group-summary .group-summary-list li { + margin-bottom: 4px; +} + +.group-summary .group-summary-list li:last-child { + margin-bottom: 0; +} + .annotation-section h3 { font-size: 11px; font-weight: 600; diff --git a/crates/diffcore-tauri/ui/src/types.ts b/crates/diffcore-tauri/ui/src/types.ts index ebd0a914..5e22f595 100644 --- a/crates/diffcore-tauri/ui/src/types.ts +++ b/crates/diffcore-tauri/ui/src/types.ts @@ -47,6 +47,24 @@ export interface FileChange { symbols_changed: string[]; } +export type GroupType = "Feat" | "Fix" | "Perf" | "Refactor" | "Test" | "Docs" | "Build" | "Ci" | "Chore"; + +export type Risk = "Low" | "Medium" | "High" | "Critical"; + +export type ImpactScope = "Local" | "Module" | "CrossCutting" | "System"; + +export type ReviewComplexity = "Trivial" | "Simple" | "Moderate" | "Complex"; + +export type ReviewFocus = + | "Correctness" + | "Security" + | "Concurrency" + | "Performance" + | "DataIntegrity" + | "Compatibility" + | "ErrorHandling" + | "ApiContract"; + export interface FlowGroup { id: string; name: string; @@ -55,6 +73,15 @@ export interface FlowGroup { edges: FlowEdge[]; risk_score: number; review_order: number; + /** Review metadata — heuristic floor populates risk/group_type/impact; the rest needs the metadata pass. */ + group_type?: GroupType | null; + description?: string | null; + risk?: Risk | null; + impact?: ImpactScope | null; + complexity?: ReviewComplexity | null; + review_focus?: ReviewFocus[]; + summary?: string[]; + invariant?: string | null; } export type InfraCategory = @@ -164,22 +191,12 @@ export interface ResolvedPr { // ── LLM Annotation Types ── -/** Pass 1 overview response — per-group summaries + overall summary. */ +/** Pass 1 overview response — PR-level only. Per-group metadata lives on FlowGroup. */ export interface Pass1Response { - groups: Pass1GroupAnnotation[]; overall_summary: string; suggested_review_order: string[]; } -/** Per-group annotation from Pass 1 overview. */ -export interface Pass1GroupAnnotation { - id: string; - name: string; - summary: string; - review_order_rationale: string; - risk_flags: string[]; -} - /** Pass 2 deep analysis response for a single group. */ export interface Pass2Response { group_id: string; @@ -209,13 +226,13 @@ export interface Annotations { export interface LlmSettings { annotations_enabled: boolean; refinement_enabled: boolean; + metadata_enabled: boolean; provider: string; model: string; api_key_source: string; has_api_key: boolean; refinement_provider: string; refinement_model: string; - refinement_max_iterations: number; global_config_path: string; codex_available: boolean; codex_authenticated: boolean; diff --git a/crates/diffcore-tauri/ui/tests/e2e/activity-stream.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/activity-stream.spec.ts index 21974095..50b2528b 100644 --- a/crates/diffcore-tauri/ui/tests/e2e/activity-stream.spec.ts +++ b/crates/diffcore-tauri/ui/tests/e2e/activity-stream.spec.ts @@ -28,20 +28,16 @@ test.describe("AI activity stream", () => { await waitForDemoApp(page); }); - test("shows a rich overview timeline and lets the user switch back to annotations", async ({ page }) => { - await page.getByRole("button", { name: "Summarize PR" }).click(); + test("shows a rich deep-analysis timeline and lets the user switch back to annotations", async ({ page }) => { + await page.getByRole("button", { name: "Analyze This Flow" }).click(); const panel = page.getByTestId("activity-panel"); const log = page.getByTestId("activity-log"); await expect(panel).toBeVisible(); - await expect(panel).toContainText("Summarizing PR"); await expect(panel).toContainText("Codex CLI/default"); await expect(page.getByTestId("activity-stats")).toContainText("events"); - await expect(log).toContainText("Preparing overview request"); - await expect(log).toContainText("Searching the repo"); - await expect(log).toContainText("Reading files"); - await expect(log).toContainText("Writing PR-ready summary"); + await expect(log).toContainText("Preparing deep analysis request"); await expect(panel.locator(".activity-live-badge")).toHaveText("Saved"); await page.getByTestId("annotations-tab").click(); @@ -54,13 +50,13 @@ test.describe("AI activity stream", () => { await setLlmSettings(page, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "openai", model: "gpt-5.4", api_key_source: "~/.diffcore/config.toml", has_api_key: true, refinement_provider: "openai", refinement_model: "gpt-5.4", - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml", codex_available: true, codex_authenticated: true, @@ -93,13 +89,13 @@ test.describe("AI activity stream", () => { await setLlmSettings(page, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "openai", model: "gpt-5.4", api_key_source: "~/.diffcore/config.toml", has_api_key: true, refinement_provider: "openai", refinement_model: "gpt-5.4", - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml", codex_available: false, codex_authenticated: false, @@ -107,7 +103,7 @@ test.describe("AI activity stream", () => { claude_authenticated: false, }); - await page.getByRole("button", { name: "Summarize PR" }).click(); + await page.getByRole("button", { name: "Analyze This Flow" }).click(); await expect(page.getByTestId("activity-direct-api-note")).toBeVisible(); await expect(page.getByTestId("activity-direct-api-note")).toContainText("Direct API mode"); @@ -249,13 +245,13 @@ test.describe("AI activity stream", () => { await setLlmSettings(page, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "openai", model: "gpt-5.4", api_key_source: "~/.diffcore/config.toml", has_api_key: true, refinement_provider: "openai", refinement_model: "gpt-5.4", - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml", codex_available: true, codex_authenticated: true, diff --git a/crates/diffcore-tauri/ui/tests/e2e/flow-replay.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/flow-replay.spec.ts index 38b4da7d..b8b5a6ac 100644 --- a/crates/diffcore-tauri/ui/tests/e2e/flow-replay.spec.ts +++ b/crates/diffcore-tauri/ui/tests/e2e/flow-replay.spec.ts @@ -269,7 +269,7 @@ test.describe("Flow Replay Mode", () => { const groups = page.locator(".group-item"); const groupCount = await groups.count(); if (groupCount >= 2) { - await groups.nth(1).click(); + await groups.nth(1).locator(".group-name").click(); await page.waitForTimeout(500); // Replay should be exited diff --git a/crates/diffcore-tauri/ui/tests/e2e/group-metadata.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/group-metadata.spec.ts new file mode 100644 index 00000000..53199143 --- /dev/null +++ b/crates/diffcore-tauri/ui/tests/e2e/group-metadata.spec.ts @@ -0,0 +1,204 @@ +/** + * Group review metadata — Playwright E2E tests. + * + * Covers the desktop surface from specs/group-metadata.md §6.1: + * - group_type / risk chips in the left scan list, and no description there + * - the fixed-layout summary block in the right panel + * - absent fields omitted entirely, with the block holding its height + * - review_focus truncating instead of wrapping + */ +import { test, expect, type Page, type Locator } from "@playwright/test"; + +async function waitForAnalysis(page: Page) { + await expect(page.locator(".summary")).toBeVisible({ timeout: 10_000 }); + await expect(page.locator(".group-item.selected .file-list")).toBeVisible({ timeout: 5_000 }); +} + +/** The group with every metadata field populated. */ +function fullGroup(page: Page): Locator { + return page.locator(".group-item", { hasText: "POST /api/users creation flow" }); +} + +/** The group with only the heuristic floor: risk, group_type, impact. */ +function floorGroup(page: Page): Locator { + return page.locator(".group-item", { hasText: "Email notification worker" }); +} + +test.describe("Group metadata — left list", () => { + test.beforeEach(async ({ page }) => { + await page.goto("/"); + await waitForAnalysis(page); + }); + + test("01 — group header shows type and risk chips", async ({ page }) => { + const chips = fullGroup(page).locator(".group-meta-row .group-meta-chip"); + await expect(chips).toHaveText(["FEAT", "HIGH"]); + }); + + test("02 — floor-only group still shows both chips", async ({ page }) => { + const chips = floorGroup(page).locator(".group-meta-row .group-meta-chip"); + await expect(chips).toHaveText(["CHORE", "LOW"]); + }); + + test("03 — risk chip carries the level for colouring", async ({ page }) => { + await expect( + fullGroup(page).locator(".group-meta-chip-risk"), + ).toHaveAttribute("data-risk", "high"); + await expect( + page.locator(".group-item", { hasText: "auth/refresh" }).locator(".group-meta-chip-risk"), + ).toHaveAttribute("data-risk", "critical"); + }); + + test("04 — description never appears in the scan list", async ({ page }) => { + await expect(page.locator(".panel-left")).not.toContainText( + "Add user creation with validation", + ); + }); +}); + +test.describe("Group metadata — right panel", () => { + test.beforeEach(async ({ page }) => { + await page.goto("/"); + await waitForAnalysis(page); + }); + + test("05 — verdict row reads type, risk, impact, complexity", async ({ page }) => { + const verdict = page.getByTestId("group-review-meta").locator(".review-meta-verdict"); + await expect(verdict.locator(".review-meta-item")).toHaveText([ + "FEAT", + "HIGH RISK", + "CROSS-CUTTING", + "COMPLEX", + ]); + }); + + test("06 — focus, description and invariant render as plain text", async ({ page }) => { + const meta = page.getByTestId("group-review-meta"); + await expect(meta.locator(".review-meta-focus")).toHaveText( + "Focus: correctness, security, data-integrity", + ); + await expect(meta.locator(".review-meta-description")).toHaveText( + "Add user creation with validation and a persisted audit trail.", + ); + await expect(meta.locator(".review-meta-invariant")).toHaveText( + "Invariant: A user row and its audit entry must be written in the same transaction.", + ); + }); + + test("07 — absent fields are omitted, no placeholder and no Invariant label", async ({ page }) => { + await floorGroup(page).locator(".group-name").click(); + const meta = page.getByTestId("group-review-meta"); + + await expect(meta.locator(".review-meta-item")).toHaveText(["CHORE", "LOW RISK", "LOCAL"]); + await expect(meta.locator(".review-meta-focus")).toHaveText(""); + await expect(meta.locator(".review-meta-description")).toHaveText(""); + await expect(meta.locator(".review-meta-invariant")).toHaveText(""); + await expect(meta).not.toContainText("Invariant:"); + await expect(meta).not.toContainText("Focus:"); + await expect(meta).not.toContainText("—"); + }); + + test("08 — block keeps the same height across groups with different fields", async ({ page }) => { + const meta = page.getByTestId("group-review-meta"); + const withEverything = await meta.boundingBox(); + + await floorGroup(page).locator(".group-name").click(); + await expect(meta.locator(".review-meta-item").first()).toHaveText("CHORE"); + const withFloorOnly = await meta.boundingBox(); + + expect(withEverything).not.toBeNull(); + expect(withFloorOnly).not.toBeNull(); + expect(withFloorOnly!.height).toBe(withEverything!.height); + expect(withFloorOnly!.y).toBe(withEverything!.y); + }); + + test("09 — review focus truncates rather than wrapping", async ({ page }) => { + const focus = page.getByTestId("group-review-meta").locator(".review-meta-focus"); + + await expect(focus).toHaveCSS("white-space", "nowrap"); + await expect(focus).toHaveCSS("text-overflow", "ellipsis"); + + const threeEntries = (await focus.boundingBox())!.height; + + await page.locator(".group-item", { hasText: "auth/refresh" }).locator(".group-name").click(); + await expect(focus).toHaveText("Focus: security, concurrency"); + const twoEntries = (await focus.boundingBox())!.height; + + expect(threeEntries).toBe(twoEntries); + }); + + test("10 — existing group detail still renders below the block", async ({ page }) => { + const panel = page.locator(".panel-right"); + await expect(panel.getByTestId("annotations-panel")).toContainText("POST /api/users"); + + const metaY = (await panel.getByTestId("group-review-meta").boundingBox())!.y; + const detailY = (await panel.getByTestId("annotations-panel").boundingBox())!.y; + expect(metaY).toBeLessThan(detailY); + }); + + test("11 — a multi-point summary renders as bullets under the fixed block", async ({ page }) => { + const summary = page.getByTestId("group-summary"); + + await expect(summary.locator(".group-summary-list li")).toHaveCount(3); + await expect(summary.locator(".group-summary-text")).toHaveCount(0); + + const metaY = (await page.getByTestId("group-review-meta").boundingBox())!.y; + const summaryY = (await summary.boundingBox())!.y; + expect(metaY).toBeLessThan(summaryY); + }); + + test("12 — a single-point summary renders as prose, not a one-item list", async ({ page }) => { + await page.locator(".group-item", { hasText: "auth/refresh" }).locator(".group-name").click(); + + const summary = page.getByTestId("group-summary"); + await expect(summary.locator(".group-summary-text")).toBeVisible(); + await expect(summary.locator(".group-summary-list")).toHaveCount(0); + }); + + test("13 — a group with no summary renders no summary section", async ({ page }) => { + await floorGroup(page).locator(".group-name").click(); + await expect(page.getByTestId("group-summary")).toHaveCount(0); + }); + + test("13b — descriptions survive a refinement instead of vanishing with the old groups", async ({ page }) => { + await expect(page.getByTestId("group-summary")).toBeVisible(); + const before = await page.locator(".review-meta-description").textContent(); + + await page.locator(".btn-refine").click(); + await page.waitForTimeout(2000); + await page.getByTestId("annotations-tab").click(); + + // Refinement rebuilds groups from scratch; the review metadata has to come + // back with them rather than leaving the panel bare until another click. + await expect(page.getByTestId("group-review-meta")).toBeVisible(); + await expect(page.locator(".review-meta-description")).not.toBeEmpty(); + await expect(page.getByTestId("group-summary")).toBeVisible(); + expect(before).not.toBeNull(); + }); + + test("13c — the refinement rationale stays collapsed instead of burying the group", async ({ page }) => { + await page.locator(".btn-refine").click(); + await page.waitForTimeout(2000); + await page.getByTestId("annotations-tab").click(); + + const verdict = page.getByTestId("refinement-verdict"); + await expect(verdict).toBeVisible(); + + // The one-line verdict is glanceable; the prose behind it is not, so it + // must not be competing with the group's own summary for attention. + const reasoning = verdict.locator(".refinement-verdict-reasoning"); + await expect(reasoning).toBeHidden(); + + const metaY = (await page.getByTestId("group-review-meta").boundingBox())!.y; + const verdictY = (await verdict.boundingBox())!.y; + expect(metaY).toBeLessThan(verdictY); + + await verdict.locator("summary").click(); + await expect(reasoning).toBeVisible(); + }); + + test("14 — the PR-level overall summary stays out of the group panel", async ({ page }) => { + await expect(page.getByTestId("group-review-meta")).toBeVisible(); + await expect(page.getByTestId("pr-overview")).toHaveCount(0); + }); +}); diff --git a/crates/diffcore-tauri/ui/tests/e2e/hardening.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/hardening.spec.ts index 02f1f802..a696bf5c 100644 --- a/crates/diffcore-tauri/ui/tests/e2e/hardening.spec.ts +++ b/crates/diffcore-tauri/ui/tests/e2e/hardening.spec.ts @@ -171,13 +171,13 @@ function generateNoApiKeySettings() { return { annotations_enabled: true, refinement_enabled: false, + metadata_enabled: false, provider: "anthropic", model: "claude-sonnet-4-6", api_key_source: "", has_api_key: false, refinement_provider: "anthropic", refinement_model: "claude-sonnet-4-6", - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml", codex_available: false, codex_authenticated: false, @@ -458,15 +458,14 @@ test.describe("Hardening — LLM Controls", () => { // ═══════════════════════════════════════════════════════════════════ test.describe("Hardening — LLM Annotations", () => { - test("27 — summarize PR: idle button state", async ({ page }) => { + test("27 — PR overview arrives from the analyze path, with no button", async ({ page }) => { await page.goto("/"); await waitForAnalysis(page); - // Verify summarize button is visible and enabled - const btn = page.locator(".btn-summarize"); - await expect(btn).toBeVisible(); - await expect(btn).toContainText("Summarize PR"); - await expect(btn).not.toBeDisabled(); + // Pass 1 is folded into the analyze path, so there is no button to press: + // the overview arrives on its own and waits behind the PR Overview toggle. + await expect(page.locator(".btn-summarize")).toHaveCount(0); + await expect(page.locator(".btn-pr-overview")).toBeVisible(); // Verify provider badge await expect(page.locator(".llm-provider-badge")).toContainText("Codex CLI/default"); @@ -480,23 +479,18 @@ test.describe("Hardening — LLM Annotations", () => { await page.goto("/"); await waitForAnalysis(page); - // Click summarize — the right panel switches to the LLM activity stream - await page.locator(".btn-summarize").click(); - // Wait for the mock activity job to start and finish - await expect(page.locator(".annotation-section.llm-loading:not(.llm-setup-cta)")).toBeVisible(); - await expect(page.locator(".annotation-section.llm-loading:not(.llm-setup-cta)")).toBeHidden({ timeout: 15_000 }); - // Results render in the Info tab of the right panel + // No click: the overview came in with the analysis. Reveal it. await page.getByRole("tab", { name: "Info" }).click(); + await page.locator(".btn-pr-overview").click(); - // Verify LLM overview rendered + await expect(page.getByTestId("pr-overview")).toBeVisible(); await expect(page.locator(".llm-summary").first()).toBeVisible(); - // Verify risk flags shown - await expect(page.locator(".risk-flag").first()).toBeVisible(); - // Verify review rationale - await expect(page.locator(".llm-rationale")).toBeVisible(); - // Summarize button should be gone (overview loaded) - await expect(page.locator(".btn-summarize")).not.toBeVisible(); + // Going back to the group shows the group's own review metadata instead. + await page.locator(".btn", { hasText: "Back to group" }).click(); + await expect(page.getByTestId("pr-overview")).toHaveCount(0); + await expect(page.getByTestId("group-review-meta")).toBeVisible(); + await expect(page.locator(".review-meta-invariant")).toBeVisible(); await page.locator(".panel-right").screenshot({ path: path.join(SCREENSHOTS_DIR, "28-summarize-complete.png"), @@ -546,9 +540,9 @@ test.describe("Hardening — LLM Annotations", () => { await dismissAiSetupIfVisible(page); // Verify buttons show setup-required copy and are disabled - const summarizeBtn = page.locator(".btn-summarize"); - await expect(summarizeBtn).toContainText("Summarize PR (Setup required)"); - await expect(summarizeBtn).toHaveClass(/no-api-key/); + const analyzeFlowBtn = page.locator(".btn-analyze-flow"); + await expect(analyzeFlowBtn).toContainText("Analyze Flow (Setup required)"); + await expect(analyzeFlowBtn).toHaveClass(/no-api-key/); await page.locator(".annotation-actions").screenshot({ path: path.join(SCREENSHOTS_DIR, "30-buttons-no-api-key.png"), @@ -571,13 +565,13 @@ test.describe("Hardening — Refinement", () => { }, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "anthropic", model: "claude-sonnet-4-6", api_key_source: "ANTHROPIC_API_KEY", has_api_key: true, refinement_provider: "anthropic", refinement_model: "claude-sonnet-4-6", - refinement_max_iterations: 1, }); await page.waitForTimeout(300); @@ -601,13 +595,13 @@ test.describe("Hardening — Refinement", () => { }, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "anthropic", model: "claude-sonnet-4-6", api_key_source: "ANTHROPIC_API_KEY", has_api_key: true, refinement_provider: "anthropic", refinement_model: "claude-sonnet-4-6", - refinement_max_iterations: 1, }); await page.waitForTimeout(300); @@ -638,13 +632,13 @@ test.describe("Hardening — Refinement", () => { }, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "anthropic", model: "claude-sonnet-4-6", api_key_source: "ANTHROPIC_API_KEY", has_api_key: true, refinement_provider: "anthropic", refinement_model: "claude-sonnet-4-6", - refinement_max_iterations: 1, }); await page.waitForTimeout(300); @@ -672,13 +666,13 @@ test.describe("Hardening — Refinement", () => { }, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "anthropic", model: "claude-sonnet-4-6", api_key_source: "ANTHROPIC_API_KEY", has_api_key: true, refinement_provider: "anthropic", refinement_model: "claude-sonnet-4-6", - refinement_max_iterations: 1, }); await page.waitForTimeout(300); @@ -702,6 +696,36 @@ test.describe("Hardening — Refinement", () => { }); }); + test("34c — the PR overview survives a refinement", async ({ page }) => { + await page.goto("/"); + await waitForAnalysis(page); + + await page.evaluate((settings) => { + (window as any).__TEST_API__.setLlmSettings(settings); + }, { + annotations_enabled: true, + refinement_enabled: true, + metadata_enabled: false, + provider: "anthropic", + model: "claude-sonnet-4-6", + api_key_source: "ANTHROPIC_API_KEY", + has_api_key: true, + refinement_provider: "anthropic", + refinement_model: "claude-sonnet-4-6", + }); + await page.waitForTimeout(300); + + await page.locator(".btn-refine").click(); + await page.waitForTimeout(2000); + + // Refining must not cost the user their PR overview + await page.getByTestId("annotations-tab").click(); + await page.locator(".btn-pr-overview").click(); + + await expect(page.getByTestId("pr-overview")).toBeVisible(); + await expect(page.locator(".llm-summary").first()).toBeVisible(); + }); + test("34b — refinement toggle crossfades the group list instead of hard-swapping it", async ({ page }) => { await page.goto("/"); await waitForAnalysis(page); @@ -711,13 +735,13 @@ test.describe("Hardening — Refinement", () => { }, { annotations_enabled: true, refinement_enabled: true, + metadata_enabled: false, provider: "anthropic", model: "claude-sonnet-4-6", api_key_source: "ANTHROPIC_API_KEY", has_api_key: true, refinement_provider: "anthropic", refinement_model: "claude-sonnet-4-6", - refinement_max_iterations: 1, }); await page.waitForTimeout(300); diff --git a/crates/diffcore-tauri/ui/tests/e2e/onboarding.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/onboarding.spec.ts index 0cdedba7..ad59e0b9 100644 --- a/crates/diffcore-tauri/ui/tests/e2e/onboarding.spec.ts +++ b/crates/diffcore-tauri/ui/tests/e2e/onboarding.spec.ts @@ -17,13 +17,13 @@ function baseMissingSettings(overrides: Record = {}) { return { annotations_enabled: false, refinement_enabled: false, + metadata_enabled: false, provider: "openai", model: "gpt-5.4", api_key_source: "none", has_api_key: false, refinement_provider: "openai", refinement_model: "gpt-5.4", - refinement_max_iterations: 1, global_config_path: "~/.diffcore/config.toml", codex_available: false, codex_authenticated: false, @@ -47,7 +47,7 @@ test.describe("AI onboarding", () => { })); await expect(page.getByTestId("ai-onboarding")).toHaveCount(0); - await expect(page.locator(".btn-summarize")).toBeEnabled(); + await expect(page.locator(".btn-analyze-flow")).toBeEnabled(); await expect(page.locator(".llm-provider-badge")).toContainText("Codex CLI/default"); }); @@ -57,7 +57,7 @@ test.describe("AI onboarding", () => { codex_authenticated: true, })); - await expect(page.locator(".btn-summarize")).toBeEnabled(); + await expect(page.locator(".btn-analyze-flow")).toBeEnabled(); await expect(page.locator(".llm-provider-badge")).toContainText("Codex CLI/default"); await page.evaluate(() => { @@ -70,7 +70,7 @@ test.describe("AI onboarding", () => { await page.getByTestId("ai-card-codex").getByRole("button", { name: "Use Codex CLI" }).click(); await expect(onboarding).not.toBeVisible(); - await expect(page.locator(".btn-summarize")).toBeEnabled(); + await expect(page.locator(".btn-analyze-flow")).toBeEnabled(); await expect(page.locator(".llm-provider-badge")).toContainText("Codex CLI/default"); }); @@ -85,7 +85,7 @@ test.describe("AI onboarding", () => { await page.getByTestId("api-key-save").click(); await expect(onboarding).not.toBeVisible(); - await expect(page.locator(".btn-summarize")).toBeEnabled(); + await expect(page.locator(".btn-analyze-flow")).toBeEnabled(); await expect(page.locator(".llm-provider-badge")).toContainText("OpenAI API/gpt-5.4"); }); diff --git a/crates/diffcore-tauri/ui/tests/e2e/tauri-audit.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/tauri-audit.spec.ts index 8bef6edb..19b3407b 100644 --- a/crates/diffcore-tauri/ui/tests/e2e/tauri-audit.spec.ts +++ b/crates/diffcore-tauri/ui/tests/e2e/tauri-audit.spec.ts @@ -484,7 +484,7 @@ test.describe("State Desync Prevention", () => { // Click the second group const secondGroup = page.locator(".group-item:not(.infra-group)").nth(1); - await secondGroup.click(); + await secondGroup.locator(".group-name").click(); await page.waitForTimeout(500); // The selected file should have changed (new group's first file) @@ -534,7 +534,7 @@ test.describe("State Desync Prevention", () => { // Click a different group const secondGroup = page.locator(".group-item:not(.infra-group)").nth(1); - await secondGroup.click(); + await secondGroup.locator(".group-name").click(); await page.waitForTimeout(500); // Replay should be exited diff --git a/crates/diffcore-tauri/ui/tests/e2e/visual-polish.spec.ts b/crates/diffcore-tauri/ui/tests/e2e/visual-polish.spec.ts index 6d59d943..7ac54027 100644 --- a/crates/diffcore-tauri/ui/tests/e2e/visual-polish.spec.ts +++ b/crates/diffcore-tauri/ui/tests/e2e/visual-polish.spec.ts @@ -110,7 +110,7 @@ test.describe("Visual Polish — Screenshot Baseline", () => { // Click on the second group (auth) const secondGroup = page.locator(".group-item").nth(1); - await secondGroup.click(); + await secondGroup.locator(".group-name").click(); await page.waitForTimeout(1500); // Wait for graph re-render await page.screenshot({ diff --git a/docs/grouping-overhaul.md b/docs/grouping-overhaul.md index 1245e86c..7b243d09 100644 --- a/docs/grouping-overhaul.md +++ b/docs/grouping-overhaul.md @@ -33,7 +33,6 @@ The group cap is now configurable through: Iterative refinement is now configurable through: -- `.diffcore.toml` via `[llm.refinement].max_iterations` - `diffcore analyze --refine-iterations ` ### 3. Iterative refinement plumbing diff --git a/docs/screenshots/01-loaded-analysis.png b/docs/screenshots/01-loaded-analysis.png index 6e5b334a..962e997d 100644 Binary files a/docs/screenshots/01-loaded-analysis.png and b/docs/screenshots/01-loaded-analysis.png differ diff --git a/docs/screenshots/02-flow-groups-panel.png b/docs/screenshots/02-flow-groups-panel.png index 8167297d..465925a5 100644 Binary files a/docs/screenshots/02-flow-groups-panel.png and b/docs/screenshots/02-flow-groups-panel.png differ diff --git a/docs/screenshots/04-annotations-panel.png b/docs/screenshots/04-annotations-panel.png index b8bed55a..c31b87d9 100644 Binary files a/docs/screenshots/04-annotations-panel.png and b/docs/screenshots/04-annotations-panel.png differ diff --git a/docs/screenshots/05-second-group-selected.png b/docs/screenshots/05-second-group-selected.png index fdbac92f..6e482531 100644 Binary files a/docs/screenshots/05-second-group-selected.png and b/docs/screenshots/05-second-group-selected.png differ diff --git a/docs/screenshots/06-third-group-low-risk.png b/docs/screenshots/06-third-group-low-risk.png index 78ec7175..4312e942 100644 Binary files a/docs/screenshots/06-third-group-low-risk.png and b/docs/screenshots/06-third-group-low-risk.png differ diff --git a/docs/screenshots/07-second-file-selected.png b/docs/screenshots/07-second-file-selected.png index 8f9beb7b..05420e6b 100644 Binary files a/docs/screenshots/07-second-file-selected.png and b/docs/screenshots/07-second-file-selected.png differ diff --git a/docs/screenshots/08-keyboard-navigation.png b/docs/screenshots/08-keyboard-navigation.png index 280ac165..6f074f8b 100644 Binary files a/docs/screenshots/08-keyboard-navigation.png and b/docs/screenshots/08-keyboard-navigation.png differ diff --git a/docs/screenshots/09-group-keyboard-navigation.png b/docs/screenshots/09-group-keyboard-navigation.png index 10f7e587..5b3c9b5b 100644 Binary files a/docs/screenshots/09-group-keyboard-navigation.png and b/docs/screenshots/09-group-keyboard-navigation.png differ diff --git a/docs/screenshots/11-flow-graph.png b/docs/screenshots/11-flow-graph.png index 982e4767..332e2676 100644 Binary files a/docs/screenshots/11-flow-graph.png and b/docs/screenshots/11-flow-graph.png differ diff --git a/docs/screenshots/12-error-state.png b/docs/screenshots/12-error-state.png index cec45827..e404f4d0 100644 Binary files a/docs/screenshots/12-error-state.png and b/docs/screenshots/12-error-state.png differ diff --git a/docs/screenshots/15-branch-dropdown-open.png b/docs/screenshots/15-branch-dropdown-open.png index e70cb1e6..bcb9c9a9 100644 Binary files a/docs/screenshots/15-branch-dropdown-open.png and b/docs/screenshots/15-branch-dropdown-open.png differ diff --git a/docs/screenshots/17-branch-dropdown-many.png b/docs/screenshots/17-branch-dropdown-many.png index 01a306db..f4262183 100644 Binary files a/docs/screenshots/17-branch-dropdown-many.png and b/docs/screenshots/17-branch-dropdown-many.png differ diff --git a/docs/screenshots/22-settings-panel.png b/docs/screenshots/22-settings-panel.png index 147c0267..80b668ee 100644 Binary files a/docs/screenshots/22-settings-panel.png and b/docs/screenshots/22-settings-panel.png differ diff --git a/docs/screenshots/24-api-key-missing.png b/docs/screenshots/24-api-key-missing.png index 0896e7b0..010e3ae1 100644 Binary files a/docs/screenshots/24-api-key-missing.png and b/docs/screenshots/24-api-key-missing.png differ diff --git a/docs/screenshots/25-refinement-settings-expanded.png b/docs/screenshots/25-refinement-settings-expanded.png index 5a516614..1881a68e 100644 Binary files a/docs/screenshots/25-refinement-settings-expanded.png and b/docs/screenshots/25-refinement-settings-expanded.png differ diff --git a/docs/screenshots/28-summarize-complete.png b/docs/screenshots/28-summarize-complete.png index b3d1943e..ee481c3b 100644 Binary files a/docs/screenshots/28-summarize-complete.png and b/docs/screenshots/28-summarize-complete.png differ diff --git a/docs/screenshots/29-deep-analysis-complete.png b/docs/screenshots/29-deep-analysis-complete.png index 1e5ae70e..b991b29f 100644 Binary files a/docs/screenshots/29-deep-analysis-complete.png and b/docs/screenshots/29-deep-analysis-complete.png differ diff --git a/docs/screenshots/32-refinement-complete.png b/docs/screenshots/32-refinement-complete.png index 4d143df5..731bcd7e 100644 Binary files a/docs/screenshots/32-refinement-complete.png and b/docs/screenshots/32-refinement-complete.png differ diff --git a/docs/screenshots/33-refinement-change-indicators.png b/docs/screenshots/33-refinement-change-indicators.png index e285a97a..64141afc 100644 Binary files a/docs/screenshots/33-refinement-change-indicators.png and b/docs/screenshots/33-refinement-change-indicators.png differ diff --git a/docs/screenshots/34-refinement-original-view.png b/docs/screenshots/34-refinement-original-view.png index d7accec9..2fa9dfce 100644 Binary files a/docs/screenshots/34-refinement-original-view.png and b/docs/screenshots/34-refinement-original-view.png differ diff --git a/docs/screenshots/35-graph-node-selected.png b/docs/screenshots/35-graph-node-selected.png index 262b91ee..e43a2a75 100644 Binary files a/docs/screenshots/35-graph-node-selected.png and b/docs/screenshots/35-graph-node-selected.png differ diff --git a/docs/screenshots/36-graph-legend-expanded.png b/docs/screenshots/36-graph-legend-expanded.png index 2d2cc385..390ee5e1 100644 Binary files a/docs/screenshots/36-graph-legend-expanded.png and b/docs/screenshots/36-graph-legend-expanded.png differ diff --git a/docs/screenshots/37-graph-fullscreen.png b/docs/screenshots/37-graph-fullscreen.png index ccf3c7f0..94b68df0 100644 Binary files a/docs/screenshots/37-graph-fullscreen.png and b/docs/screenshots/37-graph-fullscreen.png differ diff --git a/docs/screenshots/41-error-state-real.png b/docs/screenshots/41-error-state-real.png index 8ae22f96..9944a634 100644 Binary files a/docs/screenshots/41-error-state-real.png and b/docs/screenshots/41-error-state-real.png differ diff --git a/docs/screenshots/43-large-dataset.png b/docs/screenshots/43-large-dataset.png index 2292f073..69ed3166 100644 Binary files a/docs/screenshots/43-large-dataset.png and b/docs/screenshots/43-large-dataset.png differ diff --git a/docs/screenshots/44-large-dataset-scrolled.png b/docs/screenshots/44-large-dataset-scrolled.png index c20edffe..1c62e747 100644 Binary files a/docs/screenshots/44-large-dataset-scrolled.png and b/docs/screenshots/44-large-dataset-scrolled.png differ diff --git a/docs/screenshots/46-responsive-narrow.png b/docs/screenshots/46-responsive-narrow.png index b90bc06a..13bdf87f 100644 Binary files a/docs/screenshots/46-responsive-narrow.png and b/docs/screenshots/46-responsive-narrow.png differ diff --git a/docs/screenshots/47-responsive-wide.png b/docs/screenshots/47-responsive-wide.png index 49311d4f..db9bd885 100644 Binary files a/docs/screenshots/47-responsive-wide.png and b/docs/screenshots/47-responsive-wide.png differ diff --git a/docs/screenshots/48-responsive-minimum.png b/docs/screenshots/48-responsive-minimum.png index 8bfc5be6..379a63b6 100644 Binary files a/docs/screenshots/48-responsive-minimum.png and b/docs/screenshots/48-responsive-minimum.png differ diff --git a/docs/screenshots/50-pr-preview-switched-branch.png b/docs/screenshots/50-pr-preview-switched-branch.png index a5436823..aa80d11c 100644 Binary files a/docs/screenshots/50-pr-preview-switched-branch.png and b/docs/screenshots/50-pr-preview-switched-branch.png differ diff --git a/docs/screenshots/51-replay-active.png b/docs/screenshots/51-replay-active.png index 3f4fd27a..217bd67d 100644 Binary files a/docs/screenshots/51-replay-active.png and b/docs/screenshots/51-replay-active.png differ diff --git a/docs/screenshots/52-replay-step-2.png b/docs/screenshots/52-replay-step-2.png index 1158acef..3cfb736c 100644 Binary files a/docs/screenshots/52-replay-step-2.png and b/docs/screenshots/52-replay-step-2.png differ diff --git a/docs/screenshots/53-replay-visited-checks.png b/docs/screenshots/53-replay-visited-checks.png index 1158acef..3cfb736c 100644 Binary files a/docs/screenshots/53-replay-visited-checks.png and b/docs/screenshots/53-replay-visited-checks.png differ diff --git a/docs/screenshots/54-replay-last-step.png b/docs/screenshots/54-replay-last-step.png index 4aab2cf6..f6973dfc 100644 Binary files a/docs/screenshots/54-replay-last-step.png and b/docs/screenshots/54-replay-last-step.png differ diff --git a/docs/screenshots/60-analysis-loaded.png b/docs/screenshots/60-analysis-loaded.png index 6e5b334a..962e997d 100644 Binary files a/docs/screenshots/60-analysis-loaded.png and b/docs/screenshots/60-analysis-loaded.png differ diff --git a/docs/screenshots/69-flow-groups-panel.png b/docs/screenshots/69-flow-groups-panel.png index 8167297d..465925a5 100644 Binary files a/docs/screenshots/69-flow-groups-panel.png and b/docs/screenshots/69-flow-groups-panel.png differ diff --git a/docs/screenshots/70-second-group.png b/docs/screenshots/70-second-group.png index 4f2e15a8..3990a2d6 100644 Binary files a/docs/screenshots/70-second-group.png and b/docs/screenshots/70-second-group.png differ diff --git a/docs/screenshots/71-replay-mode.png b/docs/screenshots/71-replay-mode.png index 3f4fd27a..217bd67d 100644 Binary files a/docs/screenshots/71-replay-mode.png and b/docs/screenshots/71-replay-mode.png differ diff --git a/docs/screenshots/72-annotations-panel.png b/docs/screenshots/72-annotations-panel.png index 2afd9669..0a4e19ff 100644 Binary files a/docs/screenshots/72-annotations-panel.png and b/docs/screenshots/72-annotations-panel.png differ diff --git a/docs/screenshots/80-editor-no-comments.png b/docs/screenshots/80-editor-no-comments.png index 6e5b334a..962e997d 100644 Binary files a/docs/screenshots/80-editor-no-comments.png and b/docs/screenshots/80-editor-no-comments.png differ diff --git a/docs/screenshots/84-monaco-glyph-check.png b/docs/screenshots/84-monaco-glyph-check.png index 4bb9bbe8..6ce50b5c 100644 Binary files a/docs/screenshots/84-monaco-glyph-check.png and b/docs/screenshots/84-monaco-glyph-check.png differ diff --git a/docs/screenshots/86-glyph-hover.png b/docs/screenshots/86-glyph-hover.png index 1d011a33..83ca98ba 100644 Binary files a/docs/screenshots/86-glyph-hover.png and b/docs/screenshots/86-glyph-hover.png differ diff --git a/docs/screenshots/87-glyph-click-activates-comment.png b/docs/screenshots/87-glyph-click-activates-comment.png index 4fd73169..20390ea0 100644 Binary files a/docs/screenshots/87-glyph-click-activates-comment.png and b/docs/screenshots/87-glyph-click-activates-comment.png differ diff --git a/docs/screenshots/annotations-panel.png b/docs/screenshots/annotations-panel.png index 2afd9669..0a4e19ff 100644 Binary files a/docs/screenshots/annotations-panel.png and b/docs/screenshots/annotations-panel.png differ diff --git a/docs/screenshots/comments-gutter.png b/docs/screenshots/comments-gutter.png index d7d06a33..84daf2fb 100644 Binary files a/docs/screenshots/comments-gutter.png and b/docs/screenshots/comments-gutter.png differ diff --git a/docs/screenshots/group-review-metadata.png b/docs/screenshots/group-review-metadata.png new file mode 100644 index 00000000..63fe4078 Binary files /dev/null and b/docs/screenshots/group-review-metadata.png differ diff --git a/docs/screenshots/hero-analysis.png b/docs/screenshots/hero-analysis.png index 6e5b334a..eb068255 100644 Binary files a/docs/screenshots/hero-analysis.png and b/docs/screenshots/hero-analysis.png differ diff --git a/docs/screenshots/replay-mode.png b/docs/screenshots/replay-mode.png index 3f4fd27a..217bd67d 100644 Binary files a/docs/screenshots/replay-mode.png and b/docs/screenshots/replay-mode.png differ diff --git a/docs/screenshots/second-group.png b/docs/screenshots/second-group.png index ad5d745b..3990a2d6 100644 Binary files a/docs/screenshots/second-group.png and b/docs/screenshots/second-group.png differ diff --git a/extensions/vscode/src/types.ts b/extensions/vscode/src/types.ts index f476ada4..bbd51442 100644 --- a/extensions/vscode/src/types.ts +++ b/extensions/vscode/src/types.ts @@ -79,6 +79,15 @@ export interface Entrypoint { entrypoint_type: EntrypointType; } +export type GroupType = + | "Feat" | "Fix" | "Perf" | "Refactor" | "Test" | "Docs" | "Build" | "Ci" | "Chore"; +export type Risk = "Low" | "Medium" | "High" | "Critical"; +export type ImpactScope = "Local" | "Module" | "CrossCutting" | "System"; +export type ReviewComplexity = "Trivial" | "Simple" | "Moderate" | "Complex"; +export type ReviewFocus = + | "Correctness" | "Security" | "Concurrency" | "Performance" + | "DataIntegrity" | "Compatibility" | "ErrorHandling" | "ApiContract"; + export interface FlowGroup { id: string; name: string; @@ -87,6 +96,14 @@ export interface FlowGroup { edges: FlowEdge[]; risk_score: number; review_order: number; + group_type?: GroupType | null; + description?: string | null; + risk?: Risk | null; + impact?: ImpactScope | null; + complexity?: ReviewComplexity | null; + review_focus?: ReviewFocus[]; + summary?: string[]; + invariant?: string | null; } export interface InfrastructureGroup { @@ -120,16 +137,7 @@ export interface AnalysisOutput { // LLM annotation types (from schema.rs) -export interface Pass1GroupAnnotation { - id: string; - name: string; - summary: string; - review_order_rationale: string; - risk_flags: string[]; -} - export interface Pass1Response { - groups: Pass1GroupAnnotation[]; overall_summary: string; suggested_review_order: string[]; } diff --git a/extensions/vscode/src/webviewPanel.ts b/extensions/vscode/src/webviewPanel.ts index b92a7b65..304539c1 100644 --- a/extensions/vscode/src/webviewPanel.ts +++ b/extensions/vscode/src/webviewPanel.ts @@ -56,7 +56,6 @@ export class AnnotationsPanel { } private buildHtml(group: FlowGroup): string { - const pass1Group = this.pass1?.groups.find((g) => g.id === group.id); const pass2 = this.pass2Map.get(group.id); return /* html */ ` @@ -105,7 +104,7 @@ export class AnnotationsPanel { ${group.entrypoint ? `· Entry: ${escapeHtml(group.entrypoint.symbol)} (${group.entrypoint.entrypoint_type})` : ""}
- ${pass1Group ? renderPass1(pass1Group) : ""} + ${renderReviewMetadata(group)} ${pass2 ? renderPass2(pass2) : ""} @@ -149,12 +148,40 @@ function riskBadge(score: number): string { return `LOW ${score.toFixed(2)}`; } -function renderPass1(annotation: { summary: string; risk_flags: string[]; review_order_rationale: string }): string { +function renderReviewMetadata(group: FlowGroup): string { + const verdict = [group.group_type, group.risk, group.impact, group.complexity] + .flatMap((v) => (v ? [escapeHtml(hyphenateVariant(v).toUpperCase())] : [])); + const focus = (group.review_focus ?? []).map((f) => hyphenateVariant(f).toLowerCase()); + + const summary = group.summary ?? []; + if ( + verdict.length === 0 && focus.length === 0 && summary.length === 0 && + !group.description && !group.invariant + ) { + return ""; + } + return ` -

LLM Summary

-

${escapeHtml(annotation.summary)}

- ${annotation.risk_flags.length > 0 ? `
${annotation.risk_flags.map((f) => `${escapeHtml(f)}`).join("")}
` : ""} -

${escapeHtml(annotation.review_order_rationale)}

`; +

How to review this

+ ${verdict.length > 0 ? `
${verdict.join(" · ")}
` : ""} + ${focus.length > 0 ? `
${focus.map((f) => `${escapeHtml(f)}`).join("")}
` : ""} + ${group.description ? `

${escapeHtml(group.description)}

` : ""} + ${renderSummary(group.summary ?? [])} + ${group.invariant ? `

Invariant: ${escapeHtml(group.invariant)}

` : ""}`; +} + +function renderSummary(summary: string[]): string { + if (summary.length === 0) { + return ""; + } + if (summary.length === 1) { + return `

${escapeHtml(summary[0])}

`; + } + return `
    ${summary.map((point) => `
  • ${escapeHtml(point)}
    • `).join("")}
`; +} + +function hyphenateVariant(variant: string): string { + return variant.replace(/([a-z])([A-Z])/g, "$1-$2"); } function renderPass2(pass2: Pass2Response): string { diff --git a/specs/group-metadata.md b/specs/group-metadata.md new file mode 100644 index 00000000..edc415e6 --- /dev/null +++ b/specs/group-metadata.md @@ -0,0 +1,392 @@ +# Group Review Metadata — Specification + +Origin: [issue #13](https://github.com/jamesaphoenix/diff-core/issues/13) and the design discussion in its comments. + +## Problem + +A user who runs Analyze → Refine ends up with better groupings and no prose. To get any narrative they must take a second, separate, deliberate action — "Summarize PR" in the desktop app, or `--annotate` on the CLI — and nothing in the refine flow suggests that action exists. + +Worse, the prose they eventually get answers the wrong question. `Pass1GroupAnnotation.summary` describes *what changed*. A reviewer opening a 60-file agent-authored PR needs to know *how to review this group*: how risky it is, what class of change it is, what property to verify while reading. + +Three concrete defects: + +1. **Discoverability** — group narrative exists only behind a manual second call that the primary flow never mentions. +2. **Wrong question** — narrative summaries restate mechanically observable facts instead of directing review attention. +3. **Separate keyed object** — annotations live in `Annotations.overview.groups`, keyed by group id, disjoint from the `FlowGroup` they describe. Consumers must join two structures to render one card. + +## Goals + +- Every group carries review metadata **on the group itself**, not in a side-car keyed object. +- Metadata is available on the free deterministic path, not only after a paid pass. +- The important bits render in a **fixed layout that can be glanced at**; detail sits below. +- Ranking and review order stay deterministic and reproducible. +- The golden eval corpus does not become model-version-sensitive. + +--- + +## 1. Data Model + +### 1.1 New enums (`diffcore-core/src/types.rs`) + +```rust +pub enum GroupType { Feat, Fix, Perf, Refactor, Test, Docs, Build, Ci, Chore } + +pub enum Risk { Low, Medium, High, Critical } + +pub enum ImpactScope { Local, Module, CrossCutting, System } + +pub enum ReviewComplexity { Trivial, Simple, Moderate, Complex } + +pub enum ReviewFocus { + Correctness, Security, Concurrency, Performance, + DataIntegrity, Compatibility, ErrorHandling, ApiContract, +} +``` + +All derive `Debug, Clone, Serialize, Deserialize, PartialEq, JsonSchema`. + +### 1.2 `FlowGroup` extension + +```rust +pub struct FlowGroup { + // existing fields unchanged + pub id: String, + pub name: String, + pub entrypoint: Option, + pub files: Vec, + pub edges: Vec, + pub risk_score: f64, + pub review_order: u32, + + // new + #[serde(default)] + pub group_type: Option, + #[serde(default)] + pub description: Option, + #[serde(default)] + pub risk: Option, + #[serde(default)] + pub impact: Option, + #[serde(default)] + pub complexity: Option, + #[serde(default)] + pub review_focus: Vec, + #[serde(default)] + pub summary: Vec, + #[serde(default)] + pub invariant: Option, +} +``` + +All seven fields ship in the first cut. `#[serde(default)]` throughout means previously written analysis JSON still deserializes. + +### 1.3 `risk_score` vs `Risk` + +`risk_score: f64` remains deterministic and remains the **only** input to review ranking. `Risk` is a label derived from it. The metadata pass may override the label; it may never write the score. + +Rationale: letting a model move the score makes review order non-reproducible and shifts the entire golden-eval baseline whenever a model version changes. + +### 1.4 `summary` is the detail tier + +`description` is the one-line caption that sits with the chips; `summary` is +what the reader drops to when the caption isn't enough. It answers "what does +this group achieve", **sized to the change**: a single entry when one sentence +covers it, more only when the group genuinely does several things, capped at +`MAX_SUMMARY_BULLETS` (5). + +It is a `Vec`, not a markdown string, which is what keeps §4.2's +no-markdown rule intact while still producing bullets. The list *is* the +structure — the UI renders one entry as prose and several as an unordered list, +with no parser and no renderer anywhere in the path. Models emit leading `-` +and `*` markers regardless of the schema, so `apply_metadata` strips them. + +### 1.5 `invariant` is the point + +`description` answers "what changed". `invariant` answers "what property should I verify while reading this". The second is what makes the panel worth looking at: + +```text +FIX · HIGH RISK · CROSS-CUTTING · COMPLEX +Focus: concurrency, data-integrity + +Move job claiming behind a Redis lock. + +Invariant: Two workers must never successfully claim the same job. +``` + +Metadata must not restate LOC, file counts, or anything else already visible in the group card. + +--- + +## 2. Pipeline Placement + +### 2.1 A separate pass, not a refinement field + +Refinement is patch-op shaped and its `RefinementResponse` contract is **unchanged** by this spec. It cannot carry per-group metadata, because it does not know the ids of the groups it produces: `apply_split` mints `group_refined_{n}` at apply time (`llm/refinement.rs:860`) and `apply_merge` reuses the first source id (`llm/refinement.rs:303`). A model answering the refinement prompt cannot key metadata to groups that do not yet exist. + +The pipeline is therefore: + +```text +deterministic clustering + ↓ +rank → risk_score → heuristic floor + ↓ +[optional] refinement ops → apply → re-score → heuristic floor again + ↓ +final groups ──────→ metadata pass ──→ FlowGroup fields +``` + +The floor runs twice on the refinement path, and that is load-bearing. +`apply_refinement` mints split products with `..Default::default()` and merged +groups with `risk_score: 0.0`, so after refinement the metadata is empty for +every group it touched and the score it derives from is wrong. `rank::rescore_groups` +recomputes `risk_score` (leaving `review_order` alone — refinement may have +re-ranked deliberately) and the floor is then re-applied on top. This also fixes +a pre-existing bug: before this spec, nothing re-scored after refinement, so +merged groups sorted as the least risky change in the diff. + +The metadata pass consumes **final** groups. It runs whether or not refinement ran, which is what allows descriptions on a plain deterministic grouping. + +The original issue framed the fix as "fold this into refinement instead of a separate call". The actual complaint was that the second step was *manual and undiscoverable*, not that it was a second call. An automatic second pass resolves it without welding two unrelated contracts together. + +### 2.2 The pass re-runs after refinement + +Refinement rebuilds groups, so their metadata goes with them: split products are +constructed with `..Default::default()` and merges start empty. The desktop +therefore fires the metadata pass twice — once after analyze, once after any +refinement that changed the grouping — rather than exposing a third button. + +This needs `AppState::last_analysis` to be an `Arc>`. The desktop +refines through `start_refine_groups`, which spawns a `'static` background task +and cannot borrow `State<'_, AppState>`; before this change that task never wrote +its result back, so every command reading `last_analysis` — `describe_groups`, +and `annotate_overview` before it — kept answering about pre-refinement groups. + +### 2.3 Metadata never re-derives from merge rules + +When refinement merges a `Fix` group and a `Perf` group, the result is not computed by precedence rules over the inputs. The metadata pass sees the assembled final group and re-assesses every field from scratch. + +--- + +## 3. Provenance + +### 3.1 Heuristic floor + +The metadata pass is optional and costs money. Fields that can be honestly inferred without a model are populated deterministically first: + +| Field | Deterministic floor | +|-------|--------------------| +| `risk` | Bucketed from `risk_score` | +| `group_type` | Path conventions (`tests/**` → Test, `*.md` → Docs, `.github/**` → Ci, …) | +| `impact` | Spread of the group's files across module roots and directories | +| `description` | None — empty | +| `invariant` | None — empty | +| `review_focus` | None — empty | +| `summary` | None — empty | +| `complexity` | None — empty | + +**`impact` is not derived from graph fan-out**, despite that being the obvious +reading. `collect_internal_edges` (`cluster/bfs.rs:78`) keeps only edges whose +endpoints are *both* inside the group, so `FlowGroup::edges` cannot cross a +group boundary by construction, and the `SymbolGraph` is gone by the time +groups are finalized. A cross-group proxy — promote to `CrossCutting` when +another group also touched the same directory — was prototyped and rejected: on +the `simple_express_app` fixture it labelled two sibling one-file route groups +as cross-cutting, the opposite of the truth. `impact` therefore counts distinct +module roots and directories within the group itself, and +`sibling_groups_in_one_directory_stay_local` pins the rejected behaviour. + +`review_focus` has a tempting mapping from `RiskIndicators` (`has_auth_change` → Security). It is deliberately not used: the mapping is lossy enough to point reviewers at the wrong concern, and an empty chip row is better than a wrong one. `invariant` is the field where a heuristic guess is most harmful — a wrong invariant sends a reviewer hunting for a property that was never at stake. + +### 3.2 LLM override + +When the metadata pass runs, its output wins on every field, including the three with floors. The floor exists to make the free path useful, not to constrain the model. + +Exception, permanently: `risk_score`. See §1.3. + +If the eval later shows the deterministic `impact` beating the model's — plausible, since the heuristic reads real graph edges while the model reads a path list — pin `impact` then. Do not pin it pre-emptively. + +--- + +## 4. Prompt and Response + +### 4.1 Request + +Per batch: + +- **Full detail** for the batch's own groups: names, file paths, roles, diff content. +- **Read-only index of every final group** in the analysis: id, name, file paths, `risk_score`. No diff content. + +The index is what makes cross-group judgment (`ImpactScope::CrossCutting`) possible from inside a batch. It is immutable input derived from the already-applied grouping, so batch ordering cannot affect any result. + +Batches must never read other batches' *results*. That would make output depend on completion order, which under concurrent dispatch is nondeterministic, which breaks the structural assertions in §7. + +### 4.2 Response constraints + +Enforced in the prompt and in the JSON schema description, then re-enforced on the consuming side: + +- `review_focus`: at most 3 entries. +- `invariant`: one sentence. +- `description`: one line. +- `summary`: at most 5 entries, each a bare sentence with no leading bullet marker. +- `description` and `invariant` are **plain text**, not markdown. Both are short enough to have no structure to mark up, and both are consumed by a fixed-layout glance panel and by downstream agents that do not benefit from stripping markup. + +--- + +## 5. Batching and Concurrency + +- Batch size from `MetadataConfig.batch_size`, default 20 groups per call. +- Batches dispatched concurrently. +- Results keyed by group id and re-sorted by `review_order` before serialization, so concurrency never reaches the output JSON. + +Sizing by token estimate rather than group count is the correct eventual answer. Group count is an adequate proxy until the large-diff track demonstrates otherwise. + +Above `LARGE_DIFF_PARTITION_THRESHOLD` (2000 files, `cluster/mod.rs:58`) the pass still runs, batched. The heuristic floor covers whatever it cannot reach. + +--- + +## 6. Surfaces + +### 6.1 Desktop + +- **Left group list** — `group_type` and `risk` as chips in the group header. No description line; a description in a scan-list defeats scanning. +- **Right panel** — the fixed-layout block (chip row, `description`, `Invariant:`), then `summary` in its own section directly beneath, then existing detail. + `summary` sits *outside* the fixed-layout block on purpose: its height varies + from one to five entries, and reserving the worst case would defeat the glance + row above it. +- **PR-level overview** — not rendered in the group panel; it competed with the + group's own prose, which is the problem this spec exists to fix. It lives + behind a `PR Overview` toggle in the annotations panel, and completing + "Summarize PR" reveals it so the click has visible output. Selecting any group + returns to the group view. + + Gating it on "no group is selected" does **not** work: analysis auto-selects + the first group and nothing but dismissing an empty group ever clears the + selection, so the overview would be unreachable and Summarize PR would spend + money to display nothing. +- Absent fields are **omitted entirely**. No `—` placeholders, no "run refine to fill this in" nudges. +- `review_focus` truncates rather than wraps. + +### 6.2 Firing policy + +- **Desktop/web**: fires automatically after analyze via the `describe_groups` command, gated on the `metadata_enabled` setting. The backend re-checks `llm.metadata.enabled` itself, so a stale UI flag cannot force a paid call. +- **CLI**: explicit `--describe` flag. `diffcore analyze` in someone's CI must not start billing them silently. + + Pass 1 fires from the same analyze path, gated on the existing + `annotations_enabled` setting, so the desktop is down to two LLM buttons: + **Refine** (restructures groups — a real decision) and **Analyze This Flow** + (Pass 2, billed per group — must stay manual). Both auto-fired passes call the + plain commands rather than the streaming jobs: a streaming job sets + `activityJob`, and the effect at `App.tsx:579` switches the right panel to the + Activity tab on that, which would throw the user out of the groups they just + analyzed. The consequence is that **Pass 1 no longer appears in the Activity + tab** — Refine and Analyze This Flow still do. + +### 6.3 CLI + +- `--describe` runs the metadata pass. +- `--annotate` retains Pass 1 for the **PR-level overview only**. + +--- + +## 7. Eval + +Metadata is emitted and snapshotted but **non-gating**. + +Asserted on every group, every run: +- Schema validity — enum variants parse, no unknown values. +- Cap compliance — `review_focus` ≤ 3, `invariant` single sentence, `description` single line. +- Presence — every group has whatever its provenance tier promises. + +Not asserted: `description` and `invariant` content quality. Scoring prose against a golden string produces a corpus that goes red when a provider ships a new checkpoint, which trains everyone to ignore corpus failures. + +--- + +## 8. Configuration + +New `MetadataConfig` under `llm`, sibling to `RefinementConfig`: + +```toml +[llm.metadata] +enabled = true # default +provider = "anthropic" +model = "claude-haiku-4-5-20251001" +key_cmd = "op read op://vault/item/field" +batch_size = 20 # default +``` + +It gets its own provider/model rather than inheriting refinement's because the two passes want different models: refinement is a reasoning task, writing a one-line invariant is not. + +--- + +## 9. Removals + +### 9.1 `Pass1GroupAnnotation` + +Deleted entirely. Its `summary` is superseded by `description`, its `risk_flags` by `risk` + `review_focus`, its `review_order_rationale` by `description`, and its per-group `name` duplicates the name the group already has. + +`Pass1Response` retains only its PR-level overview fields. + +This is the "separate keyed object" the issue discussion argued against: metadata belongs on the group, not in a structure a consumer has to join against it. + +### 9.2 `RefinementConfig.max_iterations` + +Dead. It is parsed, validated (`config.rs:353`), and merged across global/local config (`config.rs:487`), and read by no refinement code. It describes an evaluator-optimizer loop that does not exist: `judge.rs` exposes a standalone `run_judge_evaluation`, and the CLI refine path applies the refinement or logs a warning and keeps the deterministic groups (`main.rs:595`). + +Delete the field and correct the `RefinementConfig` doc comment at `config.rs:170`, which describes the same nonexistent loop. + +### 9.3 The `max_iterations` UI control + +`LlmSettings.refinement_max_iterations` (Tauri IPC) and its "Max iterations" +number input in the desktop settings panel are removed along with the config +field. The control let a user pick a value between 1 and 10 that nothing ever +read. + +### 9.4 Pass 1 consumers + +Deleting `Pass1GroupAnnotation` breaks two surfaces that must be updated in the +same change, because `annotations.overview.groups` no longer exists in the +output JSON: + +- **Desktop UI** — the "LLM Summary" panel block, and `copyPrDescription`, which + built its `# Review Flow` section from per-group Pass 1 summaries. That now + reads `FlowGroup::description`. +- **VS Code extension** — `renderPass1` in `webviewPanel.ts`, replaced by a + `renderReviewMetadata` that reads the group's own fields. + +### 9.5 CLAUDE.md + +The overview claims refinement uses an "evaluator-optimizer loop [that] scores v1 vs v2, keeps whichever is better." It does not. Correct the text. + +--- + +## 10. Compatibility + +- `FlowGroup`'s new fields are all `#[serde(default)]`, so previously written analysis JSON deserializes unchanged. +- `RefinementResponse` is untouched, so the 8 cassettes under `crates/diffcore-core/tests/fixtures/vcr_adversarial_refinement/` stay valid. The metadata pass records its own. +- Deleting `Pass1GroupAnnotation` is a breaking change to the annotations JSON shape. Any Pass 1 cassettes need re-recording. + +--- + +## 11. Phases + +| Phase | Scope | +|-------|-------| +| 1 | Enums + `FlowGroup` fields + serde defaults. No behavior change. | +| 2 | Heuristic floor: `risk` bucketing, `group_type` path conventions, `impact` fan-out. Free path is now useful. | +| 3 | `MetadataConfig`, metadata pass, schema, batching, concurrency, result merge. | +| 4 | Desktop chip row + right-panel fixed layout; settings opt-out. | +| 5 | CLI `--describe`; `--annotate` narrowed to PR-level overview. | +| 6 | Removals (§9) and eval assertions (§7). | + +## 12. Acceptance + +1. `diffcore analyze` with no provider configured emits `risk`, `group_type`, and `impact` on every group. +2. The same run emits no `description`, `summary`, `invariant`, `review_focus`, or `complexity`. +2b. A group with a one-entry `summary` renders as prose; several entries render as bullets. +3. `diffcore analyze --describe` populates all eight on every group. +4. `--describe` without `--refine` produces metadata on deterministic groups. +5. Two runs of `--describe` over the same diff produce byte-identical field ordering. +6. `review_order` and `risk_score` are identical with and without `--describe`. +7. A ≥100-group analysis batches, and no group is missing metadata. +8. Analysis JSON written before this change still deserializes. +9. A group with no `invariant` renders no `Invariant:` label. diff --git a/specs/readme.md b/specs/readme.md index ca8d3e8a..93f37f0f 100644 --- a/specs/readme.md +++ b/specs/readme.md @@ -5,6 +5,7 @@ | [diff-analyzer](./diff-analyzer.md) | diffcore — semantic diff review tool with ranked data-flow grouping, Tauri app + VS Code extension | Active (Phase 12 complete — 7/18 acceptance tests passed, 11 require GUI/VS Code/human review) | | [web-server-mode](./web-server-mode.md) | Host the Tauri UI as a browser web app via a headless axum binary (`diffcore-web`) for remote diff review | Active (v1 implemented) | | [improved-clustering](./improved-clustering.md) | Reduce infrastructure bloat: path-based entrypoints for all languages, bidirectional BFS, infrastructure redefinition + sub-grouping | Complete (Phases 1-6 done — core types, bidirectional BFS, path-based entrypoints, sub-clustering, consumer updates, spec updates) | +| [group-metadata](./group-metadata.md) | Per-group review metadata (type, risk, impact, focus, invariant, description) on `FlowGroup`, populated by a heuristic floor plus an optional batched LLM pass | Planned (spec only) | ## Completed Tasks