diff --git a/docs/reference.md b/docs/reference.md index 8ff7203..04eba95 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -397,11 +397,11 @@ A deployment runs when something it deploys changed. A bundle carries a project' By default a deployment watches the repository it deploys. `whenChanged` widens that, because a deployment does not always depend only on its own repository: an image that builds another repository's binary into itself has to redeploy when *that* repository changes, or it quietly ships a stale one. Write `"whenChanged": ["api", "api-client", "shared-tools"]` on the deployment and it runs when any of them changed. A literal `"*"` runs it on every landing, as in `landing.lanes..branches`. `whenChanged` is held to a shape that cannot silently mean nothing: the list must be non-empty and free of repeats, every id must name a repository of this project, and `"*"` must stand alone rather than sit beside named repositories. Ids are checked even when `"*"` is present, so a typo cannot ride along unvalidated. All of this is refused rather than ignored, because watching a repository that does not exist means never deploying, and a typo is exactly what that looks like. A push deployment reports only that its own repository's merge triggered it, so watching another repository is refused there outright — use `mode: "command"` for a deployment something else triggers. The deployments a plan leaves out are recorded in its `deploymentsSkipped`, with what each one watches, and printed under `Deployments not run:` — for the same reason lane absences are printed, so a step missing on purpose never reads like one lost to a bug. Skipping resolves against `needs`: a deployment nothing triggers is still planned when a deployment that *is* running depends on it, transitively, because `B needs A` means A has to run. -A terminal landing has to carry everything the bundle changed. It archives the bundle and removes its worktrees, so a changed repository left out of it is work stranded on a branch nobody will land — while the forge says the feature shipped. Knit refuses to plan one that does not cover every changed repository, naming them and which fix applies: a missing review means `knit publish create`, and a repository the project's `merge.repoOrder` excludes under `includeUnlisted: false` means adding it to the order. Intermediate destinations are deliberately allowed to carry a subset; that is what `laneAbsent` records. +A terminal landing has to carry everything the bundle changed. It archives the bundle and removes its worktrees, so a changed repository left out of it is work stranded on a branch nobody will land — while the forge says the feature shipped. Knit refuses to plan one that does not cover every changed repository, naming them and which fix applies: a missing review means `knit publish create`, and a repository the project's `merge.repoOrder` excludes under `includeUnlisted: false` means adding it to the order. Intermediate destinations are deliberately allowed to carry a subset; that is what `laneAbsent` records. The hosted path holds the same line: `knit land apply --from-artifact` refuses a terminal landing, declared or inferred, when a changed repository has no recorded review, naming it, exactly as the local plan does. -A plan describes the bundle as it was when the plan was generated, so it records `changedRepos` and each repository's `bundleHeads`. Committing more work makes that plan a description of the past: the new repository would never merge, and deployments would be scoped to a change set that no longer exists. Both `knit land apply` and `knit land resume` refuse a plan whose pin no longer matches and tell you to regenerate it with `knit land plan --force`. `knit land update` moves feature heads on purpose, so it re-pins the plan it just prepared rather than invalidating it — update-then-land keeps working. Plans written before pinning existed carry no pin and are accepted unpinned. +A plan describes the bundle as it was when the plan was generated, so it records `changedRepos` and each repository's `bundleHeads`. Committing more work makes that plan a description of the past: the new repository would never merge, and deployments would be scoped to a change set that no longer exists. Both `knit land apply` and `knit land resume` refuse a plan whose pin no longer matches and tell you to regenerate it with `knit land plan --force`. `knit land update` moves feature heads on purpose, so it re-pins the plan it just prepared rather than invalidating it — update-then-land keeps working. Plans written before pinning existed carry no pin and are accepted unpinned. The same pins decide what a finished run means: asking for the destination that last succeeded — `knit land --lane staging` again after committing more work — plans the new work instead of reporting the old run, because a run whose plan no longer describes the bundle is history rather than an answer. -`knit land resume` finishes a landing exactly as `knit land apply` does. A resumed terminal run records the landed node, archives the bundle, removes generated worktrees, clears the workspace's active bundle and honours `--tag`/`auto-tag`; it takes the same `--keep-worktrees`, `--tag` and `--no-tag` flags for that reason. +`knit land resume` finishes a landing exactly as `knit land apply` does. A resumed terminal run records the landed node, archives the bundle, removes generated worktrees, clears the workspace's active bundle and honours `--tag`/`auto-tag`; it takes the same `--keep-worktrees`, `--tag` and `--no-tag` flags for that reason. A run records the steps of the plan it started from, so `knit land resume` refuses a run whose plan was regenerated in between, naming the steps that were added or removed, and points at a fresh `knit land apply` instead. `landing.targets` remains the branch-keyed deployment mechanism for raw/common targets. `knit land --target staging` selects `landing.targets.staging.deployments` for the whole landing; without `--target` or `--lane`, recorded per-repo review bases select matching targets and mixed bases can select more than one. A target can declare multiple repo deployments; repo-scoped entries are included only for reviews landing into that target. Target deployment ids must be unique across targets that can be selected together. diff --git a/src/commands/land/artifact.rs b/src/commands/land/artifact.rs index ae4c91e..b7d8bd7 100644 --- a/src/commands/land/artifact.rs +++ b/src/commands/land/artifact.rs @@ -148,6 +148,28 @@ pub fn apply_land_from_artifact( destination == Some(repo.base_branch.as_str()) }) }); + // The bundle's last stop has to carry everything the bundle changed, the + // rule the local plan enforces too: a terminal landing archives the + // bundle, so a changed repository with no review to merge would be left + // stranded on its feature branch while the forge says the feature + // shipped. Refused before anything moves. + if terminal { + let unpublished = changed_repo_ids + .iter() + .filter(|repo_id| bundle.repos.iter().any(|repo| repo.id == **repo_id)) + .filter(|repo_id| publication_for_repo(&bundle, repo_id).is_none()) + .map(String::as_str) + .collect::>(); + if !unpublished.is_empty() { + let one = unpublished.len() == 1; + bail!( + "This landing is terminal and would archive the bundle, but {} {} no recorded review. Landing now would strand that work on its feature branch. Publish {} first, or land into an intermediate destination.", + unpublished.join(", "), + if one { "has" } else { "have" }, + if one { "it" } else { "them" } + ); + } + } // Mirrors the local plan: an intermediate explicit destination, lane or // raw target, is reached by merging the feature branches; the terminal // destination merges the reviews. Without either, each review is merged diff --git a/src/commands/land/execute.rs b/src/commands/land/execute.rs index db49fac..3a36ee7 100644 --- a/src/commands/land/execute.rs +++ b/src/commands/land/execute.rs @@ -38,16 +38,18 @@ pub(super) fn execute_run( for wave in &waves { let mut pending: Vec<(&LandStep, usize)> = Vec::new(); for step_id in wave { - let step = plan - .steps - .iter() - .find(|s| &s.id == step_id) - .expect("validated plan order references a real step"); - let run_index = run - .steps - .iter() - .position(|run_step| run_step.id == step.id) - .expect("run contains every plan step"); + let Some(step) = plan.steps.iter().find(|s| &s.id == step_id) else { + bail!( + "land plan order references step `{step_id}`, which the plan does not contain" + ); + }; + let Some(run_index) = run.steps.iter().position(|run_step| run_step.id == step.id) + else { + bail!( + "land run does not record step `{}`; the plan was changed after the run started, so start a new landing with `knit land apply`", + step.id + ); + }; if run.steps[run_index].status == LandStatus::Succeeded { continue; } @@ -360,10 +362,11 @@ pub(super) fn step_waves(steps: &[LandStep], order: &[String]) -> Result, lane_name: Option<&str>) -> Res lane_name.as_deref(), &plan, ); - if same_destination.is_err() - && run.status == LandStatus::Succeeded - && run.rolled_back_at.is_none() - { - drop(active); - return generate_land_plan( - None, - None, - true, - target_branch.as_deref(), - lane_name.as_deref(), - ); + if run.status == LandStatus::Succeeded && run.rolled_back_at.is_none() { + // The same destination asked for again after more work was + // committed is the same situation: the finished run's plan no + // longer describes the bundle, so the operator is asking to land + // the new work, not to hear about the old run. A plan written + // before pinning existed carries no pin and still reads as the + // finished run. + let stale_for_same_destination = same_destination.is_ok() + && validate::ensure_plan_matches_bundle_state(&active, &plan).is_err(); + if same_destination.is_err() || stale_for_same_destination { + drop(active); + return generate_land_plan( + None, + None, + true, + target_branch.as_deref(), + lane_name.as_deref(), + ); + } } same_destination?; display::print_run_status(&active, &run, &path); @@ -540,6 +548,7 @@ pub fn resume_land_run( } let plan_path = resolve_stored_path(&active.root, &run.plan_path); let plan: LandPlan = read_json(&plan_path)?; + ensure_run_matches_plan(&run, &plan)?; validate::validate_plan_for_bundle(&active, &plan)?; validate::preflight_required_checks(&active, &plan.require_checks, skip_checks)?; let order = validate::ordered_step_ids(&plan.steps)?; @@ -562,6 +571,41 @@ pub fn resume_land_run( ) } +/// A run records the steps of the plan it was started from. If that plan file +/// was regenerated since (`knit land plan --force` after committing more work, +/// which the stale-pin check tells the operator to do), its steps no longer +/// describe the run: continuing would execute steps the run never recorded, or +/// wait on steps that no longer exist. Refuse instead of panicking on the +/// mismatch, and name what differs. +fn ensure_run_matches_plan(run: &LandRun, plan: &LandPlan) -> Result<()> { + let run_ids: BTreeSet<&str> = run.steps.iter().map(|step| step.id.as_str()).collect(); + let plan_ids: BTreeSet<&str> = plan.steps.iter().map(|step| step.id.as_str()).collect(); + if run_ids == plan_ids { + return Ok(()); + } + let added = plan_ids.difference(&run_ids).copied().collect::>(); + let removed = run_ids.difference(&plan_ids).copied().collect::>(); + let mut detail = Vec::new(); + if !added.is_empty() { + detail.push(format!( + "the plan now has {} {}, which the run never recorded", + if added.len() == 1 { "step" } else { "steps" }, + added.join(", ") + )); + } + if !removed.is_empty() { + detail.push(format!( + "the run recorded {} {}, which the plan no longer has", + if removed.len() == 1 { "step" } else { "steps" }, + removed.join(", ") + )); + } + bail!( + "This run was recorded against a different plan: {}. The plan was regenerated after the run started, so the run cannot continue. Start a new landing with `knit land apply`; steps whose reviews already merged are recognised as already landed.", + detail.join("; ") + ); +} + pub fn show_land_status(run_path: Option<&Path>) -> Result<()> { let active = load_active_bundle()?; if let Some(path) = resolve_land_run_path(&active, run_path)? { diff --git a/tests/land.rs b/tests/land.rs index 37b7e01..dc6f380 100644 --- a/tests/land.rs +++ b/tests/land.rs @@ -1421,6 +1421,65 @@ fn latest_land_run(workspace: &Path) -> (std::path::PathBuf, Value) { (path, run) } +/// A run records the steps of the plan it started from. Regenerating the plan +/// in between (`knit land plan --force` after more work, say) gives it a +/// different step list; resuming used to panic on the mismatch instead of +/// saying what happened. +#[test] +fn land_resume_refuses_a_run_whose_plan_was_regenerated() { + let root = unique_temp_dir(); + let (workspace, fake_bin, fake_gh_dir) = publish_two_repo_bundle(&root); + knit_with_fake_gh(&workspace, ["land", "plan"], &fake_bin, &fake_gh_dir); + write_half_failing_plan(&workspace, None); + + let failed = knit_fails_with_fake_gh( + &workspace, + ["land", "apply", "--no-remote"], + &fake_bin, + &fake_gh_dir, + ); + assert!(failed.contains("stopped at step gate"), "{failed}"); + + // The plan is rewritten underneath the run: the gate it recorded is gone, + // and a step it never recorded is appended. + let plan_path = workspace.join(".knit/land-plans/venue-capacity.land.json"); + let mut plan: Value = serde_json::from_str(&fs::read_to_string(&plan_path).unwrap()).unwrap(); + let steps = plan["steps"].as_array_mut().unwrap(); + steps.retain(|step| step["id"].as_str() != Some("gate")); + for step in steps.iter_mut() { + if step["id"].as_str() == Some("merge-frontend") { + step["needs"] = json!(["merge-backend"]); + } + } + steps.push(json!({ + "id": "smoke", + "type": "run", + "cwd": ".", + "command": ["sh", "-c", "true"], + "needs": ["merge-frontend"] + })); + fs::write(&plan_path, serde_json::to_string_pretty(&plan).unwrap()).unwrap(); + + let resume = knit_fails_with_fake_gh(&workspace, ["land", "resume"], &fake_bin, &fake_gh_dir); + assert!( + resume.contains("recorded against a different plan"), + "{resume}" + ); + assert!(resume.contains("smoke"), "{resume}"); + assert!(resume.contains("gate"), "{resume}"); + assert!(resume.contains("knit land apply"), "{resume}"); + assert!(!resume.contains("panicked"), "{resume}"); + + // Nothing moved: backend merged during apply, frontend still has not, and + // the run was left as it was. + let order = fs::read_to_string(fake_gh_dir.join("merge-order.txt")).unwrap(); + assert_eq!(order.lines().collect::>(), vec!["backend"]); + let (_, run) = latest_land_run(&workspace); + assert_eq!(run["status"].as_str(), Some("failed")); + + fs::remove_dir_all(root).unwrap(); +} + #[test] fn land_rollback_creates_revert_prs_for_merged_steps_of_failed_run() { let root = unique_temp_dir(); @@ -2125,6 +2184,65 @@ fn bundle_lands_into_staging_then_production() { fs::remove_dir_all(root).unwrap(); } +/// A finished run is an answer only while its plan still describes the +/// bundle. After more work is committed, asking for the same lane again is a +/// request to land that work, not to hear about the old run. +#[test] +fn landing_the_same_lane_again_after_new_work_plans_the_new_work() { + let root = unique_temp_dir(); + let (workspace, fake_bin, fake_gh_dir) = publish_lane_bundle( + &root, + "staging twice", + json!({ + "staging": { "defaultBranch": "staging" }, + "production": { "defaultBranch": "main" } + }), + &["staging"], + ); + + knit_with_fake_gh( + &workspace, + ["land", "--lane", "staging"], + &fake_bin, + &fake_gh_dir, + ); + let first = knit_with_fake_gh( + &workspace, + ["land", "--lane", "staging", "apply", "--no-remote"], + &fake_bin, + &fake_gh_dir, + ); + assert!(first.contains("bundle stays open"), "{first}"); + + append_line( + &workspace.join(".knit/worktrees/staging-twice/backend/app.txt"), + "more staging work", + ); + knit(&workspace, ["commit", "--all", "-m", "More staging work"]); + + let plan = knit_with_fake_gh( + &workspace, + ["land", "--lane", "staging"], + &fake_bin, + &fake_gh_dir, + ); + assert!(plan.contains("Lane: staging"), "{plan}"); + assert!(plan.contains("stays open on success"), "{plan}"); + assert!(!plan.contains("Land run"), "{plan}"); + + let again = knit_with_fake_gh( + &workspace, + ["land", "--lane", "staging", "apply", "--no-remote"], + &fake_bin, + &fake_gh_dir, + ); + assert!(again.contains("bundle stays open"), "{again}"); + let staging = git(&root.join("backend.git"), ["log", "--oneline", "staging"]); + assert!(staging.contains("More staging work"), "{staging}"); + + fs::remove_dir_all(root).unwrap(); +} + #[test] fn bare_land_after_a_lane_landing_plans_the_terminal_destination() { let root = unique_temp_dir(); @@ -2821,6 +2939,121 @@ fn artifact_lane_accepts_absent_repositories() { fs::remove_dir_all(root).unwrap(); } +/// The hosted path enforces the local plan's rule: a terminal landing archives +/// the bundle, so every changed repository needs a review to merge, or its +/// work is stranded on a branch nobody will land. +#[test] +fn artifact_terminal_landing_refuses_to_strand_an_unpublished_repository() { + let root = unique_temp_dir(); + let (_backend_remote, backend, _c1) = init_remote_repo(&root, "backend"); + let (_frontend_remote, frontend, _c2) = init_remote_repo(&root, "frontend"); + let workspace = root.join("workspace"); + fs::create_dir_all(&workspace).unwrap(); + + knit(&workspace, ["bundle", "artifact strand"]); + knit( + &workspace, + [ + "bundle", + "add", + backend.to_str().unwrap(), + frontend.to_str().unwrap(), + ], + ); + for repo_id in ["backend", "frontend"] { + append_line( + &workspace + .join(".knit/worktrees/artifact-strand") + .join(repo_id) + .join("app.txt"), + "artifact strand change", + ); + } + knit( + &workspace, + ["commit", "--all", "-m", "Artifact strand change"], + ); + + let artifact = workspace.join(".knit/bundles/artifact-strand.bundle.json"); + let mut payload: Value = serde_json::from_str(&fs::read_to_string(&artifact).unwrap()).unwrap(); + for repo in payload["repos"].as_array_mut().unwrap() { + let id = repo["id"].as_str().unwrap().to_string(); + repo["remote"] = json!(format!("https://github.com/acme/{id}.git")); + } + // Both repositories have recorded work; only backend has a review. + payload["publications"] = json!([{ + "repoId": "backend", + "provider": "github", + "kind": "pull_request", + "number": 101, + "url": "https://github.com/acme/backend/pull/101", + "baseBranch": "main", + "headBranch": "knit/artifact-strand", + "state": "OPEN", + "title": "artifact strand (backend)", + "updatedAt": "2026-06-06T00:00:00.000Z" + }]); + fs::write(&artifact, serde_json::to_string_pretty(&payload).unwrap()).unwrap(); + + let fake_gh_dir = root.join("fake-gh"); + let fake_bin = root.join("fake-bin"); + write_fake_gh(&fake_bin, &fake_gh_dir); + let api_base = spawn_fake_github_api(&fake_gh_dir); + let out = root.join("artifact-strand.out.bundle.json"); + let env = [ + ("GH_TOKEN", "gho_fake_token"), + ("KNIT_GITHUB_API_TRANSPORT", "curl-ipv4"), + ("KNIT_GITHUB_API_BASE", api_base.as_str()), + ]; + let args = |lifecycle: Option<&str>| { + let mut args = vec![ + "land".to_string(), + "apply".to_string(), + "--from-artifact".to_string(), + artifact.to_string_lossy().to_string(), + "--out".to_string(), + out.to_string_lossy().to_string(), + ]; + args.extend(lifecycle.map(str::to_string)); + args + }; + + let declared = knit_fails_with_fake_gh_env( + &root, + args(Some("--terminal")), + &fake_bin, + &fake_gh_dir, + &env, + ); + assert!( + declared.contains("frontend has no recorded review"), + "{declared}" + ); + assert!(declared.contains("archive the bundle"), "{declared}"); + assert!( + declared.contains("land into an intermediate destination"), + "{declared}" + ); + // Refused before anything moved: no merge, no retarget, no output. + assert!(!fake_gh_dir.join("api-backend-merge.json").exists()); + assert!(!fake_gh_dir.join("api-backend-edit.json").exists()); + assert!(!fake_gh_dir.join("api-backend-merges.json").exists()); + assert!(!out.exists()); + + // Without a declared answer, the review on the configured base makes the + // landing terminal, and the same rule applies. + let inferred = knit_fails_with_fake_gh_env(&root, args(None), &fake_bin, &fake_gh_dir, &env); + assert!( + inferred.contains("frontend has no recorded review"), + "{inferred}" + ); + assert!(!fake_gh_dir.join("api-backend-merge.json").exists()); + assert!(!fake_gh_dir.join("api-backend-edit.json").exists()); + assert!(!out.exists()); + + fs::remove_dir_all(root).unwrap(); +} + /// A lane is an environment, reached by merging branches, so it does not need /// a review. Without one, nothing about the landing may read as terminal: /// an empty set of reviewed repositories is not "every repository reaches its