diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index 0dcb9508..284694e3 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -179,11 +179,8 @@ pub trait Host { )) } - fn update_selected(&self, names: &[String]) -> Result, CommandError> { - let names = parse_update_selection(names)?; - if let [name] = names.as_slice() { - return self.update(Some(name)); - } + fn update_selected(&self, items: &[UpdatePlanItem]) -> Result, CommandError> { + validate_update_selection(items)?; Err(CommandError::unsupported_host( "Selected Skill updates are unavailable on this host", )) @@ -1498,12 +1495,12 @@ impl Host for LocalHost { .collect()) } - fn update_selected(&self, names: &[String]) -> Result, CommandError> { - let names = parse_update_selection(names)?; + fn update_selected(&self, items: &[UpdatePlanItem]) -> Result, CommandError> { + validate_update_selection(items)?; let scope = InstallScope::Project; let known = self.known_targets(scope)?; let store = self.store(scope); - apply_update_selection(self, names, store, known) + apply_update_selection(self, items, store, known) } fn update_check(&self, requested: Option<&str>) -> Result { @@ -1691,20 +1688,31 @@ struct PreparedUpdateSelection { fn apply_update_selection( host: &LocalHost, - names: Vec, + items: &[UpdatePlanItem], store: LocalStore, known: Vec, ) -> Result, CommandError> { let provider = host.remote_provider()?; let mut pending = Vec::new(); - for name in names { - let skill_name = - skilld_core::SkillName::parse(name.clone()).map_err(CommandError::domain)?; + for item in items { + let name = item.name().as_str().to_owned(); + let UpdateRelation::Available { + locked_commit_sha, + latest_commit_sha, + .. + } = item.relation() + else { + unreachable!("the update selection was validated") + }; + let skill_name = item.name().clone(); let view = store .verify_content(&skill_name, &known) .map_err(CommandError::store)?; - let LockedSource::Remote { source, .. } = &view.skill.source else { - continue; + let LockedSource::Remote { + source, commit_sha, .. + } = &view.skill.source + else { + return Err(stale_update_plan(&name)); }; if !matches!( view.skill.source_status, @@ -1717,24 +1725,25 @@ fn apply_update_selection( } let selector = skilld_core::RemoteSelector::parse(source).map_err(CommandError::remote)?; if matches!(selector.source().r#ref, Some(SourceRef::Commit { .. })) { - continue; + return Err(stale_update_plan(&name)); + } + let installed_commit_sha = + CommitSha::parse(commit_sha.clone()).map_err(update_model_error)?; + if &installed_commit_sha != locked_commit_sha { + return Err(stale_update_plan(&name)); } let latest_commit = provider .latest_commit(&selector, false) .map_err(CommandError::remote)?; - let LockedSource::Remote { commit_sha, .. } = &view.skill.source else { - unreachable!("the update candidate has a remote source") - }; - let locked_commit_sha = CommitSha::parse(commit_sha.clone()).map_err(update_model_error)?; - if latest_commit.commit_sha == locked_commit_sha { - continue; + if &latest_commit.commit_sha != latest_commit_sha { + return Err(stale_update_plan(&name)); } let comparison = RemoteUpdateComparison::new( skill_name.as_str(), &selector.source().owner, &selector.source().repository, - locked_commit_sha, - latest_commit.commit_sha.clone(), + locked_commit_sha.clone(), + latest_commit_sha.clone(), latest_commit.access, ) .map_err(CommandError::remote)?; @@ -1743,7 +1752,7 @@ fn apply_update_selection( skill_name, view, selector, - expected_commit: latest_commit.commit_sha, + expected_commit: latest_commit_sha.clone(), comparison, }); } @@ -2081,26 +2090,38 @@ fn selected_names( } } -fn parse_update_selection(names: &[String]) -> Result, CommandError> { - if names.is_empty() { +fn validate_update_selection(items: &[UpdatePlanItem]) -> Result<(), CommandError> { + if items.is_empty() { return Err(CommandError::usage( "INVALID_SELECTION", "Select at least one Skill", )); } let mut unique = BTreeSet::new(); - let mut parsed = Vec::with_capacity(names.len()); - for name in names { - let name = skilld_core::SkillName::parse(name.clone()).map_err(CommandError::domain)?; - if !unique.insert(name.clone()) { + for item in items { + if !unique.insert(item.name()) { return Err(CommandError::usage( "INVALID_SELECTION", "Select each Skill once", )); } - parsed.push(name.to_string()); } - Ok(parsed) + for item in items { + if !matches!(item.relation(), UpdateRelation::Available { .. }) { + return Err(CommandError::usage( + "INVALID_SELECTION", + "Select only Skills with available updates", + )); + } + } + Ok(()) +} + +fn stale_update_plan(name: &str) -> CommandError { + CommandError::operation( + "STALE_UPDATE_PLAN", + format!("Skill {name} changed after review. Review its commits again"), + ) } fn unavailable_update( diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index dfe34675..788e2f99 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -19,7 +19,7 @@ use skilld_core::{ CheckResult, CommitAuthor, CommitSha, CommitSummary, InstallMode, InstallOperation, InstallRequest, InstallScope, InstallSource, LockedSource, PreparedFile, RemoteError, RemoteSelector, RepositoryVisibility, ResolvedSource, SearchResponse, SignatureAlgorithm, - SourceProvider, SourceStatus, TrustedRootPin, UpdatePlanV1, UpdateRelation, + SourceProvider, SourceStatus, TrustedRootPin, UpdatePlanItem, UpdatePlanV1, UpdateRelation, }; const ROOT_DOMAIN: &[u8] = b"skilld-trusted-key-v1\0"; @@ -1528,6 +1528,7 @@ fn provider(content: &str) -> Arc { struct BatchProvider { version: Mutex<&'static str>, + latest_commit: Mutex, prepared_names: Mutex>, fail_name: Mutex>, relation: Mutex, @@ -1615,7 +1616,8 @@ impl RemoteProvider for BatchProvider { _direct: bool, ) -> Result { Ok(skilld_command::RemoteLatestCommit { - commit_sha: CommitSha::parse("f".repeat(40)).unwrap(), + commit_sha: CommitSha::parse(self.latest_commit.lock().unwrap().to_string().repeat(40)) + .unwrap(), access: RemoteComparisonAccess::PublicGithub, }) } @@ -1678,6 +1680,7 @@ fn multi_skill_update_prepares_then_commits_every_artifact() { fs::create_dir_all(&project).unwrap(); let provider = Arc::new(BatchProvider { version: Mutex::new("first"), + latest_commit: Mutex::new('f'), prepared_names: Mutex::new(vec![]), fail_name: Mutex::new(None), relation: Mutex::new(RemoteComparisonRelation::Ahead), @@ -1721,6 +1724,7 @@ fn multi_skill_update_changes_nothing_when_one_artifact_cannot_prepare() { fs::create_dir_all(&project).unwrap(); let provider = Arc::new(BatchProvider { version: Mutex::new("first"), + latest_commit: Mutex::new('f'), prepared_names: Mutex::new(vec![]), fail_name: Mutex::new(None), relation: Mutex::new(RemoteComparisonRelation::Ahead), @@ -1766,6 +1770,7 @@ fn plain_update_rejects_a_source_that_moved_behind() { fs::create_dir_all(&project).unwrap(); let provider = Arc::new(BatchProvider { version: Mutex::new("first"), + latest_commit: Mutex::new('f'), prepared_names: Mutex::new(vec![]), fail_name: Mutex::new(None), relation: Mutex::new(RemoteComparisonRelation::Ahead), @@ -1795,6 +1800,20 @@ fn plain_update_rejects_a_source_that_moved_behind() { ); } +fn reviewed_updates(host: &LocalHost, names: &[&str]) -> Vec { + let plan = host.update_check(None).unwrap(); + names + .iter() + .map(|name| { + plan.items() + .iter() + .find(|item| item.name().as_str() == *name) + .unwrap() + .clone() + }) + .collect() +} + #[test] fn selected_skill_update_commits_only_the_exact_subset() { let temporary = tempfile::tempdir().unwrap(); @@ -1802,6 +1821,7 @@ fn selected_skill_update_commits_only_the_exact_subset() { fs::create_dir_all(&project).unwrap(); let provider = Arc::new(BatchProvider { version: Mutex::new("first"), + latest_commit: Mutex::new('f'), prepared_names: Mutex::new(vec![]), fail_name: Mutex::new(None), relation: Mutex::new(RemoteComparisonRelation::Ahead), @@ -1821,10 +1841,9 @@ fn selected_skill_update_commits_only_the_exact_subset() { } provider.prepared_names.lock().unwrap().clear(); *provider.version.lock().unwrap() = "second"; + let reviewed = reviewed_updates(&host, &["gamma", "alpha"]); - let lines = host - .update_selected(&["gamma".to_owned(), "alpha".to_owned()]) - .unwrap(); + let lines = host.update_selected(&reviewed).unwrap(); assert_eq!(lines, ["Updated Skill gamma.", "Updated Skill alpha."]); assert_eq!(*provider.prepared_names.lock().unwrap(), ["gamma", "alpha"]); @@ -1837,24 +1856,34 @@ fn selected_skill_update_commits_only_the_exact_subset() { } #[test] -fn selected_skill_update_rejects_empty_duplicate_and_invalid_names() { +fn selected_skill_update_rejects_empty_duplicate_and_unavailable_items() { let temporary = tempfile::tempdir().unwrap(); let host = LocalHost::new( temporary.path().join("project"), temporary.path().join("data"), ); + let current = UpdatePlanItem::new( + skilld_core::SkillName::parse("alpha").unwrap(), + UpdateRelation::Current { + commit_sha: CommitSha::parse("1".repeat(40)).unwrap(), + }, + ); let empty = host.update_selected(&[]).unwrap_err(); let duplicate = host - .update_selected(&["alpha".to_owned(), "alpha".to_owned()]) + .update_selected(&[current.clone(), current.clone()]) .unwrap_err(); - let invalid = host.update_selected(&["../alpha".to_owned()]).unwrap_err(); + let invalid_relation = host.update_selected(&[current]).unwrap_err(); assert_eq!(empty.code, "INVALID_SELECTION"); assert_eq!(empty.message, "Select at least one Skill"); assert_eq!(duplicate.code, "INVALID_SELECTION"); assert_eq!(duplicate.message, "Select each Skill once"); - assert_eq!(invalid.code, "INVALID_SOURCE"); + assert_eq!(invalid_relation.code, "INVALID_SELECTION"); + assert_eq!( + invalid_relation.message, + "Select only Skills with available updates" + ); } #[test] @@ -1864,6 +1893,7 @@ fn selected_skill_update_changes_nothing_when_one_selected_artifact_fails() { fs::create_dir_all(&project).unwrap(); let provider = Arc::new(BatchProvider { version: Mutex::new("first"), + latest_commit: Mutex::new('f'), prepared_names: Mutex::new(vec![]), fail_name: Mutex::new(None), relation: Mutex::new(RemoteComparisonRelation::Ahead), @@ -1883,11 +1913,10 @@ fn selected_skill_update_changes_nothing_when_one_selected_artifact_fails() { } provider.prepared_names.lock().unwrap().clear(); *provider.version.lock().unwrap() = "second"; + let reviewed = reviewed_updates(&host, &["alpha", "gamma"]); *provider.fail_name.lock().unwrap() = Some("gamma"); - let error = host - .update_selected(&["alpha".to_owned(), "gamma".to_owned()]) - .unwrap_err(); + let error = host.update_selected(&reviewed).unwrap_err(); assert_eq!(error.code, "CHECK_BLOCKED"); assert_eq!(*provider.prepared_names.lock().unwrap(), ["alpha", "gamma"]); @@ -1899,6 +1928,44 @@ fn selected_skill_update_changes_nothing_when_one_selected_artifact_fails() { } } +#[test] +fn selected_skill_update_rejects_a_head_that_changed_after_review() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(BatchProvider { + version: Mutex::new("first"), + latest_commit: Mutex::new('f'), + prepared_names: Mutex::new(vec![]), + fail_name: Mutex::new(None), + relation: Mutex::new(RemoteComparisonRelation::Ahead), + }); + let host = LocalHost::new(project.clone(), temporary.path().join("data")) + .with_remote_provider(provider.clone()); + host.install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::Remote( + "skilld:skilld-dev/skills/alpha".to_owned(), + )), + scope: InstallScope::Project, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); + provider.prepared_names.lock().unwrap().clear(); + *provider.version.lock().unwrap() = "second"; + let reviewed = reviewed_updates(&host, &["alpha"]); + *provider.latest_commit.lock().unwrap() = 'e'; + + let error = host.update_selected(&reviewed).unwrap_err(); + + assert_eq!(error.code, "STALE_UPDATE_PLAN"); + assert!(provider.prepared_names.lock().unwrap().is_empty()); + assert_eq!( + fs::read_to_string(project.join(".skills/alpha/SKILL.md")).unwrap(), + "---\nname: alpha\ndescription: first\n---\n" + ); +} + #[test] fn verify_reports_changed_bytes_and_stale_sources() { let temporary = tempfile::tempdir().unwrap(); diff --git a/crates/skilld-native/src/update_ui.rs b/crates/skilld-native/src/update_ui.rs index 159a9e0b..ad3919dc 100644 --- a/crates/skilld-native/src/update_ui.rs +++ b/crates/skilld-native/src/update_ui.rs @@ -20,7 +20,7 @@ use ratatui::style::{Color, Modifier, Style}; use ratatui::text::{Line, Text}; use ratatui::widgets::{Block, List, ListItem, ListState, Paragraph, Tabs}; use skilld_command::{CommandError, Host}; -use skilld_core::{CommitHistory, UpdatePlanV1, UpdateRelation, UpdateRetryAfter}; +use skilld_core::{CommitHistory, UpdatePlanItem, UpdatePlanV1, UpdateRelation, UpdateRetryAfter}; use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; use url::Url; @@ -229,14 +229,20 @@ pub trait InteractiveUpdateHost: Send + Sync + 'static { pub struct CommandInteractiveUpdateHost { host: Arc, - commits: Mutex>, + plan: Mutex, +} + +#[derive(Default)] +struct CachedInteractivePlan { + commits: BTreeMap, + items: BTreeMap, } impl CommandInteractiveUpdateHost { pub fn new(host: Arc) -> Self { Self { host, - commits: Mutex::new(BTreeMap::new()), + plan: Mutex::new(CachedInteractivePlan::default()), } } } @@ -245,18 +251,24 @@ impl InteractiveUpdateHost for CommandInteracti fn load_candidates(&self) -> Result, InteractiveUpdateError> { let plan = self.host.update_check(None).map_err(command_error)?; let (candidates, commits) = prepare_interactive_plan(&plan); - *self.commits.lock().map_err(|_| { + let items = plan + .items() + .iter() + .filter(|item| matches!(item.relation(), UpdateRelation::Available { .. })) + .map(|item| (item.name().as_str().to_owned(), item.clone())) + .collect(); + *self.plan.lock().map_err(|_| { InteractiveUpdateError::new( "SERVICE_UNAVAILABLE", - "The repository commit cache could not be updated.", + "The reviewed update plan could not be saved.", ) - })? = commits; + })? = CachedInteractivePlan { commits, items }; Ok(candidates) } fn load_commits(&self, comparisons: &[ComparisonId]) -> Vec { - let commits = match self.commits.lock() { - Ok(commits) => commits, + let plan = match self.plan.lock() { + Ok(plan) => plan, Err(_) => { return comparisons .iter() @@ -276,7 +288,7 @@ impl InteractiveUpdateHost for CommandInteracti comparisons .iter() .cloned() - .map(|comparison| match commits.get(&comparison) { + .map(|comparison| match plan.commits.get(&comparison) { Some(page) => CommitLoadResult::ready(comparison, page.clone()), None => CommitLoadResult::failed( comparison, @@ -290,7 +302,38 @@ impl InteractiveUpdateHost for CommandInteracti } fn apply(&self, names: &[String]) -> Vec { - match self.host.update_selected(names) { + let items = self + .plan + .lock() + .map_err(|_| { + InteractiveUpdateError::new( + "SERVICE_UNAVAILABLE", + "The reviewed update plan could not be read.", + ) + }) + .and_then(|plan| { + names + .iter() + .map(|name| { + plan.items.get(name).cloned().ok_or_else(|| { + InteractiveUpdateError::new( + "STALE_UPDATE_PLAN", + format!("Skill {name} needs another commit review."), + ) + }) + }) + .collect::, _>>() + }); + let items = match items { + Ok(items) => items, + Err(error) => { + return names + .iter() + .map(|name| ApplyResult::failed(name, error.clone())) + .collect(); + } + }; + match self.host.update_selected(&items) { Ok(_) => names.iter().map(ApplyResult::updated).collect(), Err(error) => { let error = command_error(error); diff --git a/crates/skilld-native/tests/update_ui.rs b/crates/skilld-native/tests/update_ui.rs index 156a1a95..debe72ac 100644 --- a/crates/skilld-native/tests/update_ui.rs +++ b/crates/skilld-native/tests/update_ui.rs @@ -396,7 +396,7 @@ fn fixture_host_resolves_model_effects_without_terminal_access() { struct PlanHost { plan: UpdatePlanV1, - selections: Mutex>>, + selections: Mutex>>, apply_error: Option, } @@ -417,14 +417,14 @@ impl Host for PlanHost { Ok(self.plan.clone()) } - fn update_selected(&self, names: &[String]) -> Result, CommandError> { - self.selections.lock().unwrap().push(names.to_vec()); + fn update_selected(&self, items: &[UpdatePlanItem]) -> Result, CommandError> { + self.selections.lock().unwrap().push(items.to_vec()); if let Some(error) = self.apply_error.clone() { return Err(error); } - Ok(names + Ok(items .iter() - .map(|name| format!("Updated Skill {name}.")) + .map(|item| format!("Updated Skill {}.", item.name().as_str())) .collect()) } } @@ -463,6 +463,14 @@ fn command_plan_host(apply_error: Option) -> Arc { ) .unwrap(), ); + let second_available = UpdatePlanItem::new( + SkillName::parse("smoke-skill").unwrap(), + UpdateRelation::Available { + locked_commit_sha: locked.clone(), + latest_commit_sha: latest.clone(), + ahead_by: NonZeroU64::new(1).unwrap(), + }, + ); let unavailable = UpdatePlanItem::new( SkillName::parse("review-skill").unwrap(), UpdateRelation::Unavailable { @@ -479,7 +487,9 @@ fn command_plan_host(apply_error: Option) -> Arc { UpdateRelation::Current { commit_sha: locked }, ); Arc::new(PlanHost { - plan: UpdatePlanV1::new(UpdatePlan::new(vec![available, unavailable, current]).unwrap()), + plan: UpdatePlanV1::new( + UpdatePlan::new(vec![available, second_available, unavailable, current]).unwrap(), + ), selections: Mutex::new(Vec::new()), apply_error, }) @@ -515,7 +525,16 @@ fn command_host_maps_update_plans_commits_and_exact_selection() { interactive.apply(&["grill-me".to_owned()]), [ApplyResult::updated("grill-me")] ); - assert_eq!(*host.selections.lock().unwrap(), [["grill-me".to_owned()]]); + let selections = host.selections.lock().unwrap(); + assert_eq!(selections.len(), 1); + assert_eq!(selections[0][0].name().as_str(), "grill-me"); + assert!(matches!( + selections[0][0].relation(), + UpdateRelation::Available { + latest_commit_sha, + .. + } if latest_commit_sha.as_str() == "2222222222222222222222222222222222222222" + )); } #[test] @@ -525,7 +544,8 @@ fn command_host_reports_one_atomic_failure_for_every_selected_skill() { "A required check failed.", ))); let interactive = CommandInteractiveUpdateHost::new(host.clone()); - let names = ["grill-me".to_owned(), "review-skill".to_owned()]; + interactive.load_candidates().unwrap(); + let names = ["grill-me".to_owned(), "smoke-skill".to_owned()]; let results = interactive.apply(&names);