From e5e41fbefeca6059f60f35b02ea1b9625a9bc726 Mon Sep 17 00:00:00 2001 From: Fini Date: Sat, 18 Jul 2026 16:34:24 +0800 Subject: [PATCH] fix(agent): auto-fix ring concentricity and stop degrading on quality rejections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A concentricity rejection used to discard a full-skills design outright, and the retry ladder then dropped skill rungs for a failure that had nothing to do with capacity — converting one strict check into a minimal-tier final output. Ring stacks now get a tier-2 force-center repair (centring ignores padding — jian absolute positioning never adds it — and only structurally hopeless stacks still reject), and the ladder distinguishes quality rejections from transport failures: a self-check rejection retries at the SAME skill tier with the rejection reason injected as feedback, while transport errors keep the existing degradation path. Remaining fatal checks are classified for follow-up. --- crates/op-orchestrator/src/append.rs | 1 + crates/op-orchestrator/src/concurrent.rs | 33 ++- .../src/dashboard_columns_tests.rs | 1 + crates/op-orchestrator/src/plan.rs | 15 ++ crates/op-orchestrator/src/plan_normalize.rs | 7 + .../src/plan_normalize_screen_groups_tests.rs | 1 + crates/op-orchestrator/src/plan_repair.rs | 2 + crates/op-orchestrator/src/prompt.rs | 16 ++ .../src/prompt_resolved_style_tests.rs | 1 + crates/op-orchestrator/src/prompt_tests.rs | 88 ++++++++ .../src/radial_preinsert_tests.rs | 173 +++++++++++++++- crates/op-orchestrator/src/radial_repair.rs | 45 ++-- .../src/radial_repair_force_center.rs | 193 ++++++++++++++++++ .../src/radial_repair_force_center_tests.rs | 154 ++++++++++++++ crates/op-orchestrator/src/retry.rs | 50 +++++ .../src/retry_subtask_tests.rs | 1 + crates/op-orchestrator/src/run_tests.rs | 125 ++++++++++++ crates/op-orchestrator/src/scaffold_tests.rs | 1 + .../src/screen_groups_tests.rs | 1 + .../op-orchestrator/src/spawn_concurrent.rs | 1 + crates/op-orchestrator/src/subagent.rs | 1 + .../src/subagent_reveal_tests.rs | 1 + 22 files changed, 882 insertions(+), 29 deletions(-) create mode 100644 crates/op-orchestrator/src/radial_repair_force_center.rs create mode 100644 crates/op-orchestrator/src/radial_repair_force_center_tests.rs diff --git a/crates/op-orchestrator/src/append.rs b/crates/op-orchestrator/src/append.rs index e443295e6..b1c08d1bd 100644 --- a/crates/op-orchestrator/src/append.rs +++ b/crates/op-orchestrator/src/append.rs @@ -103,6 +103,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }) .collect(), style_guide_name: None, diff --git a/crates/op-orchestrator/src/concurrent.rs b/crates/op-orchestrator/src/concurrent.rs index 145a668b8..8bedaef8a 100644 --- a/crates/op-orchestrator/src/concurrent.rs +++ b/crates/op-orchestrator/src/concurrent.rs @@ -55,7 +55,7 @@ use crate::model_profile::ModelTier; use crate::plan::{OrchestratorPlan, Subtask}; -use crate::retry::is_non_retryable; +use crate::retry::{is_non_retryable, is_self_check_rejection}; use crate::screen_groups::ScreenGroup; use crate::subagent::{apply_command_with_reveal, reveal_now_millis, run_subtask_with_reveal_at}; use crate::types::{AbortFlag, DesignRequest, DocSink, LlmClient, Progress, SubtaskOutcome}; @@ -221,7 +221,32 @@ pub(crate) async fn run_subtask_retry_ladder( o.error.is_some() && o.node_count == 0 && !abort.is_set() && !non_retryable }; - // Attempt 2 — reduced_complexity iff Basic tier. + // A self-check quality rejection (`orchestration_self_check` fatally + // rejected otherwise-real, otherwise-parsed content) is not evidence the + // model needs a narrower skill set — the content was fine except for the + // one flagged issue, so throwing skills away on attempt 2 only makes the + // REST of the design worse while doing nothing to fix that issue. Skill + // downgrade stays reserved for attempt 1 failures that actually suggest + // the model is struggling with the full prompt (stream errors, parse + // failures, blank output). Instead, attempt 2 retries at the SAME + // complexity/skill tier with the rejection reason echoed into the + // prompt (`prompt.rs`'s `retry_feedback` block) so the model can fix + // exactly that issue. + let attempt1_self_check_rejection = outcome1 + .error + .as_deref() + .is_some_and(is_self_check_rejection); + let attempt2_subtask = if attempt1_self_check_rejection { + Subtask { + retry_feedback: outcome1.error.clone(), + ..subtask.clone() + } + } else { + subtask.clone() + }; + + // Attempt 2 — reduced_complexity iff Basic tier AND attempt 1 wasn't a + // self-check quality rejection. let outcome2 = if retryable(&outcome1) { tracing::warn!( subtask = %subtask.id, @@ -238,13 +263,13 @@ pub(crate) async fn run_subtask_retry_ladder( }); Some( run_subtask_with_reveal_at( - subtask, + &attempt2_subtask, plan, request, llm, sink, abort, - tier == ModelTier::Basic, + tier == ModelTier::Basic && !attempt1_self_check_rejection, false, agent_indicator_epoch, reveal_now_millis(), diff --git a/crates/op-orchestrator/src/dashboard_columns_tests.rs b/crates/op-orchestrator/src/dashboard_columns_tests.rs index a9c232348..7c5641327 100644 --- a/crates/op-orchestrator/src/dashboard_columns_tests.rs +++ b/crates/op-orchestrator/src/dashboard_columns_tests.rs @@ -36,6 +36,7 @@ fn subtask(id: &str, label: &str, elements: Option<&str>) -> Subtask { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/plan.rs b/crates/op-orchestrator/src/plan.rs index 630821cdc..9abcc0d2d 100644 --- a/crates/op-orchestrator/src/plan.rs +++ b/crates/op-orchestrator/src/plan.rs @@ -81,6 +81,15 @@ pub struct Subtask { /// port of TS `SubTask.existingSectionLabels` (`ai-types.ts:134`)。 #[serde(default, skip_serializing_if = "Option::is_none")] pub existing_section_labels: Option>, + /// Set by the retry ladder (`concurrent::run_subtask_retry_ladder`) on + /// attempt 2 ONLY when attempt 1 failed a self-check quality rejection + /// (not a transport/parse failure) — the rejection message, echoed back + /// into the retry prompt so the model can fix exactly that issue while + /// keeping the same skill tier. Execution-only, like + /// `generated_root_id`: never persisted, never present on a freshly + /// planned subtask. + #[serde(skip)] + pub retry_feedback: Option, } /// 规划阶段的完整产物。字段对齐规划语料 `decomposition.md`。 @@ -200,6 +209,7 @@ pub fn build_fallback_plan(req: &DesignRequest) -> OrchestratorPlan { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }, Subtask { id: "main-content".into(), @@ -219,6 +229,7 @@ pub fn build_fallback_plan(req: &DesignRequest) -> OrchestratorPlan { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }, ], style_guide_name: None, @@ -248,6 +259,7 @@ pub fn build_fallback_plan(req: &DesignRequest) -> OrchestratorPlan { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } }) .collect(); @@ -411,6 +423,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; assert!(st.existing_section_labels.is_none()); } @@ -431,6 +444,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: Some(vec!["Hero".into(), "About".into()]), + retry_feedback: None, }; let labels = st.existing_section_labels.as_ref().unwrap(); assert_eq!(labels[0], "Hero"); @@ -453,6 +467,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let json = serde_json::to_string(&st).expect("serialize"); assert!( diff --git a/crates/op-orchestrator/src/plan_normalize.rs b/crates/op-orchestrator/src/plan_normalize.rs index ff654d8eb..aac61dbef 100644 --- a/crates/op-orchestrator/src/plan_normalize.rs +++ b/crates/op-orchestrator/src/plan_normalize.rs @@ -125,6 +125,7 @@ fn ensure_requested_bottom_nav_subtask(plan: &mut OrchestratorPlan, req: &Design screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }); } @@ -270,6 +271,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } @@ -556,6 +558,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; // chart subtask — LLM-provided height 300, within [inferred*0.6, inferred*1.6] // inferred for "chart" = 320 → min=192, max=512 → 300 in range → keep 300 @@ -572,6 +575,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; // metric subtask — LLM-provided height 0 (invalid) → use inferred = 160 let st_metric = Subtask { @@ -587,6 +591,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; OrchestratorPlan { root_frame: RootFrameSpec { @@ -687,6 +692,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }], style_guide_name: None, }; @@ -727,6 +733,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }], style_guide_name: None, }; diff --git a/crates/op-orchestrator/src/plan_normalize_screen_groups_tests.rs b/crates/op-orchestrator/src/plan_normalize_screen_groups_tests.rs index 5b513b698..947699468 100644 --- a/crates/op-orchestrator/src/plan_normalize_screen_groups_tests.rs +++ b/crates/op-orchestrator/src/plan_normalize_screen_groups_tests.rs @@ -36,6 +36,7 @@ fn subtask_with_screen(id: &str, label: &str, screen: Option<&str>) -> Subtask { screen: screen.map(str::to_string), generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/plan_repair.rs b/crates/op-orchestrator/src/plan_repair.rs index d823d6c46..63f04d680 100644 --- a/crates/op-orchestrator/src/plan_repair.rs +++ b/crates/op-orchestrator/src/plan_repair.rs @@ -360,6 +360,7 @@ fn coerce_subtask( screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }); } @@ -412,6 +413,7 @@ fn coerce_subtask( screen, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }) } diff --git a/crates/op-orchestrator/src/prompt.rs b/crates/op-orchestrator/src/prompt.rs index 6627c1c18..b6fb6bf4a 100644 --- a/crates/op-orchestrator/src/prompt.rs +++ b/crates/op-orchestrator/src/prompt.rs @@ -1096,6 +1096,22 @@ CRITICAL LAYOUT CONSTRAINTS:\n\ // (Mobile UI guardrails now load from the `mobile-ui` skill — see the // `isMobileScreen` flag + dynamic-content setup above.) + // Self-check quality-rejection feedback (retry ladder, attempt 2 only — + // see `retry::is_self_check_rejection` + `concurrent::run_subtask_retry_ladder`). + // Echoed back instead of silently narrowing the skill set: the content + // was otherwise real, so the model just needs to fix the one flagged + // geometry/structure issue, at the SAME skill tier attempt 1 used. + if let Some(reason) = subtask.retry_feedback.as_ref() { + user_prompt.push_str(&format!( + "\n\nSELF-CHECK FIX REQUIRED: your previous attempt at this exact \ + section was rejected before insertion for this reason: {reason}\n\ +- Regenerate the section addressing that reason specifically — do not change \ + anything else about the approach.\n\ +- Keep using the full skill set and design detail from your previous attempt; \ + the rejection was a geometry/structure issue, not a signal to simplify." + )); + } + // Port of orchestrator-sub-agent.ts:739-748 — APPEND MODE prompt injection. if let Some(labels) = subtask.existing_section_labels.as_ref() { if !labels.is_empty() { diff --git a/crates/op-orchestrator/src/prompt_resolved_style_tests.rs b/crates/op-orchestrator/src/prompt_resolved_style_tests.rs index 19dc7c599..86716db77 100644 --- a/crates/op-orchestrator/src/prompt_resolved_style_tests.rs +++ b/crates/op-orchestrator/src/prompt_resolved_style_tests.rs @@ -46,6 +46,7 @@ fn subtask() -> Subtask { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/prompt_tests.rs b/crates/op-orchestrator/src/prompt_tests.rs index af2645485..326d31e82 100644 --- a/crates/op-orchestrator/src/prompt_tests.rs +++ b/crates/op-orchestrator/src/prompt_tests.rs @@ -132,6 +132,7 @@ fn subagent_prompt_carries_subtask_and_script_format() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); assert!(cr.user_prompt.contains("Hero")); @@ -160,6 +161,7 @@ fn subagent_prompt_reduced_complexity_carries_script_format() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), true, false); assert!(cr.user_prompt.contains("Hero")); @@ -190,6 +192,7 @@ fn subagent_prompt_carries_ts_layout_contract() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }, Subtask { id: "categories".into(), @@ -204,6 +207,7 @@ fn subagent_prompt_carries_ts_layout_contract() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }, ]; let (cr, _) = bsp( @@ -249,6 +253,7 @@ fn subagent_prompt_minimal_skills_has_schema_and_script_format() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; // minimal_skills=true: the system prompt should contain schema skill // content plus the script-gen protocol suffix, but NOT layout/text-rules @@ -287,6 +292,7 @@ fn subagent_prompt_reduced_complexity_basic_is_shorter_than_full() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; // req() uses model "claude" which is Full tier — no narrowing. // Use a basic-tier model to test narrowing. @@ -327,6 +333,7 @@ fn subagent_prompt_reduced_complexity_full_tier_skill_filtering_is_noop() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; // req() uses "claude" which maps to Full tier → reduced_complexity's skill // narrowing is a no-op there (unlike Basic, which drops to the @@ -375,6 +382,7 @@ fn subagent_prompt_reduced_complexity_keeps_script_gen_even_on_full_tier() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (full_cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); let (reduced_cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), true, false); @@ -493,6 +501,7 @@ fn subagent_prompt_basic_mobile_food_keeps_mobile_app_skill() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (_, report) = bsp( @@ -546,6 +555,7 @@ fn subagent_prompt_honors_explicit_radius_and_spacing_numbers() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, _) = bsp( @@ -606,6 +616,7 @@ fn mobile_food_prompt_avoids_fixed_food_template() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, _) = bsp( @@ -683,6 +694,7 @@ fn chinese_mobile_food_prompt_carries_language_consistency_rule() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, report) = bsp( @@ -865,6 +877,7 @@ fn subtask() -> crate::plan::Subtask { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } @@ -987,6 +1000,7 @@ fn subagent_prompt_append_mode_injected_when_labels_present() { screen: None, generated_root_id: None, existing_section_labels: Some(vec!["Hero".into(), "Pricing".into()]), + retry_feedback: None, }; let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); assert!( @@ -1020,6 +1034,7 @@ fn subagent_prompt_no_append_mode_when_labels_none() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); assert!( @@ -1045,6 +1060,7 @@ fn subagent_prompt_no_append_mode_when_labels_empty() { screen: None, generated_root_id: None, existing_section_labels: Some(vec![]), + retry_feedback: None, }; let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); assert!( @@ -1053,6 +1069,74 @@ fn subagent_prompt_no_append_mode_when_labels_empty() { ); } +// ── retry_feedback: self-check rejection echoed into the retry prompt ───── + +/// When the retry ladder sets `retry_feedback` (attempt 2 after a self-check +/// quality rejection — see `concurrent::run_subtask_retry_ladder`), the +/// user prompt must carry the rejection reason and tell the model to keep +/// its full skill set, not simplify. +#[test] +fn subagent_prompt_injects_self_check_feedback_when_present() { + let st = crate::plan::Subtask { + id: "hero".into(), + label: "Hero".into(), + region: crate::plan::Region { + width: 1200.0, + height: 400.0, + }, + id_prefix: "hero".into(), + parent_frame_id: None, + elements: None, + screen: None, + generated_root_id: None, + existing_section_labels: None, + retry_feedback: Some( + "self-check failed: radial-stack-not-concentric at n14: progress-ring track, \ + progress arc, and measurable centre content must share one point" + .into(), + ), + }; + let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); + assert!( + cr.user_prompt.contains("SELF-CHECK FIX REQUIRED"), + "user_prompt must contain the self-check feedback block" + ); + assert!( + cr.user_prompt.contains("radial-stack-not-concentric"), + "user_prompt must echo the actual rejection reason verbatim" + ); + assert!( + cr.user_prompt.contains("Keep using the full skill set"), + "user_prompt must tell the model NOT to simplify in response to the rejection" + ); +} + +/// When `retry_feedback` is `None` (attempt 1, or any non-quality-rejection +/// retry), the user prompt must NOT contain the self-check feedback block. +#[test] +fn subagent_prompt_omits_self_check_feedback_when_absent() { + let st = crate::plan::Subtask { + id: "hero".into(), + label: "Hero".into(), + region: crate::plan::Region { + width: 1200.0, + height: 400.0, + }, + id_prefix: "hero".into(), + parent_frame_id: None, + elements: None, + screen: None, + generated_root_id: None, + existing_section_labels: None, + retry_feedback: None, + }; + let (cr, _) = bsp(&st, &plan(), &req(), AbortFlag::new(), false, false); + assert!( + !cr.user_prompt.contains("SELF-CHECK FIX REQUIRED"), + "user_prompt must not mention self-check feedback when there is none" + ); +} + // ── B0: subtask_intent ──────────────────────────────────────────────────── /// subtask_intent must include the original request prompt, the subtask label, @@ -1082,6 +1166,7 @@ fn subtask_intent_includes_prompt_label_and_hints() { screen: Some("home".into()), generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let intent = subtask_intent(&req, &sub); assert!( @@ -1384,6 +1469,7 @@ fn basic_tier_components_prompt_keeps_both_manifest_and_teaching() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let lib = library_with(5); @@ -1499,6 +1585,7 @@ fn tight_budget_dashboard_keeps_component_composition() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let lib = library_with(5); @@ -1599,6 +1686,7 @@ fn tight_budget_dashboard_without_library_does_not_pin_component_composition() { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }; let (cr, report) = build_subagent_prompt( diff --git a/crates/op-orchestrator/src/radial_preinsert_tests.rs b/crates/op-orchestrator/src/radial_preinsert_tests.rs index 96f5b336f..ebf9eda10 100644 --- a/crates/op-orchestrator/src/radial_preinsert_tests.rs +++ b/crates/op-orchestrator/src/radial_preinsert_tests.rs @@ -217,6 +217,65 @@ fn auto_fix_overlays_safe_nested_authored_ring_before_insert() { ); } +#[test] +fn non_square_wrapper_and_padded_wrapper_are_now_centred_instead_of_retried() { + // Concentricity is a pure geometric contract: a wrapper's aspect ratio + // and its padding value used to make the strict tier-1 patch decline + // outright (it also resizes/reorders, so it wants a confident square + // ring before touching anything) — but neither actually prevents + // computing a correct centred position, so the lenient tier-2 pass + // (radial_repair_force_center) must now salvage both instead of + // leaving the subtask to retry. + // (wrapper mutation, expected track x/y, expected progress x/y) — track + // is the 120x120 arc, progress the 116x116 arc (`fixed_ring`), so a + // correct centred fix does NOT put them at the same point; it puts + // each at the centre of its own size against the wrapper's full box. + type Point = (f64, f64); + type Case = (fn(&mut Value), Point, Point); + let cases: [Case; 2] = [ + (|ring| ring["width"] = json!(240), (60.0, 0.0), (62.0, 2.0)), + (|ring| ring["padding"] = json!(8), (0.0, 0.0), (2.0, 2.0)), + ]; + + for (index, (mutate, expected_track, expected_progress)) in cases.into_iter().enumerate() { + let mut ring = fixed_ring(Some("horizontal")); + mutate(&mut ring); + let mut nodes: Vec = serde_json::from_value(json!([ring])).expect("valid forest"); + let before = check_generated_nodes(&nodes, 390.0); + assert!( + has_radial_issue(&before), + "case {index} precondition: {before:?}" + ); + + assert!( + auto_fix_fixable_issues(&mut nodes, 390.0), + "case {index} must be salvaged, not retried" + ); + + let repaired = serde_json::to_value(&nodes).expect("serialize repaired forest"); + let ring = find_id(&repaired, "ring").expect("ring"); + assert_eq!(ring["layout"], json!("none"), "case {index}"); + let track = find_id(ring, "track").expect("track"); + let progress = find_id(ring, "progress").expect("progress"); + assert_eq!( + (track["x"].as_f64(), track["y"].as_f64()), + (Some(expected_track.0), Some(expected_track.1)), + "case {index}: track" + ); + assert_eq!( + (progress["x"].as_f64(), progress["y"].as_f64()), + (Some(expected_progress.0), Some(expected_progress.1)), + "case {index}: progress" + ); + + let after = check_generated_nodes(&nodes, 390.0); + assert!( + !after.has_fatal(), + "case {index} must pass self-check after the lenient fix: {after:?}" + ); + } +} + #[test] fn explicit_but_unrepairable_radial_shapes_are_rejected_without_guessing() { let cases = [ @@ -225,11 +284,6 @@ fn explicit_but_unrepairable_radial_shapes_are_rejected_without_guessing() { ring["width"] = json!("fill_container"); ring }, - { - let mut ring = fixed_ring(Some("horizontal")); - ring["width"] = json!(240); - ring - }, { let mut ring = fixed_ring(Some("horizontal")); ring["children"][1]["width"] = json!(60); @@ -270,11 +324,6 @@ fn high_confidence_but_unsafe_stacks_remain_fatal_for_retry() { }); ring }, - { - let mut ring = fixed_ring(Some("horizontal")); - ring["padding"] = json!(8); - ring - }, { let mut ring = fixed_ring(Some("horizontal")); ring["children"][2]["width"] = json!(160); @@ -439,3 +488,107 @@ fn pure_repair_walks_object_subtrees_without_editor_state() { ); assert_eq!(root["layout"], json!("vertical")); } + +/// One "Progress Ring" wrapper reconstructed verbatim from the real +/// `savings-goals` subtask script recovered from `0717-6-cc.log` (attempt +/// 2's full script text — attempt 1's own raw nodes weren't logged, only +/// the fatal issue codes for 3 rings at n14/n27/n40, and attempt 2 never +/// reached self-check because of an unrelated JS syntax error). Same +/// `ringSize`, `innerRadius`, `startAngle`, and `Ring Center` box the model +/// actually authored for each goal card. The one addition — `padding: 4` +/// on the ring wrapper — is the plausible source of attempt 1's rejection: +/// this exact structure without it already round-trips through the +/// existing strict tier-1 patch (see +/// `auto_fix_overlays_safe_nested_authored_ring_before_insert` above), so +/// something about attempt 1's copy must have tripped one of tier 1's +/// confidence gates for the fix to have failed twice in a row; padding is +/// the most representative example both because this same script pads +/// every card (`padding: 20`) and because it's exactly the gate this task +/// converts to tier 2. +fn savings_goal_ring(suffix: &str, pct: u32) -> Value { + json!({ + "type": "frame", + "id": format!("ring-{suffix}"), + "name": "Progress Ring", + "width": 56, + "height": 56, + "layout": "none", + "padding": 4, + "children": [ + {"type": "ellipse", "id": format!("track-{suffix}"), "name": "Ring Track", + "width": 56, "height": 56, "innerRadius": 0.82, + "fill": [{"type": "solid", "color": "#F1F5F9"}]}, + {"type": "ellipse", "id": format!("progress-{suffix}"), "name": "Ring Progress", + "width": 56, "height": 56, "innerRadius": 0.82, "startAngle": -90, + "sweepAngle": (360 * pct / 100), + "fill": [{"type": "solid", "color": "#0D9488"}]}, + {"type": "frame", "id": format!("centre-{suffix}"), "name": "Ring Center", + "width": 40, "height": 20, "layout": "horizontal", + "alignItems": "center", "justifyContent": "center", + "children": [ + {"type": "text", "id": format!("pct-{suffix}"), + "content": format!("{pct}%"), "width": "fit_content", "textGrowth": "auto"} + ]} + ] + }) +} + +#[test] +fn savings_goal_rings_from_the_0717_6_cc_incident_are_salvaged_by_auto_fix() { + let rail = json!({ + "type": "frame", + "id": "goals-rail", + "name": "Savings Goals Rail", + "width": "fill_container", + "height": "fit_content", + "layout": "horizontal", + "gap": 12, + "children": [ + savings_goal_ring("emergency-fund", 72), + savings_goal_ring("new-car", 45), + savings_goal_ring("vacation", 88), + ] + }); + let mut nodes: Vec = serde_json::from_value(json!([rail])).expect("valid rail"); + + let before = check_generated_nodes(&nodes, 390.0); + assert!( + has_radial_issue(&before), + "precondition: must reproduce the fatal rejection: {before:?}" + ); + + assert!( + auto_fix_fixable_issues(&mut nodes, 390.0), + "the real savings-goals rings must be salvaged on attempt 1, not \ + fall through to a minimal-skills salvage rung" + ); + + let repaired = serde_json::to_value(&nodes).expect("serialize repaired rail"); + for suffix in ["emergency-fund", "new-car", "vacation"] { + let ring = find_id(&repaired, &format!("ring-{suffix}")).expect("ring"); + let track = find_id(ring, &format!("track-{suffix}")).expect("track"); + let progress = find_id(ring, &format!("progress-{suffix}")).expect("progress"); + assert_eq!( + (track["x"].as_f64(), track["y"].as_f64()), + (Some(0.0), Some(0.0)), + "ring {suffix}: 56x56 track centred in the 56x56 wrapper sits at the origin" + ); + assert_eq!( + (track["x"].as_f64(), track["y"].as_f64()), + (progress["x"].as_f64(), progress["y"].as_f64()), + "ring {suffix}: track and progress are the same size, so they share one point" + ); + let centre = find_id(ring, &format!("centre-{suffix}")).expect("centre"); + assert_eq!( + (centre["x"].as_f64(), centre["y"].as_f64()), + (Some(8.0), Some(18.0)), + "ring {suffix}: the 40x20 Ring Center the model authored centres at (8, 18)" + ); + } + + let after = check_generated_nodes(&nodes, 390.0); + assert!( + !after.has_fatal(), + "salvaged rail must pass self-check on the same attempt: {after:?}" + ); +} diff --git a/crates/op-orchestrator/src/radial_repair.rs b/crates/op-orchestrator/src/radial_repair.rs index db54518c7..6b4505196 100644 --- a/crates/op-orchestrator/src/radial_repair.rs +++ b/crates/op-orchestrator/src/radial_repair.rs @@ -6,6 +6,9 @@ use serde_json::Value; use crate::types::DocSink; +#[path = "radial_repair_force_center.rs"] +mod radial_repair_force_center; + #[derive(Clone, Copy)] struct Rect { w: f64, @@ -57,28 +60,38 @@ pub(crate) fn is_radial_stack_in_flow(v: &Value) -> bool { /// True when an authored track/progress pair cannot be shown concentrically /// on the first reveal. Besides flex-flow stacks this catches `layout:none` /// wrappers whose child coordinates, fixed geometry, or painter order are -/// incomplete. Those cases must be repaired before insertion or retried; the -/// late cleanup should not be the first moment the ring looks correct. +/// incomplete. Concentricity is a pure geometric contract, so this defers to +/// the strict high-confidence patch when it applies, and otherwise to the +/// lenient tier-2 centring check (`radial_repair_force_center`) — a ring is +/// only genuinely unsafe once *both* tiers agree it can't be resolved from +/// the authored/estimated geometry alone. pub(crate) fn is_authored_radial_stack_unsafe(v: &Value) -> bool { if radial_layers(v).is_none() { return false; } - let Some(patch) = authored_radial_patch(v) else { - return true; - }; - authored_patch_changes(v, &patch) + match authored_radial_patch(v) { + Some(patch) => authored_patch_changes(v, &patch), + None => radial_repair_force_center::is_still_off_center(v), + } } -/// Normalize high-confidence radial stacks directly in an authored JSON -/// forest. This is intentionally independent of `EditorState`: it runs before +/// Normalize radial stacks directly in an authored JSON forest. This is +/// intentionally independent of `EditorState`: it runs before /// `InsertSubtree`, while the existing sink-based repair remains the late /// resolved-layout fallback. /// -/// Only fixed, near-square wrappers with similarly sized authored arcs and -/// fully measurable direct children are changed. A high-confidence candidate -/// with unmeasurable centre content stays untouched so self-check retries it; -/// malformed but explicit track/progress pairs are left untouched so -/// self-check can request a retry rather than guessing wrapper geometry. +/// Two tiers, in order: +/// 1. The strict, high-confidence patch below — near-square wrappers with +/// similarly sized authored arcs and fully measurable direct children — +/// which also fills in missing arc/centre dimensions and fixes painter +/// order. +/// 2. When tier 1 declines (non-square wrapper, out-of-range arc ratio, +/// ambiguous painter order, asymmetric padding, …), the lenient tier-2 +/// pass re-centres whatever tier 1 left alone: concentricity is a +/// geometry fact once `radial_layers` already recognised the ring, so +/// there is no confidence gate left to apply. Only a child whose size +/// can be neither read nor estimated at all stays untouched, so +/// self-check keeps reporting it instead of guessing. pub(crate) fn repair_authored_radial_stacks(value: &mut Value) -> bool { match value { Value::Array(nodes) => { @@ -94,8 +107,10 @@ pub(crate) fn repair_authored_radial_stacks(value: &mut Value) -> bool { } fn repair_authored_radial_node(node: &mut Value) -> bool { - let patch = authored_radial_patch(node); - let mut changed = patch.is_some_and(|patch| apply_authored_radial_patch(node, patch)); + let mut changed = match authored_radial_patch(node) { + Some(patch) => apply_authored_radial_patch(node, patch), + None => radial_repair_force_center::force_concentric_radial_stack(node), + }; if let Some(children) = node.get_mut("children").and_then(Value::as_array_mut) { for child in children { changed |= repair_authored_radial_node(child); diff --git a/crates/op-orchestrator/src/radial_repair_force_center.rs b/crates/op-orchestrator/src/radial_repair_force_center.rs new file mode 100644 index 000000000..edfa51b42 --- /dev/null +++ b/crates/op-orchestrator/src/radial_repair_force_center.rs @@ -0,0 +1,193 @@ +//! Lenient (tier-2) concentric repair for authored radial stacks. +//! +//! `authored_radial_patch` (in the parent module) is the strict, +//! high-confidence pass: besides repositioning, it also *resizes* things +//! (fills in missing arc/centre dimensions) and gates on wrapper aspect +//! ratio, arc-to-parent-size ratio, and zero padding to be confident that's +//! safe. Those gates are about whether the ring is worth reshaping +//! automatically — not about whether it's safe to *centre*. +//! +//! Concentricity and front-to-back paint order carry no such wrapper-shape +//! judgment call. Once `radial_layers` has already recognised a genuine +//! track/progress (or segmented-donut) ring — and `radial_layer_order` has +//! derived an unambiguous centre/progress/track order for it — sharing one +//! centre point and painting centre-then-progress-then-track are geometric +//! and structural facts, not guesses, regardless of the wrapper's aspect +//! ratio. Padding doesn't even enter into it: jian positions an explicit +//! `x`/`y` child of a `layout:"none"` frame from the frame's *border* box +//! (taffy's absolute-layout algorithm only adds `border`, never `padding`, +//! to an inset that's already set) — so the wrapper's `padding` value is +//! irrelevant to where a centred child actually renders, and centring +//! always targets the full `width`×`height` box regardless of it. +//! +//! So this module re-centres and reorders whatever `authored_radial_patch` +//! declined to touch for wrapper-shape or padding reasons, without resizing +//! anything it didn't already have a size for. What still keeps a stack +//! unfixable here (echoed back instead of guessed): more than one +//! centre-content child (ambiguous order), a child whose size can be +//! neither read nor estimated at all, a child that doesn't fit inside the +//! wrapper's box no matter how it's centred (an authoring-size bug, not a +//! position bug), or arc diameters too mismatched to plausibly be the same +//! ring (`radial_layers` classifies track vs. progress by sweep angle / +//! naming alone, not by size, so nothing upstream has already vetted that +//! the pairing is size-plausible). + +use serde_json::Value; + +use super::{ + estimated_subtree_size, has_numeric, is_arc_ellipse, numeric, radial_layer_order, + radial_layers, valid_size, MIN_SAFE_ARC_DIAMETER_RATIO, +}; + +/// `(x, y, width, height)`. +type Placement = (f64, f64, f64, f64); + +/// Re-centre every direct child of `node` on the wrapper's full box and fix +/// its paint order to centre/progress/track. Returns `false` (no-op) when +/// `node` isn't a recognised radial stack with an unambiguous layer order, +/// or when the wrapper's own size or any child's size can't be read or +/// estimated — those cases are left for the caller to report rather than +/// guess at. +pub(super) fn force_concentric_radial_stack(node: &mut Value) -> bool { + let Some(order) = radial_layer_order(node) else { + return false; + }; + let Some(placements) = concentric_placements(node) else { + return false; + }; + + let mut changed = set_if_different(node, "layout", Value::String("none".into())); + changed |= set_if_different(node, "gap", Value::from(0.0)); + changed |= set_if_different(node, "justifyContent", Value::String("start".into())); + changed |= set_if_different(node, "alignItems", Value::String("start".into())); + + let kids = node + .get_mut("children") + .and_then(Value::as_array_mut) + .expect("radial_layer_order already confirmed children exist"); + changed |= order.iter().copied().ne(0..kids.len()); + + // Pair each original child with its computed placement before + // reordering, so the permutation below carries the right placement + // along with the right child. + let mut slots: Vec> = std::mem::take(kids) + .into_iter() + .zip(placements) + .map(Some) + .collect(); + for index in order { + let Some((mut child, (x, y, width, height))) = slots.get_mut(index).and_then(Option::take) + else { + continue; + }; + let had_width = has_numeric(&child, "width"); + let had_height = has_numeric(&child, "height"); + changed |= set_if_different(&mut child, "x", Value::from(x)); + changed |= set_if_different(&mut child, "y", Value::from(y)); + if !had_width { + changed |= set_if_different(&mut child, "width", Value::from(width.round())); + } + if !had_height { + changed |= set_if_different(&mut child, "height", Value::from(height.round())); + } + kids.push(child); + } + changed +} + +/// True when `node` is a recognised radial stack that this lenient pass +/// still can't call safe: the layer order is ambiguous, the children +/// aren't already in centre/progress/track order, it isn't overlaid +/// (`layout:none`), a child's authored x/y doesn't match the centred +/// placement, or the placements can't even be computed (unmeasurable +/// wrapper/child size). `node` not being a radial stack at all is *not* +/// unsafe — there's nothing to check — so this checks `radial_layers` +/// itself first rather than reusing `radial_layer_order`'s `None`, which +/// also covers the very different "ambiguous order" case. +pub(super) fn is_still_off_center(node: &Value) -> bool { + if radial_layers(node).is_none() { + return false; + } + let Some(order) = radial_layer_order(node) else { + return true; + }; + let Some(placements) = concentric_placements(node) else { + return true; + }; + if node.get("layout").and_then(Value::as_str) != Some("none") { + return true; + } + if order.iter().copied().ne(0..placements.len()) { + return true; + } + let kids = node + .get("children") + .and_then(Value::as_array) + .map(Vec::as_slice) + .unwrap_or(&[]); + placements + .iter() + .zip(kids) + .any(|((x, y, _width, _height), child)| { + numeric(child, "x") != Some(*x) || numeric(child, "y") != Some(*y) + }) +} + +/// The centred `(x, y, width, height)` every direct child of `node` should +/// sit at against the wrapper's full `width`×`height` box (padding is +/// deliberately not subtracted — see the module doc: jian positions an +/// explicit-inset absolute child from the border box, ignoring the +/// parent's padding, so a padding-aware centre would be centring against a +/// box the renderer never actually uses). Indexed in the child array's +/// *original* order — callers that also reorder must carry each placement +/// along with its child through the permutation, not re-look-up by new +/// position. `width`/`height` are the authored size when present, else the +/// best estimate used to compute the centred `x`/`y` — callers only need +/// to persist them when the child had none authored. `None` means there is +/// nothing safe to compute: the wrapper's own size or some child's size +/// can't be read or estimated, a child doesn't fit inside the wrapper's +/// box at all (an authoring-size bug translation can't paper over), or the +/// arcs' diameters are too mismatched to plausibly be the same ring. +fn concentric_placements(node: &Value) -> Option> { + let box_w = numeric(node, "width")?; + let box_h = numeric(node, "height")?; + if !valid_size(box_w, box_h) { + return None; + } + + let kids = node.get("children").and_then(Value::as_array)?; + let mut placements = Vec::with_capacity(kids.len()); + let mut arc_diameters = Vec::new(); + for child in kids { + let estimate = estimated_subtree_size(child); + let width = numeric(child, "width").or_else(|| estimate.map(|size| size.0))?; + let height = numeric(child, "height").or_else(|| estimate.map(|size| size.1))?; + if !valid_size(width, height) || width > box_w + 0.5 || height > box_h + 0.5 { + return None; + } + if is_arc_ellipse(child) { + arc_diameters.push(width.max(height)); + } + let x = ((box_w - width) / 2.0).round(); + let y = ((box_h - height) / 2.0).round(); + placements.push((x, y, width, height)); + } + let min_arc = arc_diameters.iter().copied().fold(f64::INFINITY, f64::min); + let max_arc = arc_diameters.iter().copied().fold(0.0, f64::max); + if max_arc > 0.0 && min_arc / max_arc < MIN_SAFE_ARC_DIAMETER_RATIO { + return None; + } + Some(placements) +} + +fn set_if_different(node: &mut Value, key: &str, value: Value) -> bool { + if node.get(key) == Some(&value) { + return false; + } + node[key] = value; + true +} + +#[cfg(test)] +#[path = "radial_repair_force_center_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/radial_repair_force_center_tests.rs b/crates/op-orchestrator/src/radial_repair_force_center_tests.rs new file mode 100644 index 000000000..96215abfe --- /dev/null +++ b/crates/op-orchestrator/src/radial_repair_force_center_tests.rs @@ -0,0 +1,154 @@ +use super::*; +use serde_json::json; + +/// A ring shaped like the wrapper this module targets: an unambiguous +/// track/progress pair plus one centre-content child, all direct children +/// of a `frame`. Children start in authoring order (track, progress, +/// centre) — canonical paint order is centre, progress, track — so tests +/// also exercise the reordering half of the fix. +fn ring(width: f64, height: f64, extra: Option) -> Value { + let mut node = json!({ + "type": "frame", + "id": "ring", + "width": width, + "height": height, + "layout": "vertical", + "children": [ + {"type": "ellipse", "id": "track", "width": 56, "height": 56, "innerRadius": 0.82}, + {"type": "ellipse", "id": "progress", "width": 56, "height": 56, + "innerRadius": 0.82, "startAngle": -90, "sweepAngle": 230}, + {"type": "frame", "id": "centre", "width": 32, "height": 18, + "children": [{"type": "text", "id": "pct", "content": "64%"}]} + ] + }); + if let Some(extra) = extra { + for (key, value) in extra.as_object().expect("extra must be an object") { + node[key] = value.clone(); + } + } + node +} + +fn child<'a>(v: &'a Value, id: &str) -> &'a Value { + v["children"] + .as_array() + .expect("children") + .iter() + .find(|c| c["id"] == id) + .unwrap_or_else(|| panic!("missing child {id}")) +} + +#[test] +fn centres_a_non_square_wrapper_tier_one_declines_on_aspect_alone() { + let mut node = ring(240.0, 56.0, None); + assert!(is_still_off_center(&node), "precondition: not yet centred"); + + assert!(force_concentric_radial_stack(&mut node)); + + assert_eq!(node["layout"], json!("none")); + let (track, progress) = (child(&node, "track"), child(&node, "progress")); + assert_eq!( + (track["x"].as_f64(), track["y"].as_f64()), + (Some(92.0), Some(0.0)), + "56-wide arc centred in a 240-wide box sits at (240-56)/2 = 92" + ); + assert_eq!( + (track["x"].as_f64(), track["y"].as_f64()), + (progress["x"].as_f64(), progress["y"].as_f64()), + "track and progress are the same size, so they share one point" + ); + assert!(!is_still_off_center(&node), "must be safe after the fix"); +} + +#[test] +fn ignores_padding_because_jian_positions_absolute_children_from_the_border_box() { + let mut padded = ring(56.0, 56.0, Some(json!({"padding": 8}))); + let mut bare = ring(56.0, 56.0, None); + + assert!(force_concentric_radial_stack(&mut padded)); + assert!(force_concentric_radial_stack(&mut bare)); + + assert_eq!( + child(&padded, "track")["x"], + child(&bare, "track")["x"], + "padding must not shift the computed centre — jian ignores it for \ + layout:none absolute children (only `border` is added to an \ + explicit inset, never `padding`)" + ); + assert!(!is_still_off_center(&padded)); +} + +#[test] +fn reorders_children_to_centre_progress_track_paint_order() { + let mut node = ring(56.0, 56.0, None); + let before: Vec<&str> = node["children"] + .as_array() + .unwrap() + .iter() + .map(|c| c["id"].as_str().unwrap()) + .collect(); + assert_eq!(before, ["track", "progress", "centre"], "precondition"); + + assert!(force_concentric_radial_stack(&mut node)); + + let after: Vec<&str> = node["children"] + .as_array() + .unwrap() + .iter() + .map(|c| c["id"].as_str().unwrap()) + .collect(); + assert_eq!(after, ["centre", "progress", "track"]); +} + +#[test] +fn declines_when_a_child_does_not_fit_in_the_wrapper_at_all() { + let mut node = ring(56.0, 56.0, None); + node["children"][2]["width"] = json!(160); + + assert!(!force_concentric_radial_stack(&mut node)); + assert!( + is_still_off_center(&node), + "an oversized child is an authoring bug, not a position bug" + ); +} + +#[test] +fn declines_when_arc_diameters_are_too_mismatched_to_be_the_same_ring() { + let mut node = ring(120.0, 120.0, None); + node["children"][0]["width"] = json!(120); + node["children"][0]["height"] = json!(120); + node["children"][1]["width"] = json!(60); + node["children"][1]["height"] = json!(60); + + assert!(!force_concentric_radial_stack(&mut node)); + assert!(is_still_off_center(&node)); +} + +#[test] +fn declines_when_more_than_one_direct_child_is_non_arc_centre_content() { + let mut node = ring(56.0, 56.0, None); + node["children"].as_array_mut().unwrap().push( + json!({"type": "text", "id": "extra-label", "width": 20, "height": 10, "content": "x"}), + ); + + assert!(!force_concentric_radial_stack(&mut node)); + assert!(is_still_off_center(&node)); +} + +#[test] +fn is_a_no_op_on_a_node_that_is_not_a_radial_stack_at_all() { + let mut plain = json!({ + "type": "frame", + "id": "row", + "width": 200, + "height": 40, + "layout": "horizontal", + "children": [ + {"type": "ellipse", "id": "a", "width": 32, "height": 32}, + {"type": "ellipse", "id": "b", "width": 32, "height": 32} + ] + }); + + assert!(!force_concentric_radial_stack(&mut plain)); + assert!(!is_still_off_center(&plain)); +} diff --git a/crates/op-orchestrator/src/retry.rs b/crates/op-orchestrator/src/retry.rs index 38ff3c1ad..9c9e42587 100644 --- a/crates/op-orchestrator/src/retry.rs +++ b/crates/op-orchestrator/src/retry.rs @@ -29,6 +29,26 @@ pub(crate) fn is_non_retryable(msg: &str) -> bool { || lower.contains("invalid proxy configuration") } +/// True when a subtask failed because our own post-generation self-check +/// (`orchestration_self_check`) rejected otherwise-parsed, otherwise-real +/// content — a geometry/quality judgment on THIS model's output — rather +/// than a transport, parsing, or capability failure (stream error, script +/// syntax error, blank output, `InsertSubtree` rejection). +/// +/// The distinction matters for the retry ladder +/// (`concurrent::run_subtask_retry_ladder`): a quality rejection is not +/// evidence the model needs a narrower skill set to succeed — the content +/// was otherwise fine, so throwing away skills on attempt 2 only makes the +/// *rest* of the design worse while doing nothing to fix the one flagged +/// issue. Skill-tier downgrade should stay reserved for the failures that +/// actually suggest the model is struggling with the full prompt (timeouts, +/// stream errors, safety-scanner stalls) — see `subagent::run_subtask_with_reveal_at`'s +/// `fail(format!("self-check failed: {message}"))` call site for the exact +/// prefix this matches. +pub(crate) fn is_self_check_rejection(msg: &str) -> bool { + msg.starts_with("self-check failed: ") +} + #[cfg(test)] mod tests { use super::*; @@ -91,6 +111,36 @@ mod tests { fn socket_closed_is_retryable() { assert!(!is_non_retryable("socket closed unexpectedly")); } + + // ── is_self_check_rejection ───────────────────────────────────────────── + + #[test] + fn self_check_failure_message_is_a_self_check_rejection() { + assert!(is_self_check_rejection( + "self-check failed: radial-stack-not-concentric at n14: ..." + )); + } + + #[test] + fn stream_error_is_not_a_self_check_rejection() { + assert!(!is_self_check_rejection( + "stream disconnected before completion" + )); + } + + #[test] + fn script_parse_error_is_not_a_self_check_rejection() { + assert!(!is_self_check_rejection( + "script error: unexpected end of string" + )); + } + + #[test] + fn blank_container_is_not_a_self_check_rejection() { + assert!(!is_self_check_rejection( + "blank container root produced no content nodes" + )); + } } #[cfg(test)] diff --git a/crates/op-orchestrator/src/retry_subtask_tests.rs b/crates/op-orchestrator/src/retry_subtask_tests.rs index f43b5c91a..f51d5147d 100644 --- a/crates/op-orchestrator/src/retry_subtask_tests.rs +++ b/crates/op-orchestrator/src/retry_subtask_tests.rs @@ -55,6 +55,7 @@ fn failed_subtask(parent_frame_id: Option<&str>) -> Subtask { screen: Some("Home".into()), generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/run_tests.rs b/crates/op-orchestrator/src/run_tests.rs index 4251edd3b..e5cfaae73 100644 --- a/crates/op-orchestrator/src/run_tests.rs +++ b/crates/op-orchestrator/src/run_tests.rs @@ -48,6 +48,25 @@ fn req_standard() -> DesignRequest { } } +// Basic tier model id — the ONLY tier where attempt 2's `reduced_complexity` +// flag has any effect at all (`compact_skills::apply_skill_filter`'s doc: +// "Basic tier only"), so it's the tier that can actually distinguish this +// module's quality-vs-transport retry-ladder split. +fn req_basic() -> DesignRequest { + DesignRequest { + prompt: "a landing page".into(), + // "glm-4-plus" matches Basic tier in model_profile table + model: Some("glm-4-plus".into()), + provider: None, + design_md: None, + concurrency: 1, + append_context: None, + validation_enabled: true, + + visual_ref_enabled: false, + } +} + const PLAN_JSON: &str = r##"{ "rootFrame": { "id": "root", "name": "Page", "width": 1200, "height": 800, "layout": "vertical", "gap": 0, @@ -80,6 +99,25 @@ fn node_json(prefix: &str) -> String { ) } +// A radial ring whose progress arc (60px) is far smaller than its track +// (120px) — `orchestration_self_check`'s `radial-stack-not-concentric` +// flags this, and neither repair tier can auto-fix it (the arc-diameter +// mismatch is too implausible to guess a fix for; see +// `radial_preinsert_tests::explicit_but_unrepairable_radial_shapes_are_rejected_without_guessing`). +// This parses fine as script-gen — the rejection comes from self-check, not +// from a parse/stream failure — so it's the fixture for proving the retry +// ladder treats a QUALITY rejection differently from a transport failure. +fn radial_reject_script() -> String { + r##"I(null, {"type":"frame","name":"Ring Section","x":0,"y":0,"width":1200,"height":300,"children":[ + {"type":"frame","name":"Steps Ring","width":120,"height":120,"children":[ + {"type":"ellipse","name":"track","width":120,"height":120,"innerRadius":0.82,"sweepAngle":360,"fill":[{"type":"solid","color":"#22C55E"}]}, + {"type":"ellipse","name":"progress","width":60,"height":60,"innerRadius":0.82,"startAngle":-90,"sweepAngle":264,"fill":[{"type":"solid","color":"#22C55E"}]}, + {"type":"frame","name":"centre","width":80,"height":44,"children":[{"type":"text","content":"64%"}]} + ]} + ]});"## + .into() +} + fn existing_root_json( id: &str, name: &str, @@ -415,6 +453,93 @@ fn subtask_retries_on_attempt1_zero_succeeds_on_attempt2() { assert_eq!(sink.batch_depth, 0); } +/// A self-check quality rejection on attempt 1 must NOT downgrade attempt +/// 2's skill tier, even on a Basic-tier model that would otherwise always +/// narrow to `retryAllowed` on retry — the content was real, just flagged +/// for one geometry issue, so throwing skills away only makes the rest of +/// the design worse. Attempt 2's prompt must also carry the rejection +/// reason so the model can fix exactly that issue. +#[test] +fn self_check_rejection_keeps_full_skills_and_injects_feedback_on_attempt2() { + let llm = ScriptedLlm::new(vec![ + ScriptResponse::Text(PLAN_JSON.into()), + // hero attempt 1 (Basic tier, full complexity): parses fine, but + // self-check fatally rejects the mismatched ring — zero nodes land. + ScriptResponse::Text(radial_reject_script()), + // hero attempt 2: must stay full complexity despite Basic tier. + ScriptResponse::Text(node_json("hero")), + // feat attempt 1: succeeds normally. + ScriptResponse::Text(node_json("feat")), + ]); + let mut sink = VecDocSink::new(); + let mut on_progress = |_p: Progress| {}; + let summary = futures::executor::block_on(Orchestrator::new().run( + req_basic(), + &mut sink, + &llm, + &mut on_progress, + &AbortFlag::new(), + &stub_providers(), + )) + .expect("attempt 2 recovers after the self-check rejection"); + assert_eq!(summary.subtasks.len(), 2); + + // Call order: [0] planning, [1] hero attempt 1, [2] hero attempt 2, [3] feat. + let prompts = llm.system_prompts(); + assert_eq!(prompts.len(), 4, "unexpected call count: {prompts:?}"); + assert_eq!( + prompts[1], prompts[2], + "attempt 2 after a self-check rejection must resolve the IDENTICAL \ + (full) skill set attempt 1 used — a Basic-tier model would \ + otherwise narrow this to the retryAllowed set" + ); +} + +/// The mirror case: an attempt-1 TRANSPORT failure (not a self-check +/// rejection) on a Basic-tier model must still downgrade attempt 2 to +/// `reduced_complexity`, exactly as before this task — skill downgrade +/// stays reserved for failures that suggest the model is struggling with +/// the full prompt, not for a quality gate on otherwise-fine content. +#[test] +fn transport_failure_still_downgrades_skills_on_attempt2_for_basic_tier() { + use crate::types::LlmError; + let llm = ScriptedLlm::new(vec![ + ScriptResponse::Text(PLAN_JSON.into()), + // hero attempt 1: a stream error — NOT a self-check rejection. + ScriptResponse::Fail(LlmError { + message: "stream disconnected before completion".into(), + aborted: false, + }), + // hero attempt 2: reduced_complexity narrows the skill set. + ScriptResponse::Text(node_json("hero")), + // feat attempt 1: succeeds normally. + ScriptResponse::Text(node_json("feat")), + ]); + let mut sink = VecDocSink::new(); + let mut on_progress = |_p: Progress| {}; + let summary = futures::executor::block_on(Orchestrator::new().run( + req_basic(), + &mut sink, + &llm, + &mut on_progress, + &AbortFlag::new(), + &stub_providers(), + )) + .expect("attempt 2 recovers after the transport failure"); + assert_eq!(summary.subtasks.len(), 2); + + let prompts = llm.system_prompts(); + assert_eq!(prompts.len(), 4, "unexpected call count: {prompts:?}"); + assert!( + prompts[2].len() < prompts[1].len(), + "attempt 2 after a plain transport failure must still narrow to the \ + reduced-complexity skill set on Basic tier (attempt 1: {} chars, \ + attempt 2: {} chars)", + prompts[1].len(), + prompts[2].len() + ); +} + /// Subtask fails all 3 attempts → `OrchestratorError::AllFailed` with the /// final failure context. #[test] diff --git a/crates/op-orchestrator/src/scaffold_tests.rs b/crates/op-orchestrator/src/scaffold_tests.rs index c4a82d839..65303e9af 100644 --- a/crates/op-orchestrator/src/scaffold_tests.rs +++ b/crates/op-orchestrator/src/scaffold_tests.rs @@ -205,6 +205,7 @@ fn st(id: &str, label: &str) -> Subtask { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/screen_groups_tests.rs b/crates/op-orchestrator/src/screen_groups_tests.rs index cc42a4a07..763f53733 100644 --- a/crates/op-orchestrator/src/screen_groups_tests.rs +++ b/crates/op-orchestrator/src/screen_groups_tests.rs @@ -21,6 +21,7 @@ fn subtask_with_screen(id: &str, screen: Option<&str>) -> Subtask { screen: screen.map(|s| s.to_string()), generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/spawn_concurrent.rs b/crates/op-orchestrator/src/spawn_concurrent.rs index 0e439035e..af9fef607 100644 --- a/crates/op-orchestrator/src/spawn_concurrent.rs +++ b/crates/op-orchestrator/src/spawn_concurrent.rs @@ -136,6 +136,7 @@ fn plan_from_state(sink: &dyn DocSink, specs: &[SpawnAgentSpec]) -> Orchestrator screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, }) .collect(); diff --git a/crates/op-orchestrator/src/subagent.rs b/crates/op-orchestrator/src/subagent.rs index c5624253e..8dc245d1f 100644 --- a/crates/op-orchestrator/src/subagent.rs +++ b/crates/op-orchestrator/src/subagent.rs @@ -755,6 +755,7 @@ mod tests { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } } diff --git a/crates/op-orchestrator/src/subagent_reveal_tests.rs b/crates/op-orchestrator/src/subagent_reveal_tests.rs index e907e7cc8..d9181f837 100644 --- a/crates/op-orchestrator/src/subagent_reveal_tests.rs +++ b/crates/op-orchestrator/src/subagent_reveal_tests.rs @@ -52,6 +52,7 @@ fn f2_subtask() -> Subtask { screen: None, generated_root_id: None, existing_section_labels: None, + retry_feedback: None, } }