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..e3cc3a83 100644 --- a/src/automation/skill_writer.rs +++ b/src/automation/skill_writer.rs @@ -7,25 +7,44 @@ 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, - 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, - 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; +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 +/// `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 +53,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 +110,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 +198,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 +435,8 @@ fn skill_draft_from_proposal( enum SkillProposalAction { Create, Update, + Merge, + Archive, } impl SkillProposalAction { @@ -347,6 +444,8 @@ impl SkillProposalAction { match self { SkillProposalAction::Create => "create", SkillProposalAction::Update => "update", + SkillProposalAction::Merge => "merge", + SkillProposalAction::Archive => "archive", } } } @@ -361,6 +460,8 @@ 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}'")), } } 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/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_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")); +}