From 3b9fefde26ba9d196cb54019ace2ca4f9792aecb Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 9 Aug 2026 19:57:24 +0800 Subject: [PATCH] fix(ai): compact sub-agent skills before spending the budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sub-agent prompts budgeted first and compacted second, so the knapsack paid for skills the compaction was about to delete — and when the bill came due it evicted the deck material instead. A weak model then designed a deck with no deck guidance at all, silently, because the corpus on disk was still correct. The order is now compaction first, and the wrapper that made the wrong order one call away is gone rather than kept for symmetry. `deck-contract` joins the sub-agent allow-set for the same reason its two neighbours are there, and the deck budget arm asks the single classifier instead of comparing widths inline: the hand-rolled bound had no aspect gate, so a tall 1920-wide page claimed the deck budget and spent it on slide teaching it could not use. Claude-Session: https://claude.ai/code/session_01FqKQqNj8exYwopGDpYUU7x --- crates/op-orchestrator/src/compact_skills.rs | 7 ++ .../src/prompt_components_tests.rs | 38 ++++++- .../src/prompt_deck_skill_tests.rs | 86 ++++++++++++++- .../src/prompt_style_skills.rs | 43 ++++++-- crates/op-orchestrator/src/prompt_subagent.rs | 100 ++++++++++-------- 5 files changed, 215 insertions(+), 59 deletions(-) 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()