diff --git a/crates/op-orchestrator/src/compact_skills.rs b/crates/op-orchestrator/src/compact_skills.rs index e171d4d6e..db83abf1f 100644 --- a/crates/op-orchestrator/src/compact_skills.rs +++ b/crates/op-orchestrator/src/compact_skills.rs @@ -168,6 +168,15 @@ fn compact_subagent_skills( // resolve layer; a no-op when the flag is off, required when // on (same pattern as `elements` below). "element-manifest", + // Component-instance teaching — gated on `hasReusableComponents` + // at the resolve layer (only resolves in when a component library + // is loaded). A no-op when the flag is off (the skill was never + // matched), required when on: without it a Basic-tier model gets + // the AVAILABLE COMPONENTS manifest but no teaching on HOW to emit + // `{type:"ref",ref:"",descendants:{...}}`, so it ignores the + // components and builds from scratch (0 ref instances). Same + // flag-gated allow-set pattern as `elements` / `element-manifest`. + "component-composition", "layout", "overflow", "text-rules", @@ -214,6 +223,14 @@ fn compact_subagent_skills( // under the tight retry budget. (Codex review 2026-06-06.) "design-system", "cjk-typography", + // Kept on the retry too: `build_subagent_prompt` still injects + // the AVAILABLE COMPONENTS manifest on a reduced-complexity + // retry (the manifest block is gated on the library being + // non-empty, NOT on `reduced_complexity`), so dropping the + // teaching here would leave the model the component list with + // no `ref` syntax — the exact mismatch this fix removes. + // Flag-gated at the resolve layer, so a no-op without a library. + "component-composition", ]; next.retain(|s| RETRY_ALLOWED.contains(&s.skill_name())); } @@ -434,18 +451,18 @@ mod tests { #[test] fn basic_tier_allow_set_drops_non_allowed_skills() { - // `component-composition` / `examples` are not in the Basic allow-set. + // `examples` is not in the Basic allow-set; `component-composition` IS + // (flag-gated — only resolves in when a component library exists, then + // must survive so the model gets the `ref` teaching). let input = skills(&[ "schema", "layout", "cjk-typography", - "component-composition", "examples", "jsonl-format-simplified", ]); let out = filter(input, ModelTier::Basic, false, false); let got = names(&out); - assert!(!got.contains(&"component-composition")); assert!(!got.contains(&"examples")); assert!(got.contains(&"schema")); assert!(got.contains(&"layout")); @@ -453,6 +470,29 @@ mod tests { assert!(got.contains(&"jsonl-format-simplified")); } + #[test] + fn basic_tier_allow_set_keeps_component_composition() { + // Regression guard: when a component library is loaded the + // `component-composition` skill resolves in behind the + // `hasReusableComponents` flag. The Basic-tier allow-set used to drop + // it (DropReason::TierFiltered) even with budget room, so a weak model + // got the AVAILABLE COMPONENTS manifest but no `ref` teaching → 0 + // component instances. It must now survive — non-reduced AND reduced. + let input = skills(&["schema", "layout", "component-composition"]); + // Non-reduced Basic. + let out = filter(input.clone(), ModelTier::Basic, false, false); + assert!( + names(&out).contains(&"component-composition"), + "Basic tier must keep component-composition when a library is present" + ); + // Reduced-complexity Basic retry (manifest is still injected on retry). + let out_reduced = filter(input, ModelTier::Basic, false, true); + assert!( + names(&out_reduced).contains(&"component-composition"), + "Basic reduced retry must keep component-composition (manifest still injected)" + ); + } + #[test] fn standard_tier_keeps_non_allowed_skills() { // The allow-set is Basic-only; Standard/Full keep everything the base diff --git a/crates/op-orchestrator/src/manifest.rs b/crates/op-orchestrator/src/manifest.rs index d296cb6b4..1f88f1211 100644 --- a/crates/op-orchestrator/src/manifest.rs +++ b/crates/op-orchestrator/src/manifest.rs @@ -215,6 +215,11 @@ fn build_element_entry( }; } + // `el:"ref"` (component instance) and every semantic-builder kind route + // through `build_element`. The ref kind maps to a `PenNode::Ref` (the same + // instance node the raw `type:ref` path emits) so a manifest can compose + // from the document's component library; refs nest under sections via `in` + // exactly like any other element leaf. let args = element_args(&object); match op_mcp::element_manifest::build_element(&kind, &args) { Ok(built) => { @@ -668,6 +673,60 @@ Here is the design: ); } + #[test] + fn ref_element_line_builds_a_component_instance() { + // `{"el":"ref","ref":"X"}` → a top-level PenNode::Ref(target=X), so a + // manifest can instantiate a library component within the el-line + // protocol (not the contradicting raw `type:ref` syntax). + let text = r#"{"el":"ref","ref":"shadcn-btn-primary"}"#; + let outcome = parse_manifest(text).expect("manifest"); + assert_eq!(outcome.element_lines, 1); + assert_eq!(outcome.dropped_lines, 0); + assert_eq!(outcome.nodes.len(), 1); + match &outcome.nodes[0] { + PenNode::Ref(reference) => assert_eq!(reference.target, "shadcn-btn-primary"), + other => panic!("expected PenNode::Ref, got {other:?}"), + } + } + + #[test] + fn ref_element_nests_under_its_section_via_in() { + // A ref leaf nests under a section exactly like any other element. + let text = r#" +{"el":"section","direction":"horizontal","role":"actions"} +{"el":"ref","in":1,"ref":"shadcn-btn-primary"} +{"el":"ref","in":1,"ref":"shadcn-btn-secondary"} +"#; + let outcome = parse_manifest(text).expect("manifest"); + assert_eq!(outcome.element_lines, 3); + assert_eq!(outcome.nodes.len(), 1, "one section root"); + let kids = frame_children(&outcome.nodes[0]); + assert_eq!(kids.len(), 2, "both refs nested in the section"); + assert!( + kids.iter().all(|n| matches!(n, PenNode::Ref(_))), + "section children are component instances" + ); + } + + #[test] + fn ref_element_descendant_overrides_carry_through() { + // `descendants` (a JSON object) survives the manifest → builder → + // PenNode::Ref round trip so per-instance text overrides stick. + let text = r#"{"el":"ref","ref":"shadcn-btn-primary","descendants":{"shadcn-btn-primary-label":{"content":"Get started"}}}"#; + let outcome = parse_manifest(text).expect("manifest"); + assert_eq!(outcome.nodes.len(), 1); + match &outcome.nodes[0] { + PenNode::Ref(reference) => { + let overrides = reference + .descendants + .as_ref() + .expect("descendants carried through"); + assert!(overrides.contains_key("shadcn-btn-primary-label")); + } + other => panic!("expected PenNode::Ref, got {other:?}"), + } + } + #[test] fn forbidden_id_fields_are_stripped_with_warning() { let text = r#"{"el":"badge","label":"New","id":"my-id","parent_id":"root"}"#; diff --git a/crates/op-orchestrator/src/prompt.rs b/crates/op-orchestrator/src/prompt.rs index f289530f1..a5ea707c4 100644 --- a/crates/op-orchestrator/src/prompt.rs +++ b/crates/op-orchestrator/src/prompt.rs @@ -25,7 +25,7 @@ use op_ai_skills::style_guide::{ extract_style_guide_values, select_style_guide, style_guide_registry, SelectOptions, }; use op_ai_skills::{ - budget::trim_by_budget, + budget::trim_by_budget_pinned, get_skills_by_phase, resolver::{filter_by_intent, inject_dynamic_content}, DropReason, DroppedSkill, Phase, ResolveOptions, ResolvedSkill, SkillLoadEntry, @@ -496,7 +496,11 @@ fn resolve_generation_skills_after_prompt_filter( minimal_skills, reduced_complexity, ); - let trimmed = trim_by_budget(&filtered_entries, total_budget, intent); + // Honor caller-pinned skills (force-included, budget-exempt) — same + // mechanism `resolve_skills` uses on the non-mobile path. Empty by default, + // so a no-library mobile generation is unchanged. + let pinned: Vec<&str> = opts.pinned_skills.iter().map(String::as_str).collect(); + let trimmed = trim_by_budget_pinned(&filtered_entries, total_budget, intent, &pinned); for entry in &filtered_entries { if !trimmed.iter().any(|kept| kept.meta.name == entry.meta.name) { @@ -587,7 +591,17 @@ const COMPONENT_CATEGORY_ORDER: &[&str] = &[ /// [`MAX_COMPONENT_MANIFEST_ENTRIES`], plus a one-line instruction pointing the /// model at the `ref` + `descendants` syntax taught by the /// `component-composition` skill. -fn available_components_manifest(components: &ComponentLibrary) -> Option { +/// +/// `manifest_on` branches the one-line instantiation instruction by which +/// output protocol is active. The default raw/loop path emits a bare PenNode +/// (`{"type":"ref",…}`); the element-manifest path emits an `el` line +/// (`{"el":"ref",…}`). Telling a manifest-arm model to emit raw-node syntax +/// contradicts the dominant `el`-line contract, so it emits rich `el` kinds and +/// 0 component refs — the instruction must match the live protocol. +fn available_components_manifest( + components: &ComponentLibrary, + manifest_on: bool, +) -> Option { if components.is_empty() { return None; } @@ -601,9 +615,14 @@ fn available_components_manifest(components: &ComponentLibrary) -> Option Option\"}` (override its \ - text/fill via `descendants` — see the component-composition rules). Only build an \ - element by hand when no component above fits." - .to_string(), - ); + // Branch the instantiation example by the active output protocol so the + // model is never told to emit a syntax that contradicts its line contract. + // The example is COMPLETE (not just the envelope) so this block is + // self-sufficient: even if the component-composition skill were ever trimmed + // out, the model still has a usable, copy-pasteable instruction. + let instruction = if manifest_on { + "To use one, emit it as a manifest line — `el:\"ref\"`, the component id, nest it \ + under a section with `in:`, and override its text/fill via `descendants` \ + (NO `id` field — the system assigns ids). Example:\n \ + {\"el\":\"section\",\"role\":\"actions\"}\n \ + {\"el\":\"ref\",\"in\":1,\"ref\":\"\",\"descendants\":{\"\":{\"content\":\"Get started\"}}}\n\ + Only build an element by hand when no component above fits." + } else { + "To use one, emit a single node — `type:\"ref\"`, the component id, its `_parent`, and \ + override its text/fill via `descendants` (it needs no `children`). Example:\n \ + {\"_parent\":\"\",\"id\":\"\",\"type\":\"ref\",\"ref\":\"\",\"descendants\":{\"\":{\"content\":\"Get started\"}}}\n\ + Only build an element by hand when no component above fits." + }; + lines.push(instruction.to_string()); Some(lines.join("\n")) } @@ -701,7 +733,7 @@ fn build_subagent_prompt_with_manifest( // `hasReusableComponents` flag (loads the `component-composition` skill). // `None` when the registry is empty, so the default no-component path is // byte-for-byte unchanged. - let component_manifest = available_components_manifest(components); + let component_manifest = available_components_manifest(components, manifest_on); let has_reusable_components = component_manifest.is_some(); // design.md payload for the `{{designMdContent}}` template. If the @@ -813,10 +845,24 @@ fn build_subagent_prompt_with_manifest( ModelTier::Full => None, }; + // Force-include the component-instance teaching whenever a reusable-component + // library is loaded. When a library is present the model already receives the + // AVAILABLE COMPONENTS *list*; the `component-composition` skill carries the + // HOW-to-instantiate teaching (`ref` + `descendants` syntax) — without it the + // model gets the catalog but no usable instruction and emits 0 refs. On the + // tight non-mobile Basic budget (5200, of which base skills already use + // ~3900) the skill is otherwise dropped by BudgetExhausted, so we pin it + // (budget-exempt) here. Empty on every no-library path ⇒ no change there. + let pinned_skills = if has_reusable_components { + vec!["component-composition".to_string()] + } else { + Vec::new() + }; let opts = ResolveOptions { flags, dynamic_content, budget_override, + pinned_skills, ..Default::default() }; let intent = subtask_intent(req, subtask); diff --git a/crates/op-orchestrator/src/prompt_tests.rs b/crates/op-orchestrator/src/prompt_tests.rs index ae5731556..8b7da626e 100644 --- a/crates/op-orchestrator/src/prompt_tests.rs +++ b/crates/op-orchestrator/src/prompt_tests.rs @@ -1117,6 +1117,37 @@ fn components_prompt_injects_manifest_and_ref_teaching() { ); } +/// The AVAILABLE COMPONENTS instantiation instruction must match the active +/// output protocol. In raw/loop mode it teaches `{"type":"ref",…}`; in the +/// element-manifest arm it teaches `{"el":"ref",…}`. Telling a manifest-arm +/// model to emit raw-node syntax contradicts the el-line contract → 0 refs. +#[test] +fn components_manifest_instruction_matches_active_protocol() { + let lib = library_with(3); + + // Raw/loop protocol (manifest off): teach the bare PenNode `type:ref`. + let raw = available_components_manifest(&lib, false).expect("library present"); + assert!( + raw.contains("\"type\":\"ref\""), + "raw protocol must teach type:ref, got:\n{raw}" + ); + assert!( + !raw.contains("\"el\":\"ref\""), + "raw protocol must NOT teach el:ref, got:\n{raw}" + ); + + // Element-manifest protocol (manifest on): teach the `el:ref` line. + let man = available_components_manifest(&lib, true).expect("library present"); + assert!( + man.contains("\"el\":\"ref\""), + "manifest protocol must teach el:ref, got:\n{man}" + ); + assert!( + !man.contains("\"type\":\"ref\""), + "manifest protocol must NOT teach the contradicting type:ref, got:\n{man}" + ); +} + /// A large library is capped: the manifest lists at most /// `MAX_COMPONENT_MANIFEST_ENTRIES` and notes the remainder, so the prompt /// budget can't be blown by a 200-master kit. @@ -1146,3 +1177,293 @@ fn large_component_library_is_capped() { "listed {listed} entries exceeds cap {MAX_COMPONENT_MANIFEST_ENTRIES}" ); } + +/// Regression guard for the tier-drop bug (smoke `OPENPENCIL_SMOKE_LIBRARY` +/// scenario): a Basic-tier model with a component library loaded must get BOTH +/// the AVAILABLE COMPONENTS manifest (concrete ids) AND the +/// `component-composition` teaching (the `ref` + `descendants` syntax) in its +/// assembled subtask prompt. +/// +/// Before the fix the `component-composition` skill resolved in behind the +/// `hasReusableComponents` flag but was then dropped by the Basic-tier +/// `ALLOWED` allow-set (DropReason::TierFiltered) even with budget room +/// (`budget_used < budget_max`) — so a weak model saw the component list with +/// no instruction on how to emit a `ref` node and built everything from +/// scratch (0 component instances). Reproduces the real smoke path: a MiniMax +/// (Basic-tier) mobile screen, whose 9200-token budget has room to spare. +#[test] +fn basic_tier_components_prompt_keeps_both_manifest_and_teaching() { + // Sanity: the model classifies as Basic, the path that drops non-allowed + // skills via the allow-set (the bug surface). + assert_eq!( + resolve_model_profile("minimax-m3").tier, + ModelTier::Basic, + "test fixture must exercise the Basic tier" + ); + + // A Basic-tier mobile request — the actual smoke scenario. Mobile routes + // through the wider 9200-token budget so the drop is provably tier-caused, + // not budget-caused. + let basic_req = DesignRequest { + prompt: "Design a 402x874 mobile shop home screen with product cards, \ + search, and bottom navigation using the available components" + .into(), + model: Some("minimax-m3".into()), + ..req() + }; + let mut mobile_plan = plan(); + mobile_plan.root_frame.width = 402.0; + mobile_plan.root_frame.height = 874.0; + let mobile_subtask = Subtask { + id: "main-content".into(), + label: "Main Content".into(), + region: Region { + width: 402.0, + height: 640.0, + }, + id_prefix: "main-content".into(), + parent_frame_id: Some("page".into()), + elements: Some("product cards, search bar, category chips".into()), + screen: None, + generated_root_id: None, + existing_section_labels: None, + }; + + let lib = library_with(5); + let (cr, report) = build_subagent_prompt( + &mobile_subtask, + &mobile_plan, + &basic_req, + AbortFlag::new(), + false, + false, + &lib, + ); + let sys = &cr.system_prompt; + + // The drop the fix removes was budget-room-permitting: prove there was + // headroom so the original drop can only have been the tier allow-set. + assert!( + report.budget_used < report.budget_max, + "fixture must have budget headroom (the bug dropped despite room); report={report:?}" + ); + + // (1) The AVAILABLE COMPONENTS manifest reached the system prompt with + // concrete ids — it is a plain appended block, never tier-dropped. + assert!( + sys.contains("AVAILABLE COMPONENTS"), + "Basic-tier prompt must carry the components manifest" + ); + assert!( + sys.contains("comp-0") && sys.contains("comp-4"), + "manifest must list the concrete component ids" + ); + assert!( + sys.contains("\"type\":\"ref\""), + "manifest must point at the ref node syntax" + ); + + // (2) The component-composition TEACHING skill survived the Basic allow-set + // (this is the part the bug dropped). + assert!( + report + .included + .iter() + .any(|s| s.name == "component-composition"), + "Basic tier must KEEP component-composition when a library is present; report={report:?}" + ); + assert!( + !report + .dropped + .iter() + .any(|s| s.name == "component-composition"), + "component-composition must not be tier-dropped; dropped={:?}", + report.dropped + ); + + // (3) The teaching skill's actual body (the `ref` + `descendants` rules) + // is present in the system prompt, not just listed in the report. + assert!( + sys.contains("COMPONENT COMPOSITION"), + "the component-composition skill body must be in the system prompt" + ); + assert!( + sys.contains("descendants"), + "the prompt must teach overriding instance content via descendants" + ); +} + +/// Regression guard for the BUDGET-drop bug — the non-mobile dashboard path. +/// +/// The earlier `basic_tier_components_prompt_keeps_both_*` test covers the +/// MOBILE path (9200-token budget with headroom), which only ever exercised the +/// TIER allow-set drop. The real loss in production is on the NON-MOBILE, +/// Basic-tier dashboard path: `budget_max = 5200`, base skills alone consume +/// ~3900, so the flag-gated `component-composition` skill (~1200 tok) does NOT +/// fit and was dropped with `DropReason::BudgetExhausted` — the model got the +/// AVAILABLE COMPONENTS list but no `ref` + `descendants` teaching and emitted 0 +/// instances (`("component-composition","budget")` ×4 subtasks across runs). +/// +/// The force-include pin (prompt.rs: `pinned_skills` when `has_reusable_components`, +/// threaded into `trim_by_budget_pinned`) keeps it budget-exempt. This test +/// reproduces the EXACT scenario that dropped it (wide plan ⇒ 5200 budget, a +/// library present, budget already exhausted) and asserts the teaching survives. +#[test] +fn tight_budget_dashboard_force_includes_component_composition() { + // Basic tier is the path that overrides the budget down to 5200 when the + // plan is NOT a mobile full screen (the bug surface). + assert_eq!( + resolve_model_profile("minimax-m3").tier, + ModelTier::Basic, + "fixture must exercise the Basic tier (5200 budget on non-mobile)" + ); + + // A wide (non-mobile) dashboard plan → is_mobile_full_screen = false → + // budget_override = Some(5200), the exact tight path that budget-dropped it. + let basic_req = DesignRequest { + prompt: "Design a 1280x800 analytics dashboard with metric cards, \ + a chart panel, and a data table using the available components" + .into(), + model: Some("minimax-m3".into()), + ..req() + }; + let mut dash_plan = plan(); + dash_plan.root_frame.width = 1280.0; + dash_plan.root_frame.height = 800.0; + let dash_subtask = Subtask { + id: "main".into(), + label: "Main".into(), + region: Region { + width: 1280.0, + height: 600.0, + }, + id_prefix: "main".into(), + parent_frame_id: Some("page".into()), + elements: Some("metric cards, chart, table".into()), + screen: None, + generated_root_id: None, + existing_section_labels: None, + }; + + let lib = library_with(5); + let (cr, report) = build_subagent_prompt( + &dash_subtask, + &dash_plan, + &basic_req, + AbortFlag::new(), + false, + false, + &lib, + ); + let sys = &cr.system_prompt; + + // (0) Prove this is the TIGHT path: the 5200 budget is genuinely exhausted — + // budget_used >= budget_max — so the survival of component-composition can + // ONLY be the force-include pin, not leftover headroom. (Before the fix this + // same exhaustion is what dropped it with DropReason::BudgetExhausted.) + assert_eq!( + report.budget_max, 5200, + "non-mobile Basic must use the 5200 budget" + ); + assert!( + report.budget_used >= report.budget_max, + "fixture must EXHAUST the budget so the pin is the only thing keeping the \ + skill (the bug dropped it here); report={report:?}" + ); + + // (1) The component-composition TEACHING skill survived the tight budget. + assert!( + report + .included + .iter() + .any(|s| s.name == "component-composition"), + "tight 5200 budget must FORCE-INCLUDE component-composition when a library \ + is present; report={report:?}" + ); + // (2) It is NOT recorded as a budget drop (the exact regression). + assert!( + !report + .dropped + .iter() + .any(|s| s.name == "component-composition"), + "component-composition must not be budget-dropped on the 5200 path; \ + dropped={:?}", + report.dropped + ); + // (3) The skill BODY (the ref + descendants rules) is in the system prompt. + assert!( + sys.contains("COMPONENT COMPOSITION"), + "the component-composition skill body must reach the tight-budget prompt" + ); + assert!( + sys.contains("descendants"), + "the prompt must teach overriding instance content via descendants" + ); + // (4) The AVAILABLE COMPONENTS manifest with concrete ids is also present — + // both halves (LIST + HOW) reach the model on the tight path. + assert!( + sys.contains("AVAILABLE COMPONENTS"), + "tight-budget prompt must carry the components manifest" + ); + assert!( + sys.contains("comp-0") && sys.contains("comp-4"), + "manifest must list the concrete component ids" + ); +} + +/// The force-include is gated on a library being present: with NO components, a +/// tight-budget Basic dashboard prompt must NOT pin (or contain) the +/// component-composition skill — proving the pin is additive and never changes +/// normal no-library generation. +#[test] +fn tight_budget_dashboard_without_library_does_not_pin_component_composition() { + let basic_req = DesignRequest { + prompt: "Design a 1280x800 analytics dashboard with metric cards, \ + a chart panel, and a data table" + .into(), + model: Some("minimax-m3".into()), + ..req() + }; + let mut dash_plan = plan(); + dash_plan.root_frame.width = 1280.0; + dash_plan.root_frame.height = 800.0; + let dash_subtask = Subtask { + id: "main".into(), + label: "Main".into(), + region: Region { + width: 1280.0, + height: 600.0, + }, + id_prefix: "main".into(), + parent_frame_id: Some("page".into()), + elements: Some("metric cards, chart, table".into()), + screen: None, + generated_root_id: None, + existing_section_labels: None, + }; + + let (cr, report) = build_subagent_prompt( + &dash_subtask, + &dash_plan, + &basic_req, + AbortFlag::new(), + false, + false, + &ComponentLibrary::default(), + ); + assert!( + !report + .included + .iter() + .any(|s| s.name == "component-composition"), + "no library ⇒ component-composition must not be force-included; report={report:?}" + ); + assert!( + !cr.system_prompt.contains("AVAILABLE COMPONENTS"), + "no library ⇒ no components manifest" + ); + assert!( + !cr.system_prompt.contains("COMPONENT COMPOSITION"), + "no library ⇒ no component-composition teaching" + ); +}