artifact terminal coverage and resume safety (knit)

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.<name>.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.
 

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::<Vec<_>>();
+        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

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<Vec<Vec
             if !remaining.contains(step_id) {
                 continue;
             }
-            let step = steps
-                .iter()
-                .find(|s| &s.id == step_id)
-                .expect("order references a real step");
+            let Some(step) = steps.iter().find(|s| &s.id == step_id) else {
+                bail!(
+                    "land plan order references step `{step_id}`, which the plan does not contain"
+                );
+            };
             if step.needs.iter().all(|need| satisfied.contains(need)) {
                 wave.push(step_id.clone());
             }

src/commands/land/mod.rs

@@ -40,6 +40,7 @@ use crate::store::{
 };
 use crate::time::now_iso;
 use anyhow::{bail, Context, Result};
+use std::collections::BTreeSet;
 use std::fs;
 use std::path::{Path, PathBuf};
 
@@ -110,18 +111,25 @@ pub fn land_default(target_branch: Option<&str>, 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::<Vec<_>>();
+    let removed = run_ids.difference(&plan_ids).copied().collect::<Vec<_>>();
+    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)? {