diff --git a/changelog.d/fixed/collection-replacements.md b/changelog.d/fixed/collection-replacements.md new file mode 100644 index 0000000000..ea29857e5a --- /dev/null +++ b/changelog.d/fixed/collection-replacements.md @@ -0,0 +1 @@ +- Collection installs replace members recorded below the catalog minimum in either repository order and refuse incompatible retained revisions before writing. Snapshot holds apply only to explicit collection members and preserve automatic skills' revision choices. Shared plan previews name every new subscription. diff --git a/crates/cli/src/commands/add_collection.rs b/crates/cli/src/commands/add_collection.rs index 0de0cc1ce9..2be631ae6f 100644 --- a/crates/cli/src/commands/add_collection.rs +++ b/crates/cli/src/commands/add_collection.rs @@ -1,16 +1,16 @@ -//! `kendex add https://kendex.ai/c/` — one link, one preview, then: +//! `kendex add https://kendex.ai/c/`: one link, one preview, then: //! subscribe each repository the scope lacks (at the snapshot commit) and //! install every member. Existing subscriptions are reused only when -//! their revision matches the snapshot; the steps refuse before anything -//! changes otherwise. +//! their revision matches the snapshot. Core refuses a mismatch before +//! anything changes and checks the whole collection before this command +//! applies its single plan. -use kendex_core::engine::ops::{self, AddRequest}; use kendex_core::env::Env; use kendex_core::model::Scope; use kendex_core::registry::{CurlFetch, collections}; use kendex_core::source_ops::{self, SourceAction}; -use super::advisory::{Listing, print_safety}; +use super::advisory::Listing; use super::engine_common::{apply_report, ask_before_writing, print_report}; use super::ledger::{Folded, Wrote, say_ledger}; use super::offers::Blocked; @@ -57,26 +57,25 @@ pub fn run(env: &Env, scope: &Scope, id: &str, yes: bool, allow_effects: bool) - ), yes, )?; - // Every repository is fetched and every member proven present before - // the first mutation — a collection whose third repository is broken - // must refuse up front, not leave the first two half-installed. - prevalidate(env, scope, &steps)?; - // Each add plan also checks the recorded revisions it keeps. A later - // step can refuse one after earlier steps installed. The error is - // held until the close has reported those writes. - let (mut closing, failed) = - install_steps(steps, |step, wrote| install_step(env, scope, step, wrote)); + let prepared = source_ops::prepare_collection(env, scope, &steps)?; + let subscribed: Vec = prepared.subscription_names().map(str::to_owned).collect(); + let report = prepared.plan(env)?; + let mut wrote = Written::default(); + let failed = install_report(env, &report, &mut wrote).err(); + if failed.is_none() { + for name in subscribed { + say(&format!("{}: subscribed to '{name}'", scope_label(scope))); + } + } + let mut closing = closing(vec![wrote]); let skipped = std::mem::take(&mut closing.skipped); - // A member skipped on conflict is not a step failure, and it is not - // installed either: the line would claim a member the ledger then - // names as skipped, terminal or not. + // A successful apply can skip a conflicting member. The success line + // must not claim that member installed when the ledger names it skipped. if failed.is_none() && skipped.is_empty() { say("collection installed — kendex recorded every member at its version"); } - // The same close `add ` gives, over every step at once: a - // collection is one install, and a run that opened a frame has to end - // on what it wrote, skipped and flagged like any other. The parts are - // the ones each step already counted, never re-derived. + // The collection's single apply supplies the same ledger as an item + // add: what it wrote, skipped and flagged. let close = || { say_ledger( scope, @@ -117,9 +116,7 @@ pub fn run(env: &Env, scope: &Scope, id: &str, yes: bool, allow_effects: bool) - /// registration of the folder the packages landed in. /// /// The parts are handed in so the order can be asked without a registry, a -/// network and a git host — the same reason [`install_steps`] takes its -/// installer. What it decides is the whole of what a reviewer of this file -/// would otherwise have to read the callers to know. +/// network and a git host. /// /// The registration runs on every arm where packages landed, and never /// behind the account's answer: a package's declared installer exiting @@ -127,9 +124,8 @@ pub fn run(env: &Env, scope: &Scope, id: &str, yes: bool, allow_effects: bool) - /// folder the app cannot see is what the registration exists to prevent. A /// run that wrote nothing installed into nowhere and registers nothing. /// -/// What comes back is what the run reports — the step's failure first, -/// then the account's — and, separately, a registry refusal to be said -/// beside it rather than in place of it. +/// The apply failure takes priority over a repository-effects failure. +/// A registry refusal is reported beside that failure. type Refusal = Box; fn finish( @@ -144,9 +140,8 @@ fn finish( false => Ok(()), }; if let Some(error) = failed { - // A step failed with earlier steps already installed. What they - // wrote is reported before the error goes up, and the repository - // account is not asked for on a run that is already failing. + // A failed apply reports its ledger before the error goes up. + // Repository effects are not run after an installation failure. close(); return (Err(error), registered(wrote).err()); } @@ -158,36 +153,8 @@ fn finish( } } -/// Take the steps in order, stopping at the first failure, and reduce -/// what the run wrote into its close. -/// -/// The written set, never the succeeded set. A step is not outcome-free -/// before it fails: it can apply a new subscription, apply the pins that -/// hold reused members at the snapshot, and print blocked and safety rows -/// before a later fetch returns an error. Dropping that with the error -/// closes a run that changed the repository on a ledger naming less than -/// it did. Each step fills its own [`Written`] as it goes, so the outcome -/// is kept whichever way the step returned. -fn install_steps( - steps: Vec, - mut install: impl FnMut(kendex_core::source_ops::CollectionStep, &mut Written) -> CliResult, -) -> (Closing, Option>) { - let mut written: Vec = Vec::new(); - let mut failed: Option> = None; - for step in steps { - let mut wrote = Written::default(); - let outcome = install(step, &mut wrote); - written.push(wrote); - if let Err(error) = outcome { - failed = Some(error); - break; - } - } - (closing(written), failed) -} - /// What the run says at the end: the ledger's count, the collapsed -/// repository-effects screen, and the rows each step could not settle. +/// repository-effects screen, and the members the plan could not install. struct Closing { count: Option, pending: Vec, @@ -201,7 +168,7 @@ struct Closing { fn closing(written: Vec) -> Closing { // One screen for the collection, and one question. // - // A package can arrive by more than one route in a single command — the + // A package can arrive by more than one route in a single command: the // repository that carries it, and a dependency of something else — and a // person should read what it does to their repository once. Collapsed by // name over the whole run's writes. @@ -214,13 +181,9 @@ fn closing(written: Vec) -> Closing { } let pending: Vec = once.into_values().cloned().collect(); - // Read off what the run applied, not off the member plans alone: a - // reused source whose member is already declared plans nothing and - // still writes — its subscription, or the pin that holds it at the - // snapshot — and a ledger deciding from the plan would call that run - // up to date over changes it had just made. `None` only where - // nothing was planned and nothing was written, the way `add` reads a - // scope that had nothing to do. + // Subscriptions and snapshot pins are part of the collection plan. + // Count applied operations and keep a planned apply that wrote nothing + // distinct from an empty plan. let count = wrote_count( written.iter().map(|step| step.applied).sum(), written.iter().any(|step| step.planned), @@ -244,26 +207,21 @@ fn closing(written: Vec) -> Closing { /// Whether the run has a count to report, and what it is. /// -/// `None` only where nothing was planned and nothing was written, which -/// is how `add` reads a scope that had nothing to do. Read off the member -/// plans alone it says "up to date" over a run that wrote: a reused -/// source whose member is already declared plans nothing and still writes -/// its subscription, or the pin holding it at the snapshot. +/// An empty plan that wrote nothing has no count. A planned apply that +/// wrote nothing reports zero. Applied operations always have a count. fn wrote_count(applied: usize, planned_anything: bool) -> Option { (applied > 0 || planned_anything).then_some(applied) } -/// What one step wrote, and what it could not, gathered as it writes it. -/// The collection's closing ledger and its repository-effects screen are -/// the sum of these, so each step hands back the counts it already took -/// rather than leaving them to be worked out again — and a step that -/// fails hands back everything it had written before it did. +/// The collection plan's findings and its apply count. A failed apply +/// retains the findings for the ledger; only a completed apply supplies +/// the count of written operations. #[derive(Default)] struct Written { - /// The repository effects this step's plan declared. + /// The repository effects the collection plan declared. effects: Vec, - /// Whether the step planned anything at all, which is the one thing - /// separating a silent run from one that wrote nothing. + /// Whether the collection plan contains operations, separating an + /// empty plan from an apply that wrote nothing. planned: bool, blocked: Vec, skipped: Vec<(kendex_core::model::ItemKind, String)>, @@ -271,199 +229,27 @@ struct Written { applied: usize, } -/// Subscribe (or reuse), install every member, and — for a reused -/// subscription that may track a moved branch — pin each member to the -/// snapshot commit so what installs is the snapshot, not the branch head. -/// -/// A fresh subscription rides in the members' own plan, so an install the -/// add refuses — a member no tool on this machine can take — leaves the -/// scope subscribed to nothing rather than holding a subscription it -/// installed nothing from. Every write lands in `wrote` before the next -/// fallible call, so the error path carries what the step had already -/// done. -fn install_step( +/// Apply the core collection plan and retain its outcome for the ledger. +fn install_report( env: &Env, - scope: &Scope, - step: kendex_core::source_ops::CollectionStep, + report: &kendex_core::engine::EngineReport, wrote: &mut Written, ) -> CliResult { - let members: Vec<(kendex_core::model::ItemKind, String)> = step - .members() - .map(|(kind, name)| (kind, name.clone())) - .collect(); - let request = AddRequest { - agents: step.agents.clone(), - skills: step.skills.clone(), - hooks: step.hooks.clone(), - commands: step.commands.clone(), - mcp_servers: step.mcp_servers.clone(), - output_styles: step.output_styles.clone(), - ..AddRequest::default() - }; - // Prevalidation fetched both snapshots and reused subscriptions. The - // write loop reads those checked cache resolutions without fetching a - // branch that could select another catalog after an earlier write. - let (report, subscribed) = match &step.action { - SourceAction::Reuse { name } => { - let report = ops::add( - env, - scope, - &AddRequest { - source: Some(name.clone()), - ..request - }, - )?; - (report, None) - } - SourceAction::Subscribe { reference } => { - let subscribed = source_ops::subscribe_and_install(env, scope, reference, &request)?; - (subscribed.report, Some(subscribed.name)) - } - }; - // The step's own plan goes to the caller, which discloses over the - // whole collection at once. Nothing here runs an effect. wrote.effects.extend(report.repo_effects.iter().cloned()); wrote.planned = !report.plan.is_empty(); - let blocked = print_report(env, &report, Listing::Attention); + let blocked = print_report(env, report, Listing::Attention); wrote.skipped.extend(report.skipped_asked()); wrote.blocked.extend(blocked); wrote.scored.extend(report.safety.iter().cloned()); - wrote.applied += apply_report(env, &report)?; - if let Some(name) = subscribed { - say(&format!("{}: subscribed to '{name}'", scope_label(scope))); - } - if let SourceAction::Reuse { .. } = &step.action - && let Some(commit) = &step.commit - { - for (kind, name) in &members { - let pinned = kendex_core::package::set_rev(env, scope, *kind, name, Some(commit))?; - // The pin's rows reach the collection's closing ledger with - // the step's own, which speaks for what this folds. - print_safety(&pinned.safety, Listing::Attention); - wrote.scored.extend(pinned.safety.iter().cloned()); - wrote.applied += apply_report(env, &pinned)?; - } - } - Ok(()) -} - -/// Fetch collection snapshots and reused subscription resolutions before -/// the first step. A cached branch may still match the snapshot even when -/// its upstream already requires a newer engine. -fn prevalidate( - env: &Env, - scope: &Scope, - steps: &[kendex_core::source_ops::CollectionStep], -) -> CliResult { - for step in steps { - prevalidate_snapshot(env, step)?; - } - let manifest = kendex_core::engine::ops::manifest_for_reading(env, scope)?; - for step in steps { - if let SourceAction::Reuse { name } = &step.action - && let Some(decl) = manifest.sources.get(name) - && let Some(repo) = &decl.repo - { - kendex_core::remote::sync(env, repo, decl.rev.as_deref())?; - } - } - kendex_core::source::check_catalog_compatibility(env, scope, &manifest)?; - Ok(()) -} - -fn prevalidate_snapshot(env: &Env, step: &kendex_core::source_ops::CollectionStep) -> CliResult { - let resolution = kendex_core::remote::sync(env, &step.repo, step.commit.as_deref()) - .map_err(|error| format!("{}: {error}", step.repo))?; - let sealed = kendex_core::source_read::SealedSource::open(&resolution.root)?; - let config = - kendex_core::source::source_config(&sealed, kendex_core::source::repo_leaf(&step.repo))?; - for (kind, name) in step.members() { - if kendex_core::source::find_item(&sealed, &config, kind, name).is_none() { - return Err(format!( - "{} does not offer {} '{name}' at the collection's snapshot — nothing was installed", - step.repo, - kind.name() - ) - .into()); - } - } + wrote.applied += apply_report(env, report)?; Ok(()) } #[cfg(test)] mod tests { - use super::{Written, finish, install_steps, wrote_count}; - use kendex_core::repo_effects::{DeclaredEffects, RepoEffects}; - use kendex_core::source_ops::{CollectionStep, SourceAction}; - - fn step(repo: &str) -> CollectionStep { - CollectionStep { - repo: repo.to_owned(), - commit: None, - action: SourceAction::Subscribe { - reference: repo.to_owned(), - }, - agents: Vec::new(), - skills: vec!["guard".to_owned()], - hooks: Vec::new(), - commands: Vec::new(), - mcp_servers: Vec::new(), - output_styles: Vec::new(), - } - } - - fn declared(name: &str) -> DeclaredEffects { - DeclaredEffects { - name: name.to_owned(), - root: std::path::PathBuf::from("packages").join(name), - effects: RepoEffects { - summary: "arms this repository's commit hooks".to_owned(), - writes: Vec::new(), - installer: None, - uninstaller: None, - checker: None, - staged_checker: None, - removal: None, - notes: Vec::new(), - companions: Vec::new(), - unknown_keys: Vec::new(), - }, - } - } - - /// A step that subscribed and then failed changed the repository, so - /// the close names what it wrote: its subscription in the ledger's - /// count, and the declaration it installed on the repository-effects - /// screen. Read off the steps that succeeded alone, a run that had - /// just written twice closes on one. - #[test] - fn a_failed_step_still_names_what_it_wrote() { - let (closing, failed) = install_steps( - vec![step("owner/first"), step("owner/second")], - |step, wrote: &mut Written| { - wrote.applied += 1; - wrote.planned = true; - wrote.effects.push(declared(&step.repo)); - match step.repo.as_str() { - "owner/second" => Err("owner/second moved under the run".into()), - _ => Ok(()), - } - }, - ); - assert!(failed.is_some()); - assert_eq!(closing.count, Some(2)); - let named: Vec<&str> = closing - .pending - .iter() - .map(|effects| effects.name.as_str()) - .collect(); - assert_eq!(named, ["owner/first", "owner/second"]); - } - - /// A run that wrote reports what it wrote, whatever the member plans - /// said. The pin and the subscription are writes the member plan - /// never carries, so a ledger deciding from that plan alone calls a - /// run that has just changed the scope up to date. + use super::{finish, wrote_count}; + /// Applied operations have a count regardless of the planned flag. + /// An empty plan differs from a planned apply that wrote nothing. #[test] fn a_run_that_wrote_never_reads_as_up_to_date() { assert_eq!(wrote_count(2, false), Some(2)); @@ -476,8 +262,7 @@ mod tests { /// Every way a collection run can end, and what each does about the /// registry. A collection needs a directory service, a git host and a - /// registry to reach this by any other route, so the parts are handed - /// in — the shape `install_steps` above is already tested through. + /// registry to reach this by any other route, so the parts are handed in. /// /// The claim every row makes is the one the issue turns on: the folder /// the packages landed in is registered on every arm where they @@ -485,7 +270,7 @@ mod tests { /// nothing was written. #[test] fn the_close_registers_wherever_packages_landed() { - // wrote, a step failed, the effects step's answer, the registry's; + // wrote, the apply failed, the effects answer, the registry's; // then what ran (closed, effects, registered), whether the run // came back ok, and whether a registry refusal was handed back to // be said beside it. @@ -503,7 +288,7 @@ mod tests { false, ), ( - "step failed, wrote", + "apply failed, wrote", true, true, true, @@ -513,7 +298,7 @@ mod tests { false, ), ( - "step failed, wrote nothing", + "apply failed, wrote nothing", false, true, true, diff --git a/crates/core/src/engine/ops.rs b/crates/core/src/engine/ops.rs index 3b6f81b5b8..d13a568328 100644 --- a/crates/core/src/engine/ops.rs +++ b/crates/core/src/engine/ops.rs @@ -11,7 +11,7 @@ use crate::model::{HarnessId, ItemKind, Scope}; mod add; pub use add::{AddRequest, add, add_seeded, requested_kinds, targets_for}; -pub(crate) use add::{protect_installed, source_repo_label}; +pub(crate) use add::{PreparedAddition, protect_installed, source_repo_label}; /// Every kind a manifest declares by name. Plugins are excluded: they carry /// only an enabled flag, in their own table. diff --git a/crates/core/src/engine/ops/add.rs b/crates/core/src/engine/ops/add.rs index 7845855088..3ea49bc735 100644 --- a/crates/core/src/engine/ops/add.rs +++ b/crates/core/src/engine/ops/add.rs @@ -122,98 +122,159 @@ pub fn add_seeded( manifest_file: manifest::manifest_file_name(env, scope), }); } - let mut manifest = manifest_for_mutation(env, scope)?; - // Arrival is the manifest gaining a declaration, and it is the one - // thing that applies a settings template. Committed state, so a clone - // carrying no lock re-arrives nothing. Read expanded, because a bundle - // declaration accounts for its members and their own declarations are - // taken away as it does. - let declared = crate::engine::installed::skills_installed(env, scope, &manifest); - if let Some((name, decl)) = seed { - manifest.sources.insert(name, decl); - } - let mut notes = Vec::new(); - // A request that names its harnesses has answered which tools it goes - // to. One that leaves them to the scope reads the scope's list as - // written, filled from the machine only where it declares none. - if request.harnesses.is_none() - && let Some(left_out) = super::settle_defaults(env, scope, &mut manifest) - { - notes.push(left_out); - } - // Before a byte of the manifest is persisted: a request whose tools can - // take none of what it asks for would plan nothing, apply nothing, and - // report success. Asked once the scope's list is settled, so a request - // leaving the tools to that list is judged by the list the declaration - // would actually be written with — on a machine with no tool and no - // list, an empty one. - let targets = - crate::engine::desired::requested_or_default(request.harnesses.as_deref(), &manifest); - if let Some(reason) = lands_nowhere(request, &targets, scope) { - return Err(CoreError::InstallsNowhere { reason }); - } - let lock = crate::lock::load(&lock_path(env, scope))?; - let (mut groups, context) = place::place(env, scope, &mut manifest, request)?; - let all_source = match (request.all, &context) { - (false, _) => None, - (true, Some(ctx)) => Some(ctx.clone()), - (true, None) => Some(pick::default_source(&manifest)?), - }; - if let Some(source_name) = &all_source { - groups.entry(source_name.clone()).or_default(); + let mut prepared = PreparedAddition::new(env, scope)?; + prepared.include(env, request, seed, None, None)?; + prepared.plan(env) +} + +/// One add's declarations and recorded readings, shared by preparation, +/// compatibility checks and planning. Collections include every request +/// before checking retained revisions, so their repository order cannot +/// turn a replacement into a retained incompatible installation. +pub(crate) struct PreparedAddition { + scope: Scope, + manifest: Manifest, + lock: Lock, + declared: BTreeSet, + declaring: BTreeSet, + notes: Vec, + options: PlanOptions, +} + +impl PreparedAddition { + pub(crate) fn new(env: &Env, scope: &Scope) -> Result { + let manifest = manifest_for_mutation(env, scope)?; + let declared = crate::engine::installed::skills_installed(env, scope, &manifest); + Ok(Self { + scope: scope.clone(), + manifest, + lock: crate::lock::load(&lock_path(env, scope))?, + declared, + declaring: BTreeSet::new(), + notes: Vec::new(), + options: PlanOptions::for_additions([]), + }) } - let mut optional_offers: Vec<(String, String)> = Vec::new(); - let mut declaring: BTreeSet = BTreeSet::new(); - for (source_name, wanted) in &groups { - declaring.extend(add_from( - env, - scope, - &mut manifest, - &lock, - request, - source_name, - wanted, - all_source.as_deref() == Some(source_name), - &mut notes, - &mut optional_offers, - )?); + pub(crate) fn subscription_name(&self, repo: &str) -> String { + manifest::catalog_alias(&self.manifest, repo) } - // A choice naming an optional dependency nothing offers is an error - // that leaves the manifest exactly as it was — never a silently - // ignored flag. - for wanted in &request.optional { - if !optional_offers.iter().any(|(_, name)| name == wanted) { - return Err(CoreError::NoSuchOptional { - name: wanted.clone(), - source_name: groups.keys().cloned().collect::>().join(", "), - }); + + pub(crate) fn include( + &mut self, + env: &Env, + request: &AddRequest, + seed: Option<(String, crate::manifest::SourceDecl)>, + revision: Option<&str>, + member_hold_at: Option<&str>, + ) -> Result<()> { + let scope = &self.scope; + let manifest = &mut self.manifest; + let lock = &self.lock; + let notes = &mut self.notes; + if let Some((name, decl)) = seed { + manifest.sources.insert(name, decl); } - } - for (parent, name) in optional_offers { - let taken = manifest.optional_dependencies.entry(parent).or_default(); - if !taken.contains(&name) { - taken.push(name); - taken.sort(); + // A request that names its harnesses has answered which tools it goes + // to. One that leaves them to the scope reads the scope's list as + // written, filled from the machine only where it declares none. + if request.harnesses.is_none() + && let Some(left_out) = super::settle_defaults(env, scope, manifest) + { + notes.push(left_out); + } + // Before a byte of the manifest is persisted: a request whose tools can + // take none of what it asks for would plan nothing, apply nothing, and + // report success. Asked once the scope's list is settled, so a request + // leaving the tools to that list is judged by the list the declaration + // would actually be written with — on a machine with no tool and no + // list, an empty one. + let targets = + crate::engine::desired::requested_or_default(request.harnesses.as_deref(), manifest); + if let Some(reason) = lands_nowhere(request, &targets, scope) { + return Err(CoreError::InstallsNowhere { reason }); + } + let (mut groups, context) = place::place(env, scope, manifest, request)?; + let all_source = match (request.all, &context) { + (false, _) => None, + (true, Some(ctx)) => Some(ctx.clone()), + (true, None) => Some(pick::default_source(manifest)?), + }; + if let Some(source_name) = &all_source { + groups.entry(source_name.clone()).or_default(); + } + + let mut optional_offers: Vec<(String, String)> = Vec::new(); + let mut declaring: BTreeSet = BTreeSet::new(); + for (source_name, wanted) in &groups { + declaring.extend(add_from( + env, + scope, + manifest, + lock, + request, + source_name, + wanted, + all_source.as_deref() == Some(source_name), + notes, + &mut optional_offers, + revision, + member_hold_at, + )?); + } + // A choice naming an optional dependency nothing offers is an error + // that leaves the manifest exactly as it was — never a silently + // ignored flag. + for wanted in &request.optional { + if !optional_offers.iter().any(|(_, name)| name == wanted) { + return Err(CoreError::NoSuchOptional { + name: wanted.clone(), + source_name: groups.keys().cloned().collect::>().join(", "), + }); + } } + for (parent, name) in optional_offers { + let taken = manifest.optional_dependencies.entry(parent).or_default(); + if !taken.contains(&name) { + taken.push(name); + taken.sort(); + } + } + + self.declaring.extend(declaring); + let mut supplied_settings = std::mem::take(&mut self.options.supplied_settings); + supplied_settings.extend(request.settings.iter().cloned()); + self.options = PlanOptions { + arriving_skills: &crate::engine::installed::skills_installed(env, scope, manifest) + - &self.declared, + supplied_settings, + ..PlanOptions::for_additions(self.declaring.iter().cloned()) + }; + Ok(()) } - // Source edits reach the requested packages. The add keeps every other - // installation at its recorded revision. - let options = PlanOptions { - arriving_skills: &crate::engine::installed::skills_installed(env, scope, &manifest) - - &declared, - supplied_settings: request.settings.clone(), - ..PlanOptions::for_additions(declaring.iter().cloned()) - }; - let mut report = plan_scope(env, scope, &manifest, &lock, &options)?; - report.notes.extend(notes); - report - .notes - .extend(moved(&lock, &report.record, &declaring)); - report.asked = crate::engine::Asked::Named(declaring); - ensure_manifest_persisted(env, scope, &manifest, &mut report)?; - Ok(report) + pub(crate) fn check_catalog_compatibility(&self, env: &Env) -> Result<()> { + crate::source::check_catalog_compatibility(env, &self.scope, &self.manifest)?; + crate::engine::desired::hold::held_planning( + env, + &self.scope, + &self.manifest, + &self.lock, + &self.options, + )?; + Ok(()) + } + + pub(crate) fn plan(self, env: &Env) -> Result { + let mut report = plan_scope(env, &self.scope, &self.manifest, &self.lock, &self.options)?; + report.notes.extend(self.notes); + report + .notes + .extend(moved(&self.lock, &report.record, &self.declaring)); + report.asked = crate::engine::Asked::Named(self.declaring); + ensure_manifest_persisted(env, &self.scope, &self.manifest, &mut report)?; + Ok(report) + } } /// Every package this request declares that its record already holds at @@ -268,8 +329,10 @@ fn add_from( take_all: bool, notes: &mut Vec, optional_offers: &mut Vec<(String, String)>, + revision: Option<&str>, + member_hold_at: Option<&str>, ) -> Result> { - let ready = source::require_ready(env, scope, source_name, manifest)?; + let ready = source::require_ready_at(env, scope, source_name, manifest, revision)?; let hold_at = hold_commit(request, source_name, &ready)?; let hold_at = hold_at.as_deref(); let sealed = crate::source_read::SealedSource::open(&ready.root)?; @@ -356,6 +419,14 @@ fn add_from( (ItemKind::OutputStyle, output_styles), ] { for name in names { + // Collections freeze their explicit members. Automatic agent + // skills keep their revision choices; direct add --hold still + // freezes the complete request expansion through hold_at. + let hold_at = if kind == ItemKind::Skill && !wanted.skills.contains(&name) { + hold_at + } else { + member_hold_at.or(hold_at) + }; declare( env, scope, diff --git a/crates/core/src/source_ops.rs b/crates/core/src/source_ops.rs index bd23efcc41..784e4fd441 100644 --- a/crates/core/src/source_ops.rs +++ b/crates/core/src/source_ops.rs @@ -183,7 +183,9 @@ pub fn add_source(env: &Env, scope: &Scope, name: &str, reference: &str) -> Resu mod collection; mod subscribe; -pub use collection::{CollectionStep, SourceAction, collection_steps}; +pub use collection::{ + CollectionStep, PreparedCollection, SourceAction, collection_steps, prepare_collection, +}; pub use subscribe::{ Subscribed, install_from, install_project_carrying, install_project_from_personal, subscribe, subscribe_and_install, subscribe_project_to, @@ -230,7 +232,12 @@ impl PreparedSourceChange { pub fn plan(self, env: &Env) -> Result { let mut report = persist_and_plan(env, &self.scope, self.manifest)?; if let Some((name, decl)) = self.subscription { - subscribe::announce_subscription(env, &mut report, &self.scope, &name, &decl); + subscribe::announce_subscriptions( + env, + &mut report, + &self.scope, + [(name.as_str(), &decl)], + ); } Ok(report) } diff --git a/crates/core/src/source_ops/collection.rs b/crates/core/src/source_ops/collection.rs index 010ea51bb7..59ba927b76 100644 --- a/crates/core/src/source_ops/collection.rs +++ b/crates/core/src/source_ops/collection.rs @@ -58,6 +58,95 @@ pub enum SourceAction { Subscribe { reference: String }, } +/// A whole collection's checked additions. Planning consumes the same +/// manifest and installation record that the compatibility check read. +pub struct PreparedCollection { + scope: Scope, + addition: crate::engine::ops::PreparedAddition, + subscriptions: Vec<(String, crate::manifest::SourceDecl)>, +} + +impl PreparedCollection { + pub fn subscription_names(&self) -> impl Iterator { + self.subscriptions.iter().map(|(name, _)| name.as_str()) + } + + pub fn check_catalog_compatibility(&self, env: &Env) -> Result<()> { + self.addition.check_catalog_compatibility(env) + } + + pub fn plan(self, env: &Env) -> Result { + self.check_catalog_compatibility(env)?; + let mut report = self.addition.plan(env)?; + super::subscribe::announce_subscriptions( + env, + &mut report, + &self.scope, + self.subscriptions + .iter() + .map(|(name, decl)| (name.as_str(), decl)), + ); + Ok(report) + } +} + +/// Resolve every snapshot and prepare every declaration before a plan can +/// render. A retained incompatible revision therefore refuses before any +/// part of the collection installs. +pub fn prepare_collection( + env: &Env, + scope: &Scope, + steps: &[CollectionStep], +) -> Result { + for step in steps { + crate::remote::sync(env, &step.repo, step.commit.as_deref())?; + } + let manifest = crate::engine::ops::manifest_for_reading(env, scope)?; + for step in steps { + if let SourceAction::Reuse { name } = &step.action + && let Some(decl) = manifest.sources.get(name) + && let Some(repo) = &decl.repo + { + crate::remote::sync(env, repo, decl.rev.as_deref())?; + } + } + let mut addition = crate::engine::ops::PreparedAddition::new(env, scope)?; + let mut subscriptions = Vec::new(); + for step in steps { + let (name, seed) = match &step.action { + SourceAction::Reuse { name } => (name.clone(), None), + SourceAction::Subscribe { reference } => { + let declared = super::subscribe::declared(env, scope, reference, None)?; + let name = addition.subscription_name(&step.repo); + subscriptions.push((name.clone(), declared.decl.clone())); + (name.clone(), Some((name, declared.decl))) + } + }; + let request = crate::engine::ops::AddRequest { + source: Some(name), + agents: step.agents.clone(), + skills: step.skills.clone(), + hooks: step.hooks.clone(), + commands: step.commands.clone(), + mcp_servers: step.mcp_servers.clone(), + output_styles: step.output_styles.clone(), + ..Default::default() + }; + let member_hold_at = match &step.action { + SourceAction::Reuse { .. } => step.commit.as_deref(), + SourceAction::Subscribe { .. } => None, + }; + addition.include(env, &request, seed, step.commit.as_deref(), member_hold_at)?; + } + let prepared = PreparedCollection { + scope: scope.clone(), + addition, + subscriptions, + }; + prepared.check_catalog_compatibility(env)?; + Ok(prepared) +} + pub fn collection_steps( env: &Env, scope: &Scope, diff --git a/crates/core/src/source_ops/subscribe.rs b/crates/core/src/source_ops/subscribe.rs index c7e4806594..cba9b32047 100644 --- a/crates/core/src/source_ops/subscribe.rs +++ b/crates/core/src/source_ops/subscribe.rs @@ -47,7 +47,7 @@ pub fn subscribe( } = declared(env, scope, reference, alias)?; manifest.sources.insert(name.clone(), decl.clone()); let mut report = persist_and_plan(env, scope, manifest)?; - announce_subscription(env, &mut report, scope, &name, &decl); + announce_subscriptions(env, &mut report, scope, [(name.as_str(), &decl)]); Ok(subscribed(report, name, decl, lead)) } @@ -75,7 +75,7 @@ pub fn subscribe_and_install( }; let mut report = crate::engine::ops::add_seeded(env, scope, &request, Some((name.clone(), decl.clone())))?; - announce_subscription(env, &mut report, scope, &name, &decl); + announce_subscriptions(env, &mut report, scope, [(name.as_str(), &decl)]); Ok(subscribed(report, name, decl, lead)) } @@ -218,7 +218,7 @@ pub fn subscribe_project_to( .sources .insert(source_name.to_owned(), decl.clone()); let mut report = persist_and_plan(env, &scope, manifest)?; - announce_subscription(env, &mut report, &scope, source_name, &decl); + announce_subscriptions(env, &mut report, &scope, [(source_name, &decl)]); Ok(report) } @@ -282,7 +282,7 @@ pub fn install_project_carrying( &request, Some((source_name.to_owned(), decl.clone())), )?; - announce_subscription(env, &mut report, &scope, source_name, &decl); + announce_subscriptions(env, &mut report, &scope, [(source_name, &decl)]); Ok(report) } @@ -430,22 +430,34 @@ fn normalize_tree( crate::source_ref::split_tree_ref(reference, &refs, ref_and_path) } -/// The §4.1 preview line: every subscribe names its destination scope, -/// manifest path, and the source's alias, repo, and revision — which -/// scope a click mutates is never inferred from the page being looked at. -pub(super) fn announce_subscription( +/// Name every subscription in the shared manifest-write description and +/// retain a note for each one. The scope and path identify the destination +/// a user approves, including when several subscriptions share one write. +pub(super) fn announce_subscriptions<'a>( env: &Env, report: &mut EngineReport, scope: &Scope, - name: &str, - decl: &SourceDecl, + subscriptions: impl IntoIterator, ) { - let what = match (&decl.repo, &decl.rev) { - (Some(repo), Some(rev)) => format!("{repo} @ {rev}"), - (Some(repo), None) => repo.clone(), - (None, _) => decl.path.clone().unwrap_or_default(), - }; - let said = format!("Subscribes {} to '{name}' ({what}) — ", scope.label()); + let destinations: Vec = subscriptions + .into_iter() + .map(|(name, decl)| { + let what = match (&decl.repo, &decl.rev) { + (Some(repo), Some(rev)) => format!("{repo} @ {rev}"), + (Some(repo), None) => repo.clone(), + (None, _) => decl.path.clone().unwrap_or_default(), + }; + format!("'{name}' ({what})") + }) + .collect(); + if destinations.is_empty() { + return; + } + let said = format!( + "Subscribes {} to {}: ", + scope.label(), + destinations.join(", ") + ); // The file is the op's to name, not this line's: the write goes where // the plan landed it, and a spelling derived again here would name // somewhere else the moment a directory on the way is a link. @@ -453,13 +465,21 @@ pub(super) fn announce_subscription( for op in &mut report.plan.ops { if let crate::apply::Op::WriteManifest { .. } = &op.op { op.description = crate::apply::Description::around(said.clone(), ""); - written = Some(op.line()); + written = op + .description_parts() + .into_iter() + .find_map(|part| match part { + crate::apply::DescriptionPart::Path(path) => Some(path), + crate::apply::DescriptionPart::Text(_) => None, + }); } } // No write, no landed place to name — the file the subscription would // go in is all there is to say, and nothing disagrees with it. - report.notes.push( - written - .unwrap_or_else(|| format!("{said}{}", manifest::manifest_path(env, scope).display())), + let path = written.unwrap_or_else(|| manifest::manifest_path(env, scope).display().to_string()); + report.notes.extend( + destinations + .into_iter() + .map(|destination| format!("Subscribes {} to {destination}: {path}", scope.label())), ); } diff --git a/crates/core/tests/collections.rs b/crates/core/tests/collections.rs index c256f5e1e9..a48c0e69f4 100644 --- a/crates/core/tests/collections.rs +++ b/crates/core/tests/collections.rs @@ -306,3 +306,296 @@ fn a_member_repo_cannot_be_a_filesystem_path() { ); } } + +#[allow(clippy::unwrap_used)] +fn replacement_catalog(catalog: &std::path::Path, names: &[&str]) { + for name in names { + fs::create_dir_all(catalog.join(format!("skills/{name}"))).unwrap(); + fs::write( + catalog.join(format!("skills/{name}/SKILL.md")), + format!("---\nname: {name}\ndescription: fixture\n---\nBody.\n"), + ) + .unwrap(); + } + fs::write(catalog.join("kendex.toml"), "min_kendex = '0.0.0'\n").unwrap(); + test_util::git(catalog, &["init", "-q", "-b", "main"]); + test_util::git(catalog, &["add", "-A"]); + test_util::git(catalog, &["commit", "-q", "-m", "compatible"]); +} + +/// A released CLI could record a catalog minimum it ignored. The real +/// installation record supplies every field except that historical commit. +#[test] +#[allow(clippy::unwrap_used)] +fn collection_replacements_and_retained_readings_are_checked_before_installation() { + use kendex_core::{apply, engine, error::CoreError, lock, source_ops}; + + for (reverse, retained) in [(false, false), (true, false), (false, true), (true, true)] { + let tmp = tempfile::tempdir().unwrap(); + let home = rooted(&tmp); + let env = Env::fake(&home, FakeOs::Linux); + let project = home.join("consumer"); + fs::create_dir_all(project.join(".claude")).unwrap(); + let first = home.join("a"); + let last = home.join("z"); + replacement_catalog(&first, &["fresh"]); + replacement_catalog(&last, &["held", "retained"]); + fs::write(last.join("kendex.toml"), "min_kendex = '999999.0.0'\n").unwrap(); + test_util::git(&last, &["add", "-A"]); + test_util::git(&last, &["commit", "-q", "-m", "historical minimum"]); + let historical = test_util::git(&last, &["rev-parse", "HEAD"]) + .trim() + .to_owned(); + fs::write(last.join("kendex.toml"), "min_kendex = '0.0.0'\n").unwrap(); + test_util::git(&last, &["add", "-A"]); + test_util::git(&last, &["commit", "-q", "-m", "compatible replacement"]); + let first_commit = test_util::git(&first, &["rev-parse", "HEAD"]) + .trim() + .to_owned(); + let last_commit = test_util::git(&last, &["rev-parse", "HEAD"]) + .trim() + .to_owned(); + let first_repo = format!("file://{}", first.display()); + let last_repo = format!("file://{}", last.display()); + fs::write(project.join("kendex.toml"), format!( + "schema = 7\n[sources.first]\nrepo = '{first_repo}'\nrev = 'main'\n[sources.last]\nrepo = '{last_repo}'\nrev = 'main'\n[install]\nharnesses = ['claude']\nmethod = 'copy'\n[skills.held]\nsource = 'last'\n{}", + if retained { "[skills.retained]\nsource = 'last'\n" } else { "" })).unwrap(); + let scope = Scope::Project { + root: project.clone(), + }; + kendex_core::remote::sync(&env, &first_repo, None).unwrap(); + kendex_core::remote::sync(&env, &last_repo, None).unwrap(); + let installed = engine::plan_apply(&env, &scope, &engine::PlanOptions::current()).unwrap(); + apply::execute(&env, &installed.plan).unwrap(); + kendex_core::remote::sync(&env, &last_repo, Some(&historical)).unwrap(); + let path = lock::lock_path(&env, &scope); + let mut record = lock::load(&path).unwrap(); + for entry in record.entries.values_mut() { + entry.source_commit = Some(historical.clone()); + } + lock::save(&path, &record).unwrap(); + let collection = Collection { + id: "i".to_owned(), + name: "replacement".to_owned(), + members: vec![ + member(&first_repo, "fresh", Some(&first_commit)), + member(&last_repo, "held", Some(&last_commit)), + ], + }; + let mut steps = collection_steps(&env, &scope, &collection).unwrap(); + if reverse { + steps.reverse(); + } + let before_manifest = fs::read(project.join("kendex.toml")).unwrap(); + let before_record = fs::read(&path).unwrap(); + let outcome = source_ops::prepare_collection(&env, &scope, &steps) + .and_then(|prepared| prepared.plan(&env)) + .and_then(|report| { + apply::execute(&env, &report.plan)?; + Ok(report) + }); + if retained { + assert!(matches!(outcome, Err(CoreError::CatalogNeedsEngine { .. }))); + assert_eq!( + fs::read(project.join("kendex.toml")).unwrap(), + before_manifest + ); + assert_eq!(fs::read(&path).unwrap(), before_record); + assert!(!project.join(".claude/skills/fresh").exists()); + } else { + outcome.unwrap(); + let after = lock::load(&path).unwrap(); + for (name, commit) in [("fresh", &first_commit), ("held", &last_commit)] { + assert!( + project + .join(format!(".claude/skills/{name}/SKILL.md")) + .is_file() + ); + assert_eq!( + after + .entries + .values() + .find(|entry| entry.name == name) + .unwrap() + .source_commit + .as_ref(), + Some(commit) + ); + } + } + } +} + +struct AgentCollectionFixture { + _tmp: tempfile::TempDir, + env: Env, + scope: Scope, + repo: String, + first: String, + snapshot: String, +} + +#[allow(clippy::unwrap_used)] +fn agent_collection_fixture(held: bool) -> AgentCollectionFixture { + use kendex_core::{apply, engine, remote}; + let tmp = tempfile::tempdir().unwrap(); + let home = rooted(&tmp); + let env = Env::fake(&home, FakeOs::Linux); + let catalog = home.join("catalog"); + replacement_catalog(&catalog, &["automatic", "fresh"]); + let first = test_util::git(&catalog, &["rev-parse", "HEAD"]) + .trim() + .to_owned(); + fs::create_dir_all(catalog.join("agents")).unwrap(); + fs::write( + catalog.join("agents/rust.md"), + "---\nname: rust\ndescription: fixture\nrole: engineer\n---\nBody.\n", + ) + .unwrap(); + fs::write( + catalog.join("kendex.toml"), + "min_kendex = '0.0.0'\n[role-skills]\nengineer = ['automatic', 'fresh']\n", + ) + .unwrap(); + test_util::git(&catalog, &["add", "-A"]); + test_util::git(&catalog, &["commit", "-q", "-m", "agent snapshot"]); + let snapshot = test_util::git(&catalog, &["rev-parse", "HEAD"]) + .trim() + .to_owned(); + let repo = format!("file://{}", catalog.display()); + let project = home.join("consumer"); + fs::create_dir_all(project.join(".claude")).unwrap(); + let pin = if held { + format!("rev = '{first}'\n") + } else { + String::new() + }; + fs::write(project.join("kendex.toml"), format!( + "schema = 7\n[sources.cat]\nrepo = '{repo}'\n[install]\nharnesses = ['claude']\nmethod = 'copy'\n[skills.automatic]\nsource = 'cat'\n{pin}")).unwrap(); + let scope = Scope::Project { root: project }; + remote::sync(&env, &repo, Some(&first)).unwrap(); + remote::sync(&env, &repo, None).unwrap(); + let installed = engine::plan_apply(&env, &scope, &engine::PlanOptions::current()).unwrap(); + apply::execute(&env, &installed.plan).unwrap(); + AgentCollectionFixture { + _tmp: tmp, + env, + scope, + repo, + first, + snapshot, + } +} + +#[allow(clippy::unwrap_used)] +fn assert_agent_collection_holds( + fixture: &AgentCollectionFixture, + automatic_rev: Option<&str>, + fresh_rev: Option<&str>, +) { + use kendex_core::{lock, manifest}; + let after = manifest::load_current(&manifest::manifest_path(&fixture.env, &fixture.scope)) + .unwrap() + .unwrap(); + assert_eq!( + after.agents["rust"].rev.as_deref(), + Some(fixture.snapshot.as_str()) + ); + assert_eq!(after.skills["automatic"].rev.as_deref(), automatic_rev); + assert_eq!(after.skills["fresh"].rev.as_deref(), fresh_rev); + let record = lock::load(&lock::lock_path(&fixture.env, &fixture.scope)).unwrap(); + for (kind, name, commit) in [ + (ItemKind::Agent, "rust", fixture.snapshot.as_str()), + ( + ItemKind::Skill, + "automatic", + automatic_rev.unwrap_or(fixture.snapshot.as_str()), + ), + (ItemKind::Skill, "fresh", fixture.snapshot.as_str()), + ] { + assert_eq!( + record + .entries + .values() + .find(|entry| entry.kind == kind && entry.name == name) + .unwrap() + .source_commit + .as_deref(), + Some(commit) + ); + } +} + +/// A collection names an agent but expands its role skills automatically. +/// Only named members gain snapshot holds. A direct add --hold freezes its +/// automatic expansion too, including a skill already held elsewhere. +#[test] +#[allow(clippy::unwrap_used)] +fn collection_member_holds_preserve_automatic_skill_revision_choices() { + use kendex_core::{apply, engine, source_ops}; + + for (direct_hold, held, explicit_skill) in [ + (false, false, false), + (false, true, false), + (false, true, true), + (true, false, false), + (true, true, false), + ] { + let fixture = agent_collection_fixture(held); + let AgentCollectionFixture { + env, + scope, + repo, + first, + snapshot, + .. + } = &fixture; + + let report = if direct_hold { + engine::ops::add( + env, + scope, + &engine::ops::AddRequest { + source: Some("cat".to_owned()), + agents: vec!["rust".to_owned()], + hold: true, + ..Default::default() + }, + ) + .unwrap() + } else { + let mut members = vec![CollectionMember { + repo: repo.clone(), + kind: ItemKind::Agent, + name: "rust".to_owned(), + commit: Some(snapshot.clone()), + }]; + if explicit_skill { + members.push(member(repo, "automatic", Some(snapshot))); + } + let collection = Collection { + id: "i".to_owned(), + name: "agent".to_owned(), + members, + }; + let steps = collection_steps(env, scope, &collection).unwrap(); + source_ops::prepare_collection(env, scope, &steps) + .unwrap() + .plan(env) + .unwrap() + }; + apply::execute(env, &report.plan).unwrap(); + let automatic_rev = if direct_hold || explicit_skill { + Some(snapshot.as_str()) + } else if held { + Some(first.as_str()) + } else { + None + }; + assert_agent_collection_holds( + &fixture, + automatic_rev, + direct_hold.then_some(snapshot.as_str()), + ); + } +} diff --git a/docs/architecture/sources.md b/docs/architecture/sources.md index 214882dca0..7f29dd1de7 100644 --- a/docs/architecture/sources.md +++ b/docs/architecture/sources.md @@ -20,7 +20,7 @@ The directory is a remote, so it gets a catalog's distrust and a mirror's offlin - Do parse a subscription reference, never guess it: the two validators in `crates/core/src/source_ref.rs`, `parse_typed` for what a person types and `parse_untrusted` for untrusted rows and deep links, refuse a leading `-`, a `..` component in a repository name or URL, which a local path keeps, and a percent-escape that smuggles a separator. - Do refuse a `kendex.toml` name that cannot be a path, with the reason, at `crates/core/src/names.rs`. - Do fail discovery closed: a plugin registry wins outright, else a parsed control file declares the layout, else the search runs, and an unreadable control file makes the source unusable with a finding. -- Do require a catalog's root `min_kendex` to be a valid Semantic Version at or below the running engine. `source::config` owns the verdict. Multi-scope installation commands and verification check their selected subscription resolutions and package or bundle revisions before rendering any selected scope. Manual and session checks read every selected scope's compatibility verdict before the first record settlement. A catalog refusal or a compatibility read that misses the shared deep-read deadline withholds every selected record, including the global record in report-only mode. Report-only withholds project records; compatible checks can still settle global records. Collections check their subscriptions and snapshots before the first step. Each collection add plan checks the recorded revisions it keeps before rendering that step. A minimum refusal leaves that plan's installed-package and project-file writes untouched: manifests, locks, generated inventory, harness files, trash moves, repository effects and Pi settlement. Earlier collection steps can already have written when a later step refuses a remaining recorded revision. Catalog downloads and their cache records remain permitted. CLI bootstrap also remains permitted: executable registration and first-run terms state. A missing key keeps the existing reading contract. +- Do require a catalog's root `min_kendex` to be a valid Semantic Version at or below the running engine. `source::config` owns the verdict. Multi-scope installation commands and verification check their selected subscription resolutions and package or bundle revisions before rendering any selected scope. Manual and session checks read every selected scope's compatibility verdict before the first record settlement. A catalog refusal or a compatibility read that misses the shared deep-read deadline withholds every selected record, including the global record in report-only mode. Report-only withholds project records; compatible checks can still settle global records. Collections prepare all additions in core and check the resulting manifest, installation record and reading options before rendering or writing. Compatible collection replacements release their historical readings in every repository order. A retained incompatible reading refuses the entire collection. A minimum refusal leaves installed-package and project-file writes untouched: manifests, locks, generated inventory, harness files, trash moves, repository effects and Pi settlement. Catalog downloads and their cache records remain permitted. CLI bootstrap also remains permitted: executable registration and first-run terms state. A missing key keeps the existing reading contract. - Do say where a name comes from: a bare name searching every enabled subscription refuses on two offers, naming both spellings, and the default catalog is reached only when nothing else offers the name. - Do date an offered item by the newest commit that touched what it contains, never by the bare tip. - Do let a run select the default catalog's revision through `KENDEX_CATALOG_REV`; a declared `rev` outranks it. The variable changes no declaration on disk.