diff --git a/crates/op-orchestrator/src/compact_skills.rs b/crates/op-orchestrator/src/compact_skills.rs index 3e4feb46a..9b7e4b9d0 100644 --- a/crates/op-orchestrator/src/compact_skills.rs +++ b/crates/op-orchestrator/src/compact_skills.rs @@ -183,6 +183,13 @@ fn compact_subagent_skills( // none of the 16:9 contract — measured 2026-08-04. "slides", "deck-patterns", + // The cross-tier deck laws (overflow → split the page, density + // budgets, narrative arc, deck-specific slop bans). Same + // keyword gate and the same reason as the two above: without an + // entry here the allow-set drops it wholesale on Basic / + // Standard, which is the exact 2026-08-04 `slides` failure — + // and it is silent, because the file on disk stays correct. + "deck-contract", "icon-catalog", "style-defaults", "elements", diff --git a/crates/op-orchestrator/src/prompt_components_tests.rs b/crates/op-orchestrator/src/prompt_components_tests.rs index c4c53b203..d3737b394 100644 --- a/crates/op-orchestrator/src/prompt_components_tests.rs +++ b/crates/op-orchestrator/src/prompt_components_tests.rs @@ -408,11 +408,20 @@ fn tight_budget_dashboard_keeps_component_composition() { report.budget_max, 5200, "non-mobile Basic must use the 5200 budget" ); + // The pin only proves something under real budget pressure, and there is + // more of it than before: the always-kept Base skills alone now sum PAST + // the 5200 ceiling, so by the time the knapsack reaches Domain skills + // there is no room at all and an unpinned `component-composition` would be + // skipped outright. + // + // This used to assert "some skill was dropped for BudgetExhausted". That + // stopped being true when the sub-agent compaction moved BEFORE the + // knapsack (`resolve_generation_skills_after_prompt_filter`): the optional + // dashboard/depth skills that used to lose the budget race are now removed + // by the Basic allow-set first, and are reported as `TierFiltered`. The + // pin is unchanged — its competitors simply never reach the race. assert!( - report - .dropped - .iter() - .any(|s| matches!(s.reason, op_ai_skills::DropReason::BudgetExhausted)), + report.budget_used > report.budget_max, "fixture must still exercise budget pressure; report={report:?}" ); @@ -454,6 +463,27 @@ fn tight_budget_dashboard_keeps_component_composition() { sys.contains("comp-0") && sys.contains("comp-4"), "manifest must list the concrete component ids" ); + // (5) The budget never paid for a skill the tier filter was about to + // delete. Every skill the Basic allow-set removes must be reported as + // `TierFiltered`, never as `BudgetExhausted` — the two reasons are the + // observable difference between compacting before and after the knapsack. + // Measured on this fixture: six skills (product-principles, + // jian-components, design-system, dashboard, design-principles, + // role-definitions) are allow-set removals, and under the old order they + // competed for — and consumed — part of a 5200-token budget first. + for name in ["design-system", "dashboard", "role-definitions"] { + let reason = report + .dropped + .iter() + .find(|s| s.name == name) + .map(|s| s.reason); + assert_eq!( + reason, + Some(op_ai_skills::DropReason::TierFiltered), + "{name} must be removed by the tier filter BEFORE the knapsack, \ + so it never consumes budget; report={report:?}" + ); + } } /// The force-include is gated on a library being present: with NO components, a diff --git a/crates/op-orchestrator/src/prompt_deck_skill_tests.rs b/crates/op-orchestrator/src/prompt_deck_skill_tests.rs index 19f2c7dd6..536962920 100644 --- a/crates/op-orchestrator/src/prompt_deck_skill_tests.rs +++ b/crates/op-orchestrator/src/prompt_deck_skill_tests.rs @@ -100,6 +100,18 @@ const DECK_PATTERNS_MARKERS: [&str; 5] = [ "a rectangle does not render its children", ]; +/// The third deck skill (2026-08-09). It joined a phase budget that was +/// already full — `op-ai-skills` raised `Phase::Generation` 12000 → 13200 and +/// the deck tier arms below moved with it — so it is exactly the kind of skill +/// that gets silently squeezed out, and exactly why it is guarded here. +const DECK_CONTRACT_MARKERS: [&str; 5] = [ + "DECK CONTRACT", + "## Law 1 — overflow splits the page, it never shrinks the type", + "FORBIDDEN as fixes: a smaller font", + "**Ghost deck test**", + "## Deck slop — each is a recognisable fingerprint", +]; + /// Full / Standard / Basic all reach the model with the deck corpus whole. /// One model id per tier — the tier is what selects the budget arm and the /// `compact_skills` allow-set, and each of those dropped the deck skills on @@ -120,7 +132,11 @@ fn every_tier_receives_the_deck_corpus_intact() { ); let prompt = &call.system_prompt; - for marker in SLIDES_MARKERS.iter().chain(DECK_PATTERNS_MARKERS.iter()) { + for marker in SLIDES_MARKERS + .iter() + .chain(DECK_PATTERNS_MARKERS.iter()) + .chain(DECK_CONTRACT_MARKERS.iter()) + { assert!( prompt.contains(marker), "model {model:?}: assembled system prompt is missing {marker:?} — \ @@ -137,7 +153,7 @@ fn every_tier_receives_the_deck_corpus_intact() { } // The tails specifically — the knapsack cuts from the end. - for name in ["slides", "deck-patterns"] { + for name in ["slides", "deck-patterns", "deck-contract"] { assert!( prompt.contains(skill_tail(name)), "model {model:?}: {name} reached the prompt without its last line — \ @@ -162,6 +178,51 @@ fn every_tier_receives_the_deck_corpus_intact() { } } +/// Nothing may lose the budget race while the budget still has room. +/// +/// This is the ordering defect, stated as an invariant. The sub-agent +/// compaction used to run AFTER the knapsack, so the budget was spent on +/// skills the compaction then deleted and never handed back: this exact deck +/// fixture reported `design-principles` as `BudgetExhausted` while the report +/// showed 652 tokens of headroom, because at knapsack time `design-system` +/// (554, deleted moments later) had been holding most of it. With the +/// compaction moved in front, the same fixture resolves 13069/13200 with an +/// empty budget-drop list. +/// +/// If this fails because the corpus genuinely outgrew the phase budget, that +/// is the alarm working — the fix is the phase budget, not this assertion. +#[test] +fn no_skill_loses_the_budget_race_while_the_budget_has_room() { + for model in ["claude-opus-5", "kimi-k2.5", "glm-4.6"] { + let (_call, report) = build_subagent_prompt( + &deck_subtask(), + &deck_plan(), + &deck_request(model), + AbortFlag::new(), + false, + false, + &op_editor_core::ComponentLibrary::default(), + ); + let budget_dropped: Vec<&str> = report + .dropped + .iter() + .filter(|s| matches!(s.reason, op_ai_skills::DropReason::BudgetExhausted)) + .map(|s| s.name.as_str()) + .collect(); + assert!( + report.budget_used <= report.budget_max, + "model {model:?}: the deck set must fit its budget; report={report:?}" + ); + assert!( + budget_dropped.is_empty(), + "model {model:?}: {budget_dropped:?} lost the budget race with \ + {} tokens still free — the knapsack is paying for skills the \ + compaction deletes again", + report.budget_max - report.budget_used, + ); + } +} + /// The budget arm must be selected by the deck's ARTBOARD, not by the prompt /// wording — a deck plan whose request never says "deck" still needs the room. #[test] @@ -179,6 +240,27 @@ fn the_deck_budget_arm_keys_off_the_projector_artboard() { assert!(!is_deck_board(&mobile)); } +/// A deck-WIDE artboard that is not a deck SHAPE keeps the ordinary page +/// budget. This is the behaviour change from routing `is_deck_board` through +/// `classify_root_form` (2026-08-09): the old `w >= 1600 && h >= 900` had no +/// aspect gate, so a 1920×2000 long-scroll page claimed ~13200 tokens and +/// spent them on 16:9 slide teaching it can never apply — while the page +/// skills it actually needed competed for what was left. Losing the override +/// here is the fix, not a regression. +#[test] +fn a_tall_page_at_deck_width_does_not_claim_the_deck_budget() { + let mut long_page = deck_plan(); + long_page.root_frame.height = 2000.0; + assert!( + !is_deck_board(&long_page), + "1920x2000 is a long page (aspect 1.04), not a projector board" + ); + // The band still accepts a board a model sized slightly off 16:9. + let mut near_16_9 = deck_plan(); + near_16_9.root_frame.height = 1000.0; + assert!(is_deck_board(&near_16_9), "1920x1000 is still a board"); +} + /// A non-deck plan must be byte-for-byte unaffected: the deck skills are /// keyword-gated, so they must not appear on an ordinary page prompt, and the /// tier budget for that page must stay where it was. diff --git a/crates/op-orchestrator/src/prompt_style_skills.rs b/crates/op-orchestrator/src/prompt_style_skills.rs index 1659a877e..ad139da86 100644 --- a/crates/op-orchestrator/src/prompt_style_skills.rs +++ b/crates/op-orchestrator/src/prompt_style_skills.rs @@ -3,15 +3,11 @@ use super::*; -/// Resolve the generation-phase skill set against a message, returning the -/// full AgentContext (skills + load report). The report is held for B3b's -/// IntentMiss/BudgetExhausted merge. -pub(super) fn resolve_generation_skills( - message: &str, - opts: &op_ai_skills::ResolveOptions, -) -> op_ai_skills::AgentContext { - op_ai_skills::resolve_skills(op_ai_skills::Phase::Generation, message, opts) -} +// `resolve_generation_skills` (a bare `resolve_skills` wrapper) used to serve +// every sub-agent path except Basic-mobile. It is gone rather than kept for +// symmetry: calling it meant budgeting BEFORE the compaction, which is the +// defect `resolve_generation_skills_after_prompt_filter` below exists to +// avoid, so leaving it in reach would leave the wrong order one call away. /// 该 plan 是否代表一整屏移动端页面。 /// @@ -44,8 +40,19 @@ pub(super) fn is_mobile_full_screen(plan: &OrchestratorPlan) -> bool { /// against ~6200 of always-kept Base skills) BOTH are dropped for /// `BudgetExhausted` and a weak model designs a deck with no deck guidance at /// all — measured 2026-08-04, before this arm existed. +/// Routed through the single form classifier rather than comparing widths +/// here. The hand-rolled `w >= 1600 && h >= 900` this replaces was a fourth +/// geometric literal alongside the three in `design_form`, and it was LOOSER +/// in the one way that matters: with no aspect gate, a 1920×2000 long page +/// claimed the deck budget and spent it on slide teaching it could not use. +/// Under the classifier that page reads as [`DesignForm::Page`] and keeps the +/// ordinary page budget — see the test that pins that case. pub(super) fn is_deck_board(plan: &OrchestratorPlan) -> bool { - plan.root_frame.width >= 1600.0 && plan.root_frame.height >= 900.0 + crate::design_type::classify_root_form( + Some(plan.root_frame.width), + Some(plan.root_frame.height), + ) + .is_deck_board() } /// Build the sub-agent style-guide instruction block for the planner-selected @@ -203,6 +210,22 @@ pub(super) fn push_resolved_string_tokens( } } +/// Resolve the generation skill set with the sub-agent compaction applied +/// BEFORE the budget knapsack, so the budget is never spent on a skill the +/// compaction is about to delete. This is the order every sub-agent prompt +/// uses; see the call site in `prompt_subagent` for what the other order cost. +/// +/// Mirrors `op_ai_skills::resolve_skills` step for step (phase filter → intent +/// / flag match → dynamic-content injection → `trim_by_budget_pinned`) with +/// the compaction inserted between the third and fourth. **The one thing it +/// does not mirror is memory**: `resolve_skills` derives `{{recentHistory}}` +/// from `opts.memory.generation_history`, and this reimplementation falls back +/// to the empty-history text that derivation produces. That is exact — and +/// only exact — while the caller passes no memory, which the sub-agent path +/// does not (`ResolveOptions { .., ..Default::default() }`). A caller that +/// starts populating `memory` must route through `resolve_skills` or teach +/// this function the same derivation; the history helpers are private to +/// `op-ai-skills`, which is why this note exists instead of a call. pub(super) fn resolve_generation_skills_after_prompt_filter( intent: &str, opts: &ResolveOptions, diff --git a/crates/op-orchestrator/src/prompt_subagent.rs b/crates/op-orchestrator/src/prompt_subagent.rs index a8c842abd..c09e2ed56 100644 --- a/crates/op-orchestrator/src/prompt_subagent.rs +++ b/crates/op-orchestrator/src/prompt_subagent.rs @@ -86,7 +86,8 @@ pub(super) fn build_subagent_prompt_core( components: &ComponentLibrary, screen_routes: &[(String, String)], ) -> (CallRequest, SkillLoadReport) { - // Resolve the full generation skill set, then apply tier-gated filtering. + // Apply tier-gated filtering, then resolve the generation skill set under + // the budget — that order, not the reverse; see the resolve call below. let model_id = req.model.as_deref().unwrap_or(""); let tier = resolve_model_profile(model_id).tier; @@ -195,21 +196,35 @@ pub(super) fn build_subagent_prompt_core( // compact filter; the `mobile-ui` rules used to be appended to the user prompt // (uncounted) and now live in a budgeted skill, so the budget grows by ~its // size — the TOTAL prompt is unchanged, the rules just moved user→system. - // A deck board gets its own arm for the same reason mobile does: the - // `slides` + `deck-patterns` teaching is ~4000 tokens on top of ~6200 of - // always-kept Base skills, so under the plain 5200 / 6500 arms both are - // dropped for BudgetExhausted and the model designs slides with no slide - // guidance. 11500 covers the always-kept Base set (which on this path - // includes `style-defaults`, loaded by the `noStyleGuideMatch` flag) plus - // `cjk-typography` and both deck skills whole; at 10600 `slides` still lost - // its tail, which `prompt_deck_skill_tests` asserts against. + // A deck board gets its own arm for the same reason mobile does: the deck + // teaching (`slides` + `deck-patterns` + `deck-contract`, ~5600 tokens) + // sits on top of ~6000 of always-kept Base skills, so under the plain + // 5200 / 6500 arms it is dropped for BudgetExhausted and the model designs + // slides with no slide guidance. + // + // The arm is the Generation phase default rather than a literal, because + // the deck path IS the worst case that default was last sized for + // (`Phase::Generation` moved 12000 → 13200 when `deck-contract` landed). + // Restating it as a number is what let the old 11500 rot when the corpus + // grew under it: the deck skills were then silently dropped/tail-cut, + // which `prompt_deck_skill_tests` now asserts against. + // + // Measured 2026-08-09 on that file's fixtures, every resolved skill + // untruncated: Basic 11529/13200, Standard and Full both 12548/13200. + // Standard lands on Full's exact skill set here — at an unbounded budget + // it also carries `design-principles` (12986), and at 13200 the deck + // corpus crowds that Knowledge skill out. That is NOT this arm's doing: + // Full tier reads the same default and loses it identically, so the deck + // load simply fills the phase. Buying it back means raising the phase + // default, which belongs to the corpus owner, not to this override. let is_deck = is_deck_board(plan); + let deck_budget = Phase::Generation.default_budget(); let budget_override = match tier { ModelTier::Basic if is_mobile_layout || is_mobile_screen => Some(9200), - ModelTier::Basic if is_deck => Some(11500), + ModelTier::Basic if is_deck => Some(deck_budget), ModelTier::Basic => Some(5200), ModelTier::Standard if is_mobile_layout => Some(9500), - ModelTier::Standard if is_deck => Some(11500), + ModelTier::Standard if is_deck => Some(deck_budget), ModelTier::Standard => Some(6500), ModelTier::Full => None, }; @@ -235,30 +250,28 @@ pub(super) fn build_subagent_prompt_core( ..Default::default() }; let intent = subtask_intent(req, subtask); + // Filter BEFORE the budget knapsack, on every path. + // + // This used to be the Basic-mobile path only; everything else resolved + // first and compacted second, which meant the knapsack paid for skills the + // compaction was about to delete. `design-system` (554 tokens) is the + // standing example: `design_system_covered` is true on essentially every + // real request, so the budget bought it, the filter dropped it, and the + // 554 tokens were never returned to the skills that had just lost to it — + // a deck prompt reported 12548/13200 while `design-principles` (438) sat + // in the dropped list as BudgetExhausted, because at knapsack time only 98 + // tokens were actually free. Ordering, not sizing: raising the ceiling + // would have hidden it rather than fixed it. let (mut filtered, resolve_report, filter_drops) = - if tier == ModelTier::Basic && is_mobile_screen { - resolve_generation_skills_after_prompt_filter( - &intent, - &opts, - tier, - is_mobile_screen, - design_system_covered, - minimal_skills, - reduced_complexity, - ) - } else { - let agent_ctx = resolve_generation_skills(&intent, &opts); - let resolved = agent_ctx.skills; - let (filtered, filter_drops) = apply_skill_filter( - resolved, - tier, - is_mobile_screen, - design_system_covered, - minimal_skills, - reduced_complexity, - ); - (filtered, agent_ctx.report, filter_drops) - }; + resolve_generation_skills_after_prompt_filter( + &intent, + &opts, + tier, + is_mobile_screen, + design_system_covered, + minimal_skills, + reduced_complexity, + ); // Script-gen REPLACES the raw-JSONL output format — carrying a JSONL skill // alongside it feeds the model two contradictory output contracts. Keep // this guard even though the JSONL skills are no longer mounted by the @@ -466,15 +479,16 @@ CRITICAL LAYOUT CONSTRAINTS:\n\ // Assemble the per-subtask skill-load report from the FINAL skill set // (post tier/dedup filtering). `budget_max` reflects the tier budget // override. Full-tier falls through to `Phase::Generation::default_budget()` - // (12000, raised from 8000 in op-ai-skills — see that constant's doc - // comment) because image-rich data-list sections (restaurants/products - // with ratings/prices) overflowed 8000 tokens and truncated their - // scripts to zero generated nodes. This used to be a bare `12000` - // literal that only affected this diagnostic number — `resolve_skills` - // (called above via `resolve_generation_skills`) independently fell - // back to the OLD 8000 default for a `None` override, so Full tier's - // real skill trimming silently ran at 8000 while this report claimed - // 12000. Deriving both from the same constant keeps them honest. + // (13200 today — see that constant's doc comment for both raises: 8000 → + // 12000 because image-rich data-list sections overflowed and truncated + // their scripts to zero generated nodes, then 12000 → 13200 when + // `deck-contract` joined the deck corpus). This used to be a bare literal + // that only affected this diagnostic number — `resolve_skills` (called + // above via `resolve_generation_skills`) independently fell back to the + // OLD default for a `None` override, so Full tier's real skill trimming + // silently ran at one number while this report claimed another. Deriving + // both from the same constant keeps them honest, and is why the deck arm + // above reads the constant instead of restating it. let budget_max = budget_override.unwrap_or_else(|| Phase::Generation.default_budget()); let included: Vec = filtered .iter()