fix(stella-cli): record skill_usage on every execution path, not just the deck (#1872) - #1896
Merged
Merged
Conversation
added 4 commits
August 6, 2026 04:38
… the deck Store::record_skill_usage had exactly one caller — the deck's turn dispatch — so the default `stella run` pipeline path, the raw one-shot, goal runs, and every other headless surface injected skills but wrote no skill_usage rows. The Sessions tab, the memory tab's skills aggregate, and skill appraisal (which feeds retirement) all under-counted for the paths that produce most usage. The recorder now lives at the shared execution seam (agent::skill_usage::stamp_execution_and_record_skill_usage), beside the execution-id stamp every turn-building path already performs: the pipeline path, run_turn, both goal-turn drivers, and the deck all hit one function. Semantics are the deck's, now shared: one batch per execution, written at turn start, best-effort. selected_skills now honours the A/B recall control, so a control turn that injects no skills records no phantom usage — aligning the report with the injection channels it describes. Closes #1872
…usage-all-paths
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Reviewer's GuideCentralizes skill-usage telemetry and execution-id stamping into a shared agent seam so all turn-building paths consistently record skill_usage rows and honor A/B recall control, while preserving deck semantics and adding targeted tests. Flow diagram for shared skill_usage execution seam and callersflowchart TD
subgraph Turn_builders
run_pipeline_one_shot[run_pipeline_one_shot]
run_turn[run_turn]
run_goal_turn[run_goal_turn]
run_goal_pipeline_turn[run_goal_pipeline_turn]
run_deck_session[run_deck_session]
end
seam[stamp_and_record_skill_usage]
memory_set[SessionMemory.set_execution_id]
selected[SessionMemory.selected_skills]
pinned[skill_manager.pinned_versions]
record[Store.record_skill_usage]
run_pipeline_one_shot --> seam
run_turn --> seam
run_goal_turn --> seam
run_goal_pipeline_turn --> seam
run_deck_session --> seam
seam --> memory_set
memory_set --> selected
selected -->|non_empty, ab_not_suppressed| pinned
selected -->|empty_or_ab_suppressed| end_noop[(no skill_usage rows)]
pinned --> record
record --> end_rows[(skill_usage rows keyed by execution_id)]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Store::record_skill_usagehad exactly one call site — the deck's turn dispatch (crates/stella-cli/src/command_deck.rs) — whose own comment admitted "only for the deck path for now". So the defaultstella runpipeline path, the raw one-shot, both goal-turn drivers, and every other headless surface injected skills but wrote zeroskill_usagerows. Every reader of the table under-counted for exactly the paths that produce most usage: the observatory's Sessions tab chips, the memory tab's skills aggregate, and skill appraisal (crates/stella-cli/src/memory/appraisals.rs) — and since appraisal feeds retirement, the gap could actively mis-steer which skills get retired.Approach (as specced in the issue triage): the recorder moves to the shared execution seam. A new sibling module
crates/stella-cli/src/agent/skill_usage.rsownsstamp_and_record_skill_usage, which subsumes the execution-id stamp every turn-building path already performed and records the selected skills at their pinned versions beside it. All five turn-building paths now hit the one function: the pipeline path andrun_turninagent.rs, both goal-turn drivers inagent/goal.rs, and the deck (whose private inline copy is deleted). Paths with no session memory (sub-sessions, resume, fleet) inject no skills and correctly record nothing.Append-vs-once semantics (the table has no UNIQUE): the deck's existing semantics, now shared and documented in the module doc — one batch per execution, written at turn start (skills are injected regardless of how the turn ends). Each path begins its execution exactly once, so the append-only table holds at most one batch per execution by construction.
One deliberate behavior alignment:
SessionMemory::selected_skillsnow honours the A/B recall control. Its doc already claimed it "reports exactly what was applied", but on a control turn the injection channels (recall_block_reported,pipeline_recall_block) suppress skills whileselected_skillsdid not — so the deck recorded phantom usage for skills the model never saw, corrupting the exact appraisal signal this telemetry feeds. The guard aligns the report with the injection.No schema change:
skill_usageis already inprune.rs::DEPENDENT_TABLES.Closes #1872
The witness
main, passes here)Two rungs:
agent::skill_usage::tests::a_pipeline_shaped_execution_records_skill_usage_at_the_seam— a non-deck ("pipeline"-kind) execution flowing through the seam leaves askill_usagerow keyed to that execution. Onmainthe seam does not exist and a pipeline execution wrote zero rows (the sole recorder was the deck's inline block).memory::tests::a_control_turn_reports_no_selected_skills— verified the artisanal way: with theab_suppressedguard reverted, the test fails (test result: FAILED. 0 passed; 1 failed); with it, passes.Deck behavior is preserved structurally: the deck now calls the same seam function whose semantics (turn-start, once per execution, best-effort) are the ones its inline copy implemented;
the_seam_stamps_the_execution_id_onto_memoryproves the seam also carries the reflection execution-id stamp it replaced.The gate
cargo fmt --check(clean; fmt run on the touched crate)cargo clippy -p stella-cli --all-targets -- -D warningscleancargo test -p stella-cli --bin stella(1438 passed, 0 failed); rustdoc-D warningsclean for the crate. Full-workspace gate left to CI on this 16GB machine — flagging per build-economy policy.Closes #1872appears both above and as a commit trailerNothing left behind
agent.rs2266/2269,command_deck.rs4672/4740 — both god files shrank or stayed under their ceilings; new logic is in the sibling module per the god-file rule.origin/mainto pick up the fix(gate): unbreak all three of main's red gates in one PR, because one is not enough #1894 gate fixes (pre-existing baseline overages instella-protocol/stella-tuiwere from the stale base, not this change).Summary by Sourcery
Record skill-version usage telemetry at a shared execution seam so all turn-building paths write consistent
skill_usagerows and maintain correct execution-id stamping for reflections.New Features:
stamp_and_record_skill_usageexecution seam that stamps execution IDs onto session memory and records selected skills for all agent turn paths.Bug Fixes:
SessionMemory::selected_skillsrespects A/B recall control so control turns report no skill usage and telemetry matches actual injections.skill_usagetelemetry despite injecting skills.Enhancements:
Tests: