From 3b5e7471bbbe497ccd5f1cb081081b75715af7e8 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 2 Jul 2026 08:29:53 +0000 Subject: [PATCH 1/2] feat: stage managed skill consolidations --- src/automation/managed_skill_model.rs | 10 + src/automation/managed_skill_validation.rs | 13 +- src/automation/managed_skills.rs | 63 +- src/automation/runner.rs | 67 +- src/automation/skill_usage.rs | 5 + src/automation/skill_usage/overlap.rs | 230 +++++++ src/automation/skill_writer.rs | 644 ++++++++++++++++++- tests/agent_suite/managed_skills_test.rs | 143 +++- tests/automation_runner_test/skill_writer.rs | 194 ++++++ 9 files changed, 1326 insertions(+), 43 deletions(-) create mode 100644 src/automation/skill_usage/overlap.rs diff --git a/src/automation/managed_skill_model.rs b/src/automation/managed_skill_model.rs index f10ca63b..184fb4ad 100644 --- a/src/automation/managed_skill_model.rs +++ b/src/automation/managed_skill_model.rs @@ -192,6 +192,16 @@ pub struct ManagedSkillPendingUpdate { pub body_markdown: String, #[serde(default)] pub support_files: Vec, + /// Lifecycle state the skill transitions to when this staged change is + /// approved. `None` keeps the historical behavior (promote to `Active`). + /// Staged consolidations set `Some(Archived)`; skill content is always + /// preserved on disk (archive, never delete). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub resulting_state: Option, + /// Reviewer-facing reason recorded when the change was staged (used by + /// consolidation proposals). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub staged_reason: Option, } impl ManagedSkillPendingUpdate { diff --git a/src/automation/managed_skill_validation.rs b/src/automation/managed_skill_validation.rs index 317394cb..d0979d10 100644 --- a/src/automation/managed_skill_validation.rs +++ b/src/automation/managed_skill_validation.rs @@ -5,9 +5,9 @@ use crate::errors::{Result, TraceDecayError}; use super::managed_skill_format::target_key; use super::managed_skill_model::{ - ManagedSkill, ManagedSkillPendingUpdate, ManagedSkillUpdate, ManagedSupportFile, - SkillInstallTarget, MAX_MANAGED_SKILL_BODY_BYTES, MAX_MANAGED_SUPPORT_FILES, - MAX_MANAGED_SUPPORT_FILE_BYTES, + ManagedSkill, ManagedSkillPendingUpdate, ManagedSkillState, ManagedSkillUpdate, + ManagedSupportFile, SkillInstallTarget, MAX_MANAGED_SKILL_BODY_BYTES, + MAX_MANAGED_SUPPORT_FILES, MAX_MANAGED_SUPPORT_FILE_BYTES, }; const ALLOWED_SUPPORT_ROOTS: &[&str] = &["references", "templates", "scripts", "assets"]; @@ -241,6 +241,13 @@ pub(crate) fn validate_managed_pending_update( "managed skill staged_at must be a positive timestamp".to_string(), )); } + if let Some(resulting_state) = pending.resulting_state { + if resulting_state != ManagedSkillState::Archived { + return Err(config_error( + "managed skill pending update resulting_state must be archived".to_string(), + )); + } + } let skill = ManagedSkill { metadata: pending.metadata.clone(), body_markdown: pending.body_markdown.clone(), diff --git a/src/automation/managed_skills.rs b/src/automation/managed_skills.rs index 1d6f48dd..2055d7f2 100644 --- a/src/automation/managed_skills.rs +++ b/src/automation/managed_skills.rs @@ -351,12 +351,65 @@ pub async fn stage_managed_skill_update( metadata: staged.metadata.clone(), body_markdown: staged.body_markdown.clone(), support_files: staged.support_files.clone(), + resulting_state: None, + staged_reason: None, }; save_pending_update(profile_root, id, &pending).await?; record_skill_patch(profile_root, &staged, "staged_update".to_string()).await?; Ok(pending.into_skill()) } +/// Stages an archive transition for a managed skill as a pending update that +/// must be approved (or discarded) through the normal review lifecycle. +/// Skill content is untouched: approving only flips the state to `Archived`, +/// keeping the body and support files recoverable on disk. Pinned skills are +/// exempt, matching the Hermes curator. +pub async fn stage_managed_skill_archive( + profile_root: &Path, + id: &str, + base_checksum: &str, + reason: Option, +) -> Result { + let skill = load_managed_skill(profile_root, id).await?; + if base_checksum != skill.metadata.checksum { + return Err(config_error(format!( + "base_checksum for managed skill id '{id}' is stale" + ))); + } + if skill.pending_update.is_some() { + return Err(config_error(format!( + "managed skill '{id}' already has a pending update" + ))); + } + if skill.metadata.pinned { + return Err(config_error(format!( + "managed skill '{id}' is pinned and exempt from staged archive" + ))); + } + if skill.metadata.state == ManagedSkillState::Archived { + return Err(config_error(format!( + "managed skill '{id}' is already archived" + ))); + } + + let mut staged = skill.clone(); + staged.pending_update = None; + staged.set_state(ManagedSkillState::PendingApproval); + staged.touch(); + let pending = ManagedSkillPendingUpdate { + base_checksum: base_checksum.to_string(), + staged_at: current_metadata_timestamp(), + metadata: staged.metadata.clone(), + body_markdown: staged.body_markdown.clone(), + support_files: staged.support_files.clone(), + resulting_state: Some(ManagedSkillState::Archived), + staged_reason: reason, + }; + save_pending_update(profile_root, id, &pending).await?; + record_skill_patch(profile_root, &staged, "staged_archive".to_string()).await?; + Ok(pending.into_skill()) +} + pub async fn discard_pending_managed_skill_update( profile_root: &Path, id: &str, @@ -433,13 +486,17 @@ pub async fn approve_managed_skill(profile_root: &Path, id: &str) -> Result set_managed_skill_state(profile_root, id, ManagedSkillState::Active).await?, Some(pending) => { + let resulting_state = pending.resulting_state.unwrap_or(ManagedSkillState::Active); + let patch_target = match resulting_state { + ManagedSkillState::Archived => "approve_staged_archive", + _ => "approve_staged_update", + }; let mut promoted = pending.into_skill(); - promoted.set_state(ManagedSkillState::Active); + promoted.set_state(resulting_state); promoted.refresh_checksum(); remove_pending_update(profile_root, id).await?; save_managed_skill(profile_root, &promoted).await?; - record_skill_patch(profile_root, &promoted, "approve_staged_update".to_string()) - .await?; + record_skill_patch(profile_root, &promoted, patch_target.to_string()).await?; promoted } }; diff --git a/src/automation/runner.rs b/src/automation/runner.rs index d20d03e1..556fd07c 100644 --- a/src/automation/runner.rs +++ b/src/automation/runner.rs @@ -20,7 +20,8 @@ use super::managed_skills::list_managed_skills; use super::run_ledger::{AutomationRunLedgerRecord, AutomationTrigger}; use super::session_reflector::validate_fact_proposals; use super::skill_usage::{ - ingest_project_analytics_events, stale_skill_recommendations, summarize_skill_usage, + ingest_project_analytics_events, skill_overlap_candidates, stale_skill_recommendations, + summarize_skill_usage, DEFAULT_SKILL_OVERLAP_LIMIT, }; use super::skill_writer::{ activation_policy as skill_writer_activation_policy, skill_improvement_recommendations, @@ -539,39 +540,41 @@ async fn finalize_skill_writer_success( proposed_ops: &Value, proposals: &[Value], ) -> Result<(Value, AutomationRunLedgerRecord)> { - let (created_skills, updated_skills, rejected_skills) = - match validate_and_apply_skill_proposals( - profile_root, - run_id, - proposals, - config.auto_enable_skills, - ) - .await - { - Ok(result) => result, - Err(err) => { - finalizer - .append_failed_record( - response.model.clone(), - evidence_hash, - Some(proposed_ops.clone()), - err.to_string(), - ) - .await?; - return Err(err); - } - }; - let accepted_count = created_skills.len() + updated_skills.len(); - let rejected_count = rejected_skills.len(); + let proposal_outcome = match validate_and_apply_skill_proposals( + profile_root, + run_id, + proposals, + config.auto_enable_skills, + ) + .await + { + Ok(result) => result, + Err(err) => { + finalizer + .append_failed_record( + response.model.clone(), + evidence_hash, + Some(proposed_ops.clone()), + err.to_string(), + ) + .await?; + return Err(err); + } + }; + let accepted_count = proposal_outcome.created.len() + + proposal_outcome.updated.len() + + proposal_outcome.consolidations.len(); + let rejected_count = proposal_outcome.rejected.len(); let report = json!({ "status": if config.auto_enable_skills { "auto_enabled" } else { "needs_approval" }, "dry_run": true, "task": "skill_writer", "evidence_hash": evidence_hash, "activation_policy": activation_policy, - "created_skills": created_skills, - "updated_skills": updated_skills, - "rejected_skills": rejected_skills, + "created_skills": proposal_outcome.created, + "updated_skills": proposal_outcome.updated, + "staged_consolidations": proposal_outcome.consolidations, + "rejected_skills": proposal_outcome.rejected, "skill_improvement_recommendations": evidence .get("skill_improvement_recommendations") .cloned() @@ -587,6 +590,7 @@ async fn finalize_skill_writer_success( "skills": proposed_ops.get("skills").cloned().unwrap_or_else(|| json!([])), "created_skills": report.get("created_skills").cloned().unwrap_or_else(|| json!([])), "updated_skills": report.get("updated_skills").cloned().unwrap_or_else(|| json!([])), + "staged_consolidations": report.get("staged_consolidations").cloned().unwrap_or_else(|| json!([])), "rejected_skills": report.get("rejected_skills").cloned().unwrap_or_else(|| json!([])), })), accepted_count, @@ -595,6 +599,7 @@ async fn finalize_skill_writer_success( record.applied_ops = Some(json!({ "created_skills": report.get("created_skills").cloned().unwrap_or_else(|| json!([])), "updated_skills": report.get("updated_skills").cloned().unwrap_or_else(|| json!([])), + "staged_consolidations": report.get("staged_consolidations").cloned().unwrap_or_else(|| json!([])), })); record.rejected_ops = report.get("rejected_skills").cloned(); record.validation_report = Some(json!({ @@ -788,11 +793,14 @@ async fn build_skill_writer_evidence( })) }) .unwrap_or_default(); + let overlap_candidates = + skill_overlap_candidates(&existing_skills, DEFAULT_SKILL_OVERLAP_LIMIT); let skill_improvement_recommendations = skill_improvement_recommendations( &hits, &skill_usage_summaries, &stale_recommendations, &underused_tool_families, + &overlap_candidates, ); let evidence = json!({ "storage_scope": storage_scope, @@ -803,6 +811,7 @@ async fn build_skill_writer_evidence( "skill_usage_summaries": skill_usage_summaries, "stale_recommendations": stale_recommendations, "underused_tool_families": underused_tool_families, + "skill_overlap_candidates": overlap_candidates, "skill_improvement_recommendations": skill_improvement_recommendations, "existing_managed_skills": existing_skills .iter() @@ -1275,7 +1284,7 @@ fn build_skill_writer_prompt(evidence: &Value) -> String { "\n", "An empty skills array is a real option when the session ran smoothly with no corrections and produced no new technique, but do not reach for it as a default.\n", "\n", - "Response contract: Return only JSON with a skills array of managed skill creates or updates. New skills may omit action or use action=create and must include id, title, summary, category, body_markdown, optional targets, optional support_files with text content, and reason. Targets, when present, must be an array using cursor, codex, claude, agents, opencode, kimi, or kiro; Hermes is host-owned and must not be targeted. Updates must use action=update or action=patch, include id and base_checksum, and include at least one changed field among title, summary, category, targets, body_markdown/body, support_files, or pinned. For updates, support_files is a complete replacement list, not a partial file patch. Activation is controlled only by the runner policy; do not assume activation from your response.\n", + "Response contract: Return only JSON with a skills array of managed skill creates or updates. New skills may omit action or use action=create and must include id, title, summary, category, body_markdown, optional targets, optional support_files with text content, and reason. Targets, when present, must be an array using cursor, codex, claude, agents, opencode, kimi, or kiro; Hermes is host-owned and must not be targeted. Updates must use action=update or action=patch, include id and base_checksum, and include at least one changed field among title, summary, category, targets, body_markdown/body, support_files, or pinned. For updates, support_files is a complete replacement list, not a partial file patch. Consolidations: when skill_overlap_candidates shows overlapping managed skills, you may propose action=merge (include id for the surviving skill, base_checksum, source_skill_id, source_base_checksum, reason, and optional merged title/summary/category/targets/body_markdown/support_files) or action=archive (include id, base_checksum, reason). Consolidations are always staged for human approval and archive-only; content is never deleted. Never propose merge or archive for pinned or user-authored skills. Activation is controlled only by the runner policy; do not assume activation from your response.\n", ); format!( "{POLICY}{}", diff --git a/src/automation/skill_usage.rs b/src/automation/skill_usage.rs index f44c5b05..9f1e18ca 100644 --- a/src/automation/skill_usage.rs +++ b/src/automation/skill_usage.rs @@ -8,10 +8,15 @@ use crate::errors::{Result, TraceDecayError}; use crate::tracedecay::current_timestamp; mod analytics; +mod overlap; mod recommendations; pub(crate) use analytics::analytics_import_key_for_request; pub use analytics::{ingest_analytics_events, ingest_project_analytics_events}; +pub use overlap::{ + skill_overlap_candidates, SkillOverlapCandidate, DEFAULT_SKILL_OVERLAP_LIMIT, + SKILL_OVERLAP_CONTENT_THRESHOLD, SKILL_OVERLAP_TITLE_THRESHOLD, +}; pub use recommendations::{skill_improvement_recommendations, stale_skill_recommendations}; const SKILL_USAGE_LEDGER_FILENAME: &str = "skill_usage.json"; diff --git a/src/automation/skill_usage/overlap.rs b/src/automation/skill_usage/overlap.rs new file mode 100644 index 00000000..cd586040 --- /dev/null +++ b/src/automation/skill_usage/overlap.rs @@ -0,0 +1,230 @@ +//! Pairwise overlap detection between managed skills, used by the +//! `skill_writer` evidence bundle to surface consolidation candidates +//! (Hermes curator parity). Detection is purely lexical (token Jaccard on +//! normalized titles and bodies) and deterministic; it never mutates skills. + +use serde::{Deserialize, Serialize}; + +use super::super::managed_skills::{ManagedSkill, ManagedSkillState}; +use crate::memory::similarity::lexical_overlap; + +/// Minimum content (title+summary+body) token Jaccard for a pair to count as +/// a consolidation candidate on its own. +pub const SKILL_OVERLAP_CONTENT_THRESHOLD: f64 = 0.35; +/// Title-token Jaccard that flags a pair when combined with a weaker content +/// overlap floor. +pub const SKILL_OVERLAP_TITLE_THRESHOLD: f64 = 0.5; +/// Content overlap floor that must accompany a high title overlap. +pub const SKILL_OVERLAP_TITLE_CONTENT_FLOOR: f64 = 0.2; +/// Default cap on how many candidate pairs are surfaced as evidence. +pub const DEFAULT_SKILL_OVERLAP_LIMIT: usize = 5; + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct SkillOverlapCandidate { + pub skill_a: String, + pub skill_b: String, + pub skill_a_pinned: bool, + pub skill_b_pinned: bool, + pub title_overlap: f64, + pub content_overlap: f64, + pub score: f64, + pub shared_tokens: Vec, + pub recommendation: String, + pub reason: String, +} + +/// Computes the top overlapping managed-skill pairs above the lexical +/// thresholds. Archived and disabled skills are ignored; pairs where either +/// skill is pinned are skipped entirely (pinned skills are exempt from +/// consolidation, matching the Hermes curator). +pub fn skill_overlap_candidates( + skills: &[ManagedSkill], + limit: usize, +) -> Vec { + let eligible: Vec<&ManagedSkill> = skills + .iter() + .filter(|skill| { + matches!( + skill.metadata.state, + ManagedSkillState::Active | ManagedSkillState::PendingApproval + ) + }) + .collect(); + let mut candidates = Vec::new(); + for (index, a) in eligible.iter().enumerate() { + for b in eligible.iter().skip(index + 1) { + if a.metadata.pinned || b.metadata.pinned { + continue; + } + if let Some(candidate) = overlap_candidate(a, b) { + candidates.push(candidate); + } + } + } + candidates.sort_by(|a, b| { + b.score + .partial_cmp(&a.score) + .unwrap_or(std::cmp::Ordering::Equal) + .then_with(|| a.skill_a.cmp(&b.skill_a)) + .then_with(|| a.skill_b.cmp(&b.skill_b)) + }); + candidates.truncate(limit); + candidates +} + +fn overlap_candidate(a: &ManagedSkill, b: &ManagedSkill) -> Option { + let (_, title_overlap, _) = lexical_overlap(&a.metadata.title, &b.metadata.title); + let (content_payload, content_overlap, _) = + lexical_overlap(&skill_content_text(a), &skill_content_text(b)); + let overlapping = content_overlap >= SKILL_OVERLAP_CONTENT_THRESHOLD + || (title_overlap >= SKILL_OVERLAP_TITLE_THRESHOLD + && content_overlap >= SKILL_OVERLAP_TITLE_CONTENT_FLOOR); + if !overlapping { + return None; + } + let shared_tokens = content_payload + .get("shared_tokens") + .and_then(serde_json::Value::as_array) + .map(|tokens| { + tokens + .iter() + .filter_map(|token| token.as_str().map(str::to_string)) + .collect() + }) + .unwrap_or_default(); + Some(SkillOverlapCandidate { + skill_a: a.metadata.id.clone(), + skill_b: b.metadata.id.clone(), + skill_a_pinned: a.metadata.pinned, + skill_b_pinned: b.metadata.pinned, + title_overlap, + content_overlap, + score: content_overlap.max(title_overlap), + shared_tokens, + recommendation: "merge_or_archive_review".to_string(), + reason: format!( + "managed skills '{}' and '{}' share {:.0}% of normalized content tokens ({:.0}% of title tokens)", + a.metadata.id, + b.metadata.id, + content_overlap * 100.0, + title_overlap * 100.0 + ), + }) +} + +fn skill_content_text(skill: &ManagedSkill) -> String { + format!( + "{}\n{}\n{}", + skill.metadata.title, skill.metadata.summary, skill.body_markdown + ) +} + +#[cfg(test)] +mod tests { + use super::super::super::managed_skills::{ + default_managed_skill_targets, ManagedSkillDraft, ManagedSkillProvenance, + ManagedSkillSource, + }; + use super::*; + + fn skill(id: &str, title: &str, summary: &str, body: &str) -> ManagedSkill { + let draft = ManagedSkillDraft { + id: id.to_string(), + title: title.to_string(), + summary: summary.to_string(), + category: "workflow".to_string(), + targets: default_managed_skill_targets(), + body_markdown: body.to_string(), + support_files: Vec::new(), + provenance: ManagedSkillProvenance { + source: ManagedSkillSource::AutomationRun, + actor: "test".to_string(), + run_id: None, + }, + }; + match draft.materialize() { + Ok(skill) => skill, + Err(err) => panic!("test fixture skill should materialize: {err}"), + } + } + + fn overlapping_pair() -> (ManagedSkill, ManagedSkill) { + ( + skill( + "review-automation-runs", + "Review automation runs", + "Review automation run ledgers before approving changes.", + "Check run ledger counts, rejected proposals, and pending approval state before applying automation changes.", + ), + skill( + "automation-run-review", + "Automation run review", + "Review automation run ledgers and approval gates.", + "Check run ledger counts, rejected proposals, and approval gates before applying automation changes.", + ), + ) + } + + fn unrelated_skill() -> ManagedSkill { + skill( + "rust-error-handling", + "Rust error handling", + "Prefer thiserror enums over anyhow in library crates.", + "Model failures with typed enums. Reserve panics for invariants. Convert io errors at module boundaries.", + ) + } + + #[test] + fn detects_overlapping_pairs_above_threshold() { + let (a, b) = overlapping_pair(); + let unrelated = unrelated_skill(); + let candidates = skill_overlap_candidates(&[a, b, unrelated], DEFAULT_SKILL_OVERLAP_LIMIT); + + assert_eq!(candidates.len(), 1); + let candidate = &candidates[0]; + assert_eq!(candidate.skill_a, "review-automation-runs"); + assert_eq!(candidate.skill_b, "automation-run-review"); + assert!(candidate.content_overlap >= SKILL_OVERLAP_CONTENT_THRESHOLD); + assert_eq!(candidate.recommendation, "merge_or_archive_review"); + assert!(!candidate.shared_tokens.is_empty()); + } + + #[test] + fn skips_pairs_where_either_skill_is_pinned() { + let (mut a, mut b) = overlapping_pair(); + a.set_pinned(true); + assert!( + skill_overlap_candidates(&[a.clone(), b.clone()], DEFAULT_SKILL_OVERLAP_LIMIT) + .is_empty() + ); + + a.set_pinned(false); + b.set_pinned(true); + assert!(skill_overlap_candidates(&[a, b], DEFAULT_SKILL_OVERLAP_LIMIT).is_empty()); + } + + #[test] + fn ignores_archived_and_disabled_skills() { + let (mut a, b) = overlapping_pair(); + a.set_state(ManagedSkillState::Archived); + assert!( + skill_overlap_candidates(&[a.clone(), b.clone()], DEFAULT_SKILL_OVERLAP_LIMIT) + .is_empty() + ); + a.set_state(ManagedSkillState::Disabled); + assert!(skill_overlap_candidates(&[a, b], DEFAULT_SKILL_OVERLAP_LIMIT).is_empty()); + } + + #[test] + fn respects_the_candidate_limit() { + let (a, b) = overlapping_pair(); + let c = skill( + "automation-run-checks", + "Automation run checks", + "Review automation run ledgers before approving changes.", + "Check run ledger counts, rejected proposals, and pending approval state before applying automation changes.", + ); + let candidates = skill_overlap_candidates(&[a, b, c], 1); + assert_eq!(candidates.len(), 1); + } +} diff --git a/src/automation/skill_writer.rs b/src/automation/skill_writer.rs index fa5d6135..d7313864 100644 --- a/src/automation/skill_writer.rs +++ b/src/automation/skill_writer.rs @@ -7,25 +7,38 @@ use super::artifacts::sha256_bytes; use super::config::AutomationConfig; use super::managed_skills::{ approve_managed_skill, create_managed_skill_draft, default_managed_skill_targets, - list_managed_skills, stage_managed_skill_update, ManagedSkill, ManagedSkillDraft, - ManagedSkillProvenance, ManagedSkillSource, ManagedSkillUpdate, ManagedSupportFile, + discard_pending_managed_skill_update, list_managed_skills, stage_managed_skill_archive, + stage_managed_skill_update, ManagedSkill, ManagedSkillDraft, ManagedSkillProvenance, + ManagedSkillSource, ManagedSkillState, ManagedSkillUpdate, ManagedSupportFile, SkillInstallTarget, }; use super::skill_usage::{ skill_improvement_recommendations as usage_skill_improvement_recommendations, - SkillStaleRecommendation, SkillUsageSummary, + SkillOverlapCandidate, SkillStaleRecommendation, SkillUsageSummary, }; use super::text::truncate_chars_for_prompt; use crate::analytics::ToolFamilySignal; use crate::errors::Result; use crate::sessions::lcm::LcmGrepHit; +/// Outcome of validating and applying one batch of `skill_writer` proposals. +/// `consolidations` holds staged merge/archive records; consolidations are +/// always staged for dashboard approval and never auto-applied, even when +/// `auto_enable_skills` is set. +#[derive(Debug, Default)] +pub(crate) struct SkillProposalOutcome { + pub created: Vec, + pub updated: Vec, + pub consolidations: Vec, + pub rejected: Vec, +} + pub(crate) async fn validate_and_apply_skill_proposals( profile_root: &Path, run_id: &str, proposals: &[Value], auto_enable_skills: bool, -) -> Result<(Vec, Vec, Vec)> { +) -> Result { let mut existing_skills = list_managed_skills(profile_root) .await? .into_iter() @@ -34,6 +47,7 @@ pub(crate) async fn validate_and_apply_skill_proposals( let mut existing_ids = existing_skills.keys().cloned().collect::>(); let mut created = Vec::new(); let mut updated = Vec::new(); + let mut consolidations = Vec::new(); let mut rejected = Vec::new(); for proposal in proposals { match skill_proposal_action(proposal) { @@ -90,10 +104,65 @@ pub(crate) async fn validate_and_apply_skill_proposals( Err(reason) => rejected.push(rejected_skill(proposal, &reason)), } } + Ok(SkillProposalAction::Archive) => { + match skill_archive_from_proposal(proposal, &existing_skills) { + Ok(archive) => { + match stage_managed_skill_archive( + profile_root, + &archive.skill_id, + &archive.base_checksum, + Some(archive.reason.clone()), + ) + .await + { + Ok(skill) => { + existing_skills.insert(archive.skill_id.clone(), skill.clone()); + consolidations.push(staged_consolidation_record( + SkillProposalAction::Archive, + proposal, + &skill, + &archive.base_checksum, + None, + )); + } + Err(err) => rejected.push(rejected_skill(proposal, &err.to_string())), + } + } + Err(reason) => rejected.push(rejected_skill(proposal, &reason)), + } + } + Ok(SkillProposalAction::Merge) => { + match skill_merge_from_proposal(proposal, &existing_skills) { + Ok(merge) => match stage_skill_merge(profile_root, &merge).await { + Ok((source_skill, target_skill)) => { + existing_skills + .insert(merge.source_skill_id.clone(), source_skill.clone()); + if let Some(target_skill) = &target_skill { + existing_skills + .insert(merge.target_skill_id.clone(), target_skill.clone()); + } + consolidations.push(staged_consolidation_record( + SkillProposalAction::Merge, + proposal, + &source_skill, + &merge.source_base_checksum, + Some(&merge), + )); + } + Err(err) => rejected.push(rejected_skill(proposal, &err.to_string())), + }, + Err(reason) => rejected.push(rejected_skill(proposal, &reason)), + } + } Err(reason) => rejected.push(rejected_skill(proposal, &reason)), } } - Ok((created, updated, rejected)) + Ok(SkillProposalOutcome { + created, + updated, + consolidations, + rejected, + }) } pub(crate) fn activation_policy(config: &AutomationConfig) -> &'static str { @@ -123,9 +192,29 @@ pub(crate) fn skill_improvement_recommendations( usage_summaries: &[SkillUsageSummary], stale_recommendations: &[SkillStaleRecommendation], underused_tool_families: &[ToolFamilySignal], + overlap_candidates: &[SkillOverlapCandidate], ) -> Vec { let mut recommendations = Vec::new(); + for candidate in overlap_candidates { + recommendations.push(json!({ + "id": format!("skill_overlap:{}:{}", candidate.skill_a, candidate.skill_b), + "kind": "managed_skill_consolidation", + "priority": if candidate.score >= 0.6 { "high" } else { "medium" }, + "skill_a": candidate.skill_a, + "skill_b": candidate.skill_b, + "recommendation": candidate.recommendation, + "reason": candidate.reason, + "evidence": [ + format!("content_overlap={:.4}", candidate.content_overlap), + format!("title_overlap={:.4}", candidate.title_overlap), + format!("shared_tokens={}", candidate.shared_tokens.join(",")), + format!("pinned={}/{}", candidate.skill_a_pinned, candidate.skill_b_pinned), + ], + "source": "skill_overlap_detection", + })); + } + for recommendation in usage_skill_improvement_recommendations(usage_summaries) .into_iter() .filter(|recommendation| recommendation.improvement) @@ -340,6 +429,8 @@ fn skill_draft_from_proposal( enum SkillProposalAction { Create, Update, + Merge, + Archive, } impl SkillProposalAction { @@ -347,6 +438,8 @@ impl SkillProposalAction { match self { SkillProposalAction::Create => "create", SkillProposalAction::Update => "update", + SkillProposalAction::Merge => "merge", + SkillProposalAction::Archive => "archive", } } } @@ -361,10 +454,254 @@ fn skill_proposal_action(proposal: &Value) -> std::result::Result Ok(SkillProposalAction::Create), "update" | "patch" => Ok(SkillProposalAction::Update), + "merge" | "consolidate" => Ok(SkillProposalAction::Merge), + "archive" => Ok(SkillProposalAction::Archive), other => Err(format!("unsupported skill proposal action '{other}'")), } } +#[derive(Debug, Clone)] +struct SkillArchiveProposal { + skill_id: String, + base_checksum: String, + reason: String, +} + +#[derive(Debug, Clone)] +struct SkillMergeProposal { + target_skill_id: String, + base_checksum: String, + source_skill_id: String, + source_base_checksum: String, + reason: String, + update: Option, +} + +/// Shared safety gate for consolidation (merge/archive) targets: the skill +/// must exist, its checksum must match, it must not be pinned (Hermes-parity +/// pinned exemption), must not be user-authored, must not already be +/// archived, and must not already carry a staged update. +fn consolidation_guard<'a>( + existing_skills: &'a BTreeMap, + id: &str, + base_checksum: &str, + role: &str, +) -> std::result::Result<&'a ManagedSkill, String> { + let skill = existing_skills + .get(id) + .ok_or_else(|| format!("{role} managed skill id '{id}' does not exist"))?; + if base_checksum != skill.metadata.checksum { + return Err(format!( + "base_checksum for managed skill id '{id}' is stale" + )); + } + if skill.metadata.pinned { + return Err(format!( + "managed skill '{id}' is pinned and exempt from consolidation" + )); + } + if skill.metadata.provenance.source == ManagedSkillSource::UserDraft { + return Err(format!( + "managed skill '{id}' is user-authored and exempt from consolidation" + )); + } + if skill.metadata.state == ManagedSkillState::Archived { + return Err(format!("managed skill '{id}' is already archived")); + } + if skill.pending_update.is_some() { + return Err(format!("managed skill '{id}' already has a pending update")); + } + Ok(skill) +} + +fn skill_archive_from_proposal( + proposal: &Value, + existing_skills: &BTreeMap, +) -> std::result::Result { + let object = proposal + .as_object() + .ok_or_else(|| "proposal must be a JSON object".to_string())?; + let id = required_proposal_string(object.get("id"), "id")?; + let base_checksum = required_proposal_string(object.get("base_checksum"), "base_checksum")?; + let reason = required_proposal_string(object.get("reason"), "reason")?; + consolidation_guard(existing_skills, &id, &base_checksum, "archive")?; + Ok(SkillArchiveProposal { + skill_id: id, + base_checksum, + reason, + }) +} + +fn skill_merge_from_proposal( + proposal: &Value, + existing_skills: &BTreeMap, +) -> std::result::Result { + let object = proposal + .as_object() + .ok_or_else(|| "proposal must be a JSON object".to_string())?; + let target_skill_id = required_proposal_string(object.get("id"), "id")?; + let base_checksum = required_proposal_string(object.get("base_checksum"), "base_checksum")?; + let source_skill_id = + required_proposal_string(object.get("source_skill_id"), "source_skill_id")?; + let source_base_checksum = + required_proposal_string(object.get("source_base_checksum"), "source_base_checksum")?; + let reason = required_proposal_string(object.get("reason"), "reason")?; + if source_skill_id == target_skill_id { + return Err("merge proposal source_skill_id must differ from id".to_string()); + } + let target = consolidation_guard(existing_skills, &target_skill_id, &base_checksum, "merge")?; + consolidation_guard( + existing_skills, + &source_skill_id, + &source_base_checksum, + "merge source", + )?; + + let update = ManagedSkillUpdate { + title: optional_proposal_string(object.get("title"))?, + summary: optional_proposal_string(object.get("summary"))?, + category: optional_proposal_string(object.get("category"))?, + targets: optional_proposal_targets(object.get("targets"))?, + body_markdown: optional_proposal_string( + object.get("body_markdown").or_else(|| object.get("body")), + )?, + support_files: if object.contains_key("support_files") { + Some(support_files_from_proposal(object.get("support_files"))?) + } else { + None + }, + pinned: None, + }; + let has_update = update.title.is_some() + || update.summary.is_some() + || update.category.is_some() + || update.targets.is_some() + || update.body_markdown.is_some() + || update.support_files.is_some(); + if has_update && !merge_update_changes_target(&update, target) { + return Err(format!( + "merge proposal does not change managed skill id '{target_skill_id}'" + )); + } + Ok(SkillMergeProposal { + target_skill_id, + base_checksum, + source_skill_id, + source_base_checksum, + reason, + update: has_update.then_some(update), + }) +} + +fn merge_update_changes_target(update: &ManagedSkillUpdate, target: &ManagedSkill) -> bool { + update + .title + .as_ref() + .is_some_and(|title| target.metadata.title != *title) + || update + .summary + .as_ref() + .is_some_and(|summary| target.metadata.summary != *summary) + || update + .category + .as_ref() + .is_some_and(|category| target.metadata.category != *category) + || update + .targets + .as_ref() + .is_some_and(|targets| target.metadata.targets != *targets) + || update + .body_markdown + .as_ref() + .is_some_and(|body| target.body_markdown != *body) + || update + .support_files + .as_ref() + .is_some_and(|support_files| target.support_files != *support_files) +} + +/// Stages a merge: an optional content update on the surviving skill plus an +/// archive on the absorbed skill. Both land as pending updates for dashboard +/// approval; nothing is auto-applied and no skill content is deleted. If the +/// archive staging fails after the target update was staged, the target +/// staging is rolled back so a merge never half-lands. +async fn stage_skill_merge( + profile_root: &Path, + merge: &SkillMergeProposal, +) -> Result<(ManagedSkill, Option)> { + let staged_target = match &merge.update { + Some(update) => Some( + stage_managed_skill_update( + profile_root, + &merge.target_skill_id, + &merge.base_checksum, + update.clone(), + ) + .await?, + ), + None => None, + }; + let archive_reason = format!("merged into '{}': {}", merge.target_skill_id, merge.reason); + let staged_source = match stage_managed_skill_archive( + profile_root, + &merge.source_skill_id, + &merge.source_base_checksum, + Some(archive_reason), + ) + .await + { + Ok(skill) => skill, + Err(err) => { + if staged_target.is_some() { + discard_pending_managed_skill_update(profile_root, &merge.target_skill_id).await?; + } + return Err(err); + } + }; + Ok((staged_source, staged_target)) +} + +fn staged_consolidation_record( + action: SkillProposalAction, + proposal: &Value, + staged_source: &ManagedSkill, + archive_base_checksum: &str, + merge: Option<&SkillMergeProposal>, +) -> Value { + let reason = proposal.get("reason").cloned().unwrap_or(Value::Null); + let mut record = json!({ + "action": action.as_str(), + "proposal_action": action.as_str(), + "reason": reason.clone(), + "proposal_reason": reason, + "approval_status": "staged_consolidation", + "resulting_state": "archived", + "archived_skill_id": staged_source.metadata.id, + }); + if let Some(object) = record.as_object_mut() { + if let Some(merge) = merge { + object.insert("target_skill_id".to_string(), json!(merge.target_skill_id)); + object.insert("base_checksum".to_string(), json!(merge.base_checksum)); + object.insert("source_skill_id".to_string(), json!(merge.source_skill_id)); + object.insert( + "source_base_checksum".to_string(), + json!(merge.source_base_checksum), + ); + object.insert( + "target_update_staged".to_string(), + json!(merge.update.is_some()), + ); + } else { + object.insert( + "target_skill_id".to_string(), + json!(staged_source.metadata.id), + ); + object.insert("base_checksum".to_string(), json!(archive_base_checksum)); + } + } + record +} + fn skill_update_from_proposal( proposal: &Value, existing_skills: &BTreeMap, @@ -552,6 +889,7 @@ fn normalized_non_empty(value: &str) -> Option { #[cfg(test)] mod tests { + use super::super::managed_skills::ManagedSkillPendingUpdate; use super::*; fn assert_err_eq(result: std::result::Result, expected: &str) { @@ -561,6 +899,302 @@ mod tests { } } + fn assert_ok(result: std::result::Result) -> T { + match result { + Ok(value) => value, + Err(err) => panic!("expected ok, got error: {err}"), + } + } + + fn fixture_skill(id: &str, source: ManagedSkillSource, pinned: bool) -> ManagedSkill { + let draft = ManagedSkillDraft { + id: id.to_string(), + title: format!("{id} guidance"), + summary: format!("Guidance for {id}."), + category: "workflow".to_string(), + targets: default_managed_skill_targets(), + body_markdown: format!("Follow the {id} workflow before applying changes."), + support_files: Vec::new(), + provenance: ManagedSkillProvenance { + source, + actor: "test".to_string(), + run_id: None, + }, + }; + let mut skill = match draft.materialize() { + Ok(skill) => skill, + Err(err) => panic!("test fixture skill should materialize: {err}"), + }; + skill.set_state(ManagedSkillState::Active); + skill.set_pinned(pinned); + skill + } + + fn consolidation_fixture() -> BTreeMap { + let mut archived = + fixture_skill("archived-skill", ManagedSkillSource::AutomationRun, false); + archived.set_state(ManagedSkillState::Archived); + [ + fixture_skill("workflow-a", ManagedSkillSource::AutomationRun, false), + fixture_skill("workflow-b", ManagedSkillSource::AutomationRun, false), + fixture_skill("pinned-skill", ManagedSkillSource::AutomationRun, true), + fixture_skill("user-skill", ManagedSkillSource::UserDraft, false), + archived, + ] + .into_iter() + .map(|skill| (skill.metadata.id.clone(), skill)) + .collect() + } + + fn checksum(skills: &BTreeMap, id: &str) -> String { + skills[id].metadata.checksum.clone() + } + + #[test] + fn archive_proposals_validate_ids_checksums_and_exemptions() { + let skills = consolidation_fixture(); + let valid = assert_ok(skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "reason": "unused overlap" + }), + &skills, + )); + assert_eq!(valid.skill_id, "workflow-a"); + assert_eq!(valid.reason, "unused overlap"); + + assert_err_eq( + skill_archive_from_proposal( + &json!({"action": "archive", "id": "workflow-a", "reason": "x"}), + &skills, + ), + "base_checksum is required", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a") + }), + &skills, + ), + "reason is required", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "missing", + "base_checksum": "sha256:0000", + "reason": "x" + }), + &skills, + ), + "archive managed skill id 'missing' does not exist", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": "sha256:stale", + "reason": "x" + }), + &skills, + ), + "base_checksum for managed skill id 'workflow-a' is stale", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "pinned-skill", + "base_checksum": checksum(&skills, "pinned-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'pinned-skill' is pinned and exempt from consolidation", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "user-skill", + "base_checksum": checksum(&skills, "user-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'user-skill' is user-authored and exempt from consolidation", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "archived-skill", + "base_checksum": checksum(&skills, "archived-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'archived-skill' is already archived", + ); + } + + #[test] + fn archive_proposals_reject_targets_with_pending_updates() { + let mut skills = consolidation_fixture(); + let pending_metadata = skills["workflow-a"].metadata.clone(); + if let Some(skill) = skills.get_mut("workflow-a") { + skill.pending_update = Some(ManagedSkillPendingUpdate { + base_checksum: pending_metadata.checksum.clone(), + staged_at: 1, + metadata: pending_metadata, + body_markdown: "staged body".to_string(), + support_files: Vec::new(), + resulting_state: None, + staged_reason: None, + }); + } + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "reason": "x" + }), + &skills, + ), + "managed skill 'workflow-a' already has a pending update", + ); + } + + #[test] + fn merge_proposals_validate_source_target_and_checksums() { + let skills = consolidation_fixture(); + let merge = assert_ok(skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "body_markdown": "Merged workflow guidance covering both variants.", + "reason": "duplicate guidance" + }), + &skills, + )); + assert_eq!(merge.target_skill_id, "workflow-a"); + assert_eq!(merge.source_skill_id, "workflow-b"); + assert!(merge.update.is_some()); + + let archive_only = assert_ok(skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "reason": "target already covers the source" + }), + &skills, + )); + assert!(archive_only.update.is_none()); + + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-a", + "source_base_checksum": checksum(&skills, "workflow-a"), + "reason": "x" + }), + &skills, + ), + "merge proposal source_skill_id must differ from id", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": "sha256:stale", + "reason": "x" + }), + &skills, + ), + "base_checksum for managed skill id 'workflow-b' is stale", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "pinned-skill", + "base_checksum": checksum(&skills, "pinned-skill"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "reason": "x" + }), + &skills, + ), + "managed skill 'pinned-skill' is pinned and exempt from consolidation", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "pinned-skill", + "source_base_checksum": checksum(&skills, "pinned-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'pinned-skill' is pinned and exempt from consolidation", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "body_markdown": skills["workflow-a"].body_markdown, + "reason": "x" + }), + &skills, + ), + "merge proposal does not change managed skill id 'workflow-a'", + ); + } + + #[test] + fn consolidation_actions_parse_from_proposals() { + assert_eq!( + assert_ok(skill_proposal_action(&json!({"action": "merge"}))), + SkillProposalAction::Merge + ); + assert_eq!( + assert_ok(skill_proposal_action(&json!({"action": "consolidate"}))), + SkillProposalAction::Merge + ); + assert_eq!( + assert_ok(skill_proposal_action(&json!({"action": "archive"}))), + SkillProposalAction::Archive + ); + } + #[test] fn proposal_targets_accept_known_aliases() -> std::result::Result<(), String> { let targets = optional_proposal_targets(Some(&json!([ diff --git a/tests/agent_suite/managed_skills_test.rs b/tests/agent_suite/managed_skills_test.rs index ac19e7b3..1fe41145 100644 --- a/tests/agent_suite/managed_skills_test.rs +++ b/tests/agent_suite/managed_skills_test.rs @@ -2,9 +2,10 @@ use tracedecay::automation::managed_skills::{ approve_managed_skill, archive_managed_skill, create_managed_skill_draft, disable_managed_skill, discard_pending_managed_skill_update, list_managed_skills, load_managed_skill, managed_skill_dir, restore_managed_skill, save_managed_skill, - set_managed_skill_state, stage_managed_skill_update, update_managed_skill, ManagedSkillDraft, - ManagedSkillProvenance, ManagedSkillSource, ManagedSkillState, ManagedSkillUpdate, - ManagedSupportFile, SkillInstallTarget, MAX_MANAGED_SKILL_BODY_BYTES, + set_managed_skill_state, stage_managed_skill_archive, stage_managed_skill_update, + update_managed_skill, ManagedSkillDraft, ManagedSkillProvenance, ManagedSkillSource, + ManagedSkillState, ManagedSkillUpdate, ManagedSupportFile, SkillInstallTarget, + MAX_MANAGED_SKILL_BODY_BYTES, }; use tracedecay::automation::skill_usage::{ ingest_analytics_events, load_skill_usage_records, record_skill_usage, @@ -837,6 +838,142 @@ async fn staged_managed_skill_update_preserves_active_revision_until_approval() assert_eq!(reloaded.metadata.checksum, second_base); } +#[tokio::test] +async fn staged_managed_skill_archive_keeps_content_until_approval() { + let temp = tempfile::tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + create_managed_skill_draft(&profile_root, draft()) + .await + .unwrap(); + let active = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + let base_checksum = active.metadata.checksum.clone(); + let skill_dir = managed_skill_dir(&profile_root, "repo-hygiene").unwrap(); + + let staged = stage_managed_skill_archive( + &profile_root, + "repo-hygiene", + &base_checksum, + Some("overlaps with newer guidance".to_string()), + ) + .await + .unwrap(); + assert_eq!(staged.metadata.state, ManagedSkillState::PendingApproval); + + // Staging must not touch the active revision. + let with_pending = load_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(with_pending.metadata.state, ManagedSkillState::Active); + assert_eq!(with_pending.metadata.checksum, base_checksum); + let pending = with_pending.pending_update.as_ref().unwrap(); + assert_eq!(pending.resulting_state, Some(ManagedSkillState::Archived)); + assert_eq!( + pending.staged_reason.as_deref(), + Some("overlaps with newer guidance") + ); + + // Approval archives without deleting any content. + let approved = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(approved.metadata.state, ManagedSkillState::Archived); + assert_eq!(approved.body_markdown, active.body_markdown); + assert!(approved.pending_update.is_none()); + assert!(skill_dir.join("SKILL.md").is_file()); + assert!(skill_dir.join("references/checklist.md").is_file()); + let reloaded = load_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(reloaded.metadata.state, ManagedSkillState::Archived); + assert_eq!(reloaded.body_markdown, active.body_markdown); +} + +#[tokio::test] +async fn staged_managed_skill_archive_can_be_discarded() { + let temp = tempfile::tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + create_managed_skill_draft(&profile_root, draft()) + .await + .unwrap(); + let active = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + stage_managed_skill_archive( + &profile_root, + "repo-hygiene", + &active.metadata.checksum, + None, + ) + .await + .unwrap(); + + let discarded = discard_pending_managed_skill_update(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert!(discarded.pending_update.is_none()); + let reloaded = load_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(reloaded.metadata.state, ManagedSkillState::Active); + assert_eq!(reloaded.metadata.checksum, active.metadata.checksum); +} + +#[tokio::test] +async fn staged_managed_skill_archive_rejects_pinned_stale_and_duplicates() { + let temp = tempfile::tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + create_managed_skill_draft(&profile_root, draft()) + .await + .unwrap(); + let mut active = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + let base_checksum = active.metadata.checksum.clone(); + + let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", "sha256:stale", None) + .await + .unwrap_err(); + assert!(err.to_string().contains("is stale")); + + active.set_pinned(true); + save_managed_skill(&profile_root, &active).await.unwrap(); + let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) + .await + .unwrap_err(); + assert!(err + .to_string() + .contains("pinned and exempt from staged archive")); + + active.set_pinned(false); + save_managed_skill(&profile_root, &active).await.unwrap(); + stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) + .await + .unwrap(); + let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) + .await + .unwrap_err(); + assert!(err.to_string().contains("already has a pending update")); + + // Already-archived skills cannot be staged for archive again. + discard_pending_managed_skill_update(&profile_root, "repo-hygiene") + .await + .unwrap(); + let archived = archive_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + let err = stage_managed_skill_archive( + &profile_root, + "repo-hygiene", + &archived.metadata.checksum, + None, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("is already archived")); +} + #[tokio::test] async fn staged_managed_skill_update_rejects_no_op_patch() { let temp = tempfile::tempdir().unwrap(); diff --git a/tests/automation_runner_test/skill_writer.rs b/tests/automation_runner_test/skill_writer.rs index 01dde018..af2bada6 100644 --- a/tests/automation_runner_test/skill_writer.rs +++ b/tests/automation_runner_test/skill_writer.rs @@ -719,6 +719,200 @@ async fn skill_writer_runner_updates_existing_skills_with_checksum_precondition( ); } +#[tokio::test] +async fn skill_writer_runner_stages_consolidations_without_auto_apply() { + let _env_lock = ENV_LOCK.lock().await; + let temp = tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + let cg = init_project(temp.path()).await; + seed_session_evidence(&cg).await; + let _global_db = isolate_global_db(&cg); + let automation_provenance = || ManagedSkillProvenance { + source: ManagedSkillSource::AutomationRun, + actor: "skill_writer".to_string(), + run_id: Some("run_seed".to_string()), + }; + create_managed_skill_draft( + &profile_root, + ManagedSkillDraft { + id: "automation-run-review".to_string(), + title: "Automation run review".to_string(), + summary: "Review automation run ledgers before approving changes.".to_string(), + category: "workflow".to_string(), + targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), + body_markdown: + "Check run ledger counts, rejected proposals, and pending approval state before applying automation changes." + .to_string(), + support_files: Vec::new(), + provenance: automation_provenance(), + }, + ) + .await + .unwrap(); + create_managed_skill_draft( + &profile_root, + ManagedSkillDraft { + id: "automation-run-checks".to_string(), + title: "Automation run checks".to_string(), + summary: "Review automation run ledgers and approval gates.".to_string(), + category: "workflow".to_string(), + targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), + body_markdown: + "Check run ledger counts, rejected proposals, and approval gates before applying automation changes." + .to_string(), + support_files: Vec::new(), + provenance: automation_provenance(), + }, + ) + .await + .unwrap(); + create_managed_skill_draft( + &profile_root, + ManagedSkillDraft { + id: "pinned-automation-guide".to_string(), + title: "Pinned deployment guide".to_string(), + summary: "Deployment rollback runbook kept pinned by the user.".to_string(), + category: "workflow".to_string(), + targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), + body_markdown: "Roll back with the canary checklist and notify the release channel." + .to_string(), + support_files: Vec::new(), + provenance: automation_provenance(), + }, + ) + .await + .unwrap(); + let target = approve_managed_skill(&profile_root, "automation-run-review") + .await + .unwrap(); + let source = approve_managed_skill(&profile_root, "automation-run-checks") + .await + .unwrap(); + let mut pinned = approve_managed_skill(&profile_root, "pinned-automation-guide") + .await + .unwrap(); + pinned.set_pinned(true); + tracedecay::automation::managed_skills::save_managed_skill(&profile_root, &pinned) + .await + .unwrap(); + + let backend = SkillJsonBackend::with_activation_policy( + json!({ + "skills": [ + { + "action": "merge", + "id": "automation-run-review", + "base_checksum": target.metadata.checksum, + "source_skill_id": "automation-run-checks", + "source_base_checksum": source.metadata.checksum, + "body_markdown": "Check run ledger counts, rejected proposals, approval gates, and pending approval state before applying automation changes.", + "reason": "The two run-review skills overlap almost completely." + }, + { + "action": "archive", + "id": "pinned-automation-guide", + "base_checksum": pinned.metadata.checksum, + "reason": "Pinned skills must be rejected." + } + ] + }), + "auto_enable_after_validation", + ); + // auto_enable_skills=true proves consolidations are still staged, never + // auto-applied. + let config = AutomationConfig { + auto_enable_skills: true, + ..enabled_skill_writer_config() + }; + + let run = run_skill_writer_with_backend( + &cg, + &config, + &backend, + manual_skill_writer_options(&profile_root), + ) + .await + .unwrap(); + + assert_eq!(backend.calls(), 1); + assert_eq!(run.ledger_record.status, AutomationRunStatus::Succeeded); + assert_eq!(run.ledger_record.accepted_count, 1); + assert_eq!(run.ledger_record.rejected_count, 1); + + let consolidation = &run.report["staged_consolidations"][0]; + assert_eq!(consolidation["action"], json!("merge")); + assert_eq!( + consolidation["approval_status"], + json!("staged_consolidation") + ); + assert_eq!( + consolidation["target_skill_id"], + json!("automation-run-review") + ); + assert_eq!( + consolidation["source_skill_id"], + json!("automation-run-checks") + ); + assert_eq!( + consolidation["archived_skill_id"], + json!("automation-run-checks") + ); + assert_eq!(consolidation["resulting_state"], json!("archived")); + assert_eq!(consolidation["target_update_staged"], json!(true)); + assert!(run.report["rejected_skills"][0]["reason"] + .as_str() + .is_some_and(|reason| reason.contains("pinned"))); + + // Overlap evidence surfaced the pair and recommended consolidation. + assert!(run.report["skill_improvement_recommendations"] + .as_array() + .is_some_and( + |recommendations| recommendations.iter().any(|recommendation| { + recommendation["kind"] == "managed_skill_consolidation" + && recommendation["source"] == "skill_overlap_detection" + }) + )); + + // Nothing auto-applied: both sides only carry staged pending updates. + let staged_source = load_managed_skill(&profile_root, "automation-run-checks") + .await + .unwrap(); + assert_eq!(staged_source.metadata.state, ManagedSkillState::Active); + let source_pending = staged_source.pending_update.as_ref().unwrap(); + assert_eq!( + source_pending.resulting_state, + Some(ManagedSkillState::Archived) + ); + assert!(source_pending + .staged_reason + .as_deref() + .is_some_and(|reason| reason.contains("merged into 'automation-run-review'"))); + let staged_target = load_managed_skill(&profile_root, "automation-run-review") + .await + .unwrap(); + assert_eq!(staged_target.metadata.state, ManagedSkillState::Active); + assert_eq!(staged_target.metadata.checksum, target.metadata.checksum); + assert!(staged_target.pending_update.is_some()); + let untouched_pinned = load_managed_skill(&profile_root, "pinned-automation-guide") + .await + .unwrap(); + assert_eq!(untouched_pinned.metadata.state, ManagedSkillState::Active); + assert!(untouched_pinned.pending_update.is_none()); + + // Approving the staged consolidation archives the source (content kept) + // and applies the merged body to the target. + let archived = approve_managed_skill(&profile_root, "automation-run-checks") + .await + .unwrap(); + assert_eq!(archived.metadata.state, ManagedSkillState::Archived); + assert_eq!(archived.body_markdown, source.body_markdown); + let merged = approve_managed_skill(&profile_root, "automation-run-review") + .await + .unwrap(); + assert_eq!(merged.metadata.state, ManagedSkillState::Active); + assert!(merged.body_markdown.contains("approval gates")); +} + #[tokio::test] async fn skill_writer_runner_ledgers_malformed_backend_output() { let _env_lock = ENV_LOCK.lock().await; From 1d4a0c5425ea6c1011bcd40d480e718c7e2bbad6 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Fri, 3 Jul 2026 00:20:11 +0000 Subject: [PATCH 2/2] refactor: split skill writer consolidation --- src/automation/skill_writer.rs | 553 +---------------- src/automation/skill_writer/consolidation.rs | 560 ++++++++++++++++++ tests/agent_suite/main.rs | 1 + .../agent_suite/managed_skill_archive_test.rs | 160 +++++ tests/agent_suite/managed_skills_test.rs | 143 +---- tests/automation_runner_test/main.rs | 1 + tests/automation_runner_test/skill_writer.rs | 194 ------ .../skill_writer_consolidation.rs | 189 ++++++ 8 files changed, 924 insertions(+), 877 deletions(-) create mode 100644 src/automation/skill_writer/consolidation.rs create mode 100644 tests/agent_suite/managed_skill_archive_test.rs create mode 100644 tests/automation_runner_test/skill_writer_consolidation.rs diff --git a/src/automation/skill_writer.rs b/src/automation/skill_writer.rs index d7313864..e3cc3a83 100644 --- a/src/automation/skill_writer.rs +++ b/src/automation/skill_writer.rs @@ -7,10 +7,9 @@ use super::artifacts::sha256_bytes; use super::config::AutomationConfig; use super::managed_skills::{ approve_managed_skill, create_managed_skill_draft, default_managed_skill_targets, - discard_pending_managed_skill_update, list_managed_skills, stage_managed_skill_archive, - stage_managed_skill_update, ManagedSkill, ManagedSkillDraft, ManagedSkillProvenance, - ManagedSkillSource, ManagedSkillState, ManagedSkillUpdate, ManagedSupportFile, - SkillInstallTarget, + list_managed_skills, stage_managed_skill_archive, stage_managed_skill_update, ManagedSkill, + ManagedSkillDraft, ManagedSkillProvenance, ManagedSkillSource, ManagedSkillUpdate, + ManagedSupportFile, SkillInstallTarget, }; use super::skill_usage::{ skill_improvement_recommendations as usage_skill_improvement_recommendations, @@ -21,6 +20,13 @@ use crate::analytics::ToolFamilySignal; use crate::errors::Result; use crate::sessions::lcm::LcmGrepHit; +mod consolidation; + +use consolidation::{ + skill_archive_from_proposal, skill_merge_from_proposal, stage_skill_merge, + staged_consolidation_record, +}; + /// Outcome of validating and applying one batch of `skill_writer` proposals. /// `consolidations` holds staged merge/archive records; consolidations are /// always staged for dashboard approval and never auto-applied, even when @@ -460,248 +466,6 @@ fn skill_proposal_action(proposal: &Value) -> std::result::Result, -} - -/// Shared safety gate for consolidation (merge/archive) targets: the skill -/// must exist, its checksum must match, it must not be pinned (Hermes-parity -/// pinned exemption), must not be user-authored, must not already be -/// archived, and must not already carry a staged update. -fn consolidation_guard<'a>( - existing_skills: &'a BTreeMap, - id: &str, - base_checksum: &str, - role: &str, -) -> std::result::Result<&'a ManagedSkill, String> { - let skill = existing_skills - .get(id) - .ok_or_else(|| format!("{role} managed skill id '{id}' does not exist"))?; - if base_checksum != skill.metadata.checksum { - return Err(format!( - "base_checksum for managed skill id '{id}' is stale" - )); - } - if skill.metadata.pinned { - return Err(format!( - "managed skill '{id}' is pinned and exempt from consolidation" - )); - } - if skill.metadata.provenance.source == ManagedSkillSource::UserDraft { - return Err(format!( - "managed skill '{id}' is user-authored and exempt from consolidation" - )); - } - if skill.metadata.state == ManagedSkillState::Archived { - return Err(format!("managed skill '{id}' is already archived")); - } - if skill.pending_update.is_some() { - return Err(format!("managed skill '{id}' already has a pending update")); - } - Ok(skill) -} - -fn skill_archive_from_proposal( - proposal: &Value, - existing_skills: &BTreeMap, -) -> std::result::Result { - let object = proposal - .as_object() - .ok_or_else(|| "proposal must be a JSON object".to_string())?; - let id = required_proposal_string(object.get("id"), "id")?; - let base_checksum = required_proposal_string(object.get("base_checksum"), "base_checksum")?; - let reason = required_proposal_string(object.get("reason"), "reason")?; - consolidation_guard(existing_skills, &id, &base_checksum, "archive")?; - Ok(SkillArchiveProposal { - skill_id: id, - base_checksum, - reason, - }) -} - -fn skill_merge_from_proposal( - proposal: &Value, - existing_skills: &BTreeMap, -) -> std::result::Result { - let object = proposal - .as_object() - .ok_or_else(|| "proposal must be a JSON object".to_string())?; - let target_skill_id = required_proposal_string(object.get("id"), "id")?; - let base_checksum = required_proposal_string(object.get("base_checksum"), "base_checksum")?; - let source_skill_id = - required_proposal_string(object.get("source_skill_id"), "source_skill_id")?; - let source_base_checksum = - required_proposal_string(object.get("source_base_checksum"), "source_base_checksum")?; - let reason = required_proposal_string(object.get("reason"), "reason")?; - if source_skill_id == target_skill_id { - return Err("merge proposal source_skill_id must differ from id".to_string()); - } - let target = consolidation_guard(existing_skills, &target_skill_id, &base_checksum, "merge")?; - consolidation_guard( - existing_skills, - &source_skill_id, - &source_base_checksum, - "merge source", - )?; - - let update = ManagedSkillUpdate { - title: optional_proposal_string(object.get("title"))?, - summary: optional_proposal_string(object.get("summary"))?, - category: optional_proposal_string(object.get("category"))?, - targets: optional_proposal_targets(object.get("targets"))?, - body_markdown: optional_proposal_string( - object.get("body_markdown").or_else(|| object.get("body")), - )?, - support_files: if object.contains_key("support_files") { - Some(support_files_from_proposal(object.get("support_files"))?) - } else { - None - }, - pinned: None, - }; - let has_update = update.title.is_some() - || update.summary.is_some() - || update.category.is_some() - || update.targets.is_some() - || update.body_markdown.is_some() - || update.support_files.is_some(); - if has_update && !merge_update_changes_target(&update, target) { - return Err(format!( - "merge proposal does not change managed skill id '{target_skill_id}'" - )); - } - Ok(SkillMergeProposal { - target_skill_id, - base_checksum, - source_skill_id, - source_base_checksum, - reason, - update: has_update.then_some(update), - }) -} - -fn merge_update_changes_target(update: &ManagedSkillUpdate, target: &ManagedSkill) -> bool { - update - .title - .as_ref() - .is_some_and(|title| target.metadata.title != *title) - || update - .summary - .as_ref() - .is_some_and(|summary| target.metadata.summary != *summary) - || update - .category - .as_ref() - .is_some_and(|category| target.metadata.category != *category) - || update - .targets - .as_ref() - .is_some_and(|targets| target.metadata.targets != *targets) - || update - .body_markdown - .as_ref() - .is_some_and(|body| target.body_markdown != *body) - || update - .support_files - .as_ref() - .is_some_and(|support_files| target.support_files != *support_files) -} - -/// Stages a merge: an optional content update on the surviving skill plus an -/// archive on the absorbed skill. Both land as pending updates for dashboard -/// approval; nothing is auto-applied and no skill content is deleted. If the -/// archive staging fails after the target update was staged, the target -/// staging is rolled back so a merge never half-lands. -async fn stage_skill_merge( - profile_root: &Path, - merge: &SkillMergeProposal, -) -> Result<(ManagedSkill, Option)> { - let staged_target = match &merge.update { - Some(update) => Some( - stage_managed_skill_update( - profile_root, - &merge.target_skill_id, - &merge.base_checksum, - update.clone(), - ) - .await?, - ), - None => None, - }; - let archive_reason = format!("merged into '{}': {}", merge.target_skill_id, merge.reason); - let staged_source = match stage_managed_skill_archive( - profile_root, - &merge.source_skill_id, - &merge.source_base_checksum, - Some(archive_reason), - ) - .await - { - Ok(skill) => skill, - Err(err) => { - if staged_target.is_some() { - discard_pending_managed_skill_update(profile_root, &merge.target_skill_id).await?; - } - return Err(err); - } - }; - Ok((staged_source, staged_target)) -} - -fn staged_consolidation_record( - action: SkillProposalAction, - proposal: &Value, - staged_source: &ManagedSkill, - archive_base_checksum: &str, - merge: Option<&SkillMergeProposal>, -) -> Value { - let reason = proposal.get("reason").cloned().unwrap_or(Value::Null); - let mut record = json!({ - "action": action.as_str(), - "proposal_action": action.as_str(), - "reason": reason.clone(), - "proposal_reason": reason, - "approval_status": "staged_consolidation", - "resulting_state": "archived", - "archived_skill_id": staged_source.metadata.id, - }); - if let Some(object) = record.as_object_mut() { - if let Some(merge) = merge { - object.insert("target_skill_id".to_string(), json!(merge.target_skill_id)); - object.insert("base_checksum".to_string(), json!(merge.base_checksum)); - object.insert("source_skill_id".to_string(), json!(merge.source_skill_id)); - object.insert( - "source_base_checksum".to_string(), - json!(merge.source_base_checksum), - ); - object.insert( - "target_update_staged".to_string(), - json!(merge.update.is_some()), - ); - } else { - object.insert( - "target_skill_id".to_string(), - json!(staged_source.metadata.id), - ); - object.insert("base_checksum".to_string(), json!(archive_base_checksum)); - } - } - record -} - fn skill_update_from_proposal( proposal: &Value, existing_skills: &BTreeMap, @@ -889,7 +653,6 @@ fn normalized_non_empty(value: &str) -> Option { #[cfg(test)] mod tests { - use super::super::managed_skills::ManagedSkillPendingUpdate; use super::*; fn assert_err_eq(result: std::result::Result, expected: &str) { @@ -899,302 +662,6 @@ mod tests { } } - fn assert_ok(result: std::result::Result) -> T { - match result { - Ok(value) => value, - Err(err) => panic!("expected ok, got error: {err}"), - } - } - - fn fixture_skill(id: &str, source: ManagedSkillSource, pinned: bool) -> ManagedSkill { - let draft = ManagedSkillDraft { - id: id.to_string(), - title: format!("{id} guidance"), - summary: format!("Guidance for {id}."), - category: "workflow".to_string(), - targets: default_managed_skill_targets(), - body_markdown: format!("Follow the {id} workflow before applying changes."), - support_files: Vec::new(), - provenance: ManagedSkillProvenance { - source, - actor: "test".to_string(), - run_id: None, - }, - }; - let mut skill = match draft.materialize() { - Ok(skill) => skill, - Err(err) => panic!("test fixture skill should materialize: {err}"), - }; - skill.set_state(ManagedSkillState::Active); - skill.set_pinned(pinned); - skill - } - - fn consolidation_fixture() -> BTreeMap { - let mut archived = - fixture_skill("archived-skill", ManagedSkillSource::AutomationRun, false); - archived.set_state(ManagedSkillState::Archived); - [ - fixture_skill("workflow-a", ManagedSkillSource::AutomationRun, false), - fixture_skill("workflow-b", ManagedSkillSource::AutomationRun, false), - fixture_skill("pinned-skill", ManagedSkillSource::AutomationRun, true), - fixture_skill("user-skill", ManagedSkillSource::UserDraft, false), - archived, - ] - .into_iter() - .map(|skill| (skill.metadata.id.clone(), skill)) - .collect() - } - - fn checksum(skills: &BTreeMap, id: &str) -> String { - skills[id].metadata.checksum.clone() - } - - #[test] - fn archive_proposals_validate_ids_checksums_and_exemptions() { - let skills = consolidation_fixture(); - let valid = assert_ok(skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "reason": "unused overlap" - }), - &skills, - )); - assert_eq!(valid.skill_id, "workflow-a"); - assert_eq!(valid.reason, "unused overlap"); - - assert_err_eq( - skill_archive_from_proposal( - &json!({"action": "archive", "id": "workflow-a", "reason": "x"}), - &skills, - ), - "base_checksum is required", - ); - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a") - }), - &skills, - ), - "reason is required", - ); - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "missing", - "base_checksum": "sha256:0000", - "reason": "x" - }), - &skills, - ), - "archive managed skill id 'missing' does not exist", - ); - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "workflow-a", - "base_checksum": "sha256:stale", - "reason": "x" - }), - &skills, - ), - "base_checksum for managed skill id 'workflow-a' is stale", - ); - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "pinned-skill", - "base_checksum": checksum(&skills, "pinned-skill"), - "reason": "x" - }), - &skills, - ), - "managed skill 'pinned-skill' is pinned and exempt from consolidation", - ); - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "user-skill", - "base_checksum": checksum(&skills, "user-skill"), - "reason": "x" - }), - &skills, - ), - "managed skill 'user-skill' is user-authored and exempt from consolidation", - ); - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "archived-skill", - "base_checksum": checksum(&skills, "archived-skill"), - "reason": "x" - }), - &skills, - ), - "managed skill 'archived-skill' is already archived", - ); - } - - #[test] - fn archive_proposals_reject_targets_with_pending_updates() { - let mut skills = consolidation_fixture(); - let pending_metadata = skills["workflow-a"].metadata.clone(); - if let Some(skill) = skills.get_mut("workflow-a") { - skill.pending_update = Some(ManagedSkillPendingUpdate { - base_checksum: pending_metadata.checksum.clone(), - staged_at: 1, - metadata: pending_metadata, - body_markdown: "staged body".to_string(), - support_files: Vec::new(), - resulting_state: None, - staged_reason: None, - }); - } - assert_err_eq( - skill_archive_from_proposal( - &json!({ - "action": "archive", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "reason": "x" - }), - &skills, - ), - "managed skill 'workflow-a' already has a pending update", - ); - } - - #[test] - fn merge_proposals_validate_source_target_and_checksums() { - let skills = consolidation_fixture(); - let merge = assert_ok(skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "source_skill_id": "workflow-b", - "source_base_checksum": checksum(&skills, "workflow-b"), - "body_markdown": "Merged workflow guidance covering both variants.", - "reason": "duplicate guidance" - }), - &skills, - )); - assert_eq!(merge.target_skill_id, "workflow-a"); - assert_eq!(merge.source_skill_id, "workflow-b"); - assert!(merge.update.is_some()); - - let archive_only = assert_ok(skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "source_skill_id": "workflow-b", - "source_base_checksum": checksum(&skills, "workflow-b"), - "reason": "target already covers the source" - }), - &skills, - )); - assert!(archive_only.update.is_none()); - - assert_err_eq( - skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "source_skill_id": "workflow-a", - "source_base_checksum": checksum(&skills, "workflow-a"), - "reason": "x" - }), - &skills, - ), - "merge proposal source_skill_id must differ from id", - ); - assert_err_eq( - skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "source_skill_id": "workflow-b", - "source_base_checksum": "sha256:stale", - "reason": "x" - }), - &skills, - ), - "base_checksum for managed skill id 'workflow-b' is stale", - ); - assert_err_eq( - skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "pinned-skill", - "base_checksum": checksum(&skills, "pinned-skill"), - "source_skill_id": "workflow-b", - "source_base_checksum": checksum(&skills, "workflow-b"), - "reason": "x" - }), - &skills, - ), - "managed skill 'pinned-skill' is pinned and exempt from consolidation", - ); - assert_err_eq( - skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "source_skill_id": "pinned-skill", - "source_base_checksum": checksum(&skills, "pinned-skill"), - "reason": "x" - }), - &skills, - ), - "managed skill 'pinned-skill' is pinned and exempt from consolidation", - ); - assert_err_eq( - skill_merge_from_proposal( - &json!({ - "action": "merge", - "id": "workflow-a", - "base_checksum": checksum(&skills, "workflow-a"), - "source_skill_id": "workflow-b", - "source_base_checksum": checksum(&skills, "workflow-b"), - "body_markdown": skills["workflow-a"].body_markdown, - "reason": "x" - }), - &skills, - ), - "merge proposal does not change managed skill id 'workflow-a'", - ); - } - - #[test] - fn consolidation_actions_parse_from_proposals() { - assert_eq!( - assert_ok(skill_proposal_action(&json!({"action": "merge"}))), - SkillProposalAction::Merge - ); - assert_eq!( - assert_ok(skill_proposal_action(&json!({"action": "consolidate"}))), - SkillProposalAction::Merge - ); - assert_eq!( - assert_ok(skill_proposal_action(&json!({"action": "archive"}))), - SkillProposalAction::Archive - ); - } - #[test] fn proposal_targets_accept_known_aliases() -> std::result::Result<(), String> { let targets = optional_proposal_targets(Some(&json!([ diff --git a/src/automation/skill_writer/consolidation.rs b/src/automation/skill_writer/consolidation.rs new file mode 100644 index 00000000..4bfbea48 --- /dev/null +++ b/src/automation/skill_writer/consolidation.rs @@ -0,0 +1,560 @@ +use std::collections::BTreeMap; +use std::path::Path; + +use serde_json::{json, Value}; + +use super::super::managed_skills::{ + discard_pending_managed_skill_update, stage_managed_skill_archive, stage_managed_skill_update, + ManagedSkill, ManagedSkillSource, ManagedSkillState, ManagedSkillUpdate, +}; +use super::{ + optional_proposal_string, optional_proposal_targets, required_proposal_string, + support_files_from_proposal, SkillProposalAction, +}; +use crate::errors::Result; + +#[derive(Debug, Clone)] +pub(super) struct SkillArchiveProposal { + pub(super) skill_id: String, + pub(super) base_checksum: String, + pub(super) reason: String, +} + +#[derive(Debug, Clone)] +pub(super) struct SkillMergeProposal { + pub(super) target_skill_id: String, + pub(super) base_checksum: String, + pub(super) source_skill_id: String, + pub(super) source_base_checksum: String, + pub(super) reason: String, + pub(super) update: Option, +} + +fn consolidation_guard<'a>( + existing_skills: &'a BTreeMap, + id: &str, + base_checksum: &str, + role: &str, +) -> std::result::Result<&'a ManagedSkill, String> { + let skill = existing_skills + .get(id) + .ok_or_else(|| format!("{role} managed skill id '{id}' does not exist"))?; + if base_checksum != skill.metadata.checksum { + return Err(format!( + "base_checksum for managed skill id '{id}' is stale" + )); + } + if skill.metadata.pinned { + return Err(format!( + "managed skill '{id}' is pinned and exempt from consolidation" + )); + } + if skill.metadata.provenance.source == ManagedSkillSource::UserDraft { + return Err(format!( + "managed skill '{id}' is user-authored and exempt from consolidation" + )); + } + if skill.metadata.state == ManagedSkillState::Archived { + return Err(format!("managed skill '{id}' is already archived")); + } + if skill.pending_update.is_some() { + return Err(format!("managed skill '{id}' already has a pending update")); + } + Ok(skill) +} + +pub(super) fn skill_archive_from_proposal( + proposal: &Value, + existing_skills: &BTreeMap, +) -> std::result::Result { + let object = proposal + .as_object() + .ok_or_else(|| "proposal must be a JSON object".to_string())?; + let id = required_proposal_string(object.get("id"), "id")?; + let base_checksum = required_proposal_string(object.get("base_checksum"), "base_checksum")?; + let reason = required_proposal_string(object.get("reason"), "reason")?; + consolidation_guard(existing_skills, &id, &base_checksum, "archive")?; + Ok(SkillArchiveProposal { + skill_id: id, + base_checksum, + reason, + }) +} + +pub(super) fn skill_merge_from_proposal( + proposal: &Value, + existing_skills: &BTreeMap, +) -> std::result::Result { + let object = proposal + .as_object() + .ok_or_else(|| "proposal must be a JSON object".to_string())?; + let target_skill_id = required_proposal_string(object.get("id"), "id")?; + let base_checksum = required_proposal_string(object.get("base_checksum"), "base_checksum")?; + let source_skill_id = + required_proposal_string(object.get("source_skill_id"), "source_skill_id")?; + let source_base_checksum = + required_proposal_string(object.get("source_base_checksum"), "source_base_checksum")?; + let reason = required_proposal_string(object.get("reason"), "reason")?; + if source_skill_id == target_skill_id { + return Err("merge proposal source_skill_id must differ from id".to_string()); + } + let target = consolidation_guard(existing_skills, &target_skill_id, &base_checksum, "merge")?; + consolidation_guard( + existing_skills, + &source_skill_id, + &source_base_checksum, + "merge source", + )?; + + let update = ManagedSkillUpdate { + title: optional_proposal_string(object.get("title"))?, + summary: optional_proposal_string(object.get("summary"))?, + category: optional_proposal_string(object.get("category"))?, + targets: optional_proposal_targets(object.get("targets"))?, + body_markdown: optional_proposal_string( + object.get("body_markdown").or_else(|| object.get("body")), + )?, + support_files: if object.contains_key("support_files") { + Some(support_files_from_proposal(object.get("support_files"))?) + } else { + None + }, + pinned: None, + }; + let has_update = update.title.is_some() + || update.summary.is_some() + || update.category.is_some() + || update.targets.is_some() + || update.body_markdown.is_some() + || update.support_files.is_some(); + if has_update && !merge_update_changes_target(&update, target) { + return Err(format!( + "merge proposal does not change managed skill id '{target_skill_id}'" + )); + } + Ok(SkillMergeProposal { + target_skill_id, + base_checksum, + source_skill_id, + source_base_checksum, + reason, + update: has_update.then_some(update), + }) +} + +fn merge_update_changes_target(update: &ManagedSkillUpdate, target: &ManagedSkill) -> bool { + update + .title + .as_ref() + .is_some_and(|title| target.metadata.title != *title) + || update + .summary + .as_ref() + .is_some_and(|summary| target.metadata.summary != *summary) + || update + .category + .as_ref() + .is_some_and(|category| target.metadata.category != *category) + || update + .targets + .as_ref() + .is_some_and(|targets| target.metadata.targets != *targets) + || update + .body_markdown + .as_ref() + .is_some_and(|body| target.body_markdown != *body) + || update + .support_files + .as_ref() + .is_some_and(|support_files| target.support_files != *support_files) +} + +pub(super) async fn stage_skill_merge( + profile_root: &Path, + merge: &SkillMergeProposal, +) -> Result<(ManagedSkill, Option)> { + let staged_target = match &merge.update { + Some(update) => Some( + stage_managed_skill_update( + profile_root, + &merge.target_skill_id, + &merge.base_checksum, + update.clone(), + ) + .await?, + ), + None => None, + }; + let archive_reason = format!("merged into '{}': {}", merge.target_skill_id, merge.reason); + let staged_source = match stage_managed_skill_archive( + profile_root, + &merge.source_skill_id, + &merge.source_base_checksum, + Some(archive_reason), + ) + .await + { + Ok(skill) => skill, + Err(err) => { + if staged_target.is_some() { + discard_pending_managed_skill_update(profile_root, &merge.target_skill_id).await?; + } + return Err(err); + } + }; + Ok((staged_source, staged_target)) +} + +pub(super) fn staged_consolidation_record( + action: SkillProposalAction, + proposal: &Value, + staged_source: &ManagedSkill, + archive_base_checksum: &str, + merge: Option<&SkillMergeProposal>, +) -> Value { + let reason = proposal.get("reason").cloned().unwrap_or(Value::Null); + let mut record = json!({ + "action": action.as_str(), + "proposal_action": action.as_str(), + "reason": reason.clone(), + "proposal_reason": reason, + "approval_status": "staged_consolidation", + "resulting_state": "archived", + "archived_skill_id": staged_source.metadata.id, + }); + if let Some(object) = record.as_object_mut() { + if let Some(merge) = merge { + object.insert("target_skill_id".to_string(), json!(merge.target_skill_id)); + object.insert("base_checksum".to_string(), json!(merge.base_checksum)); + object.insert("source_skill_id".to_string(), json!(merge.source_skill_id)); + object.insert( + "source_base_checksum".to_string(), + json!(merge.source_base_checksum), + ); + object.insert( + "target_update_staged".to_string(), + json!(merge.update.is_some()), + ); + } else { + object.insert( + "target_skill_id".to_string(), + json!(staged_source.metadata.id), + ); + object.insert("base_checksum".to_string(), json!(archive_base_checksum)); + } + } + record +} + +#[cfg(test)] +mod tests { + use super::super::super::managed_skills::{ + default_managed_skill_targets, ManagedSkillDraft, ManagedSkillPendingUpdate, + ManagedSkillProvenance, + }; + use super::super::skill_proposal_action; + use super::*; + + fn assert_err_eq(result: std::result::Result, expected: &str) { + match result { + Ok(_) => panic!("expected error: {expected}"), + Err(err) => assert_eq!(err, expected), + } + } + + fn assert_ok(result: std::result::Result) -> T { + match result { + Ok(value) => value, + Err(err) => panic!("expected ok, got error: {err}"), + } + } + + fn fixture_skill(id: &str, source: ManagedSkillSource, pinned: bool) -> ManagedSkill { + let draft = ManagedSkillDraft { + id: id.to_string(), + title: format!("{id} guidance"), + summary: format!("Guidance for {id}."), + category: "workflow".to_string(), + targets: default_managed_skill_targets(), + body_markdown: format!("Follow the {id} workflow before applying changes."), + support_files: Vec::new(), + provenance: ManagedSkillProvenance { + source, + actor: "test".to_string(), + run_id: None, + }, + }; + let mut skill = match draft.materialize() { + Ok(skill) => skill, + Err(err) => panic!("test fixture skill should materialize: {err}"), + }; + skill.set_state(ManagedSkillState::Active); + skill.set_pinned(pinned); + skill + } + + fn consolidation_fixture() -> BTreeMap { + let mut archived = + fixture_skill("archived-skill", ManagedSkillSource::AutomationRun, false); + archived.set_state(ManagedSkillState::Archived); + [ + fixture_skill("workflow-a", ManagedSkillSource::AutomationRun, false), + fixture_skill("workflow-b", ManagedSkillSource::AutomationRun, false), + fixture_skill("pinned-skill", ManagedSkillSource::AutomationRun, true), + fixture_skill("user-skill", ManagedSkillSource::UserDraft, false), + archived, + ] + .into_iter() + .map(|skill| (skill.metadata.id.clone(), skill)) + .collect() + } + + fn checksum(skills: &BTreeMap, id: &str) -> String { + skills[id].metadata.checksum.clone() + } + + #[test] + fn archive_proposals_validate_ids_checksums_and_exemptions() { + let skills = consolidation_fixture(); + let valid = assert_ok(skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "reason": "unused overlap" + }), + &skills, + )); + assert_eq!(valid.skill_id, "workflow-a"); + assert_eq!(valid.reason, "unused overlap"); + + assert_err_eq( + skill_archive_from_proposal( + &json!({"action": "archive", "id": "workflow-a", "reason": "x"}), + &skills, + ), + "base_checksum is required", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a") + }), + &skills, + ), + "reason is required", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "missing", + "base_checksum": "sha256:0000", + "reason": "x" + }), + &skills, + ), + "archive managed skill id 'missing' does not exist", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": "sha256:stale", + "reason": "x" + }), + &skills, + ), + "base_checksum for managed skill id 'workflow-a' is stale", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "pinned-skill", + "base_checksum": checksum(&skills, "pinned-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'pinned-skill' is pinned and exempt from consolidation", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "user-skill", + "base_checksum": checksum(&skills, "user-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'user-skill' is user-authored and exempt from consolidation", + ); + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "archived-skill", + "base_checksum": checksum(&skills, "archived-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'archived-skill' is already archived", + ); + } + + #[test] + fn archive_proposals_reject_targets_with_pending_updates() { + let mut skills = consolidation_fixture(); + let pending_metadata = skills["workflow-a"].metadata.clone(); + if let Some(skill) = skills.get_mut("workflow-a") { + skill.pending_update = Some(ManagedSkillPendingUpdate { + base_checksum: pending_metadata.checksum.clone(), + staged_at: 1, + metadata: pending_metadata, + body_markdown: "staged body".to_string(), + support_files: Vec::new(), + resulting_state: None, + staged_reason: None, + }); + } + assert_err_eq( + skill_archive_from_proposal( + &json!({ + "action": "archive", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "reason": "x" + }), + &skills, + ), + "managed skill 'workflow-a' already has a pending update", + ); + } + + #[test] + fn merge_proposals_validate_source_target_and_checksums() { + let skills = consolidation_fixture(); + let merge = assert_ok(skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "body_markdown": "Merged workflow guidance covering both variants.", + "reason": "duplicate guidance" + }), + &skills, + )); + assert_eq!(merge.target_skill_id, "workflow-a"); + assert_eq!(merge.source_skill_id, "workflow-b"); + assert!(merge.update.is_some()); + + let archive_only = assert_ok(skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "reason": "target already covers the source" + }), + &skills, + )); + assert!(archive_only.update.is_none()); + + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-a", + "source_base_checksum": checksum(&skills, "workflow-a"), + "reason": "x" + }), + &skills, + ), + "merge proposal source_skill_id must differ from id", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": "sha256:stale", + "reason": "x" + }), + &skills, + ), + "base_checksum for managed skill id 'workflow-b' is stale", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "pinned-skill", + "base_checksum": checksum(&skills, "pinned-skill"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "reason": "x" + }), + &skills, + ), + "managed skill 'pinned-skill' is pinned and exempt from consolidation", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "pinned-skill", + "source_base_checksum": checksum(&skills, "pinned-skill"), + "reason": "x" + }), + &skills, + ), + "managed skill 'pinned-skill' is pinned and exempt from consolidation", + ); + assert_err_eq( + skill_merge_from_proposal( + &json!({ + "action": "merge", + "id": "workflow-a", + "base_checksum": checksum(&skills, "workflow-a"), + "source_skill_id": "workflow-b", + "source_base_checksum": checksum(&skills, "workflow-b"), + "body_markdown": skills["workflow-a"].body_markdown, + "reason": "x" + }), + &skills, + ), + "merge proposal does not change managed skill id 'workflow-a'", + ); + } + + #[test] + fn consolidation_actions_parse_from_proposals() { + assert_eq!( + assert_ok(skill_proposal_action(&json!({"action": "merge"}))), + SkillProposalAction::Merge + ); + assert_eq!( + assert_ok(skill_proposal_action(&json!({"action": "consolidate"}))), + SkillProposalAction::Merge + ); + assert_eq!( + assert_ok(skill_proposal_action(&json!({"action": "archive"}))), + SkillProposalAction::Archive + ); + } +} diff --git a/tests/agent_suite/main.rs b/tests/agent_suite/main.rs index 17c5e6cc..f3575d5f 100644 --- a/tests/agent_suite/main.rs +++ b/tests/agent_suite/main.rs @@ -13,6 +13,7 @@ mod agent_test; mod claude_agent_test; mod copilot_agent_test; mod kiro_agent_test; +mod managed_skill_archive_test; mod managed_skills_test; mod opencode_agent_test; mod plugin_skill_contract_test; diff --git a/tests/agent_suite/managed_skill_archive_test.rs b/tests/agent_suite/managed_skill_archive_test.rs new file mode 100644 index 00000000..815ea9a4 --- /dev/null +++ b/tests/agent_suite/managed_skill_archive_test.rs @@ -0,0 +1,160 @@ +use tracedecay::automation::managed_skills::{ + approve_managed_skill, archive_managed_skill, create_managed_skill_draft, + discard_pending_managed_skill_update, load_managed_skill, managed_skill_dir, + save_managed_skill, stage_managed_skill_archive, ManagedSkillDraft, ManagedSkillProvenance, + ManagedSkillSource, ManagedSkillState, ManagedSupportFile, SkillInstallTarget, +}; + +fn draft() -> ManagedSkillDraft { + ManagedSkillDraft { + id: "repo-hygiene".to_string(), + title: "Repository hygiene".to_string(), + summary: "Keep repository maintenance guidance current.".to_string(), + category: "maintenance".to_string(), + targets: vec![SkillInstallTarget::Cursor, SkillInstallTarget::Codex], + body_markdown: "Use focused checks before changing generated files.".to_string(), + support_files: vec![ManagedSupportFile::new( + "references/checklist.md", + b"- check dirty tree\n- run focused tests\n".to_vec(), + ) + .unwrap()], + provenance: ManagedSkillProvenance { + source: ManagedSkillSource::AutomationRun, + actor: "tracedecay".to_string(), + run_id: Some("run_123".to_string()), + }, + } +} + +#[tokio::test] +async fn staged_managed_skill_archive_keeps_content_until_approval() { + let temp = tempfile::tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + create_managed_skill_draft(&profile_root, draft()) + .await + .unwrap(); + let active = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + let base_checksum = active.metadata.checksum.clone(); + let skill_dir = managed_skill_dir(&profile_root, "repo-hygiene").unwrap(); + + let staged = stage_managed_skill_archive( + &profile_root, + "repo-hygiene", + &base_checksum, + Some("overlaps with newer guidance".to_string()), + ) + .await + .unwrap(); + assert_eq!(staged.metadata.state, ManagedSkillState::PendingApproval); + + let with_pending = load_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(with_pending.metadata.state, ManagedSkillState::Active); + assert_eq!(with_pending.metadata.checksum, base_checksum); + let pending = with_pending.pending_update.as_ref().unwrap(); + assert_eq!(pending.resulting_state, Some(ManagedSkillState::Archived)); + assert_eq!( + pending.staged_reason.as_deref(), + Some("overlaps with newer guidance") + ); + + let approved = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(approved.metadata.state, ManagedSkillState::Archived); + assert_eq!(approved.body_markdown, active.body_markdown); + assert!(approved.pending_update.is_none()); + assert!(skill_dir.join("SKILL.md").is_file()); + assert!(skill_dir.join("references/checklist.md").is_file()); + let reloaded = load_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(reloaded.metadata.state, ManagedSkillState::Archived); + assert_eq!(reloaded.body_markdown, active.body_markdown); +} + +#[tokio::test] +async fn staged_managed_skill_archive_can_be_discarded() { + let temp = tempfile::tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + create_managed_skill_draft(&profile_root, draft()) + .await + .unwrap(); + let active = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + stage_managed_skill_archive( + &profile_root, + "repo-hygiene", + &active.metadata.checksum, + None, + ) + .await + .unwrap(); + + let discarded = discard_pending_managed_skill_update(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert!(discarded.pending_update.is_none()); + let reloaded = load_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + assert_eq!(reloaded.metadata.state, ManagedSkillState::Active); + assert_eq!(reloaded.metadata.checksum, active.metadata.checksum); +} + +#[tokio::test] +async fn staged_managed_skill_archive_rejects_pinned_stale_and_duplicates() { + let temp = tempfile::tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + create_managed_skill_draft(&profile_root, draft()) + .await + .unwrap(); + let mut active = approve_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + let base_checksum = active.metadata.checksum.clone(); + + let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", "sha256:stale", None) + .await + .unwrap_err(); + assert!(err.to_string().contains("is stale")); + + active.set_pinned(true); + save_managed_skill(&profile_root, &active).await.unwrap(); + let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) + .await + .unwrap_err(); + assert!(err + .to_string() + .contains("pinned and exempt from staged archive")); + + active.set_pinned(false); + save_managed_skill(&profile_root, &active).await.unwrap(); + stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) + .await + .unwrap(); + let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) + .await + .unwrap_err(); + assert!(err.to_string().contains("already has a pending update")); + + discard_pending_managed_skill_update(&profile_root, "repo-hygiene") + .await + .unwrap(); + let archived = archive_managed_skill(&profile_root, "repo-hygiene") + .await + .unwrap(); + let err = stage_managed_skill_archive( + &profile_root, + "repo-hygiene", + &archived.metadata.checksum, + None, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("is already archived")); +} diff --git a/tests/agent_suite/managed_skills_test.rs b/tests/agent_suite/managed_skills_test.rs index 1fe41145..ac19e7b3 100644 --- a/tests/agent_suite/managed_skills_test.rs +++ b/tests/agent_suite/managed_skills_test.rs @@ -2,10 +2,9 @@ use tracedecay::automation::managed_skills::{ approve_managed_skill, archive_managed_skill, create_managed_skill_draft, disable_managed_skill, discard_pending_managed_skill_update, list_managed_skills, load_managed_skill, managed_skill_dir, restore_managed_skill, save_managed_skill, - set_managed_skill_state, stage_managed_skill_archive, stage_managed_skill_update, - update_managed_skill, ManagedSkillDraft, ManagedSkillProvenance, ManagedSkillSource, - ManagedSkillState, ManagedSkillUpdate, ManagedSupportFile, SkillInstallTarget, - MAX_MANAGED_SKILL_BODY_BYTES, + set_managed_skill_state, stage_managed_skill_update, update_managed_skill, ManagedSkillDraft, + ManagedSkillProvenance, ManagedSkillSource, ManagedSkillState, ManagedSkillUpdate, + ManagedSupportFile, SkillInstallTarget, MAX_MANAGED_SKILL_BODY_BYTES, }; use tracedecay::automation::skill_usage::{ ingest_analytics_events, load_skill_usage_records, record_skill_usage, @@ -838,142 +837,6 @@ async fn staged_managed_skill_update_preserves_active_revision_until_approval() assert_eq!(reloaded.metadata.checksum, second_base); } -#[tokio::test] -async fn staged_managed_skill_archive_keeps_content_until_approval() { - let temp = tempfile::tempdir().unwrap(); - let profile_root = temp.path().join("profile"); - create_managed_skill_draft(&profile_root, draft()) - .await - .unwrap(); - let active = approve_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - let base_checksum = active.metadata.checksum.clone(); - let skill_dir = managed_skill_dir(&profile_root, "repo-hygiene").unwrap(); - - let staged = stage_managed_skill_archive( - &profile_root, - "repo-hygiene", - &base_checksum, - Some("overlaps with newer guidance".to_string()), - ) - .await - .unwrap(); - assert_eq!(staged.metadata.state, ManagedSkillState::PendingApproval); - - // Staging must not touch the active revision. - let with_pending = load_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - assert_eq!(with_pending.metadata.state, ManagedSkillState::Active); - assert_eq!(with_pending.metadata.checksum, base_checksum); - let pending = with_pending.pending_update.as_ref().unwrap(); - assert_eq!(pending.resulting_state, Some(ManagedSkillState::Archived)); - assert_eq!( - pending.staged_reason.as_deref(), - Some("overlaps with newer guidance") - ); - - // Approval archives without deleting any content. - let approved = approve_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - assert_eq!(approved.metadata.state, ManagedSkillState::Archived); - assert_eq!(approved.body_markdown, active.body_markdown); - assert!(approved.pending_update.is_none()); - assert!(skill_dir.join("SKILL.md").is_file()); - assert!(skill_dir.join("references/checklist.md").is_file()); - let reloaded = load_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - assert_eq!(reloaded.metadata.state, ManagedSkillState::Archived); - assert_eq!(reloaded.body_markdown, active.body_markdown); -} - -#[tokio::test] -async fn staged_managed_skill_archive_can_be_discarded() { - let temp = tempfile::tempdir().unwrap(); - let profile_root = temp.path().join("profile"); - create_managed_skill_draft(&profile_root, draft()) - .await - .unwrap(); - let active = approve_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - stage_managed_skill_archive( - &profile_root, - "repo-hygiene", - &active.metadata.checksum, - None, - ) - .await - .unwrap(); - - let discarded = discard_pending_managed_skill_update(&profile_root, "repo-hygiene") - .await - .unwrap(); - assert!(discarded.pending_update.is_none()); - let reloaded = load_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - assert_eq!(reloaded.metadata.state, ManagedSkillState::Active); - assert_eq!(reloaded.metadata.checksum, active.metadata.checksum); -} - -#[tokio::test] -async fn staged_managed_skill_archive_rejects_pinned_stale_and_duplicates() { - let temp = tempfile::tempdir().unwrap(); - let profile_root = temp.path().join("profile"); - create_managed_skill_draft(&profile_root, draft()) - .await - .unwrap(); - let mut active = approve_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - let base_checksum = active.metadata.checksum.clone(); - - let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", "sha256:stale", None) - .await - .unwrap_err(); - assert!(err.to_string().contains("is stale")); - - active.set_pinned(true); - save_managed_skill(&profile_root, &active).await.unwrap(); - let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) - .await - .unwrap_err(); - assert!(err - .to_string() - .contains("pinned and exempt from staged archive")); - - active.set_pinned(false); - save_managed_skill(&profile_root, &active).await.unwrap(); - stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) - .await - .unwrap(); - let err = stage_managed_skill_archive(&profile_root, "repo-hygiene", &base_checksum, None) - .await - .unwrap_err(); - assert!(err.to_string().contains("already has a pending update")); - - // Already-archived skills cannot be staged for archive again. - discard_pending_managed_skill_update(&profile_root, "repo-hygiene") - .await - .unwrap(); - let archived = archive_managed_skill(&profile_root, "repo-hygiene") - .await - .unwrap(); - let err = stage_managed_skill_archive( - &profile_root, - "repo-hygiene", - &archived.metadata.checksum, - None, - ) - .await - .unwrap_err(); - assert!(err.to_string().contains("is already archived")); -} - #[tokio::test] async fn staged_managed_skill_update_rejects_no_op_patch() { let temp = tempfile::tempdir().unwrap(); diff --git a/tests/automation_runner_test/main.rs b/tests/automation_runner_test/main.rs index 191d3016..464497a3 100644 --- a/tests/automation_runner_test/main.rs +++ b/tests/automation_runner_test/main.rs @@ -23,3 +23,4 @@ mod runner; mod scheduler; mod session_reflector; mod skill_writer; +mod skill_writer_consolidation; diff --git a/tests/automation_runner_test/skill_writer.rs b/tests/automation_runner_test/skill_writer.rs index af2bada6..01dde018 100644 --- a/tests/automation_runner_test/skill_writer.rs +++ b/tests/automation_runner_test/skill_writer.rs @@ -719,200 +719,6 @@ async fn skill_writer_runner_updates_existing_skills_with_checksum_precondition( ); } -#[tokio::test] -async fn skill_writer_runner_stages_consolidations_without_auto_apply() { - let _env_lock = ENV_LOCK.lock().await; - let temp = tempdir().unwrap(); - let profile_root = temp.path().join("profile"); - let cg = init_project(temp.path()).await; - seed_session_evidence(&cg).await; - let _global_db = isolate_global_db(&cg); - let automation_provenance = || ManagedSkillProvenance { - source: ManagedSkillSource::AutomationRun, - actor: "skill_writer".to_string(), - run_id: Some("run_seed".to_string()), - }; - create_managed_skill_draft( - &profile_root, - ManagedSkillDraft { - id: "automation-run-review".to_string(), - title: "Automation run review".to_string(), - summary: "Review automation run ledgers before approving changes.".to_string(), - category: "workflow".to_string(), - targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), - body_markdown: - "Check run ledger counts, rejected proposals, and pending approval state before applying automation changes." - .to_string(), - support_files: Vec::new(), - provenance: automation_provenance(), - }, - ) - .await - .unwrap(); - create_managed_skill_draft( - &profile_root, - ManagedSkillDraft { - id: "automation-run-checks".to_string(), - title: "Automation run checks".to_string(), - summary: "Review automation run ledgers and approval gates.".to_string(), - category: "workflow".to_string(), - targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), - body_markdown: - "Check run ledger counts, rejected proposals, and approval gates before applying automation changes." - .to_string(), - support_files: Vec::new(), - provenance: automation_provenance(), - }, - ) - .await - .unwrap(); - create_managed_skill_draft( - &profile_root, - ManagedSkillDraft { - id: "pinned-automation-guide".to_string(), - title: "Pinned deployment guide".to_string(), - summary: "Deployment rollback runbook kept pinned by the user.".to_string(), - category: "workflow".to_string(), - targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), - body_markdown: "Roll back with the canary checklist and notify the release channel." - .to_string(), - support_files: Vec::new(), - provenance: automation_provenance(), - }, - ) - .await - .unwrap(); - let target = approve_managed_skill(&profile_root, "automation-run-review") - .await - .unwrap(); - let source = approve_managed_skill(&profile_root, "automation-run-checks") - .await - .unwrap(); - let mut pinned = approve_managed_skill(&profile_root, "pinned-automation-guide") - .await - .unwrap(); - pinned.set_pinned(true); - tracedecay::automation::managed_skills::save_managed_skill(&profile_root, &pinned) - .await - .unwrap(); - - let backend = SkillJsonBackend::with_activation_policy( - json!({ - "skills": [ - { - "action": "merge", - "id": "automation-run-review", - "base_checksum": target.metadata.checksum, - "source_skill_id": "automation-run-checks", - "source_base_checksum": source.metadata.checksum, - "body_markdown": "Check run ledger counts, rejected proposals, approval gates, and pending approval state before applying automation changes.", - "reason": "The two run-review skills overlap almost completely." - }, - { - "action": "archive", - "id": "pinned-automation-guide", - "base_checksum": pinned.metadata.checksum, - "reason": "Pinned skills must be rejected." - } - ] - }), - "auto_enable_after_validation", - ); - // auto_enable_skills=true proves consolidations are still staged, never - // auto-applied. - let config = AutomationConfig { - auto_enable_skills: true, - ..enabled_skill_writer_config() - }; - - let run = run_skill_writer_with_backend( - &cg, - &config, - &backend, - manual_skill_writer_options(&profile_root), - ) - .await - .unwrap(); - - assert_eq!(backend.calls(), 1); - assert_eq!(run.ledger_record.status, AutomationRunStatus::Succeeded); - assert_eq!(run.ledger_record.accepted_count, 1); - assert_eq!(run.ledger_record.rejected_count, 1); - - let consolidation = &run.report["staged_consolidations"][0]; - assert_eq!(consolidation["action"], json!("merge")); - assert_eq!( - consolidation["approval_status"], - json!("staged_consolidation") - ); - assert_eq!( - consolidation["target_skill_id"], - json!("automation-run-review") - ); - assert_eq!( - consolidation["source_skill_id"], - json!("automation-run-checks") - ); - assert_eq!( - consolidation["archived_skill_id"], - json!("automation-run-checks") - ); - assert_eq!(consolidation["resulting_state"], json!("archived")); - assert_eq!(consolidation["target_update_staged"], json!(true)); - assert!(run.report["rejected_skills"][0]["reason"] - .as_str() - .is_some_and(|reason| reason.contains("pinned"))); - - // Overlap evidence surfaced the pair and recommended consolidation. - assert!(run.report["skill_improvement_recommendations"] - .as_array() - .is_some_and( - |recommendations| recommendations.iter().any(|recommendation| { - recommendation["kind"] == "managed_skill_consolidation" - && recommendation["source"] == "skill_overlap_detection" - }) - )); - - // Nothing auto-applied: both sides only carry staged pending updates. - let staged_source = load_managed_skill(&profile_root, "automation-run-checks") - .await - .unwrap(); - assert_eq!(staged_source.metadata.state, ManagedSkillState::Active); - let source_pending = staged_source.pending_update.as_ref().unwrap(); - assert_eq!( - source_pending.resulting_state, - Some(ManagedSkillState::Archived) - ); - assert!(source_pending - .staged_reason - .as_deref() - .is_some_and(|reason| reason.contains("merged into 'automation-run-review'"))); - let staged_target = load_managed_skill(&profile_root, "automation-run-review") - .await - .unwrap(); - assert_eq!(staged_target.metadata.state, ManagedSkillState::Active); - assert_eq!(staged_target.metadata.checksum, target.metadata.checksum); - assert!(staged_target.pending_update.is_some()); - let untouched_pinned = load_managed_skill(&profile_root, "pinned-automation-guide") - .await - .unwrap(); - assert_eq!(untouched_pinned.metadata.state, ManagedSkillState::Active); - assert!(untouched_pinned.pending_update.is_none()); - - // Approving the staged consolidation archives the source (content kept) - // and applies the merged body to the target. - let archived = approve_managed_skill(&profile_root, "automation-run-checks") - .await - .unwrap(); - assert_eq!(archived.metadata.state, ManagedSkillState::Archived); - assert_eq!(archived.body_markdown, source.body_markdown); - let merged = approve_managed_skill(&profile_root, "automation-run-review") - .await - .unwrap(); - assert_eq!(merged.metadata.state, ManagedSkillState::Active); - assert!(merged.body_markdown.contains("approval gates")); -} - #[tokio::test] async fn skill_writer_runner_ledgers_malformed_backend_output() { let _env_lock = ENV_LOCK.lock().await; diff --git a/tests/automation_runner_test/skill_writer_consolidation.rs b/tests/automation_runner_test/skill_writer_consolidation.rs new file mode 100644 index 00000000..61f11be6 --- /dev/null +++ b/tests/automation_runner_test/skill_writer_consolidation.rs @@ -0,0 +1,189 @@ +use crate::support::*; + +#[tokio::test] +async fn skill_writer_runner_stages_consolidations_without_auto_apply() { + let _env_lock = ENV_LOCK.lock().await; + let temp = tempdir().unwrap(); + let profile_root = temp.path().join("profile"); + let cg = init_project(temp.path()).await; + seed_session_evidence(&cg).await; + let _global_db = isolate_global_db(&cg); + let automation_provenance = || ManagedSkillProvenance { + source: ManagedSkillSource::AutomationRun, + actor: "skill_writer".to_string(), + run_id: Some("run_seed".to_string()), + }; + create_managed_skill_draft( + &profile_root, + ManagedSkillDraft { + id: "automation-run-review".to_string(), + title: "Automation run review".to_string(), + summary: "Review automation run ledgers before approving changes.".to_string(), + category: "workflow".to_string(), + targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), + body_markdown: + "Check run ledger counts, rejected proposals, and pending approval state before applying automation changes." + .to_string(), + support_files: Vec::new(), + provenance: automation_provenance(), + }, + ) + .await + .unwrap(); + create_managed_skill_draft( + &profile_root, + ManagedSkillDraft { + id: "automation-run-checks".to_string(), + title: "Automation run checks".to_string(), + summary: "Review automation run ledgers and approval gates.".to_string(), + category: "workflow".to_string(), + targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), + body_markdown: + "Check run ledger counts, rejected proposals, and approval gates before applying automation changes." + .to_string(), + support_files: Vec::new(), + provenance: automation_provenance(), + }, + ) + .await + .unwrap(); + create_managed_skill_draft( + &profile_root, + ManagedSkillDraft { + id: "pinned-automation-guide".to_string(), + title: "Pinned deployment guide".to_string(), + summary: "Deployment rollback runbook kept pinned by the user.".to_string(), + category: "workflow".to_string(), + targets: tracedecay::automation::managed_skills::default_managed_skill_targets(), + body_markdown: "Roll back with the canary checklist and notify the release channel." + .to_string(), + support_files: Vec::new(), + provenance: automation_provenance(), + }, + ) + .await + .unwrap(); + let target = approve_managed_skill(&profile_root, "automation-run-review") + .await + .unwrap(); + let source = approve_managed_skill(&profile_root, "automation-run-checks") + .await + .unwrap(); + let mut pinned = approve_managed_skill(&profile_root, "pinned-automation-guide") + .await + .unwrap(); + pinned.set_pinned(true); + tracedecay::automation::managed_skills::save_managed_skill(&profile_root, &pinned) + .await + .unwrap(); + + let backend = SkillJsonBackend::with_activation_policy( + json!({ + "skills": [ + { + "action": "merge", + "id": "automation-run-review", + "base_checksum": target.metadata.checksum, + "source_skill_id": "automation-run-checks", + "source_base_checksum": source.metadata.checksum, + "body_markdown": "Check run ledger counts, rejected proposals, approval gates, and pending approval state before applying automation changes.", + "reason": "The two run-review skills overlap almost completely." + }, + { + "action": "archive", + "id": "pinned-automation-guide", + "base_checksum": pinned.metadata.checksum, + "reason": "Pinned skills must be rejected." + } + ] + }), + "auto_enable_after_validation", + ); + let config = AutomationConfig { + auto_enable_skills: true, + ..enabled_skill_writer_config() + }; + + let run = run_skill_writer_with_backend( + &cg, + &config, + &backend, + manual_skill_writer_options(&profile_root), + ) + .await + .unwrap(); + + assert_eq!(backend.calls(), 1); + assert_eq!(run.ledger_record.status, AutomationRunStatus::Succeeded); + assert_eq!(run.ledger_record.accepted_count, 1); + assert_eq!(run.ledger_record.rejected_count, 1); + + let consolidation = &run.report["staged_consolidations"][0]; + assert_eq!(consolidation["action"], json!("merge")); + assert_eq!( + consolidation["approval_status"], + json!("staged_consolidation") + ); + assert_eq!( + consolidation["target_skill_id"], + json!("automation-run-review") + ); + assert_eq!( + consolidation["source_skill_id"], + json!("automation-run-checks") + ); + assert_eq!( + consolidation["archived_skill_id"], + json!("automation-run-checks") + ); + assert_eq!(consolidation["resulting_state"], json!("archived")); + assert_eq!(consolidation["target_update_staged"], json!(true)); + assert!(run.report["rejected_skills"][0]["reason"] + .as_str() + .is_some_and(|reason| reason.contains("pinned"))); + + assert!(run.report["skill_improvement_recommendations"] + .as_array() + .is_some_and( + |recommendations| recommendations.iter().any(|recommendation| { + recommendation["kind"] == "managed_skill_consolidation" + && recommendation["source"] == "skill_overlap_detection" + }) + )); + + let staged_source = load_managed_skill(&profile_root, "automation-run-checks") + .await + .unwrap(); + assert_eq!(staged_source.metadata.state, ManagedSkillState::Active); + let source_pending = staged_source.pending_update.as_ref().unwrap(); + assert_eq!( + source_pending.resulting_state, + Some(ManagedSkillState::Archived) + ); + assert!(source_pending + .staged_reason + .as_deref() + .is_some_and(|reason| reason.contains("merged into 'automation-run-review'"))); + let staged_target = load_managed_skill(&profile_root, "automation-run-review") + .await + .unwrap(); + assert_eq!(staged_target.metadata.state, ManagedSkillState::Active); + assert_eq!(staged_target.metadata.checksum, target.metadata.checksum); + assert!(staged_target.pending_update.is_some()); + let untouched_pinned = load_managed_skill(&profile_root, "pinned-automation-guide") + .await + .unwrap(); + assert_eq!(untouched_pinned.metadata.state, ManagedSkillState::Active); + assert!(untouched_pinned.pending_update.is_none()); + + let archived = approve_managed_skill(&profile_root, "automation-run-checks") + .await + .unwrap(); + assert_eq!(archived.metadata.state, ManagedSkillState::Archived); + assert_eq!(archived.body_markdown, source.body_markdown); + let merged = approve_managed_skill(&profile_root, "automation-run-review") + .await + .unwrap(); + assert_eq!(merged.metadata.state, ManagedSkillState::Active); + assert!(merged.body_markdown.contains("approval gates")); +}