fix(stella-cli): record skill_usage on every execution path, not just the deck (#1872)

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<AgentEvent>,
     // 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::<AgentEvent>();

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<AgentEvent>,
-    // 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<Arc<stella_mcp::McpToolSet>>,
     // Phase 2 (#713): this turn's `ContextRecall`, carried from the caller.
     recall_event: Option<AgentEvent>,
-    // 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());
 

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<Store>, 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<stella_store::SkillUsageRow> = 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::Value> = 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);
+    }
+}

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<stella_store::SkillUsageRow> = 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 —

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<i64> {
+        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,