diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index aff75f029..7430297e2 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -22,6 +22,7 @@ pub mod retry; pub mod style_guide_context; pub mod timeouts; pub mod types; +pub mod validation; pub mod validation_config; pub mod validation_dump; pub mod validation_fixes; diff --git a/crates/op-orchestrator/src/validation.rs b/crates/op-orchestrator/src/validation.rs new file mode 100644 index 000000000..9429335b2 --- /dev/null +++ b/crates/op-orchestrator/src/validation.rs @@ -0,0 +1,373 @@ +//! C1: Per-round vision call — `parse_validation_response` + `validate_design_screenshot`. +//! +//! Faithful port of `apps/web/src/services/ai/design-validation.ts:137-274` +//! (`validateDesignScreenshot` + `parseValidationResponse`). +//! +//! This module is responsible for one round of the vision-validation pipeline: +//! building the user message from the node-tree dump, calling the injected +//! `VisionLlmClient` trait, parsing the JSON response, and returning a +//! `ValidationResult` (with skipped=true on any failure / stub response). +//! +//! `run_post_generation_validation` (the full loop) is Task C2. + +// C2 will consume all pub(crate) items from this module. +#![allow(dead_code)] + +use std::time::Duration; + +use crate::types::{VisionCallRequest, VisionLlmClient, VisionResponse}; +use crate::validation_config::VALIDATION_TIMEOUT_MS; +use crate::validation_fixes::apply::{StructuralFix, ValidationFix}; +use crate::validation_fixes::{is_valid_fix_value, is_valid_structural_fix, SAFE_FIX_PROPERTIES}; + +// ── Response types ──────────────────────────────────────────────────────────── + +/// The 4-field payload extracted from a vision LLM's JSON response. +/// +/// Faithful port of the `ValidationResult` shape in +/// `design-validation-fixes.ts:50-62` / `design-validation.ts:230-274`. +#[derive(Debug, Clone, Default)] +pub(crate) struct ValidationResponse { + /// Natural-language descriptions of visual issues. + pub issues: Vec, + /// Property fixes to apply to existing nodes. + pub fixes: Vec, + /// Structural fixes (addChild / removeNode). + pub structural_fixes: Vec, + /// Quality score 1–10 (0 = parse failure / not set). + pub quality_score: u8, +} + +/// Result of `validate_design_screenshot` — either a parsed response or skipped. +#[derive(Debug, Clone, Default)] +pub(crate) struct ValidationResult { + /// When `true`, the vision call was skipped (stub/unavailable/error). + /// `response` is `None` in this case. + pub skipped: bool, + /// Parsed response when available. + pub response: Option, + /// The raw text returned by the LLM (for diagnostics). + pub raw_text: Option, + /// Human-readable reason when skipped (e.g. "stub", provider error). + pub error: Option, +} + +// ── parse_validation_response ───────────────────────────────────────────────── + +/// Parse the raw text from a vision LLM into a `ValidationResponse`. +/// +/// Steps (port of `parseValidationResponse` in `design-validation.ts:230-274`): +/// 1. Strip `…` blocks. +/// 2. Try direct parse on the cleaned text. +/// 3. Try extracting the first `{…}` blob with a greedy regex and re-parsing. +/// 4. On any failure return `ValidationResponse::default()` (zero everything). +/// +/// Within each parse attempt the function: +/// - Filters `fixes` through the safe-property whitelist + value validator. +/// - Filters `structuralFixes` through `is_valid_structural_fix`. +/// - Clamps `qualityScore` to [1, 10]; 0 → 0 (parse / not-set marker). +pub(crate) fn parse_validation_response(text: &str) -> ValidationResponse { + // Strip Agent SDK tool_use XML blocks (port of TS line 260). + let cleaned = strip_tool_use_blocks(text); + let cleaned = cleaned.trim(); + + // Try direct parse. + if let Some(resp) = try_parse_json(cleaned) { + return resp; + } + + // Try extracting the outermost {…} blob. + if let Some(blob) = extract_json_object(cleaned) { + if let Some(resp) = try_parse_json(blob) { + return resp; + } + } + + ValidationResponse::default() +} + +/// Attempt to parse `json` as a `ValidationResponse`. +/// +/// Returns `None` when the text isn't valid JSON **or** when the `fixes` array +/// is missing (TS: `if (!Array.isArray(parsed.fixes)) return null`). +fn try_parse_json(json: &str) -> Option { + let value: serde_json::Value = serde_json::from_str(json).ok()?; + let obj = value.as_object()?; + + // `fixes` must be an array (TS: `if (!Array.isArray(parsed.fixes)) return null`). + let fixes_raw = obj.get("fixes")?; + if !fixes_raw.is_array() { + return None; + } + + // Parse and filter `fixes`. + let fixes: Vec = fixes_raw + .as_array() + .unwrap() + .iter() + .filter_map(|f| { + let fo = f.as_object()?; + let node_id = fo.get("nodeId")?.as_str()?.to_string(); + if node_id.is_empty() { + return None; + } + let property = fo.get("property")?.as_str()?.to_string(); + // Must be in safe-fix whitelist. + if !SAFE_FIX_PROPERTIES.iter().any(|p| p.name == property) { + return None; + } + let value = fo.get("value")?.clone(); + // Value must be valid for this property. + if !is_valid_fix_value(&property, &value) { + return None; + } + Some(ValidationFix { + node_id, + property, + value, + }) + }) + .collect(); + + // Parse and filter `structuralFixes`. + let structural_fixes: Vec = obj + .get("structuralFixes") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter(|sf| is_valid_structural_fix(sf)) + .filter_map(structural_fix_from_value) + .collect() + }) + .unwrap_or_default(); + + // Parse `issues` — must be an array of strings. + let issues: Vec = obj + .get("issues") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|v| v.as_str().map(|s| s.to_string())) + .collect() + }) + .unwrap_or_default(); + + // Parse `qualityScore` — number or numeric string; clamp to [1, 10] when > 0. + let raw_score = obj.get("qualityScore"); + let num_score: f64 = match raw_score { + Some(serde_json::Value::Number(n)) => n.as_f64().unwrap_or(0.0), + Some(serde_json::Value::String(s)) => s.parse::().unwrap_or(0.0), + _ => 0.0, + }; + let quality_score: u8 = if num_score > 0.0 { + // Port of TS: `Math.max(1, Math.min(10, Math.round(numScore)))`. + let rounded = num_score.round() as i64; + rounded.clamp(1, 10) as u8 + } else { + 0 + }; + + Some(ValidationResponse { + issues, + fixes, + structural_fixes, + quality_score, + }) +} + +/// Convert a validated `serde_json::Value` into a `StructuralFix`. +/// +/// Assumes the value has already been validated by `is_valid_structural_fix`. +fn structural_fix_from_value(value: &serde_json::Value) -> Option { + let obj = value.as_object()?; + match obj.get("action")?.as_str()? { + "removeNode" => { + let node_id = obj.get("nodeId")?.as_str()?.to_string(); + Some(StructuralFix::RemoveNode { node_id }) + } + "addChild" => { + let parent_id = obj.get("parentId")?.as_str()?.to_string(); + let index = obj + .get("index") + .and_then(|v| v.as_u64()) + .map(|n| n as usize); + let spec = obj.get("node")?.clone(); + Some(StructuralFix::AddChild { + parent_id, + index, + spec, + }) + } + _ => None, + } +} + +/// Strip `…` blocks from `text`. +/// +/// Port of TS line 260: +/// ```ts +/// text.replace(/[\s\S]*?<\/tool_use>/g, '').trim() +/// ``` +fn strip_tool_use_blocks(text: &str) -> String { + // Simple repeated scan — avoids pulling in a regex crate. + let mut result = String::with_capacity(text.len()); + let mut remaining = text; + const OPEN: &str = ""; + const CLOSE: &str = ""; + + while let Some(start) = remaining.find(OPEN) { + result.push_str(&remaining[..start]); + // Advance past the opening tag. + remaining = &remaining[start + OPEN.len()..]; + // Skip until the closing tag. + if let Some(end) = remaining.find(CLOSE) { + remaining = &remaining[end + CLOSE.len()..]; + } else { + // No closing tag — stop (don't emit the rest of the open block). + remaining = ""; + break; + } + } + result.push_str(remaining); + result +} + +/// Extract the first outermost `{…}` blob from `text` using a greedy scan. +/// +/// Port of TS: `const match = cleaned.match(/\{[\s\S]*\}/);` +fn extract_json_object(text: &str) -> Option<&str> { + let start = text.find('{')?; + // Find the matching last `}` (greedy — match all `}` chars). + let end = text.rfind('}')?; + if end >= start { + Some(&text[start..=end]) + } else { + None + } +} + +// ── validate_design_screenshot ──────────────────────────────────────────────── + +/// Build the `VisionCallRequest` for a single validation round. +/// +/// The message template (port of TS L160-166) wraps the `node_tree_dump` in a +/// fenced block; reference-comparison + round-N instructions are appended +/// conditionally (port of TS L151-158). +/// +/// **Timeout rule (port of TS L147-149):** when a `reference_screenshot` is +/// provided, the per-round timeout is doubled (`VALIDATION_TIMEOUT_MS * 2`). +/// +/// Tests assert on the returned `VisionCallRequest` directly (no LLM call +/// needed to verify message/timeout/model/provider construction). +pub(crate) fn build_vision_request( + system_prompt: &str, + image_base64: &str, + node_tree_dump: &str, + model: Option<&str>, + provider: Option<&str>, + reference_screenshot: Option<&str>, + round: u8, +) -> VisionCallRequest { + // Reference-comparison instruction (port of TS L151-153). + let reference_instruction = if reference_screenshot.is_some() { + "\n\nA REFERENCE DESIGN screenshot was also provided. Compare the current design \ +against the reference and fix any significant deviations in layout, spacing, proportions, \ +or missing elements. The reference shows the intended design — the current screenshot \ +should match its structure, visual balance, and element completeness. If elements visible \ +in the reference are missing in the current design, use structuralFixes with addChild to \ +add them." + } else { + "" + }; + + // Round-specific instruction (port of TS L155-158). + let round_instruction = if round > 1 { + format!( + "\n\nThis is validation round {round}. Previous fixes have already been applied. \ +Focus on remaining issues only — do NOT re-report issues that have already been fixed." + ) + } else { + String::new() + }; + + // Full user message (port of TS L160-166). + let message = format!( + "Analyze this UI design screenshot. Here is the node tree structure:\n\n\ +```\n{node_tree_dump}\n```\n\n\ +Cross-reference visual issues with the node IDs above. \ +Return JSON fixes using real node IDs from the tree.\ +{reference_instruction}{round_instruction}" + ); + + // Timeout doubled when reference screenshot is present (TS L147-149). + let timeout_ms = if reference_screenshot.is_some() { + VALIDATION_TIMEOUT_MS * 2 + } else { + VALIDATION_TIMEOUT_MS + }; + + VisionCallRequest { + system: system_prompt.to_string(), + message, + image_base64: image_base64.to_string(), + model: model.map(|s| s.to_string()), + provider: provider.map(|s| s.to_string()), + timeout: Duration::from_millis(timeout_ms), + } +} + +/// Per-round vision validation call. +/// +/// Builds the request via `build_vision_request`, calls +/// `vision_client.validate`, parses the response via +/// `parse_validation_response`, and returns a `ValidationResult`. +/// +/// Faithful port of `validateDesignScreenshot` in +/// `design-validation.ts:137-228`. +/// +/// 8 parameters is faithful to the TS call-site — suppress the lint. +#[allow(clippy::too_many_arguments)] +pub(crate) fn validate_design_screenshot( + vision_client: &dyn VisionLlmClient, + system_prompt: &str, + image_base64: &str, + node_tree_dump: &str, + model: Option<&str>, + provider: Option<&str>, + reference_screenshot: Option<&str>, + round: u8, +) -> ValidationResult { + let req = build_vision_request( + system_prompt, + image_base64, + node_tree_dump, + model, + provider, + reference_screenshot, + round, + ); + + match vision_client.validate(req) { + VisionResponse::Text(text) => { + let response = parse_validation_response(&text); + ValidationResult { + skipped: false, + raw_text: Some(text), + response: Some(response), + error: None, + } + } + VisionResponse::Skipped { reason } => ValidationResult { + skipped: true, + raw_text: None, + response: None, + error: reason, + }, + } +} + +// ── Tests ───────────────────────────────────────────────────────────────────── + +#[cfg(test)] +#[path = "validation_tests_c1.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/validation_tests_c1.rs b/crates/op-orchestrator/src/validation_tests_c1.rs new file mode 100644 index 000000000..440652f0b --- /dev/null +++ b/crates/op-orchestrator/src/validation_tests_c1.rs @@ -0,0 +1,316 @@ +//! C1 tests — `parse_validation_response` + `validate_design_screenshot`. + +use crate::types::{VisionCallRequest, VisionLlmClient, VisionResponse}; +use crate::validation::{ + build_vision_request, parse_validation_response, validate_design_screenshot, ValidationResult, +}; +use crate::validation_config::VALIDATION_TIMEOUT_MS; +use std::sync::Mutex; +use std::time::Duration; + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +/// A `VisionLlmClient` that records the last request and returns a canned response. +struct CapturingVisionClient { + response: VisionResponse, + captured: Mutex>, +} + +impl CapturingVisionClient { + fn with_text(text: &str) -> Self { + Self { + response: VisionResponse::Text(text.to_string()), + captured: Mutex::new(None), + } + } + fn with_skipped(reason: Option) -> Self { + Self { + response: VisionResponse::Skipped { reason }, + captured: Mutex::new(None), + } + } + fn last_req(&self) -> Option { + self.captured.lock().unwrap().clone() + } +} + +impl VisionLlmClient for CapturingVisionClient { + fn validate(&self, req: VisionCallRequest) -> VisionResponse { + *self.captured.lock().unwrap() = Some(req); + self.response.clone() + } +} + +// ── parse_validation_response tests ────────────────────────────────────────── + +/// Well-formed JSON with all 4 fields extracts correctly. +#[test] +fn parse_extracts_four_fields_from_plain_json() { + // Use escaped string to avoid # inside raw-string delimiters. + let json = "{\"issues\": [\"Text too small\", \"Button misaligned\"],\ + \"fixes\": [\ + {\"nodeId\": \"node-1\", \"property\": \"fontSize\", \"value\": 14},\ + {\"nodeId\": \"node-2\", \"property\": \"fillColor\", \"value\": \"#FF0000\"}\ + ],\ + \"structuralFixes\": [],\ + \"qualityScore\": 7}"; + let resp = parse_validation_response(json); + assert_eq!(resp.issues.len(), 2); + assert_eq!(resp.fixes.len(), 2); + assert_eq!(resp.structural_fixes.len(), 0); + assert_eq!(resp.quality_score, 7); +} + +/// JSON wrapped in markdown code fences is cleaned and parsed correctly. +#[test] +fn parse_strips_markdown_fences() { + let text = + "```json\n{\"issues\":[],\"fixes\":[],\"structuralFixes\":[],\"qualityScore\":9}\n```"; + // The JSON extractor picks up the inner object. + let resp = parse_validation_response(text); + assert_eq!(resp.quality_score, 9); + assert!(resp.fixes.is_empty()); +} + +/// JSON embedded in prose is extracted via the `{…}` greedy scan. +#[test] +fn parse_extracts_json_from_prose() { + let text = r#"Here is my analysis: {"issues":["spacing off"],"fixes":[],"structuralFixes":[],"qualityScore":5} I hope that helps."#; + let resp = parse_validation_response(text); + assert_eq!(resp.quality_score, 5); + assert_eq!(resp.issues, vec!["spacing off"]); +} + +/// Malformed JSON returns an empty default response (no panic). +#[test] +fn parse_malformed_json_returns_default() { + let resp = parse_validation_response("this is not json at all"); + assert!(resp.issues.is_empty()); + assert!(resp.fixes.is_empty()); + assert!(resp.structural_fixes.is_empty()); + assert_eq!(resp.quality_score, 0); +} + +/// Empty string returns default (no panic). +#[test] +fn parse_empty_string_returns_default() { + let resp = parse_validation_response(""); + assert_eq!(resp.quality_score, 0); +} + +/// `qualityScore` is clamped to [1, 10] when > 0; strings are coerced. +#[test] +fn parse_quality_score_clamped_and_coerced() { + // Above 10 → 10 + let resp = parse_validation_response( + r#"{"issues":[],"fixes":[],"structuralFixes":[],"qualityScore":15}"#, + ); + assert_eq!(resp.quality_score, 10); + + // Below 1 but > 0 → 1 + let resp = parse_validation_response( + r#"{"issues":[],"fixes":[],"structuralFixes":[],"qualityScore":0.3}"#, + ); + assert_eq!(resp.quality_score, 1); + + // Exactly 0 → 0 (parse-failure marker) + let resp = parse_validation_response( + r#"{"issues":[],"fixes":[],"structuralFixes":[],"qualityScore":0}"#, + ); + assert_eq!(resp.quality_score, 0); + + // String "8" → 8 + let resp = parse_validation_response( + r#"{"issues":[],"fixes":[],"structuralFixes":[],"qualityScore":"8"}"#, + ); + assert_eq!(resp.quality_score, 8); +} + +/// Fixes with invalid properties or values are filtered out. +#[test] +fn parse_filters_invalid_fixes() { + let json = r#"{ + "issues": [], + "fixes": [ + {"nodeId": "n1", "property": "dangerousProp", "value": 42}, + {"nodeId": "n2", "property": "fontSize", "value": "not_a_number"}, + {"nodeId": "n3", "property": "fontSize", "value": 16} + ], + "structuralFixes": [], + "qualityScore": 6 + }"#; + let resp = parse_validation_response(json); + // Only the valid fontSize:16 fix should survive. + assert_eq!(resp.fixes.len(), 1); + assert_eq!(resp.fixes[0].node_id, "n3"); + assert_eq!(resp.fixes[0].property, "fontSize"); +} + +/// Valid structural fixes (addChild + removeNode) are parsed. +#[test] +fn parse_structural_fixes_valid() { + let json = r#"{ + "issues": [], + "fixes": [], + "structuralFixes": [ + {"action": "removeNode", "nodeId": "old-node"}, + {"action": "addChild", "parentId": "parent-1", "node": {"type": "text", "name": "Label"}} + ], + "qualityScore": 7 + }"#; + let resp = parse_validation_response(json); + assert_eq!(resp.structural_fixes.len(), 2); +} + +/// Invalid structural fixes are filtered out. +#[test] +fn parse_structural_fixes_invalid_are_filtered() { + let json = r#"{ + "issues": [], + "fixes": [], + "structuralFixes": [ + {"action": "removeNode"}, + {"action": "addChild", "node": {"type": "text"}}, + {"action": "removeNode", "nodeId": "valid-id"} + ], + "qualityScore": 5 + }"#; + let resp = parse_validation_response(json); + // Only the valid removeNode survives. + assert_eq!(resp.structural_fixes.len(), 1); +} + +/// `…` blocks are stripped before parsing. +#[test] +fn parse_strips_tool_use_blocks() { + let text = r#"some_tool{"junk":true}{"issues":[],"fixes":[],"structuralFixes":[],"qualityScore":4}"#; + let resp = parse_validation_response(text); + assert_eq!(resp.quality_score, 4); +} + +/// Missing `fixes` array → default (TS: `if (!Array.isArray(parsed.fixes)) return null`). +#[test] +fn parse_missing_fixes_array_returns_default() { + let json = r#"{"issues":[],"structuralFixes":[],"qualityScore":7}"#; + let resp = parse_validation_response(json); + // Falls through to default. + assert_eq!(resp.quality_score, 0); + assert!(resp.fixes.is_empty()); +} + +// ── build_vision_request tests (message + timeout construction) ────────────── + +/// Round 1, no reference: standard timeout, no round-N or REFERENCE blurb. +#[test] +fn build_vision_request_round1_no_reference() { + let req = build_vision_request( + "system prompt", + "base64img==", + "node-tree-dump", + None, + None, + None, + 1, + ); + assert_eq!(req.system, "system prompt"); + assert_eq!(req.image_base64, "base64img=="); + assert_eq!(req.timeout, Duration::from_millis(VALIDATION_TIMEOUT_MS)); + assert!(req.message.contains("node-tree-dump")); + assert!(!req.message.contains("validation round")); + assert!(!req.message.contains("REFERENCE DESIGN")); +} + +/// Reference screenshot present: timeout doubles + REFERENCE blurb injected. +#[test] +fn build_vision_request_timeout_doubled_with_reference() { + let req = build_vision_request( + "sys", + "img", + "tree", + None, + None, + Some("reference-img-b64"), + 1, + ); + assert_eq!( + req.timeout, + Duration::from_millis(VALIDATION_TIMEOUT_MS * 2) + ); + assert!(req.message.contains("REFERENCE DESIGN")); +} + +/// Round > 1: "This is validation round N" instruction injected. +#[test] +fn build_vision_request_round2_instruction_injected() { + let req = build_vision_request("sys", "img", "tree", None, None, None, 2); + assert!(req.message.contains("validation round 2")); +} + +/// `model` and `provider` are forwarded into `VisionCallRequest`. +#[test] +fn build_vision_request_forwards_model_and_provider() { + let req = build_vision_request("sys", "img", "tree", Some("gpt-5"), Some("openai"), None, 1); + assert_eq!(req.model.as_deref(), Some("gpt-5")); + assert_eq!(req.provider.as_deref(), Some("openai")); +} + +// ── validate_design_screenshot tests (full dispatch + parse) ───────────────── + +/// Round 1 happy-path: client receives the request, response is parsed. +#[test] +fn validate_screenshot_happy_path_parses_response() { + let json_resp = + r#"{"issues":["padding uneven"],"fixes":[],"structuralFixes":[],"qualityScore":6}"#; + let client = CapturingVisionClient::with_text(json_resp); + + let result = validate_design_screenshot( + &client, + "system prompt", + "base64img==", + "node-tree-dump", + None, + None, + None, + 1, + ); + + assert!(!result.skipped); + let resp = result.response.unwrap(); + assert_eq!(resp.quality_score, 6); + assert_eq!(resp.issues, vec!["padding uneven"]); + + // The same VisionCallRequest the helper would build flows through to the client. + let req = client.last_req().unwrap(); + assert_eq!(req.system, "system prompt"); + assert_eq!(req.image_base64, "base64img=="); +} + +/// When the client returns `Skipped`, `ValidationResult.skipped` is `true` +/// and the reason propagates into `error`. +#[test] +fn validate_screenshot_skipped_response_propagated() { + let client = CapturingVisionClient::with_skipped(Some("no vision provider".to_string())); + + let result = validate_design_screenshot(&client, "sys", "img", "tree", None, None, None, 1); + + assert!(result.skipped); + assert!(result.response.is_none()); + assert_eq!(result.error.as_deref(), Some("no vision provider")); +} + +/// `Skipped` with no reason → `error` is `None`. +#[test] +fn validate_screenshot_skipped_no_reason() { + let client = CapturingVisionClient::with_skipped(None); + let result = validate_design_screenshot(&client, "sys", "img", "tree", None, None, None, 1); + assert!(result.skipped); + assert!(result.error.is_none()); +} + +/// Default `ValidationResult` has `skipped: false` and no response. +#[test] +fn validation_result_default() { + let r: ValidationResult = ValidationResult::default(); + assert!(!r.skipped); + assert!(r.response.is_none()); +}