diff --git a/crates/stella-cli/src/agent.rs b/crates/stella-cli/src/agent.rs index a507c72a9..e7f6cf7fa 100644 --- a/crates/stella-cli/src/agent.rs +++ b/crates/stella-cli/src/agent.rs @@ -56,6 +56,7 @@ mod persistence; mod presence; mod prompt; pub(crate) mod resume; +mod skill_usage; mod tools; pub(crate) use engine::*; @@ -76,6 +77,7 @@ pub(crate) use persistence::{ }; pub(crate) use presence::SessionPresence; pub(crate) use prompt::*; +pub(crate) use skill_usage::stamp_and_record_skill_usage; // `tool_policy` is a top-level module (`main.rs`); the re-export keeps every // session driver's `agent::PolicyToolSet` reading as "the agent's tool stack". pub(crate) use crate::tool_policy::PolicyToolSet; @@ -331,20 +333,16 @@ async fn run_pipeline_one_shot( // is refused with a reason (#453). m.register_external_providers(|message| eprintln!(" {} {message}", "!".yellow())) .await; - // Tell memory which execution this run reflects on, before the turn - // runs — the post-turn self-review is stored 1:1 with an execution, - // so a path that skips this files id-less reflection rows (NULL - // `self_rating`), which is exactly what happened on this, the primary - // headless surface, while only the deck was wired. - if let Some((_, id)) = &execution { - m.set_execution_id(*id); - } // The A/B control, armed before anything recalls (#1221): a control // turn leaves BOTH the block below and the pipeline's own recall port // frameless, and the durable schedule is what lets a // one-turn-per-process surface produce an arm at all. m.arm_recall_control(); } + // The shared seam (#1872): stamp the execution onto memory and record the + // skills the block below will inject — after the arm above, so a control + // turn that injects nothing records nothing. + stamp_and_record_skill_usage(&execution, memory.as_mut(), prompt, &cfg.workspace_root); if let Some(m) = &memory { // Frames ride the pipeline's own recall port below (`recall:` in the // ports), which recalls once, renders them into the goal message, and @@ -2059,17 +2057,16 @@ async fn run_turn( // would double the retrieval cost of every interactive turn. recall_event: Option, // The caller's session memory, borrowed for the duration of the turn so - // this execution's id can be stamped before the turn runs — the caller - // reflects with the same memory afterwards, and a reflection that cannot - // name its execution files an id-less row (NULL `self_rating`). + // the execution seam can stamp this execution's id and record its + // skill-version usage before the turn runs — the caller reflects with the + // same memory afterwards, and a reflection that cannot name its execution + // files an id-less row (NULL `self_rating`). session_memory: Option<&mut SessionMemory>, ) -> Result<(), CliFailure> { budget.begin_turn(); let turn_start = Instant::now(); let execution = begin_execution(store, kind, prompt, cfg, session); - if let (Some((_, id)), Some(m)) = (&execution, session_memory) { - m.set_execution_id(*id); - } + stamp_and_record_skill_usage(&execution, session_memory, prompt, &cfg.workspace_root); let files_before = registry.files_touched().len(); let (raw_tx, rx) = mpsc::unbounded_channel::(); diff --git a/crates/stella-cli/src/agent/goal.rs b/crates/stella-cli/src/agent/goal.rs index 1061f574a..967eebf3c 100644 --- a/crates/stella-cli/src/agent/goal.rs +++ b/crates/stella-cli/src/agent/goal.rs @@ -409,16 +409,15 @@ pub(crate) async fn run_goal_turn( // Phase 2 (#713): this turn's `ContextRecall`, carried from the caller // because recall necessarily precedes the channel it would be emitted on. recall_event: Option, - // The caller's session memory, so this round's execution id is stamped - // before the turn runs — reflection stores the self-review 1:1 with an - // execution, and an unstamped round files an id-less row. + // The caller's session memory, so the execution seam can stamp this + // round's execution id and record its skill-version usage before the turn + // runs — reflection stores the self-review 1:1 with an execution, and an + // unstamped round files an id-less row. session_memory: Option<&mut crate::memory::SessionMemory>, ) -> Result<(), String> { let turn_start = Instant::now(); let execution = begin_execution(store, "goal", goal, cfg, session); - if let (Some((_, id)), Some(m)) = (&execution, session_memory) { - m.set_execution_id(*id); - } + stamp_and_record_skill_usage(&execution, session_memory, goal, &cfg.workspace_root); let files_before = registry.files_touched().len(); // Route the VERIFIER role. `Some` only when a distinct-family verifier was @@ -573,18 +572,22 @@ async fn run_goal_pipeline_turn( mcp: Option>, // Phase 2 (#713): this turn's `ContextRecall`, carried from the caller. recall_event: Option, - // Same contract as `run_goal_turn`: stamp the execution id into the - // caller's memory before the turn runs, so reflection can name its row. + // Same contract as `run_goal_turn`: the execution seam stamps the id into + // the caller's memory and records this round's skill-version usage before + // the turn runs, so reflection can name its row. session_memory: Option<&mut crate::memory::SessionMemory>, ) -> Result<(), String> { let turn_start = Instant::now(); let execution = begin_execution(store, "goal", goal, cfg, session); - // Rebound mutable and NOT consumed by the id stamp, so the same memory - // can double as the pipeline's recall port below. + // Rebound mutable and NOT consumed by the seam, so the same memory can + // double as the pipeline's recall port below. let mut session_memory = session_memory; - if let (Some((_, id)), Some(m)) = (&execution, session_memory.as_deref_mut()) { - m.set_execution_id(*id); - } + stamp_and_record_skill_usage( + &execution, + session_memory.as_deref_mut(), + goal, + &cfg.workspace_root, + ); let files_before = registry.files_touched().len(); let model_ref = ModelRef::new(cfg.provider.id, cfg.model_id.clone()); diff --git a/crates/stella-cli/src/agent/skill_usage.rs b/crates/stella-cli/src/agent/skill_usage.rs new file mode 100644 index 000000000..078b8ace6 --- /dev/null +++ b/crates/stella-cli/src/agent/skill_usage.rs @@ -0,0 +1,183 @@ +//! Skill-version usage telemetry, recorded at the shared execution seam. +//! +//! `Store::record_skill_usage` used to have exactly one caller — the deck's +//! turn dispatch — so `stella run` (the default pipeline path), the raw +//! one-shot, goal runs, and every other headless surface injected skills but +//! wrote no `skill_usage` rows. Every reader of the table — the observatory's +//! Sessions tab, the memory tab's skills aggregate, and skill appraisal +//! (which joins selections to receipts and feeds retirement) — under-counted +//! for exactly the paths that produce most usage, and appraisal could retire +//! a skill for phantom non-usage (#1872). +//! +//! The recorder now lives beside the execution-id stamp every turn-building +//! path already performs, so recording is a property of *beginning an +//! execution with session memory*, not of which front-end ran the turn. +//! +//! Semantics are the deck's, now shared: rows land **once per execution, at +//! turn start** — the skills are injected regardless of how the turn ends, +//! and each path begins its execution exactly once, so the append-only table +//! (no UNIQUE constraint) holds at most one batch per execution by +//! construction. Best-effort like every other telemetry write: a failed +//! insert never fails the turn. + +use std::path::Path; + +use super::*; + +/// Stamp `execution` onto the session memory (the post-turn self-review is +/// stored 1:1 with an execution, so a turn that cannot name its row files an +/// id-less reflection) and record the skills recall selected for `prompt`, +/// at their pinned versions, keyed to that execution. +/// +/// Call this **after** [`SessionMemory::arm_recall_control`] has armed the +/// turn: selection honours the A/B recall control, so a control turn that +/// injects no skills records no usage. A missing store, execution, or memory +/// is a quiet no-op — those paths inject no skills either. +pub(crate) fn stamp_and_record_skill_usage( + execution: &Option<(Arc, i64)>, + memory: Option<&mut SessionMemory>, + prompt: &str, + workspace_root: &Path, +) { + let (Some((store, id)), Some(memory)) = (execution, memory) else { + return; + }; + memory.set_execution_id(*id); + let selected = memory.selected_skills(prompt); + if selected.is_empty() { + return; + } + let versions = crate::skill_manager::pinned_versions(workspace_root); + let rows: Vec = selected + .into_iter() + .map(|(skill, reason)| stella_store::SkillUsageRow { + version: versions.get(&skill).copied().unwrap_or(1), + skill, + reason, + }) + .collect(); + let _ = store.record_skill_usage(*id, &rows); +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A workspace whose skills directory holds one skill that + /// `select_skills` matches on the prompt "review the database". + fn workspace_with_reviewer_skill() -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("tempdir"); + let skill_dir = dir.path().join(".stella/skills/reviewer"); + std::fs::create_dir_all(&skill_dir).expect("skill dir"); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: reviewer\ndescription: database review\n---\nALWAYS_REVIEW_DATABASES", + ) + .expect("skill file"); + dir + } + + fn session_memory(root: &Path) -> SessionMemory { + // Workspace skills ride the same authority switch as project prompts, + // exactly as `open_for_session` resolves it for every shipped surface. + let authority = crate::settings::AuthorityPolicy { + project_prompts_allowed: true, + ..crate::settings::AuthorityPolicy::default() + }; + SessionMemory::open_for_session( + root, + false, + &authority, + &crate::rules::ResolvedRules::default(), + ) + .expect("session memory") + } + + /// The #1872 witness: a non-deck execution ("pipeline" kind, the default + /// `stella run` path) flowing through the shared seam leaves a + /// `skill_usage` row. On main only the deck's inline recorder wrote one, + /// so every headless run reported zero skill telemetry. + #[test] + fn a_pipeline_shaped_execution_records_skill_usage_at_the_seam() { + let dir = workspace_with_reviewer_skill(); + let mut memory = session_memory(dir.path()); + let store = Arc::new(Store::in_memory().expect("store")); + let id = store + .begin_execution("pipeline", "review the database", "anthropic", "claude") + .expect("begin"); + + stamp_and_record_skill_usage( + &Some((store.clone(), id)), + Some(&mut memory), + "review the database", + dir.path(), + ); + + let json = store + .export_all_json() + .expect("export") + .into_iter() + .find_map(|(table, json)| (table == "skill_usage").then_some(json)) + .expect("skill_usage export"); + let rows: Vec = serde_json::from_str(&json).expect("rows"); + assert_eq!( + rows.len(), + 1, + "the seam records the selected skill for a non-deck execution" + ); + assert_eq!(rows[0]["skill"], "reviewer"); + assert_eq!( + rows[0]["execution_id"].as_i64(), + Some(id), + "rows are keyed to the seam's execution" + ); + } + + /// The seam subsumes the per-path execution-id stamp it replaced: a + /// reflection following the turn can still name its execution row. + #[test] + fn the_seam_stamps_the_execution_id_onto_memory() { + let dir = workspace_with_reviewer_skill(); + let mut memory = session_memory(dir.path()); + let store = Arc::new(Store::in_memory().expect("store")); + let id = store + .begin_execution("goal", "unrelated prompt", "anthropic", "claude") + .expect("begin"); + + stamp_and_record_skill_usage( + &Some((store.clone(), id)), + Some(&mut memory), + "unrelated prompt", + dir.path(), + ); + + assert_eq!(memory.execution_id_for_test(), Some(id)); + assert_eq!( + store.count("skill_usage").expect("count"), + 0, + "a prompt that selects no skill records no phantom usage" + ); + } + + /// A path with no memory (sub-sessions, resume) injects no skills, so the + /// seam records nothing — and must not panic reaching for a store. + #[test] + fn absent_memory_or_execution_is_a_no_op() { + let dir = workspace_with_reviewer_skill(); + let store = Arc::new(Store::in_memory().expect("store")); + let id = store + .begin_execution("deck-sub", "review the database", "anthropic", "claude") + .expect("begin"); + + stamp_and_record_skill_usage( + &Some((store.clone(), id)), + None, + "review the database", + dir.path(), + ); + let mut memory = session_memory(dir.path()); + stamp_and_record_skill_usage(&None, Some(&mut memory), "review", dir.path()); + + assert_eq!(store.count("skill_usage").expect("count"), 0); + } +} diff --git a/crates/stella-cli/src/command_deck.rs b/crates/stella-cli/src/command_deck.rs index 3ab7a34aa..cd3d233bd 100644 --- a/crates/stella-cli/src/command_deck.rs +++ b/crates/stella-cli/src/command_deck.rs @@ -1524,39 +1524,20 @@ pub async fn run_deck_session( ); if let Some((_, id)) = &execution { last_execution_id = Some(*id); - // Tell memory which execution it is reflecting on, before the turn - // runs. The post-turn self-review is stored 1:1 with an execution, - // so a loop that cannot name the row writes nothing — which is why - // `self_rating` was NULL on every reflection row this deck ever - // recorded, and the Observatory's self-improve panels sat empty. - if let Some(m) = &mut memory { - m.set_execution_id(*id); - } } + // The shared execution seam (#1872): stamp the execution onto memory + // (the post-turn self-review is stored 1:1 with it) and record this + // turn's skill-version usage — the same seam every headless path hits, + // so the deck no longer carries a private copy of the recorder. + agent::stamp_and_record_skill_usage( + &execution, + memory.as_mut(), + &prompt, + &cfg.workspace_root, + ); let files_before = registry.files_touched().len(); let started_unix = crate::memory::unix_now_secs(); - // Skill-version usage telemetry: record which skills recall selected for - // this turn, at their pinned version, keyed to this execution. Recorded - // at turn start (the skills are injected regardless of how the turn - // ends); best-effort, and only for the deck path for now — the other - // `record_execution_end` sites can adopt it later. - if let (Some((store, id)), Some(m)) = (&execution, &memory) { - let selected = m.selected_skills(&prompt); - if !selected.is_empty() { - let versions = crate::skill_manager::pinned_versions(&cfg.workspace_root); - let rows: Vec = selected - .into_iter() - .map(|(skill, reason)| stella_store::SkillUsageRow { - version: versions.get(&skill).copied().unwrap_or(1), - skill, - reason, - }) - .collect(); - let _ = store.record_skill_usage(*id, &rows); - } - } - // Resolve the turn's tool executor from the MCP slot at dispatch: // connected servers join the session the moment the background // connect lands, and a turn that beats it runs on native tools — diff --git a/crates/stella-cli/src/memory.rs b/crates/stella-cli/src/memory.rs index b83b2a114..6e9933160 100644 --- a/crates/stella-cli/src/memory.rs +++ b/crates/stella-cli/src/memory.rs @@ -302,6 +302,11 @@ impl SessionMemory { &self.task_id } + #[cfg(test)] + pub(crate) fn execution_id_for_test(&self) -> Option { + self.execution_id + } + /// Tell memory which execution this turn's reflection belongs to, so the /// model's self-review can be stored against it. /// @@ -481,6 +486,14 @@ impl SessionMemory { /// domains/terms that selected it. Same enabled-filtered load + selection /// as [`Self::recall_block_reported`], so this reports exactly what was applied. pub fn selected_skills(&self, prompt: &str) -> Vec<(String, String)> { + // The A/B recall control gates every injection channel + // ([`Self::recall_block_reported`], [`Self::pipeline_recall_block`]), + // so it gates the report too: a control turn injects no skills, and + // recording usage for skills that never reached the prompt would + // corrupt the appraisal signal `skill_usage` feeds. + if self.ab_suppressed { + return Vec::new(); + } skills::select_skills( &self.load_skills(), prompt, diff --git a/crates/stella-cli/src/memory/tests.rs b/crates/stella-cli/src/memory/tests.rs index 60d0d98f5..f91295f9c 100644 --- a/crates/stella-cli/src/memory/tests.rs +++ b/crates/stella-cli/src/memory/tests.rs @@ -394,6 +394,34 @@ async fn ab_control_suppresses_skills_before_any_recall_section_is_built() { ); } +/// The usage report must mirror the injection channels it describes: a +/// control turn injects no skills (the test above), so `selected_skills` — +/// the source of every `skill_usage` telemetry row — must report none either, +/// or the appraisal signal counts skills the model never saw. +#[test] +fn a_control_turn_reports_no_selected_skills() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join(".stella/skills/reviewer"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: reviewer\ndescription: database review\n---\nALWAYS_REVIEW_DATABASES", + ) + .unwrap(); + let mut memory = + SessionMemory::open_with_workspace_skills(dir.path(), false, true).expect("session memory"); + assert!( + !memory.selected_skills("review the database").is_empty(), + "the fixture skill is selected on an armed turn" + ); + + memory.ab_suppressed = true; + assert!( + memory.selected_skills("review the database").is_empty(), + "a control turn reports exactly what it injected: nothing" + ); +} + /// The frames-free block for pipeline-driven turns: skills and the record /// channel ride, recalled frames do not — those are the pipeline recall /// port's job, and rendering them here too is the double-recall/double-bill