feat(orchestrator): force-include component-composition teaching when library present + el:ref/type:ref protocol-branched manifest

This commit is contained in:
Fini 2026-07-02 21:21:24 +08:00
parent 03d7c9b437
commit cc2846d618
4 changed files with 480 additions and 14 deletions

View file

@ -168,6 +168,15 @@ fn compact_subagent_skills<T: SkillNamed>(
// 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:"<id>",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<T: SkillNamed>(
// 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

View file

@ -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"}"#;

View file

@ -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<String> {
///
/// `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<String> {
if components.is_empty() {
return None;
}
@ -601,9 +615,14 @@ fn available_components_manifest(components: &ComponentLibrary) -> Option<String
}
let total = components.len();
let ref_syntax = if manifest_on {
"an `el:\"ref\"` line"
} else {
"a `ref` node"
};
let mut lines = vec![format!(
"AVAILABLE COMPONENTS ({total} reusable components in this document — \
PREFER instantiating these with a `ref` node over building from scratch):"
PREFER instantiating these with {ref_syntax} over building from scratch):"
)];
let mut listed = 0usize;
'outer: for cat in COMPONENT_CATEGORY_ORDER {
@ -626,12 +645,25 @@ fn available_components_manifest(components: &ComponentLibrary) -> Option<String
listed += 1;
}
}
lines.push(
"To use one, emit a node `{\"type\":\"ref\",\"ref\":\"<id from above>\"}` (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:<line>`, 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\":\"<id from above>\",\"descendants\":{\"<descendant-id>\":{\"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\":\"<container-id>\",\"id\":\"<your-id>\",\"type\":\"ref\",\"ref\":\"<id from above>\",\"descendants\":{\"<descendant-id>\":{\"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);

View file

@ -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"
);
}