diff --git a/src/crates/contracts/product-domains/src/lib.rs b/src/crates/contracts/product-domains/src/lib.rs index aaf508645e..11e3ccca4a 100644 --- a/src/crates/contracts/product-domains/src/lib.rs +++ b/src/crates/contracts/product-domains/src/lib.rs @@ -6,6 +6,11 @@ pub mod canvas; pub mod review; +pub use review::{ + fallback_review_plan, validate_review_plan, ReviewMode, ReviewPlan, ReviewPlanSubject, + ReviewPlanValidationError, ReviewSubjectCandidate, ReviewSubjectRole, MAX_REVIEW_SUBJECTS, +}; + #[cfg(feature = "plugin-source")] pub mod plugin_source; diff --git a/src/crates/contracts/product-domains/src/review.rs b/src/crates/contracts/product-domains/src/review.rs index 111100e7ca..deab8036db 100644 --- a/src/crates/contracts/product-domains/src/review.rs +++ b/src/crates/contracts/product-domains/src/review.rs @@ -1,6 +1,11 @@ //! Product policy for selecting the least costly sufficient review path. use serde::{Deserialize, Serialize}; +use std::collections::HashSet; + +pub const MAX_REVIEW_SUBJECTS: usize = 16; +const MAX_REVIEW_FOCUS_CHARS: usize = 8_000; +const MAX_REVIEW_CANDIDATE_ID_BYTES: usize = 256; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -77,6 +82,443 @@ pub struct ReviewQualityDecision { pub requires_consent: bool, } +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case", tag = "kind")] +pub enum ReviewSubjectCandidate { + Issue { + id: String, + web_url: String, + host: String, + project_path: String, + issue_id: String, + }, + PullRequest { + id: String, + web_url: String, + host: String, + project_path: String, + pull_request_id: String, + }, + GitRange { + id: String, + source_ref: String, + target_ref: String, + }, + Workspace { + id: String, + workspace_path: String, + }, + ExplicitFiles { + id: String, + paths: Vec, + }, + ExternalReference { + id: String, + url: String, + }, +} + +impl ReviewSubjectCandidate { + pub fn id(&self) -> &str { + match self { + Self::Issue { id, .. } + | Self::PullRequest { id, .. } + | Self::GitRange { id, .. } + | Self::Workspace { id, .. } + | Self::ExplicitFiles { id, .. } + | Self::ExternalReference { id, .. } => id, + } + } + + pub fn is_change_set(&self) -> bool { + matches!( + self, + Self::PullRequest { .. } + | Self::GitRange { .. } + | Self::Workspace { .. } + | Self::ExplicitFiles { .. } + ) + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ReviewMode { + SingleSubject, + Comparative, + RequirementTrace, + MultiSubject, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ReviewSubjectRole { + Primary, + Requirement, + Baseline, + CandidateImplementation, + SupportingReference, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct ReviewPlanSubject { + pub candidate_id: String, + pub role: ReviewSubjectRole, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct ReviewPlan { + pub mode: ReviewMode, + pub subjects: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case", tag = "kind")] +pub enum ReviewPlanValidationError { + EmptyCandidateCatalog, + TooManyCandidates { actual: usize, maximum: usize }, + TooManyPlanSubjects { actual: usize, maximum: usize }, + InvalidCandidateId, + DuplicateCandidateCatalogId { candidate_id: String }, + UnknownCandidateId { candidate_id: String }, + MissingCandidateId { candidate_id: String }, + DuplicateCandidateId { candidate_id: String }, + InvalidModeSubjects { mode: ReviewMode, details: String }, +} + +impl std::fmt::Display for ReviewPlanValidationError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::EmptyCandidateCatalog => write!(formatter, "review candidate catalog is empty"), + Self::TooManyCandidates { actual, maximum } => write!( + formatter, + "review candidate catalog contains {actual} subjects; at most {maximum} are allowed" + ), + Self::TooManyPlanSubjects { actual, maximum } => write!( + formatter, + "review plan contains {actual} subjects; at most {maximum} are allowed" + ), + Self::InvalidCandidateId => write!(formatter, "invalid review candidate id"), + Self::DuplicateCandidateCatalogId { candidate_id } => { + write!( + formatter, + "review candidate catalog repeats id: {candidate_id}" + ) + } + Self::UnknownCandidateId { candidate_id } => { + write!(formatter, "unknown review candidate id: {candidate_id}") + } + Self::MissingCandidateId { candidate_id } => { + write!(formatter, "review plan omits candidate id: {candidate_id}") + } + Self::DuplicateCandidateId { candidate_id } => { + write!( + formatter, + "review plan repeats candidate id: {candidate_id}" + ) + } + Self::InvalidModeSubjects { mode, details } => { + write!(formatter, "invalid subjects for {mode:?}: {details}") + } + } + } +} + +impl std::error::Error for ReviewPlanValidationError {} + +pub fn validate_review_plan( + candidates: &[ReviewSubjectCandidate], + plan: &ReviewPlan, +) -> Result<(), ReviewPlanValidationError> { + validate_candidate_catalog(candidates)?; + if plan.subjects.len() > MAX_REVIEW_SUBJECTS { + return Err(ReviewPlanValidationError::TooManyPlanSubjects { + actual: plan.subjects.len(), + maximum: MAX_REVIEW_SUBJECTS, + }); + } + + let candidate_ids: HashSet<&str> = candidates.iter().map(ReviewSubjectCandidate::id).collect(); + let mut referenced_ids = HashSet::with_capacity(plan.subjects.len()); + + for subject in &plan.subjects { + if !valid_candidate_id(&subject.candidate_id) { + return Err(ReviewPlanValidationError::InvalidCandidateId); + } + if !candidate_ids.contains(subject.candidate_id.as_str()) { + return Err(ReviewPlanValidationError::UnknownCandidateId { + candidate_id: subject.candidate_id.clone(), + }); + } + if !referenced_ids.insert(subject.candidate_id.as_str()) { + return Err(ReviewPlanValidationError::DuplicateCandidateId { + candidate_id: subject.candidate_id.clone(), + }); + } + } + + if let Some(candidate) = candidates + .iter() + .find(|candidate| !referenced_ids.contains(candidate.id())) + { + return Err(ReviewPlanValidationError::MissingCandidateId { + candidate_id: candidate.id().to_owned(), + }); + } + + let non_supporting_count = plan + .subjects + .iter() + .filter(|subject| subject.role != ReviewSubjectRole::SupportingReference) + .count(); + let comparison_count = plan + .subjects + .iter() + .filter(|subject| { + matches!( + subject.role, + ReviewSubjectRole::Baseline | ReviewSubjectRole::CandidateImplementation + ) + }) + .count(); + let requirement_count = plan + .subjects + .iter() + .filter(|subject| subject.role == ReviewSubjectRole::Requirement) + .count(); + let implementation_count = plan + .subjects + .iter() + .filter(|subject| subject.role == ReviewSubjectRole::CandidateImplementation) + .count(); + + let invalid_details = match plan.mode { + ReviewMode::SingleSubject if non_supporting_count != 1 => { + Some("single_subject requires exactly one non-supporting subject") + } + ReviewMode::Comparative if comparison_count < 2 => { + Some("comparative requires at least two baseline or candidate subjects") + } + ReviewMode::RequirementTrace if requirement_count == 0 || implementation_count == 0 => { + Some("requirement_trace requires requirement and candidate implementation subjects") + } + ReviewMode::MultiSubject if non_supporting_count < 2 => { + Some("multi_subject requires at least two non-supporting subjects") + } + _ => None, + }; + + if let Some(details) = invalid_details { + return Err(ReviewPlanValidationError::InvalidModeSubjects { + mode: plan.mode, + details: details.to_owned(), + }); + } + + Ok(()) +} + +pub fn fallback_review_plan( + candidates: &[ReviewSubjectCandidate], + focus: &str, +) -> Result { + validate_candidate_catalog(candidates)?; + + let has_issue = candidates + .iter() + .any(|candidate| matches!(candidate, ReviewSubjectCandidate::Issue { .. })); + let has_change_set = candidates.iter().any(ReviewSubjectCandidate::is_change_set); + let change_set_count = candidates + .iter() + .filter(|candidate| candidate.is_change_set()) + .count(); + let normalized_focus = focus + .chars() + .take(MAX_REVIEW_FOCUS_CHARS) + .collect::() + .to_lowercase(); + let comparison_requested = has_comparison_language(&normalized_focus); + let requirement_trace_requested = has_requirement_trace_language(&normalized_focus); + + if candidates.len() == 1 { + return Ok(ReviewPlan { + mode: ReviewMode::SingleSubject, + subjects: fallback_subjects(candidates, |_| ReviewSubjectRole::Primary), + }); + } + + if change_set_count >= 2 && comparison_requested { + let mut comparison_index = 0; + return Ok(ReviewPlan { + mode: ReviewMode::Comparative, + subjects: fallback_subjects(candidates, |candidate| { + if !candidate.is_change_set() { + return ReviewSubjectRole::SupportingReference; + } + let role = if comparison_index == 0 { + ReviewSubjectRole::Baseline + } else { + ReviewSubjectRole::CandidateImplementation + }; + comparison_index += 1; + role + }), + }); + } + + if candidates.len() >= 2 && comparison_requested { + return Ok(ReviewPlan { + mode: ReviewMode::Comparative, + subjects: candidates + .iter() + .enumerate() + .map(|(index, candidate)| ReviewPlanSubject { + candidate_id: candidate.id().to_owned(), + role: if index == 0 { + ReviewSubjectRole::Baseline + } else { + ReviewSubjectRole::CandidateImplementation + }, + }) + .collect(), + }); + } + + if has_issue && has_change_set && requirement_trace_requested { + return Ok(ReviewPlan { + mode: ReviewMode::RequirementTrace, + subjects: fallback_subjects(candidates, |candidate| match candidate { + ReviewSubjectCandidate::Issue { .. } => ReviewSubjectRole::Requirement, + candidate if candidate.is_change_set() => { + ReviewSubjectRole::CandidateImplementation + } + _ => ReviewSubjectRole::SupportingReference, + }), + }); + } + + Ok(ReviewPlan { + mode: ReviewMode::MultiSubject, + subjects: fallback_subjects(candidates, |_| ReviewSubjectRole::Primary), + }) +} + +fn validate_candidate_catalog( + candidates: &[ReviewSubjectCandidate], +) -> Result<(), ReviewPlanValidationError> { + if candidates.is_empty() { + return Err(ReviewPlanValidationError::EmptyCandidateCatalog); + } + if candidates.len() > MAX_REVIEW_SUBJECTS { + return Err(ReviewPlanValidationError::TooManyCandidates { + actual: candidates.len(), + maximum: MAX_REVIEW_SUBJECTS, + }); + } + + let mut candidate_ids = HashSet::with_capacity(candidates.len()); + for candidate in candidates { + if !valid_candidate_id(candidate.id()) { + return Err(ReviewPlanValidationError::InvalidCandidateId); + } + if !candidate_ids.insert(candidate.id()) { + return Err(ReviewPlanValidationError::DuplicateCandidateCatalogId { + candidate_id: candidate.id().to_owned(), + }); + } + } + + Ok(()) +} + +fn valid_candidate_id(candidate_id: &str) -> bool { + !candidate_id.trim().is_empty() + && candidate_id.len() <= MAX_REVIEW_CANDIDATE_ID_BYTES + && !candidate_id.chars().any(char::is_control) +} + +fn fallback_subjects( + candidates: &[ReviewSubjectCandidate], + mut role_for: impl FnMut(&ReviewSubjectCandidate) -> ReviewSubjectRole, +) -> Vec { + candidates + .iter() + .map(|candidate| ReviewPlanSubject { + candidate_id: candidate.id().to_owned(), + role: role_for(candidate), + }) + .collect() +} + +fn has_comparison_language(focus: &str) -> bool { + [ + "比较", "对比", "优劣", "区别", "差异", "比較", "對比", "優劣", "區別", "差異", + ] + .iter() + .any(|term| focus.contains(term)) + || focus.contains("trade-off") + || focus + .split(|character: char| !character.is_ascii_alphanumeric()) + .any(|word| { + matches!( + word, + "compare" + | "compares" + | "compared" + | "comparing" + | "comparison" + | "comparative" + | "versus" + | "vs" + | "tradeoff" + | "tradeoffs" + ) + }) +} + +fn has_requirement_trace_language(focus: &str) -> bool { + [ + "对照 issue", + "對照 issue", + "对照需求", + "對照需求", + "是否满足该需求", + "是否滿足該需求", + "实现该 issue", + "實現該 issue", + "实现该需求", + "實現該需求", + "需求覆盖", + "需求覆蓋", + "验收标准", + "驗收標準", + "需求追踪", + "需求追蹤", + "需求追溯", + ] + .iter() + .any(|term| focus.contains(term)) + || [ + "against the issue", + "against the requirement", + "against the requirements", + "implements the requirement", + "implements the requirements", + "satisfies the requirement", + "satisfies the requirements", + "requirement coverage", + "trace requirements", + "acceptance criteria", + "traceability", + "implements the issue", + "satisfies the issue", + ] + .iter() + .any(|term| focus.contains(term)) +} + pub fn decide_review_quality(request: ReviewQualityDecisionRequest) -> ReviewQualityDecision { let score = review_risk_score(&request.target); diff --git a/src/crates/contracts/product-domains/tests/review_policy.rs b/src/crates/contracts/product-domains/tests/review_policy.rs index 5e1aaad3fe..4603eaf346 100644 --- a/src/crates/contracts/product-domains/tests/review_policy.rs +++ b/src/crates/contracts/product-domains/tests/review_policy.rs @@ -1,9 +1,27 @@ use bitfun_product_domains::review::{ - decide_review_quality, ReviewExecutionMode, ReviewIntent, ReviewLevel, + decide_review_quality, fallback_review_plan, validate_review_plan, ReviewExecutionMode, + ReviewIntent, ReviewLevel, ReviewMode, ReviewPlan, ReviewPlanValidationError, ReviewQualityDecisionReason, ReviewQualityDecisionRequest, ReviewStrategyLevel, - ReviewTargetFacts, ReviewTargetResolution, + ReviewSubjectCandidate, ReviewSubjectRole, ReviewTargetFacts, ReviewTargetResolution, }; +fn issue_candidate(id: &str) -> ReviewSubjectCandidate { + ReviewSubjectCandidate::Issue { + id: id.into(), + web_url: format!("https://example.com/issues/{id}"), + host: "example.com".into(), + project_path: "owner/project".into(), + issue_id: id.into(), + } +} + +fn workspace_candidate(id: &str) -> ReviewSubjectCandidate { + ReviewSubjectCandidate::Workspace { + id: id.into(), + workspace_path: "C:/workspace/project".into(), + } +} + fn request(intent: ReviewIntent) -> ReviewQualityDecisionRequest { ReviewQualityDecisionRequest { intent, @@ -113,3 +131,160 @@ fn serialized_contract_uses_surface_friendly_names() { assert_eq!(value["strategyLevel"], "quick"); assert_eq!(value["requiresConsent"], false); } + +#[test] +fn review_plan_only_composes_subjects_without_prescribing_review_content() { + let plan: ReviewPlan = serde_json::from_value(serde_json::json!({ + "mode": "single_subject", + "subjects": [{"candidate_id": "candidate-1", "role": "primary"}] + })) + .expect("composition should not require model-owned questions or output"); + + let serialized = serde_json::to_value(plan).expect("plan should serialize"); + assert!(serialized.get("questions").is_none()); + assert!(serialized.get("output").is_none()); +} + +#[test] +fn fallback_composes_common_review_relationships_without_a_model() { + let single = vec![workspace_candidate("candidate-1")]; + assert_eq!( + fallback_review_plan(&single, "review local changes") + .unwrap() + .mode, + ReviewMode::SingleSubject + ); + + let trace = vec![ + issue_candidate("candidate-1"), + workspace_candidate("candidate-2"), + ]; + let trace_plan = fallback_review_plan(&trace, "review against the issue").unwrap(); + assert_eq!(trace_plan.mode, ReviewMode::RequirementTrace); + assert_eq!(validate_review_plan(&trace, &trace_plan), Ok(())); + + let ambiguous = fallback_review_plan(&trace, "review these subjects").unwrap(); + assert_eq!(ambiguous.mode, ReviewMode::MultiSubject); + + let ambiguous_chinese = fallback_review_plan(&trace, "审核 Issue 和 PR，要求重点看性能") + .expect("ambiguous focus should remain model-owned"); + assert_eq!(ambiguous_chinese.mode, ReviewMode::MultiSubject); + + let ambiguous_english = fallback_review_plan( + &trace, + "Review the Issue and PR; requirement: focus on performance", + ) + .expect("a bare requirement label should remain model-owned"); + assert_eq!(ambiguous_english.mode, ReviewMode::MultiSubject); +} + +#[test] +fn explicit_comparison_intent_wins_over_candidate_kind_assumptions() { + let candidates = vec![ + issue_candidate("candidate-1"), + workspace_candidate("candidate-2"), + ]; + + let plan = fallback_review_plan(&candidates, "compare the issue with local changes").unwrap(); + + assert_eq!(plan.mode, ReviewMode::Comparative); +} + +#[test] +fn fallback_compares_two_change_sets_and_keeps_issue_as_context() { + let candidates = vec![ + issue_candidate("candidate-1"), + ReviewSubjectCandidate::PullRequest { + id: "candidate-2".into(), + web_url: "https://example.com/pulls/2".into(), + host: "example.com".into(), + project_path: "owner/project".into(), + pull_request_id: "2".into(), + }, + workspace_candidate("candidate-3"), + ]; + + let plan = fallback_review_plan(&candidates, "compare the PR with local changes").unwrap(); + + assert_eq!(plan.mode, ReviewMode::Comparative); + assert_eq!( + plan.subjects[0].role, + ReviewSubjectRole::SupportingReference + ); + assert_eq!(plan.subjects[1].role, ReviewSubjectRole::Baseline); + assert_eq!( + plan.subjects[2].role, + ReviewSubjectRole::CandidateImplementation + ); + assert_eq!(validate_review_plan(&candidates, &plan), Ok(())); +} + +#[test] +fn composition_rejects_empty_duplicate_or_excessive_candidate_catalogs() { + assert_eq!( + fallback_review_plan(&[], "review"), + Err(ReviewPlanValidationError::EmptyCandidateCatalog) + ); + + let duplicates = vec![ + workspace_candidate("candidate-1"), + issue_candidate("candidate-1"), + ]; + assert!(matches!( + fallback_review_plan(&duplicates, "review"), + Err(ReviewPlanValidationError::DuplicateCandidateCatalogId { .. }) + )); + + let blank_id = vec![workspace_candidate(" ")]; + assert!(matches!( + fallback_review_plan(&blank_id, "review"), + Err(ReviewPlanValidationError::InvalidCandidateId) + )); + + let excessive = (0..17) + .map(|index| workspace_candidate(&format!("candidate-{index}"))) + .collect::>(); + assert!(matches!( + fallback_review_plan(&excessive, "review"), + Err(ReviewPlanValidationError::TooManyCandidates { .. }) + )); +} + +#[test] +fn composition_bounds_plan_subjects_ids_and_focus_before_scanning() { + let candidates = vec![ + workspace_candidate("candidate-1"), + workspace_candidate("candidate-2"), + ]; + let oversized_plan: ReviewPlan = serde_json::from_value(serde_json::json!({ + "mode": "multi_subject", + "subjects": (0..17).map(|index| serde_json::json!({ + "candidate_id": format!("candidate-{index}"), + "role": "primary" + })).collect::>() + })) + .unwrap(); + assert!(matches!( + validate_review_plan(&candidates, &oversized_plan), + Err(ReviewPlanValidationError::TooManyPlanSubjects { .. }) + )); + + let oversized_id_plan: ReviewPlan = serde_json::from_value(serde_json::json!({ + "mode": "multi_subject", + "subjects": [ + {"candidate_id": "x".repeat(257), "role": "primary"}, + {"candidate_id": "candidate-2", "role": "primary"} + ] + })) + .unwrap(); + assert_eq!( + validate_review_plan(&candidates, &oversized_id_plan), + Err(ReviewPlanValidationError::InvalidCandidateId) + ); + + let focus = format!("{} compare", "x".repeat(8_000)); + assert_eq!( + fallback_review_plan(&candidates, &focus).unwrap().mode, + ReviewMode::MultiSubject + ); +}