From 859d8c183040525332743b0f5646448da1aba461 Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Fri, 11 Sep 2026 11:20:41 -0700 Subject: [PATCH] fix(stella-cli): pick the skill holdout from the skills this turn matched The pick read the whole loaded catalog, so an ordinal could name a skill the prompt never matched. Nothing was then withheld, the slot produced no control trial, and the schedule counted it anyway. The pick now reads the matched population, and the first selection pass to score settles it on the turn's shared holdout cell, so every later pass withholds the same skill. Closes #6464 --- crates/stella-cli/src/memory.rs | 29 ++++--- .../src/memory/learning/skill_lifecycle.rs | 76 +++++++++++++++++++ crates/stella-cli/src/memory/trials.rs | 20 ++++- 3 files changed, 113 insertions(+), 12 deletions(-) diff --git a/crates/stella-cli/src/memory.rs b/crates/stella-cli/src/memory.rs index cefa067e7d..0ed59ec1f0 100644 --- a/crates/stella-cli/src/memory.rs +++ b/crates/stella-cli/src/memory.rs @@ -874,7 +874,20 @@ impl SessionMemory { if trials::holdout_kind(ordinal) != Some(stella_learn::ledger::ArtifactKind::Skill) { return None; } - let loaded = self.load_skills(); + // The population is the skills this turn's prompt matched — the + // survivors plus the ones the top-k cut threw away, which cleared the + // same score floor. A pick from the whole catalog can name a skill + // this turn never matched; nothing is then withheld, the slot + // produces no trial, and the schedule still counts it. The pick is + // settled here, at the first site to score, and read back by + // every later site (`trials::held_for`), so a turn holds one skill + // back no matter how many times it renders. + let matched: Vec<&str> = selection + .selected + .iter() + .chain(selection.over_top_k.iter()) + .map(|s| s.skill.name.as_str()) + .collect(); let counts = appraisals::control_arm_counts( &self.workspace_root, stella_learn::ledger::ArtifactKind::Skill, @@ -882,20 +895,16 @@ impl SessionMemory { let bar = stella_learn::skills::appraisal::AppraisalConfig::default() .selection .min_samples_per_arm; - let starved: Vec<&str> = loaded + let starved: Vec<&str> = matched .iter() - .map(|s| s.name.as_str()) + .copied() .filter(|name| counts.get(*name).copied().unwrap_or(0) < bar) .collect(); - let names: Vec<&str> = if starved.is_empty() { - loaded.iter().map(|s| s.name.as_str()).collect() - } else { - starved - }; - let held = stella_learn::holdout::pick(ordinal, &names)?; + let names: Vec<&str> = if starved.is_empty() { matched } else { starved }; + let held = self.held_for_skill(&names)?; let before = selection.selected.len(); selection.selected.retain(|s| s.skill.name != held); - (selection.selected.len() != before).then(|| held.to_string()) + (selection.selected.len() != before).then_some(held) } /// Render selected skills as `(name, why)` — the matched domains and terms diff --git a/crates/stella-cli/src/memory/learning/skill_lifecycle.rs b/crates/stella-cli/src/memory/learning/skill_lifecycle.rs index a4a935d8bb..0c19515be6 100644 --- a/crates/stella-cli/src/memory/learning/skill_lifecycle.rs +++ b/crates/stella-cli/src/memory/learning/skill_lifecycle.rs @@ -568,6 +568,82 @@ async fn the_holdout_picks_the_skill_whose_control_arm_is_short() { ); } +/// **The holdout cannot name a skill the turn never matched.** The pick used +/// to read the whole loaded catalog, so a turn whose prompt matched only one +/// of two skills could have the other one's name drawn — nothing was then +/// withheld, the slot produced no trial, and the schedule still counted it. +/// The pick now comes from the trigger-matched population, so a holdout +/// turn always pays for exactly one control row. +/// +/// The fixture arms the schedule so the ordinal points at the skill the +/// prompt does NOT match. Before the fix that pick withheld nothing and the +/// turn recorded no trial; after it, the pick lands on the matched skill and +/// the ledger gains a control row for it. +#[tokio::test] +async fn the_holdout_cannot_pick_a_skill_the_turn_never_matched() { + let dir = workspace_with_log(); + set_gate(dir.path(), false); + + let mut miner = session(dir.path()); + miner.auto_create_skills(&log_path(dir.path()), true); + let written = skill_files(dir.path()); + assert_eq!(written.len(), 1, "the lesson promoted into a skill"); + let name = written[0].trim_end_matches(".md").to_string(); + + // A second skill the prompt does not match, so the catalog holds a name + // the turn never scored. It sorts before the mined skill, so the first + // holdout ordinal names it when the pick reads the whole catalog. The + // pick must not be able to reach it. + let other = dir.path().join(".stella/skills/aaa-unrelated.md"); + std::fs::write( + &other, + "---\nname: aaa-unrelated\ndescription: quantum knitting patterns for sweaters\n---\n\nknit one purl one\n", + ) + .expect("write unrelated skill"); + + let mut memory = session(dir.path()); + + // Turn 1 is not a holdout turn at rate 2; turn 2 is holdout ordinal 0, + // which the arm rotation points at skills and the pick resolves to the + // first name in sorted order. In the full catalog that is + // `aaa-unrelated`; in the matched population it is the mined skill. + assert!( + !memory.arm_controls_at(0, 2), + "the plane control is off here" + ); + let _ = memory.note_turn_skills(MATCHING_PROMPT); + memory + .record_episode(MATCHING_PROMPT, EpisodeOutcome::Success, &[], 1_000, None) + .await; + + // Turn 2: the holdout. The prompt still matches only the mined skill. + assert!(!memory.arm_controls_at(0, 2), "still no plane control"); + let injected: Vec = memory + .note_turn_skills(MATCHING_PROMPT) + .into_iter() + .map(|(n, _)| n) + .collect(); + assert_eq!( + injected, + Vec::::new(), + "the holdout must withhold the matched skill, not the unmatched one" + ); + memory + .record_episode(MATCHING_PROMPT, EpisodeOutcome::Success, &[], 1_000, None) + .await; + + assert_eq!( + trials(dir.path(), &name), + vec![true, false], + "the turn records a control trial for the skill it actually matched" + ); + assert_eq!( + trials(dir.path(), "aaa-unrelated"), + Vec::::new(), + "the unmatched skill is not evidence about anything" + ); +} + /// **The two schedules cannot land on the same turn.** A turn the plane /// control already took claims no holdout number, so the holdout's counter /// advances only over the turns it could act on. diff --git a/crates/stella-cli/src/memory/trials.rs b/crates/stella-cli/src/memory/trials.rs index 612162f141..4323f3de9d 100644 --- a/crates/stella-cli/src/memory/trials.rs +++ b/crates/stella-cli/src/memory/trials.rs @@ -150,15 +150,21 @@ impl SessionMemory { /// `None` on three counts: this turn is not a holdout turn, the schedule /// is on another kind, or there was nothing to pick from. fn held_for(&self, kind: ArtifactKind, population: &[String]) -> Option { + let ids: Vec<&str> = population.iter().map(String::as_str).collect(); + self.held_for_refs(kind, &ids) + } + + /// The borrow form of [`Self::held_for`], for a caller whose population + /// is already `&str`. + fn held_for_refs(&self, kind: ArtifactKind, population: &[&str]) -> Option { let ordinal = self.holdout_ordinal?; if holdout_kind(ordinal) != Some(kind) { return None; } let mut guard = self.context_trials.lock().ok()?; if guard.held.is_none() { - let ids: Vec<&str> = population.iter().map(String::as_str).collect(); guard.held = - stella_learn::holdout::pick(ordinal, &ids).map(|id| (kind, id.to_string())); + stella_learn::holdout::pick(ordinal, population).map(|id| (kind, id.to_string())); } match &guard.held { Some((held, id)) if *held == kind => Some(id.clone()), @@ -166,6 +172,16 @@ impl SessionMemory { } } + /// The skill this turn holds back, settled from the first selection pass + /// to ask and read back by every later one. + /// + /// Skills keep their own join on the session, but the holdout pick is one + /// per turn across all three kinds, so it lives on the shared cell beside + /// the memory and rule picks rather than on `turn_skill_join`. + pub(super) fn held_for_skill(&self, population: &[&str]) -> Option { + self.held_for_refs(ArtifactKind::Skill, population) + } + /// Fold one render pass into this turn's join for `kind`. fn note_context_trial(&self, kind: ArtifactKind, offered: &[String], selected: &[String]) { let Ok(mut guard) = self.context_trials.lock() else {