diff --git a/Cargo.lock b/Cargo.lock index c79f045..81c8220 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -697,7 +697,7 @@ dependencies = [ [[package]] name = "knit-cli" -version = "0.1.0-alpha.42" +version = "0.1.0-alpha.43" dependencies = [ "anyhow", "chrono", @@ -720,7 +720,7 @@ dependencies = [ [[package]] name = "knit-runtime" -version = "0.1.0-alpha.42" +version = "0.1.0-alpha.43" dependencies = [ "anyhow", "chrono", diff --git a/Cargo.toml b/Cargo.toml index 30d4d17..89017d8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ # Published as `knit-cli` (the crates.io name `knit` is taken); the installed # binary is still `knit`. name = "knit-cli" -version = "0.1.0-alpha.42" +version = "0.1.0-alpha.43" edition = "2021" license = "Apache-2.0" description = "A local-first CLI for coordinating cross-repo feature bundles." @@ -26,7 +26,7 @@ members = [".", "crates/knit-runtime"] [dependencies] anyhow = "1" -knit-runtime = { version = "0.1.0-alpha.42", path = "crates/knit-runtime" } +knit-runtime = { version = "0.1.0-alpha.43", path = "crates/knit-runtime" } chrono = { version = "0.4", features = ["serde"] } clap = { version = "4.5", features = ["derive"] } ctrlc = { version = "3.4", features = ["termination"] } diff --git a/crates/knit-runtime/Cargo.toml b/crates/knit-runtime/Cargo.toml index 914ea71..b95708b 100644 --- a/crates/knit-runtime/Cargo.toml +++ b/crates/knit-runtime/Cargo.toml @@ -4,7 +4,7 @@ # version-control shaped; the CLI compiles it in and adapts bundle state into # the crate's RuntimeContext contract. name = "knit-runtime" -version = "0.1.0-alpha.42" +version = "0.1.0-alpha.43" edition = "2021" license = "Apache-2.0" description = "Per-bundle docker-compose runtime for knit cross-repo bundles." diff --git a/docs/release-0.1.0-alpha.43.md b/docs/release-0.1.0-alpha.43.md new file mode 100644 index 0000000..4d5e916 --- /dev/null +++ b/docs/release-0.1.0-alpha.43.md @@ -0,0 +1,10 @@ +# Knit 0.1.0-alpha.43 + +Reviews merged on the host count as done. Landing plans leave them out, and a +bundle whose reviews have all merged closes itself once its landing plan has +nothing left to run: the saved plan, or without one, the plan the project's +landing recipes would generate. `knit publish sync`, `knit land`, and +`knit publish sync --from-artifact` (with `--plan` or `--project-file`) apply +the rule, so a bundle another maintainer merged no longer waits for a landing +nobody can run. `knit land remove ` drops a step from the saved plan, and +pulling an archived bundle removes its clean worktrees. diff --git a/schemas/land-plan.schema.json b/schemas/land-plan.schema.json index 38fee5a..3962b6c 100644 --- a/schemas/land-plan.schema.json +++ b/schemas/land-plan.schema.json @@ -417,6 +417,14 @@ "default": true, "description": "Whether landing this plan finishes the bundle." }, + "alreadyMerged": { + "type": "array", + "items": { + "type": "string" + }, + "uniqueItems": true, + "description": "Repositories whose review the host had already merged when the plan was generated; they have no merge step." + }, "changedRepos": { "type": "array", "items": { diff --git a/src/cli.rs b/src/cli.rs index b82c3e8..f0ed683 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1473,6 +1473,12 @@ pub enum PublishCommand { /// Write the updated bundle JSON artifact to this path. #[arg(long)] out: Option, + /// The bundle's saved landing plan. With every review merged, the bundle closes only when this plan has no step left besides merges. + #[arg(long, requires = "from_artifact")] + plan: Option, + /// The bundle's project. Without a saved plan, the project's landing recipes decide whether steps besides merges are still due. + #[arg(long, requires = "from_artifact")] + project_file: Option, /// Sync every tracked repo instead of only repos with recorded work or publications. #[arg(long)] all: bool, @@ -1574,6 +1580,15 @@ pub enum LandCommand { #[arg(long)] force: bool, }, + /// Take steps out of the saved plan, such as a deployment you do not want to run. + Remove { + /// Step ids to remove, as `knit land` lists them. + #[arg(required = true)] + steps: Vec, + /// Plan file to edit; defaults to the bundle's plan for this destination. + #[arg(long)] + plan: Option, + }, /// Validate an exact saved plan without executing it. Validate { #[arg(long)] diff --git a/src/commands/bundle/lifecycle.rs b/src/commands/bundle/lifecycle.rs index 54f632e..dce1c7b 100644 --- a/src/commands/bundle/lifecycle.rs +++ b/src/commands/bundle/lifecycle.rs @@ -80,22 +80,26 @@ pub(crate) fn archive_active_bundle( } else { crate::commands::clean::clean_worktrees_for_bundle(active, force)? }; - let now = now_iso(); - let node_id = crate::ids::node_id("archive"); - active.bundle.state = Some(BundleState::Archived); - active.bundle.archived_at = Some(now.clone()); - active - .bundle - .nodes - .push(BundleNode::feature_archived(node_id.clone(), now, reason)); - active.bundle.head_node_id = active.bundle.nodes.last().map(|node| node.id.clone()); - active.bundle.updated_at = now_iso(); + let node_id = mark_archived(&mut active.bundle, reason); Ok(ArchiveSummary { node_id, removed_worktrees, }) } +pub(crate) fn mark_archived(bundle: &mut ChangeGroup, reason: Option) -> String { + let now = now_iso(); + let node_id = crate::ids::node_id("archive"); + bundle.state = Some(BundleState::Archived); + bundle.archived_at = Some(now.clone()); + bundle + .nodes + .push(BundleNode::feature_archived(node_id.clone(), now, reason)); + bundle.head_node_id = bundle.nodes.last().map(|node| node.id.clone()); + bundle.updated_at = now_iso(); + node_id +} + pub(crate) fn clear_workspace_active_if_matches(root: &Path, bundle_id: &str) -> Result<()> { clear_active_if_matches(root, bundle_id) } diff --git a/src/commands/bundle/merged.rs b/src/commands/bundle/merged.rs new file mode 100644 index 0000000..80c2f3f --- /dev/null +++ b/src/commands/bundle/merged.rs @@ -0,0 +1,267 @@ +//! A bundle whose reviews all merged on the host closes itself once its +//! landing plan has nothing left to run: the saved plan, or without one, the +//! plan the project's landing recipes would generate. + +use super::lifecycle::{clear_active_if_matches, mark_archived}; +use super::prune::landed_intermediate; +use super::{bundle_state, BundleStatus}; +use crate::model::ChangeGroup; +use crate::output as out; +use crate::store::{read_json, save_active_bundle, ActiveBundle}; +use anyhow::Result; +use serde_json::Value; +use std::path::Path; + +const REASON: &str = "every review merged on the host"; + +#[derive(Debug, PartialEq)] +pub(crate) enum Merged { + No, + Done, + StepsLeft(Vec), +} + +pub(crate) fn assess(bundle: &ChangeGroup, plan: Option<&Value>) -> Merged { + if bundle_state(bundle) != BundleStatus::Open || landed_intermediate(bundle) { + return Merged::No; + } + let repos = crate::commands::publish::publish_scope_repo_ids(bundle); + let all_merged = !repos.is_empty() + && repos.iter().all(|id| { + crate::providers::publication_for_repo(bundle, id) + .is_some_and(|p| p.state.eq_ignore_ascii_case("MERGED")) + }); + if !all_merged { + return Merged::No; + } + let left: Vec = plan + .and_then(|plan| plan["steps"].as_array()) + .into_iter() + .flatten() + .filter(|step| step["type"] != "merge_pr") + .filter_map(|step| step["id"].as_str().map(str::to_owned)) + .collect(); + if left.is_empty() { + Merged::Done + } else { + Merged::StepsLeft(left) + } +} + +/// `plan` overrides the saved default plan, for callers that just edited it. +pub(crate) fn close_if_merged(active: &mut ActiveBundle, plan: Option<&Value>) -> Result { + if assess(&active.bundle, None) == Merged::No { + return Ok(false); + } + let found; + let plan = match plan { + Some(plan) => plan, + None => { + let path = crate::commands::land::v2::destination_path(active, None, None); + found = if path.exists() { + read_json::(&path)? + } else { + match crate::commands::land::v2::default_plan(active, None) { + Ok(plan) => plan, + Err(error) => { + println!( + "{} every review is merged, but its landing plan could not be worked out: {error:#}", + out::heading("Still open:") + ); + return Ok(false); + } + } + }; + &found + } + }; + match assess(&active.bundle, Some(plan)) { + Merged::No => Ok(false), + Merged::StepsLeft(left) => { + println!( + "{} every review is merged; the landing plan still has {}. Run `knit land apply` to run it, or `knit land remove {}` to drop it.", + out::heading("Still open:"), + left.join(", "), + left.join(" ") + ); + Ok(false) + } + Merged::Done => { + if let Some(repo) = dirty_checkout(active)? { + println!( + "{} every review is merged, but {} has uncommitted changes.", + out::heading("Still open:"), + out::repo(&repo) + ); + return Ok(false); + } + crate::commands::bundle::archive_active_bundle( + active, + Some(REASON.to_owned()), + false, + false, + )?; + save_active_bundle(active)?; + clear_active_if_matches(&active.root, &active.bundle.id)?; + println!( + "{} {}: every review is merged and the landing plan has nothing left to run.", + out::ok("Closed"), + out::node(&active.bundle.id) + ); + if let Err(error) = + crate::commands::remote::sync_active_bundle_to_remote_if_enabled(active, &[], false) + { + println!("{} {error:#}", out::warn("remote sync skipped:")); + } + Ok(true) + } + } +} + +/// Without a saved `plan`, `project` (when given) supplies the landing +/// recipes that decide whether steps besides merges are still due. +pub(crate) fn close_artifact_if_merged( + root: &Path, + artifact_path: &Path, + bundle: &mut ChangeGroup, + plan: Option<&Value>, + project: Option<&Value>, +) -> bool { + if assess(bundle, None) == Merged::No { + return false; + } + let built; + let plan = match (plan, project) { + (Some(plan), _) => Some(plan), + (None, Some(project)) => { + let active = ActiveBundle::unlocked( + root.to_path_buf(), + artifact_path.to_path_buf(), + bundle.clone(), + ); + match crate::commands::land::v2::default_plan(&active, Some(project)) { + Ok(plan) => { + built = plan; + Some(&built) + } + Err(error) => { + println!( + "{} every review is merged, but its landing plan could not be worked out: {error:#}", + out::heading("Still open:") + ); + return false; + } + } + } + (None, None) => None, + }; + match assess(bundle, plan) { + Merged::Done => { + mark_archived(bundle, Some(REASON.to_owned())); + println!( + "{} {}: every review is merged and the landing plan has nothing left to run.", + out::ok("Closed"), + out::node(&bundle.id) + ); + true + } + Merged::StepsLeft(left) => { + println!( + "{} every review is merged; the landing plan still has {}.", + out::heading("Still open:"), + left.join(", ") + ); + false + } + Merged::No => false, + } +} + +fn dirty_checkout(active: &ActiveBundle) -> Result> { + for repo in &active.bundle.repos { + if crate::checkout::is_in_place(repo) { + continue; + } + let Some(path) = crate::checkout::checkout_dir(active, repo) else { + continue; + }; + let pending = crate::pending::path_pending_changes(&path)?; + if pending.tracked || pending.untracked { + return Ok(Some(repo.id.clone())); + } + } + Ok(None) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn bundle(states: &[(&str, &str)]) -> ChangeGroup { + let mut value = serde_json::to_value(ChangeGroup::new( + "demo".into(), + "Demo".into(), + "2026-10-09T00:00:00Z".into(), + )) + .unwrap(); + value["repos"] = states + .iter() + .map(|(id, _)| json!({"id": id, "path": format!("/tmp/{id}"), "baseBranch": "main"})) + .collect(); + value["publications"] = states + .iter() + .enumerate() + .map(|(n, (id, state))| { + json!({ + "repoId": id, "provider": "github", "kind": "pull_request", "number": n + 1, + "url": format!("https://github.com/o/{id}/pull/{}", n + 1), + "baseBranch": "main", "headBranch": "knit/demo", "state": state, + "updatedAt": "2026-10-09T00:00:00Z" + }) + }) + .collect(); + serde_json::from_value(value).unwrap() + } + + #[test] + fn a_bundle_closes_only_when_every_review_merged_and_no_step_is_left() { + let merged = bundle(&[("api", "MERGED"), ("web", "MERGED")]); + assert_eq!(assess(&merged, None), Merged::Done); + let plan = json!({"steps": [ + {"id": "merge-api", "type": "merge_pr"}, + {"id": "deploy-api", "type": "deploy"} + ]}); + assert_eq!( + assess(&merged, Some(&plan)), + Merged::StepsLeft(vec!["deploy-api".into()]) + ); + let open = bundle(&[("api", "MERGED"), ("web", "OPEN")]); + assert_eq!(assess(&open, None), Merged::No); + } + + #[test] + fn an_artifact_records_the_close_in_its_ledger() { + let mut merged = bundle(&[("api", "merged")]); + let (root, path) = (Path::new("/tmp"), Path::new("/tmp/demo.bundle.json")); + assert!(close_artifact_if_merged( + root, + path, + &mut merged, + None, + None + )); + assert_eq!(bundle_state(&merged), BundleStatus::Archived); + assert_eq!( + merged.nodes.last().map(|n| n.node_type.as_str()), + Some("feature.archived") + ); + assert!(!close_artifact_if_merged( + root, + path, + &mut merged, + None, + None + )); + } +} diff --git a/src/commands/bundle/mod.rs b/src/commands/bundle/mod.rs index 9ad4c96..6646db9 100644 --- a/src/commands/bundle/mod.rs +++ b/src/commands/bundle/mod.rs @@ -4,6 +4,7 @@ //! [`prune`]. mod lifecycle; +pub(crate) mod merged; mod prune; mod validate; @@ -11,6 +12,7 @@ pub(crate) use lifecycle::{ archive_active_bundle, clear_workspace_active_if_matches, delete_repo_feature_branch, }; pub use lifecycle::{archive_bundle, archive_dead_bundle, delete_bundle, restore_bundle}; +pub(crate) use merged::{close_artifact_if_merged, close_if_merged}; pub use prune::prune_merged_bundles; use crate::model::{BundleState, ChangeGroup}; diff --git a/src/commands/bundle/prune/assess.rs b/src/commands/bundle/prune/assess.rs index 9484c10..0c43947 100644 --- a/src/commands/bundle/prune/assess.rs +++ b/src/commands/bundle/prune/assess.rs @@ -361,7 +361,7 @@ fn assess_bundle( } /// Whether the bundle's most recent landing left it open on purpose. -fn landed_intermediate(bundle: &ChangeGroup) -> bool { +pub(crate) fn landed_intermediate(bundle: &ChangeGroup) -> bool { bundle .nodes .iter() diff --git a/src/commands/bundle/prune/mod.rs b/src/commands/bundle/prune/mod.rs index e39b5a7..670cfb2 100644 --- a/src/commands/bundle/prune/mod.rs +++ b/src/commands/bundle/prune/mod.rs @@ -18,6 +18,7 @@ mod orphans; use super::{archive_dead_bundle, bundle_json_paths, current_root, delete_bundle}; use crate::output as out; use anyhow::{bail, Result}; +pub(super) use assess::landed_intermediate; use assess::{assess_bundles, PruneAssessment, PruneCache}; use orphans::{orphan_worktree_candidates, remote_orphan_candidates, remove_orphan_worktree}; diff --git a/src/commands/land/v2/generate.rs b/src/commands/land/v2/generate.rs index cfaeb53..97dca1d 100644 --- a/src/commands/land/v2/generate.rs +++ b/src/commands/land/v2/generate.rs @@ -20,6 +20,23 @@ pub(super) fn local_project(active: &ActiveBundle) -> Result { } } +/// The default destination's plan, built but not saved. `project` stands in +/// for the workspace project when the caller has no workspace. +pub(crate) fn default_plan(active: &ActiveBundle, project: Option<&Value>) -> Result { + let project = match project { + Some(project) => project.clone(), + None => local_project(active)?, + }; + build( + active, + &serde_json::to_value(&active.bundle)?, + &project, + None, + None, + None, + ) +} + #[allow(clippy::too_many_arguments)] pub fn generate( artifact: Option<&Path>, @@ -33,7 +50,10 @@ pub fn generate( ) -> Result<()> { let active = match artifact { Some(p) => ActiveBundle::unlocked(std::env::current_dir()?, p.into(), read_json(p)?), - None => crate::store::load_active_bundle()?, + None => { + record_host_merges(&crate::store::load_active_bundle()?)?; + crate::store::load_active_bundle()? + } }; let bundle = match artifact { Some(p) => read_json(p)?, @@ -45,6 +65,19 @@ pub fn generate( None => local_project(&active)?, }; let plan = build(&active, &bundle, &project, provider, target, lane)?; + if artifact.is_none() + && target.is_none() + && lane.is_none() + && crate::commands::bundle::merged::assess(&active.bundle, Some(&plan)) + == crate::commands::bundle::merged::Merged::Done + { + let mut locked = crate::store::load_active_bundle_for_update()?; + crate::commands::bundle::close_if_merged(&mut locked, Some(&plan))?; + return Ok(()); + } + if plan["steps"].as_array().is_some_and(Vec::is_empty) { + bail!("every review is merged; the plan has nothing left to land"); + } let path = out .map(PathBuf::from) .unwrap_or_else(|| destination_path(&active, target, lane)); @@ -150,6 +183,7 @@ pub(super) fn build( )?; let mut plan = serde_json::to_value(base)?; mark_upstream_merges(active, &mut plan)?; + drop_merged_reviews(&active.bundle, &mut plan); plan["schemaVersion"] = json!("0.2"); plan["merge"] = json!({"enabled":merge_policy.enabled.unwrap_or(true)}); if !merge_policy.repositories.is_empty() { @@ -429,6 +463,9 @@ pub(super) fn build( } super::branch_checkout::configure(&mut plan)?; super::gates::require_executor(&mut plan); + if plan["steps"].as_array().is_some_and(Vec::is_empty) && plan.get("alreadyMerged").is_some() { + return Ok(plan); + } let result = validation(&plan, Some(bundle), Some(project)); if result["valid"] != true { bail!("{}", result["errors"]); @@ -436,6 +473,93 @@ pub(super) fn build( Ok(plan) } +/// Reviews merged on the host since the last sync are recorded before +/// planning, so the plan leaves them out. Hosts that cannot be read keep the +/// recorded state. +fn record_host_merges(active: &ActiveBundle) -> Result<()> { + let mut merged = Vec::new(); + for publication in &active.bundle.publications { + if !crate::providers::is_review_kind(&publication.kind) + || publication.state.eq_ignore_ascii_case("MERGED") + || publication.state.eq_ignore_ascii_case("CLOSED") + { + continue; + } + let Some(repo) = active + .bundle + .repos + .iter() + .find(|r| r.id == publication.repo_id) + else { + continue; + }; + let Ok(forge) = crate::providers::for_repo(repo) else { + continue; + }; + let cwd = + crate::checkout::checkout_dir(active, repo).unwrap_or_else(|| active.root.clone()); + let Ok(target) = + crate::contribution::target(&cwd, repo, forge.as_ref(), &publication.base_branch, true) + else { + continue; + }; + if let Ok(pr) = forge.view(&target, &publication.url) { + if pr.state.as_deref() == Some("MERGED") { + merged.push((repo.clone(), pr)); + } + } + } + if merged.is_empty() { + return Ok(()); + } + let mut locked = crate::store::load_active_bundle_for_update()?; + for (repo, pr) in &merged { + let forge = crate::providers::for_repo(repo)?; + crate::providers::upsert_publication(&mut locked.bundle, repo, forge.as_ref(), pr); + println!( + "{} {} {}", + crate::output::repo(&repo.id), + crate::output::ok("merged on the host:"), + pr.url + ); + } + crate::store::save_active_bundle(&locked) +} + +/// A review the host already merged has nothing left to merge, so its step is +/// left out and nothing waits on it. +fn drop_merged_reviews(bundle: &crate::model::ChangeGroup, plan: &mut Value) { + let merged = merged_review_repos(bundle); + let steps = plan["steps"].as_array_mut().unwrap(); + let removed: Vec = steps + .iter() + .filter(|s| s["type"] == "merge_pr") + .filter(|s| s["repoId"].as_str().is_some_and(|id| merged.contains(id))) + .filter_map(|s| s["id"].as_str().map(str::to_owned)) + .collect(); + steps.retain(|s| !removed.iter().any(|id| s["id"] == *id)); + for step in steps.iter_mut() { + for key in ["needs", "requires"] { + if let Some(list) = step.get_mut(key).and_then(Value::as_array_mut) { + list.retain(|need| !removed.iter().any(|id| need == id)); + } + } + } + if !merged.is_empty() { + plan["alreadyMerged"] = json!(merged); + } +} + +pub(crate) fn merged_review_repos(bundle: &crate::model::ChangeGroup) -> BTreeSet { + bundle + .publications + .iter() + .filter(|p| crate::providers::is_review_kind(&p.kind)) + .filter(|p| p.state.eq_ignore_ascii_case("MERGED")) + .map(|p| p.repo_id.clone()) + .collect() +} + /// A review into a repository this account cannot merge waits for the /// repository's maintainers instead of being merged. fn mark_upstream_merges(active: &ActiveBundle, plan: &mut Value) -> Result<()> { @@ -581,6 +705,10 @@ pub(crate) fn display_plan(active: &ActiveBundle, plan: &Value, path: &Path) -> effective["steps"] = json!(steps); let display: super::super::LandPlan = serde_json::from_value(effective)?; super::super::display::print_plan(active, &display, path); + if let Some(merged) = plan["alreadyMerged"].as_array().filter(|m| !m.is_empty()) { + let repos: Vec<&str> = merged.iter().filter_map(Value::as_str).collect(); + println!("Already merged: {}", repos.join(", ")); + } println!("Hash: {}", canonical_hash(plan)); if plan["merge"]["enabled"] == false { if plan["merge"]["repositories"] diff --git a/src/commands/land/v2/graph.rs b/src/commands/land/v2/graph.rs index d34f604..40c4f0d 100644 --- a/src/commands/land/v2/graph.rs +++ b/src/commands/land/v2/graph.rs @@ -659,8 +659,9 @@ pub(crate) fn validation(plan: &Value, bundle: Option<&Value>, project: Option<& .filter_map(|s| s["repoId"].as_str()) .collect(); if plan["terminal"] != false { + let already_merged = super::generate::merged_review_repos(&typed); for id in &changed { - if !merge_policy.enabled_for(id) { + if !merge_policy.enabled_for(id) || already_merged.contains(id) { continue; } if !merged.contains(id.as_str()) { diff --git a/src/commands/land/v2/mod.rs b/src/commands/land/v2/mod.rs index 74fec85..855bd1e 100644 --- a/src/commands/land/v2/mod.rs +++ b/src/commands/land/v2/mod.rs @@ -5,6 +5,7 @@ mod destinations; mod gates; mod generate; pub use destinations::destinations; +pub(crate) use generate::{default_plan, destination_path, merged_review_repos}; mod graph; mod mergeability; pub(crate) use mergeability::KnownNoEffect; @@ -13,15 +14,17 @@ pub use preflight::preflight; mod ledger; mod runtime; pub(crate) use ledger::repair_landed_nodes; +mod remove; mod sequence; +pub use remove::remove; mod source; pub use source::source; #[cfg(test)] mod tests; use crate::store::read_json; use anyhow::{bail, Result}; +pub(super) use generate::display_plan; pub use generate::generate; -pub(super) use generate::{destination_path, display_plan}; #[cfg(test)] pub(crate) use graph::bundle_fingerprint; pub(crate) use graph::{bundle_fingerprint_is_stale, canonical_hash, validation}; diff --git a/src/commands/land/v2/remove.rs b/src/commands/land/v2/remove.rs new file mode 100644 index 0000000..938d192 --- /dev/null +++ b/src/commands/land/v2/remove.rs @@ -0,0 +1,148 @@ +//! `knit land remove`: take steps out of a saved plan. Nothing waits on a +//! removed step afterwards, and the edited plan must still validate. + +use super::generate::{destination_path, local_project}; +use crate::output as out; +use crate::store::{load_active_bundle, read_json, write_json}; +use anyhow::{bail, Result}; +use serde_json::{json, Value}; +use std::path::Path; + +pub fn remove( + plan_path: Option<&Path>, + ids: &[String], + target: Option<&str>, + lane: Option<&str>, +) -> Result<()> { + let active = load_active_bundle()?; + let path = plan_path + .map(Path::to_path_buf) + .unwrap_or_else(|| destination_path(&active, target, lane)); + if !path.exists() { + bail!( + "no landing plan at {}; run `knit land` first", + path.display() + ); + } + let mut plan: Value = read_json(&path)?; + remove_steps(&mut plan, ids)?; + let bundle = serde_json::to_value(&active.bundle)?; + let project = local_project(&active)?; + let result = super::graph::validation(&plan, Some(&bundle), Some(&project)); + if result["valid"] != true { + bail!( + "the plan would no longer be valid without {}: {}", + ids.join(", "), + result["errors"] + ); + } + write_json(&path, &plan)?; + for id in ids { + println!("{} {}", out::ok("removed"), out::node(id)); + } + println!( + "{} {}", + out::heading("Plan file:"), + out::path(path.display()) + ); + // A synced plan only runs once the edited revision is on the sync remotes. + crate::commands::remote::landing::sync_finished_run(&active.root, &plan, &[], false).map_err( + |error| { + anyhow::anyhow!( + "removed, but the edited plan could not be synced, so it cannot run yet: {error:#}" + ) + }, + )?; + if target.is_none() && lane.is_none() && plan_path.is_none_or(|p| p == path) { + let mut locked = crate::store::load_active_bundle_for_update()?; + crate::commands::bundle::close_if_merged(&mut locked, Some(&plan))?; + } + Ok(()) +} + +pub(crate) fn remove_steps(plan: &mut Value, ids: &[String]) -> Result<()> { + let steps = plan["steps"].as_array_mut().into_iter().flatten(); + let known: Vec = steps + .filter_map(|s| s["id"].as_str().map(str::to_owned)) + .collect(); + for id in ids { + if !known.contains(id) { + bail!( + "the plan has no step `{id}`; its steps are: {}", + known.join(", ") + ); + } + } + if let Some(steps) = plan["steps"].as_array_mut() { + steps.retain(|s| !ids.iter().any(|id| s["id"] == *id)); + for step in steps.iter_mut() { + for key in ["needs", "requires"] { + if let Some(list) = step.get_mut(key).and_then(Value::as_array_mut) { + list.retain(|need| !ids.iter().any(|id| need == id)); + } + } + } + } + if let Some(workflow) = plan.get("workflow").cloned() { + match prune_workflow(&workflow, ids) { + Some(pruned) => plan["workflow"] = pruned, + None => { + plan.as_object_mut().unwrap().remove("workflow"); + } + } + } + Ok(()) +} + +fn prune_workflow(node: &Value, ids: &[String]) -> Option { + if let Some(id) = node.get("step") { + return (!ids.iter().any(|removed| id == removed)).then(|| node.clone()); + } + for key in ["sequence", "parallel"] { + if let Some(children) = node.get(key).and_then(Value::as_array) { + let kept: Vec = children + .iter() + .filter_map(|child| prune_workflow(child, ids)) + .collect(); + return (!kept.is_empty()).then(|| json!({ key: kept })); + } + } + Some(node.clone()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn removed_steps_leave_no_dangling_needs_or_workflow_nodes() { + let mut plan = json!({ + "steps": [ + {"id": "merge-api", "type": "merge_pr"}, + {"id": "deploy", "type": "deploy", "needs": ["merge-api"], "requires": ["merge-api"]}, + {"id": "verify", "type": "run", "needs": ["deploy"]} + ], + "workflow": {"sequence": [{"step": "merge-api"}, {"parallel": [{"step": "deploy"}]}, {"step": "verify"}]} + }); + remove_steps(&mut plan, &["deploy".to_string()]).unwrap(); + let ids: Vec<_> = plan["steps"] + .as_array() + .unwrap() + .iter() + .map(|s| s["id"].as_str().unwrap()) + .collect(); + assert_eq!(ids, ["merge-api", "verify"]); + assert_eq!(plan["steps"][1]["needs"], json!([])); + assert_eq!( + plan["workflow"], + json!({"sequence": [{"step": "merge-api"}, {"step": "verify"}]}) + ); + } + + #[test] + fn an_unknown_step_names_the_ones_that_exist() { + let mut plan = json!({"steps": [{"id": "merge-api"}]}); + let error = remove_steps(&mut plan, &["deploy".to_string()]).unwrap_err(); + assert!(error.to_string().contains("merge-api"), "{error}"); + } +} diff --git a/src/commands/land/v2/tests.rs b/src/commands/land/v2/tests.rs index 732ce8d..8d2819f 100644 --- a/src/commands/land/v2/tests.rs +++ b/src/commands/land/v2/tests.rs @@ -322,6 +322,45 @@ fn generator_includes_build_verify_and_custom_steps() { ); } #[test] +fn a_review_the_host_already_merged_gets_no_merge_step() { + let mut f = Fixture::new(); + f.bundle["repos"] = json!([ + {"id":"api","path":"api","baseBranch":"main","remote":"https://github.com/acme/api.git","featureBranch":"knit/synthetic","worktreePath":null,"headSha":"1111111111111111111111111111111111111111","baseSha":"0000000000000000000000000000000000000000"}, + {"id":"web","path":"web","baseBranch":"main","remote":"https://github.com/acme/web.git","featureBranch":"knit/synthetic","worktreePath":null,"headSha":"2222222222222222222222222222222222222222","baseSha":"0000000000000000000000000000000000000000"} + ]); + f.bundle["publications"] = json!([ + {"repoId":"api","provider":"github","kind":"pull_request","number":1,"url":"https://github.com/acme/api/pull/1","baseBranch":"main","headBranch":"knit/synthetic","state":"MERGED","updatedAt":"2026-01-01T00:00:00Z"}, + {"repoId":"web","provider":"github","kind":"pull_request","number":2,"url":"https://github.com/acme/web/pull/2","baseBranch":"main","headBranch":"knit/synthetic","state":"OPEN","updatedAt":"2026-01-01T00:00:00Z"} + ]); + let mut project = serde_json::to_value(crate::model::KnitProject::new( + "synthetic-project".into(), + crate::time::now_iso(), + )) + .unwrap(); + project["repos"] = json!([ + {"id":"api","path":"api","remote":"https://github.com/acme/api.git","baseBranch":"main"}, + {"id":"web","path":"web","remote":"https://github.com/acme/web.git","baseBranch":"main"} + ]); + project["landing"] = json!({"deployments":[{"id":"release","repoId":"api","whenChanged":["*"],"command":["true"]}]}); + let active = crate::store::ActiveBundle::unlocked( + f.dir.clone(), + f.dir.join("bundle.json"), + serde_json::from_value(f.bundle.clone()).unwrap(), + ); + let plan = generate::build(&active, &f.bundle, &project, None, None, None).unwrap(); + let steps = plan["steps"].as_array().unwrap(); + let ids: Vec<_> = steps.iter().map(|s| s["id"].as_str().unwrap()).collect(); + assert!(!ids.contains(&"merge-api"), "{ids:?}"); + assert!(ids.contains(&"merge-web"), "{ids:?}"); + let release = steps.iter().find(|s| s["id"] == "release").unwrap(); + assert_eq!(release["needs"], json!(["merge-web"])); + assert_eq!(plan["alreadyMerged"], json!(["api"])); + f.plan = plan; + f.files(); + let result = super::graph::validation(&f.plan, Some(&f.bundle), Some(&project)); + assert_eq!(result["valid"], true, "{result}"); +} +#[test] fn reverse_recovery_restores_original_not_intermediate_state() { let mut f = Fixture::new(); fs::write(f.dir.join("state"), "original").unwrap(); @@ -981,13 +1020,11 @@ fn repository_merge_mixes_staging_branches_and_reviews_but_delegates_production_ assert_eq!(production["steps"][0]["type"], "merge_pr"); assert_eq!(production["terminal"], true); assert_eq!(production["changedRepos"], json!(["app", "tools"])); - // An already merged review is retained for the executor's idempotent check. let mut f = f; f.bundle["publications"][0]["state"] = json!("MERGED"); - assert_eq!( - repository_merge_plan(&f, &project, "production").unwrap()["steps"][0]["type"], - "merge_pr" - ); + let merged = repository_merge_plan(&f, &project, "production").unwrap(); + assert_eq!(merged["steps"], json!([])); + assert_eq!(merged["alreadyMerged"], json!(["tools"])); } #[test] diff --git a/src/commands/land/validate.rs b/src/commands/land/validate.rs index 722e00f..8a2cf3d 100644 --- a/src/commands/land/validate.rs +++ b/src/commands/land/validate.rs @@ -50,8 +50,9 @@ pub(super) fn validate_plan_for_bundle(active: &ActiveBundle, plan: &LandPlan) - .filter(|s| s.step_type == LandStepKind::MergePr) .filter_map(|s| s.repo_id.as_ref()) .collect(); + let already_merged = super::v2::merged_review_repos(&active.bundle); for id in crate::commands::publish::publish_scope_repo_ids(&active.bundle) { - if !covered.contains(&id) { + if !covered.contains(&id) && !already_merged.contains(&id) { bail!("Terminal land plan omits changed repository `{id}`; it would strand work when archiving."); } } diff --git a/src/commands/publish/mod.rs b/src/commands/publish/mod.rs index 0c1e1b9..f5f0e23 100644 --- a/src/commands/publish/mod.rs +++ b/src/commands/publish/mod.rs @@ -643,7 +643,7 @@ pub fn sync_publications(selectors: &[String], all: bool, provider: Option<&str> if !failures.is_empty() { bail!("PR sync completed with failures:\n{}", failures.join("\n")); } - + crate::commands::bundle::close_if_merged(&mut active, None)?; Ok(()) } @@ -689,6 +689,8 @@ fn publication_texts( pub fn sync_publications_from_artifact( artifact_path: &Path, out_path: Option<&Path>, + plan_path: Option<&Path>, + project_path: Option<&Path>, selectors: &[String], all: bool, provider: Option<&str>, @@ -706,6 +708,16 @@ pub fn sync_publications_from_artifact( if !failures.is_empty() { bail!("PR sync completed with failures:\n{}", failures.join("\n")); } + let plan: Option = plan_path.map(crate::store::read_json).transpose()?; + let project: Option = + project_path.map(crate::store::read_json).transpose()?; + crate::commands::bundle::close_artifact_if_merged( + &cwd, + artifact_path, + &mut bundle, + plan.as_ref(), + project.as_ref(), + ); write_bundle_artifact_output(&bundle, out_path)?; Ok(()) } diff --git a/src/commands/remote/pull.rs b/src/commands/remote/pull.rs index 8c07a0b..bbab24b 100644 --- a/src/commands/remote/pull.rs +++ b/src/commands/remote/pull.rs @@ -2020,6 +2020,13 @@ fn fetch_bundles_with_options( // The replaced local artifact may know the hosted URL // from before this copy left the remote. bundle.inherit_missing_sync_target_web_urls(&local); + if crate::commands::bundle::bundle_state(&local) + == crate::commands::bundle::BundleStatus::Open + && crate::commands::bundle::bundle_state(&bundle) + == crate::commands::bundle::BundleStatus::Archived + { + bundle = remove_finished_worktrees(root, &bundle_path, bundle); + } crate::store::write_json(&bundle_path, &bundle).with_context(|| { format!("failed to write bundle `{}`", remote_bundle.slug) })?; @@ -2333,6 +2340,23 @@ fn bundle_branch_mapping(bundle: &ChangeGroup) -> String { .join(", ") } +/// A bundle closed on the remote leaves its clean checkouts behind here the +/// same way a local archive would; checkouts with changes are kept. +fn remove_finished_worktrees(root: &Path, path: &Path, bundle: ChangeGroup) -> ChangeGroup { + let mut active = + crate::store::ActiveBundle::unlocked(root.to_path_buf(), path.to_path_buf(), bundle); + if let Err(error) = crate::commands::clean::clean_worktrees_for_bundle(&mut active, false) { + println!( + " {} {error:#}", + out::warn(format!( + "{} closed remotely; checkouts kept:", + active.bundle.id + )) + ); + } + active.bundle +} + #[cfg(test)] mod publishing_roundtrip_tests { use super::*; diff --git a/src/lib.rs b/src/lib.rs index 71ea189..13ecec4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -599,6 +599,8 @@ pub fn run(cli: Cli) -> Result<()> { repos, from_artifact, out, + plan, + project_file, all, provider, github, @@ -608,6 +610,8 @@ pub fn run(cli: Cli) -> Result<()> { Some(path) => commands::sync_publications_from_artifact( &path, out.as_deref(), + plan.as_deref(), + project_file.as_deref(), &repos, all, provider.as_deref(), @@ -651,6 +655,12 @@ pub fn run(cli: Cli) -> Result<()> { commands::land::v2::show(&plan, target.as_deref(), lane.as_deref()) } Some(LandCommand::Destinations { json }) => commands::land::v2::destinations(json), + Some(LandCommand::Remove { steps, plan }) => commands::land::v2::remove( + plan.as_deref(), + &steps, + target.as_deref(), + lane.as_deref(), + ), Some(LandCommand::Validate { plan, from_artifact, diff --git a/tests/contribution_publish.rs b/tests/contribution_publish.rs index d59fd5b..d7bafec 100644 --- a/tests/contribution_publish.rs +++ b/tests/contribution_publish.rs @@ -270,6 +270,119 @@ fn sync_records_what_the_maintainers_still_have_to_do() { ); } +#[test] +fn a_bundle_whose_reviews_all_merged_closes_once_its_plan_has_nothing_left() { + let f = Fixture::new(); + f.create(); + fs::write( + f.api.join("override.json"), + json!({"state": "closed", "merged": true, "merged_at": "2026-10-09T00:00:00Z"}).to_string(), + ) + .unwrap(); + let plan = f.root.join("plan.json"); + fs::write( + &plan, + json!({"steps": [ + {"id": "merge-widget", "type": "merge_pr"}, + {"id": "deploy-widget", "type": "deploy"} + ]}) + .to_string(), + ) + .unwrap(); + let read = + |path: &PathBuf| -> Value { serde_json::from_slice(&fs::read(path).unwrap()).unwrap() }; + let kept = f.root.join("kept.json"); + let output = f.run(&[ + "publish", + "sync", + "--from-artifact", + f.artifact().to_str().unwrap(), + "--out", + kept.to_str().unwrap(), + "--plan", + plan.to_str().unwrap(), + ]); + assert!(output.contains("still has deploy-widget"), "{output}"); + assert_eq!(read(&kept)["publications"][0]["state"], "MERGED"); + assert_ne!(read(&kept)["state"], "archived"); + + let closed = f.root.join("closed.json"); + f.run(&[ + "publish", + "sync", + "--from-artifact", + f.artifact().to_str().unwrap(), + "--out", + closed.to_str().unwrap(), + ]); + assert_eq!(read(&closed)["state"], "archived"); + + let output = f.run(&["publish", "sync"]); + assert!(output.contains("Closed"), "{output}"); + assert_eq!(f.bundle()["state"], "archived"); +} + +#[test] +fn without_a_saved_plan_the_project_recipes_decide_whether_a_merged_bundle_closes() { + let f = Fixture::new(); + f.create(); + fs::write( + f.api.join("override.json"), + json!({"state": "closed", "merged": true, "merged_at": "2026-10-09T00:00:00Z"}).to_string(), + ) + .unwrap(); + let read = + |path: &PathBuf| -> Value { serde_json::from_slice(&fs::read(path).unwrap()).unwrap() }; + let repo = read(&f.artifact())["repos"][0]["id"].clone(); + let mut project = json!({ + "schemaVersion": "0.1", "kind": "KnitProject", "id": "demo", + "createdAt": "2026-10-09T00:00:00Z", "updatedAt": "2026-10-09T00:00:00Z", + "repos": [{"id": repo, "path": repo, "remote": TARGET, "baseBranch": "main"}], + "landing": {"deployments": [{"id": "deploy-widget", "repoId": repo, "command": ["true"]}]} + }); + let project_file = f.root.join("project.json"); + let sync = |out: &PathBuf| { + f.run(&[ + "publish", + "sync", + "--from-artifact", + f.artifact().to_str().unwrap(), + "--out", + out.to_str().unwrap(), + "--project-file", + project_file.to_str().unwrap(), + ]) + }; + + fs::write(&project_file, project.to_string()).unwrap(); + let kept = f.root.join("kept.json"); + let output = sync(&kept); + assert!(output.contains("still has deploy-widget"), "{output}"); + assert_ne!(read(&kept)["state"], "archived"); + + project["landing"] = json!({}); + fs::write(&project_file, project.to_string()).unwrap(); + let closed = f.root.join("closed.json"); + let output = sync(&closed); + assert!(output.contains("Closed"), "{output}"); + assert_eq!(read(&closed)["state"], "archived"); +} + +#[test] +fn land_closes_a_bundle_the_host_already_merged() { + let f = Fixture::new(); + f.create(); + fs::write( + f.api.join("override.json"), + json!({"state": "closed", "merged": true, "merged_at": "2026-10-09T00:00:00Z"}).to_string(), + ) + .unwrap(); + let output = f.run(&["land"]); + assert!(output.contains("merged on the host"), "{output}"); + assert!(output.contains("Closed"), "{output}"); + assert_eq!(f.bundle()["state"], "archived"); +} + #[test] fn wrong_same_number_url_and_wrong_review_identity_never_mutate() { let f = Fixture::new();