From 74db55989a89e9ad8eb8fa88d0fe7a980711be71 Mon Sep 17 00:00:00 2001 From: Jop Middelkamp Date: Sat, 8 Aug 2026 14:24:53 +0700 Subject: [PATCH 1/6] feat: dir-targeted git primitives for the ephemeral worktree is_working_tree_clean_at / stage_all_at / commit_at run via -C (the remove_current_worktree precedent; CommandRunner untouched) and remove_worktree removes a worktree by path with --force. Flags-table rows red-first; MockGit records path-bearing encodings and gains an add_worktree_error knob. --- src/git/mod.rs | 29 +++++++++++++++++++++++++++++ tests/common/mod.rs | 35 +++++++++++++++++++++++++++++++++-- tests/git_cli_test.rs | 8 ++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/git/mod.rs b/src/git/mod.rs index 0ced339..b632335 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -86,6 +86,18 @@ pub trait Git { // Version commits and merge-commit tagging fn stage_all(&self) -> Result<()>; fn commit(&self, message: &str) -> Result<()>; + // The `_at` family runs in another working tree via `-C` — used for the + // ephemeral version-script worktree, whose commit must never touch the + // user's checkout. Refs are shared across worktrees, so `push` needs no + // `_at` variant. + fn is_working_tree_clean_at(&self, dir: &Path) -> Result; + fn stage_all_at(&self, dir: &Path) -> Result<()>; + fn commit_at(&self, dir: &Path, message: &str) -> Result<()>; + /// Remove the worktree at `path`. Unlike `remove_current_worktree` this has + /// no last-operation constraint: the process never stands in `path`. + /// `--force`, because a failed script can leave the tree dirty and cleanup + /// must still succeed. + fn remove_worktree(&self, path: &Path) -> Result<()>; /// Tags `sha` (not HEAD) — a separate method from `create_tag` because a /// landing PR's merge commit is created by the hosting platform, not by a /// local `git merge`. @@ -404,6 +416,23 @@ impl Git for GitCli<'_> { fn stage_all(&self) -> Result<()> { self.run(&["add", "-A"]).map(|_| ()) } fn commit(&self, message: &str) -> Result<()> { self.run(&["commit", "-m", message]).map(|_| ()) } + fn is_working_tree_clean_at(&self, dir: &Path) -> Result { + let dir = dir.display().to_string(); + let output = self.run(&["-C", &dir, "status", "--porcelain"])?; + Ok(output.is_empty()) + } + fn stage_all_at(&self, dir: &Path) -> Result<()> { + let dir = dir.display().to_string(); + self.run(&["-C", &dir, "add", "-A"]).map(|_| ()) + } + fn commit_at(&self, dir: &Path, message: &str) -> Result<()> { + let dir = dir.display().to_string(); + self.run(&["-C", &dir, "commit", "-m", message]).map(|_| ()) + } + fn remove_worktree(&self, path: &Path) -> Result<()> { + let path = path.display().to_string(); + self.run(&["worktree", "remove", "--force", &path]).map(|_| ()) + } fn create_tag_at(&self, tag: &str, message: &str, sha: &str) -> Result<()> { self.run(&["tag", "-a", tag, "-m", message, sha]).map(|_| ()) } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 7b50db0..f5f2d8d 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -95,8 +95,12 @@ pub struct MockGit { /// here falls back to `head_sha`. pub branch_shas: HashMap, /// Scripted `is_working_tree_clean` answers, consumed front-first. Empty - /// (the default) falls back to `working_tree_clean`. + /// (the default) falls back to `working_tree_clean`. Shared with + /// `is_working_tree_clean_at` — one flow's clean-checks are strictly + /// ordered, whichever tree they read. pub working_tree_clean_seq: RefCell>, + /// `add_worktree` fails with this message (e.g. a leftover directory). + pub add_worktree_error: Option, _git_dir_guard: Option, } @@ -143,6 +147,7 @@ impl MockGit { tag_commits: HashMap::new(), branch_shas: HashMap::new(), working_tree_clean_seq: RefCell::new(VecDeque::new()), + add_worktree_error: None, _git_dir_guard: None, } } @@ -375,7 +380,10 @@ impl Git for MockGit { fn add_worktree(&self, path: &Path, branch: &str) -> Result<(), String> { self.calls.borrow_mut().push(format!("add_worktree:{}:{branch}", path.display())); - Ok(()) + match &self.add_worktree_error { + Some(e) => Err(e.clone()), + None => Ok(()), + } } fn stash_push_with_message(&self, msg: &str) -> Result<(), String> { @@ -436,6 +444,29 @@ impl Git for MockGit { Ok(()) } + fn is_working_tree_clean_at(&self, dir: &Path) -> Result { + self.calls.borrow_mut().push(format!("is_working_tree_clean_at:{}", dir.display())); + match self.working_tree_clean_seq.borrow_mut().pop_front() { + Some(answer) => Ok(answer), + None => Ok(self.working_tree_clean), + } + } + + fn stage_all_at(&self, dir: &Path) -> Result<(), String> { + self.calls.borrow_mut().push(format!("stage_all_at:{}", dir.display())); + Ok(()) + } + + fn commit_at(&self, dir: &Path, message: &str) -> Result<(), String> { + self.calls.borrow_mut().push(format!("commit_at:{}:{message}", dir.display())); + Ok(()) + } + + fn remove_worktree(&self, path: &Path) -> Result<(), String> { + self.calls.borrow_mut().push(format!("remove_worktree:{}", path.display())); + Ok(()) + } + fn create_tag_at(&self, tag: &str, message: &str, sha: &str) -> Result<(), String> { self.calls.borrow_mut().push(format!("create_tag_at:{tag}:{message}:{sha}")); Ok(()) diff --git a/tests/git_cli_test.rs b/tests/git_cli_test.rs index c13ef9b..0aea6a6 100644 --- a/tests/git_cli_test.rs +++ b/tests/git_cli_test.rs @@ -382,6 +382,14 @@ fn every_primitive_issues_its_documented_git_command() { ("git stash pop stash@{1}", Box::new(|g| { g.stash_pop_ref("stash@{1}").ok(); })), ("git add -A", Box::new(|g| { g.stage_all().ok(); })), ("git commit -m chore: set version 2.5.0", Box::new(|g| { g.commit("chore: set version 2.5.0").ok(); })), + // The _at family targets another worktree via -C (the ephemeral + // version-script worktree) without touching the CommandRunner seam. + ("git -C /wt status --porcelain", Box::new(|g| { g.is_working_tree_clean_at(Path::new("/wt")).ok(); })), + ("git -C /wt add -A", Box::new(|g| { g.stage_all_at(Path::new("/wt")).ok(); })), + ("git -C /wt commit -m chore: set version 2.5.0", Box::new(|g| { g.commit_at(Path::new("/wt"), "chore: set version 2.5.0").ok(); })), + // --force: a failed version script can leave the temp tree dirty and + // cleanup must still succeed. + ("git worktree remove --force /wt", Box::new(|g| { g.remove_worktree(Path::new("/wt")).ok(); })), // A separate method from create_tag: it tags a specific sha (a merge // commit), not HEAD. ("git tag -a v2.5.0 -m chore: release 2.5.0 abc123", From 2f45590603b6d7beb855055e969202871e1eee5d Mon Sep 17 00:00:00 2001 From: Jop Middelkamp Date: Sat, 8 Aug 2026 14:27:04 +0700 Subject: [PATCH 2/6] feat: VersionScript::run_in executes a target tree's own script copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second trait method (changing run's signature would break every mock encoding): re-derives the script path relative to the repo root inside dir and spawns with cwd = dir — repo content applies to the tree it lives in. ScriptCli's spawn stays one zero-policy helper; the mock's failure knobs and run counter are shared across both entry points. --- src/version_script.rs | 66 +++++++++++++++++++++++++++++++++++++------ tests/common/mod.rs | 19 ++++++++++--- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/src/version_script.rs b/src/version_script.rs index fd0c435..484c927 100644 --- a/src/version_script.rs +++ b/src/version_script.rs @@ -15,6 +15,11 @@ pub const SCRIPT_WINDOWS: &str = ".bflow/set-version.cmd"; /// without spawning a real process. pub trait VersionScript { fn run(&self, version: &str) -> Result<(), String>; + /// Run the script inside another working tree: `dir`'s own copy of the + /// script, with `dir` as the working directory. Repo content applies to + /// the tree it lives in — a checkout-less branch may carry a different + /// script than the tree bflow was started from. + fn run_in(&self, dir: &Path, version: &str) -> Result<(), String>; fn display_name(&self) -> String; } @@ -70,20 +75,34 @@ impl ScriptCli { } } -impl VersionScript for ScriptCli { - fn run(&self, version: &str) -> Result<(), String> { - // KISS: zero-policy shell, like CommandEditor::open — the spawn itself - // has no decision to test; interpret() carries the outcome policy. - let output = Command::new(&self.path) +impl ScriptCli { + /// Zero-policy shell, like CommandEditor::open — the spawn itself has no + /// decision to test; interpret() carries the outcome policy. + fn spawn_at(&self, script: &Path, cwd: &Path, version: &str) -> Result<(), String> { + let output = Command::new(script) .arg(version) - .current_dir(&self.repo_root) + .current_dir(cwd) .output() .map_err(|e| format!( "Version script {} could not be run: {e}\nMake it executable: chmod +x {} && git update-index --chmod=+x {}, then re-run the command.", - self.path.display(), self.path.display(), self.path.display(), + script.display(), script.display(), script.display(), ))?; let stderr = String::from_utf8_lossy(&output.stderr); - interpret(&self.path, output.status.code(), stderr.trim()) + interpret(script, output.status.code(), stderr.trim()) + } +} + +impl VersionScript for ScriptCli { + fn run(&self, version: &str) -> Result<(), String> { + self.spawn_at(&self.path, &self.repo_root, version) + } + + fn run_in(&self, dir: &Path, version: &str) -> Result<(), String> { + let rel = self.path.strip_prefix(&self.repo_root).map_err(|_| format!( + "Version script {} is not under the repo root {}, so it cannot be located in another worktree.", + self.path.display(), self.repo_root.display(), + ))?; + self.spawn_at(&dir.join(rel), dir, version) } fn display_name(&self) -> String { @@ -169,6 +188,37 @@ mod tests { assert_eq!(script.display_name(), "set-version.sh"); } + #[cfg(unix)] + #[test] + fn run_in_executes_the_target_trees_own_copy_with_cwd_there() { + use std::os::unix::fs::PermissionsExt; + let root = tmp_dir(); + let worktree = tmp_dir(); + for (dir, marker) in [(&root, "root"), (&worktree, "worktree")] { + std::fs::create_dir_all(dir.join(".bflow")).unwrap(); + let script = dir.join(SCRIPT_UNIX); + std::fs::write(&script, format!("#!/bin/sh\necho {marker} $1 > marker.txt\n")).unwrap(); + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + let script = ScriptCli::new(root.join(SCRIPT_UNIX), root.clone()); + + script.run_in(&worktree, "1.0.1").unwrap(); + + let marker = std::fs::read_to_string(worktree.join("marker.txt")).unwrap(); + assert_eq!(marker.trim(), "worktree 1.0.1"); + assert!(!root.join("marker.txt").exists(), "the root tree must stay untouched"); + std::fs::remove_dir_all(&root).ok(); + std::fs::remove_dir_all(&worktree).ok(); + } + + #[test] + fn run_in_errors_when_the_script_is_not_under_the_repo_root() { + let script = ScriptCli::new(PathBuf::from("/elsewhere/set-version.sh"), PathBuf::from("/repo")); + let err = script.run_in(Path::new("/wt"), "1.0.1").unwrap_err(); + assert!(err.contains("/elsewhere/set-version.sh"), "got: {err}"); + assert!(err.contains("/repo"), "got: {err}"); + } + #[test] fn run_names_the_chmod_remedy_when_the_script_cannot_be_spawned() { // A path that cannot be spawned (here: does not exist) drives the same diff --git a/tests/common/mod.rs b/tests/common/mod.rs index f5f2d8d..538126f 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -582,11 +582,10 @@ impl MockVersionScript { pub fn calls(&self) -> Vec { self.calls.borrow().clone() } -} -impl VersionScript for MockVersionScript { - fn run(&self, version: &str) -> Result<(), String> { - self.calls.borrow_mut().push(format!("run:{version}")); + /// Shared by `run` and `run_in` (LSP: both entry points honor the same + /// failure knobs and share one run counter). + fn outcome(&self) -> Result<(), String> { let mut count = self.run_call_count.borrow_mut(); *count += 1; if Some(*count) == self.fail_nth_run { @@ -597,6 +596,18 @@ impl VersionScript for MockVersionScript { None => Ok(()), } } +} + +impl VersionScript for MockVersionScript { + fn run(&self, version: &str) -> Result<(), String> { + self.calls.borrow_mut().push(format!("run:{version}")); + self.outcome() + } + + fn run_in(&self, dir: &Path, version: &str) -> Result<(), String> { + self.calls.borrow_mut().push(format!("run_in:{}:{version}", dir.display())); + self.outcome() + } fn display_name(&self) -> String { "set-version.sh".to_string() From a4820006ad0f9e011d82102ce47693b8aeec0d86 Mon Sep 17 00:00:00 2001 From: Jop Middelkamp Date: Sat, 8 Aug 2026 14:28:36 +0700 Subject: [PATCH 3/6] =?UTF-8?q?feat:=20temp=5Fworktree=5Fpath=20=E2=80=94?= =?UTF-8?q?=20collision-proof=20path=20for=20bflow-internal=20worktrees?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/worktree.rs | 16 ++++++++++++++++ tests/worktree_test.rs | 20 +++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/worktree.rs b/src/worktree.rs index 50703d3..ed0414e 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -106,6 +106,22 @@ pub fn worktree_path(repo_root: &Path, repo_name: &str, base_path: Option<&str>, base.join(folder) } +/// Compute the directory for an ephemeral, bflow-internal worktree of `branch`. +/// +/// `-bflow-tmp-`, always in the +/// repository's parent directory. The `bflow-tmp` infix keeps it disjoint from +/// `worktree_path`'s user-facing scheme, and `bflow.worktree.*` config is +/// deliberately ignored — this is internal plumbing, not a user worktree. +pub fn temp_worktree_path(repo_root: &Path, branch: &str) -> PathBuf { + let repo_name = repo_root.file_name().and_then(|n| n.to_str()).unwrap_or("repo"); + let folder = format!("{repo_name}-bflow-tmp-{}", branch.replace('/', "-")); + repo_root + .parent() + .map(Path::to_path_buf) + .unwrap_or_else(|| PathBuf::from(".")) + .join(folder) +} + /// Create a git worktree for `branch` and open it in the configured editor. /// /// `branch` must already exist. Worktree creation is fatal on error; editor-open diff --git a/tests/worktree_test.rs b/tests/worktree_test.rs index ac3999d..ccb004e 100644 --- a/tests/worktree_test.rs +++ b/tests/worktree_test.rs @@ -4,7 +4,7 @@ use std::path::{Path, PathBuf}; use std::sync::Mutex; use common::{MockEditor, MockGit, MockPrompter}; use bflow::worktree::{ - worktree_path, WorktreeConfig, EDITOR_PRESETS, + temp_worktree_path, worktree_path, WorktreeConfig, EDITOR_PRESETS, set_enabled, set_editor, set_path, use_default_path, show_status, wizard, }; @@ -47,6 +47,24 @@ fn worktree_path_expands_leading_tilde_in_custom_base() { assert_eq!(p, PathBuf::from(home).join("worktrees/beans-gitflow-feature-login")); } +// --- temp_worktree_path (pure) --- + +#[test] +fn temp_worktree_path_is_repo_parent_with_tmp_infix() { + let root = Path::new("/repos/beans-gitflow"); + let p = temp_worktree_path(root, "hotfix/1.0.1"); + assert_eq!(p, PathBuf::from("/repos/beans-gitflow-bflow-tmp-hotfix-1.0.1")); +} + +#[test] +fn temp_worktree_path_cannot_collide_with_a_user_worktree_for_the_same_branch() { + let root = Path::new("/repos/beans-gitflow"); + assert_ne!( + temp_worktree_path(root, "hotfix/1.0.1"), + worktree_path(root, "beans-gitflow", None, "hotfix/1.0.1"), + ); +} + // --- WorktreeConfig::load --- #[test] From da402c9e624f2af54690899e19a97455a6619fba Mon Sep 17 00:00:00 2001 From: Jop Middelkamp Date: Sat, 8 Aug 2026 14:30:26 +0700 Subject: [PATCH 4/6] feat: checkout-less hotfix creation runs the version script via an ephemeral worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec change: replaces the mutation-pinned hotfix_no_checkout_skips_script (mutation-audit trap 9, 'the script must never run without a checkout'). The invariant 'release/hotfix branches carry their version' now holds in every mode: after create_branch_no_checkout, bflow adds a temporary worktree, runs the branch's own script copy there, commits, removes the worktree, and pushes once — the user's checkout is never touched. Script failure falls back to warn-and-continue with the manual recovery steps (M2 precedent): a broken script must never block the hotfix. --- src/flows/mod.rs | 26 ++++++++++++++++++++++++ src/flows/start.rs | 18 +++++++++-------- tests/start_test.rs | 48 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/flows/mod.rs b/src/flows/mod.rs index 2b95ea5..32a43d0 100644 --- a/src/flows/mod.rs +++ b/src/flows/mod.rs @@ -239,6 +239,32 @@ pub(crate) fn run_version_script(git: &dyn Git, script: &dyn VersionScript, vers Ok(true) } +/// `run_version_script`'s checkout-less sibling: same rule (script → clean +/// check → stage → commit), executed in an ephemeral worktree of `branch` so +/// the user's checkout is never touched. The worktree is removed on success +/// and on failure alike; a removal failure only warns, naming the command — +/// the version commit (the actual work) is not at stake by then. +pub(crate) fn run_version_script_in_temp_worktree(git: &dyn Git, script: &dyn VersionScript, branch: &str, version: &SemVer) -> Result { + let repo_root = git.repo_root()?; + let path = crate::worktree::temp_worktree_path(&repo_root, branch); + git.add_worktree(&path, branch)?; + let result = (|| { + let release = version.to_release(); + script.run_in(&path, &release.to_string())?; + if git.is_working_tree_clean_at(&path)? { + return Ok(false); + } + git.stage_all_at(&path)?; + git.commit_at(&path, &format!("chore: set version {release}"))?; + Ok(true) + })(); + if let Err(e) = git.remove_worktree(&path) { + eprintln!("Warning: could not remove the temporary worktree: {e}"); + eprintln!(" Remove it yourself: git worktree remove --force {}", path.display()); + } + result +} + /// List `{prefix}/*` branches that are still open, excluding any that already /// shipped — reusing a shipped branch would make `bflow start` loop onto a /// dead branch forever and hotfix fan-out merge into history that already diff --git a/src/flows/start.rs b/src/flows/start.rs index 4eb5ab4..2430bc0 100644 --- a/src/flows/start.rs +++ b/src/flows/start.rs @@ -1,4 +1,4 @@ -use crate::flows::{open_versioned_branches, require_clean_tree, run_version_script}; +use crate::flows::{open_versioned_branches, require_clean_tree, run_version_script, run_version_script_in_temp_worktree}; use crate::git::Git; use crate::hosting::HostingPlatform; use crate::prompt::Prompter; @@ -320,13 +320,15 @@ fn resolve_or_create_hotfix(git: &dyn Git, hosting: &dyn HostingPlatform, cfg: & if no_checkout { git.create_branch_no_checkout(&branch, main_branch)?; if let Some(script) = script { - eprintln!( - "⚠ Version script not run: {branch} was created without checkout, so bflow cannot commit version files there." - ); - eprintln!( - " Recover manually: git switch {branch}, run {} {next}, commit, and push.", - script.display_name(), - ); + // Warn-and-continue (M2 precedent): a broken script must never + // block the hotfix — the branch push below still proceeds. + if let Err(e) = run_version_script_in_temp_worktree(git, script, &branch, &next) { + eprintln!("⚠ Version script failed in a temporary worktree: {e}"); + eprintln!( + " Recover manually: git switch {branch}, run {} {next}, commit, and push.", + script.display_name(), + ); + } } } else { git.checkout(main_branch)?; diff --git a/tests/start_test.rs b/tests/start_test.rs index 456382a..3e3e303 100644 --- a/tests/start_test.rs +++ b/tests/start_test.rs @@ -1128,23 +1128,55 @@ fn start_hotfix_fix_reuse_path_never_runs_script() { } #[test] -fn hotfix_no_checkout_skips_script() { +fn hotfix_no_checkout_runs_script_in_temp_worktree() { + // Spec change (supersedes mutation-audit trap 9's "no checkout, no + // script"): the version invariant holds in every mode. The script runs in + // an ephemeral worktree, the commit lands on hotfix/X.Y.Z before the one + // push, and the worktree is removed — the user's checkout stays untouched. let mut git = MockGit::new(); git.branches_matching = vec![]; git.tags = vec!["1.0.0".to_string()]; + git.working_tree_clean_seq.borrow_mut().extend([false]); let script = MockVersionScript::new(); start_hotfix_fix(&git, &MockHosting::new(), &RepoConfig::default(), "urgent-crash", true, None, "main", Some(&script)).unwrap(); + let wt = "/repos/beans-gitflow-bflow-tmp-hotfix-1.0.1"; assert_eq!(git.calls(), vec![ - "list_branches_matching:hotfix/*", - "list_tags", - "create_branch_no_checkout:hotfix/1.0.1:main", - "push:hotfix/1.0.1", - "create_branch_no_checkout:hotfix-fix/1.0.1/urgent-crash:hotfix/1.0.1", - "push:hotfix-fix/1.0.1/urgent-crash", + "list_branches_matching:hotfix/*".to_string(), + "list_tags".to_string(), + "create_branch_no_checkout:hotfix/1.0.1:main".to_string(), + "repo_root".to_string(), + format!("add_worktree:{wt}:hotfix/1.0.1"), + format!("is_working_tree_clean_at:{wt}"), + format!("stage_all_at:{wt}"), + format!("commit_at:{wt}:chore: set version 1.0.1"), + format!("remove_worktree:{wt}"), + "push:hotfix/1.0.1".to_string(), + "create_branch_no_checkout:hotfix-fix/1.0.1/urgent-crash:hotfix/1.0.1".to_string(), + "push:hotfix-fix/1.0.1/urgent-crash".to_string(), ]); - assert!(script.calls().is_empty()); + assert_eq!(script.calls(), vec![format!("run_in:{wt}:1.0.1")]); +} + +#[test] +fn hotfix_no_checkout_script_noop_commits_nothing() { + // The script left the temp worktree clean — no stage, no commit, worktree + // still removed, push still happens. + let mut git = MockGit::new(); + git.branches_matching = vec![]; + git.tags = vec!["1.0.0".to_string()]; + git.working_tree_clean_seq.borrow_mut().extend([true]); + let script = MockVersionScript::new(); + + start_hotfix_fix(&git, &MockHosting::new(), &RepoConfig::default(), "urgent-crash", true, None, "main", Some(&script)).unwrap(); + + let calls = git.calls(); + let wt = "/repos/beans-gitflow-bflow-tmp-hotfix-1.0.1"; + assert!(!calls.iter().any(|c| c.starts_with("stage_all_at") || c.starts_with("commit_at")), + "a no-op script must not commit; calls: {calls:?}"); + assert!(calls.contains(&format!("remove_worktree:{wt}")), "calls: {calls:?}"); + assert!(calls.contains(&"push:hotfix/1.0.1".to_string()), "calls: {calls:?}"); } // Note: the pure `message_is_breaking` string-matching logic is tested From cbae0da93fe3974ae3e7c56f875430d432f18443 Mon Sep 17 00:00:00 2001 From: Jop Middelkamp Date: Sat, 8 Aug 2026 14:32:36 +0700 Subject: [PATCH 5/6] test: failure paths for the ephemeral version-script worktree Script failure: warn, remove the worktree, still push (mutation-verified: propagating the error instead fails both tests; dropping the cleanup fails three). add_worktree failure: nothing to remove, script never runs, push still proceeds. --- tests/start_test.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/start_test.rs b/tests/start_test.rs index 3e3e303..e3fdede 100644 --- a/tests/start_test.rs +++ b/tests/start_test.rs @@ -1179,6 +1179,50 @@ fn hotfix_no_checkout_script_noop_commits_nothing() { assert!(calls.contains(&"push:hotfix/1.0.1".to_string()), "calls: {calls:?}"); } +#[test] +fn hotfix_no_checkout_script_failure_warns_removes_worktree_and_still_pushes() { + // Warn-and-continue: a broken script must never block the hotfix. The + // temp worktree is removed even on failure, and the branch still pushes — + // the end state matches the old manual-recovery warning path. + let mut git = MockGit::new(); + git.branches_matching = vec![]; + git.tags = vec!["1.0.0".to_string()]; + let mut script = MockVersionScript::new(); + script.fail = Some("boom".to_string()); + + start_hotfix_fix(&git, &MockHosting::new(), &RepoConfig::default(), "urgent-crash", true, None, "main", Some(&script)).unwrap(); + + let calls = git.calls(); + let wt = "/repos/beans-gitflow-bflow-tmp-hotfix-1.0.1"; + assert!(calls.contains(&format!("remove_worktree:{wt}")), + "the temp worktree must be removed on script failure; calls: {calls:?}"); + assert!(!calls.iter().any(|c| c.starts_with("commit_at")), + "a failed script must not commit; calls: {calls:?}"); + assert!(calls.contains(&"push:hotfix/1.0.1".to_string()), + "the hotfix branch must still push after a script failure; calls: {calls:?}"); +} + +#[test] +fn hotfix_no_checkout_add_worktree_failure_warns_without_removal() { + // Nothing was created, so there is nothing to remove — straight to the + // warn fallback, and the hotfix still pushes. + let mut git = MockGit::new(); + git.branches_matching = vec![]; + git.tags = vec!["1.0.0".to_string()]; + git.add_worktree_error = Some("fatal: a leftover directory is in the way".to_string()); + let script = MockVersionScript::new(); + + start_hotfix_fix(&git, &MockHosting::new(), &RepoConfig::default(), "urgent-crash", true, None, "main", Some(&script)).unwrap(); + + let calls = git.calls(); + assert!(!calls.iter().any(|c| c.starts_with("remove_worktree")), + "no worktree was created, none may be removed; calls: {calls:?}"); + assert!(script.calls().is_empty(), + "the script must not run without its worktree; script calls: {:?}", script.calls()); + assert!(calls.contains(&"push:hotfix/1.0.1".to_string()), + "the hotfix branch must still push; calls: {calls:?}"); +} + // Note: the pure `message_is_breaking` string-matching logic is tested // as unit tests in src/flows/start.rs. These integration tests cover the // git interaction — which ref is queried, and the develop → origin/develop From 89d4ee1b8d863355a5525d217b444dc397294258 Mon Sep 17 00:00:00 2001 From: Jop Middelkamp Date: Sat, 8 Aug 2026 14:34:54 +0700 Subject: [PATCH 6/6] =?UTF-8?q?docs:=20ephemeral-worktree=20version=20scri?= =?UTF-8?q?pt=20=E2=80=94=20README,=20bflow=20skill,=20decisions.md?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README: no-checkout hotfix section rewritten (script runs via temp worktree; warning only when the script itself fails), --no-checkout positioned as the low-level escape hatch under worktree mode. decisions.md: Landing Modes entry for the mechanism and the trap-9 supersede; Boundaries note extended for the _at primitive family. --- .claude/skills/architecture/decisions.md | 3 ++- .claude/skills/bflow/SKILL.md | 2 +- README.md | 8 +++++--- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/.claude/skills/architecture/decisions.md b/.claude/skills/architecture/decisions.md index ad96083..49a43b4 100644 --- a/.claude/skills/architecture/decisions.md +++ b/.claude/skills/architecture/decisions.md @@ -72,6 +72,7 @@ User-configurable without code changes: the worktree flow (`bflow.worktree.*` gi - **Repo policy lives in a committed `.bflow/config` file, not git config.** `mode` (`free`|`protected`) and `keep-release-branches` are team decisions a fresh clone must see identically; git config is per-clone and would silently drift on either one — the same fresh-clone argument that keeps the version script itself a repo file rather than a personal setting. Parsed by a pure `repo_config::parse`, read once (`repo_config::load`) and threaded into flows as data — same shape as `hosting/detect.rs` and `mainline::resolve_main_branch`, never a mid-flow lookup. Rejected: git config keys for `mode`/`keep-release-branches` — drift here changes what `finish` *does* (merge vs. PR, delete vs. keep), not just a cosmetic setting. - **`VersionScript` is a port with a pure `interpret` and a zero-policy spawn**, the same split as `CommandEditor::open`: `ScriptCli::run` has nothing to decide (spawn, capture exit code + stderr), so the outcome policy — exit 0 is success, any other code carries the script path/exit code/stderr, a signal names itself — lives entirely in `interpret`, a pure function unit-tested directly without a process. Discovery (`version_script::resolve`) is likewise pure, parametrized on `windows: bool` rather than reading `cfg!` internally, so both platforms are unit-tested on one OS. Resolved once, eagerly, in `main.rs`: a repo carrying only the other platform's script file errors on *every* command, not just the ones that would run it. Accepted as louder than deferring the error into the flow that needs it — the alternative is plumbing an `Err` through the lifecycle for a case that only ever means "a committed file is broken" (same spirit as the accepted mainline-resolution-on-abort gap in Known Gaps). +- **Checkout-less hotfix creation runs the version script in an ephemeral worktree** (`run_version_script_in_temp_worktree`, `flows/mod.rs` — `run_version_script`'s sibling, same script→clean-check→stage→commit rule via the `_at` primitives). The invariant "release/hotfix branches carry their version" holds in every mode; the temp worktree (`worktree_path`-disjoint `temp_worktree_path`, `bflow.worktree.*` ignored — internal plumbing) is removed on success and failure alike, and the branch's *own* script copy runs with cwd there (`VersionScript::run_in` — content belongs to the tree it applies to). A failing script falls back to warn-and-continue with manual recovery (M2 precedent): a broken script must never block a hotfix. This supersedes mutation-audit trap 9's "the script must never run without a checkout" — replaced spec test: `hotfix_no_checkout_runs_script_in_temp_worktree`. Rejected: keeping the skip+warning (a manual step inside incident response, contradicting principle 10); a hard error (a repo with worktree mode plus a version script could not start hotfixes at all). - **Protected-mode landing progress is derived, never stored.** No `FinishState` for a protected finish/sync — progress is re-derived from `HostingPlatform::merged_pr_to` plus git state on every call, never from a file. `sync_with_develop_protected` uses `landed_pr`, which compares `branch_sha` (a branch that moved on after its PR merged is not landed; new commits get a fresh PR); `finish_release_protected`/`finish_hotfix_protected` use `leg_landed` instead — see the next entry for why the two differ. Precedent: work-branch finish already treats "a merged PR" as the completion record instead of writing state. Rejected: a state file — a protected finish never merges locally, so there is no half-done local mutation for a crash to leave behind that git+PR state doesn't already answer; a state file would be a second, driftable copy of what the hosting platform already knows. - **"Landed" is two questions, not one.** *Has this leg landed at least once?* (`leg_landed`, `flows/mod.rs`) is answered by the leg's merged PR's merge commit being contained in the target (`is_ancestor` against `origin/{target}`) — stays true once the source branch gains commits afterwards, which is what lets `finish_release_protected`/`finish_hotfix_protected` resume past a leg that already completed instead of dead-ending or silently re-opening its PR. *Is anything left on this branch to delete?* (`tip_landed_somewhere`) is answered by the branch's current tip matching one of the landed legs' PR head SHAs — squash merges leave no ancestry to fall back on, so this is what stops `finish_release_cleanup`/`finish_hotfix_cleanup` from deleting commits a squash merge left in no target. Rejected: one predicate for both — head-SHA equality alone (`landed_pr`'s rule) dead-ends the finish the moment a later leg's conflict-resolution commit lands on the source branch; containment alone would delete commits sitting on the tip that never actually landed anywhere. `sync` deliberately keeps `landed_pr`'s head-SHA equality: a release with new commits after a sync landed genuinely does need syncing again, so staleness there must reopen work, not be waved through. - **Tag-mainline containment is checked before the main leg's PR lookup is trusted for tagging.** `finish_release_protected`/`finish_hotfix_protected` test `is_ancestor(tag_commit, origin/{main})` before deciding what to do with a fresh `leg_landed` result for the main leg — otherwise a superseded/re-opened main PR would make `tag_at_if_missing` compare an already-correct tag against the *wrong* PR's merge commit and reject it. Accepted narrowing: a hand-placed tag that already sits on any mainline-contained commit is taken as landed without tracing it back to a specific PR. Under squash merges — the normal case — a wrong tag's commit is never actually on the mainline, so `tag_at_if_missing`'s identity check still catches the realistic mistake. @@ -93,7 +94,7 @@ User-configurable without code changes: the worktree flow (`bflow.worktree.*` gi ## Boundaries & Extensibility -- **`Git` is 48 fine-grained primitives** so ordering logic lives in flows and mocks can record exact sequences. Rejected: coarse `finish_release()`-style methods (untestable ordering). One deliberate exception to cwd-relative primitives: `remove_current_worktree` runs via `-C
` (git refuses to remove the worktree it runs in) and must be the *last* git call of a flow — the process cwd is deleted afterwards, so remote-branch deletion is ordered before it. +- **`Git` is 48 fine-grained primitives** so ordering logic lives in flows and mocks can record exact sequences. Rejected: coarse `finish_release()`-style methods (untestable ordering). Deliberate exceptions to cwd-relative primitives: `remove_current_worktree` runs via `-C
` (git refuses to remove the worktree it runs in) and must be the *last* git call of a flow — the process cwd is deleted afterwards, so remote-branch deletion is ordered before it; and the `_at` family (`is_working_tree_clean_at`/`stage_all_at`/`commit_at`, plus `remove_worktree(path)`) targets the ephemeral version-script worktree via `-C ` — no last-call constraint there, the process never stands in it. `CommandRunner` stays cwd-free in both cases: the directory travels in the arg vector. - **`repo_root` and `worktree_root` answer different questions; neither replaces the other.** `repo_root` (`git worktree list --porcelain`, first entry) is always the MAIN tree's root — worktree *bookkeeping* (`worktree.rs`'s folder naming, `remove_current_worktree`'s `-C` target above) is deliberately anchored there no matter which worktree the command runs in. `worktree_root` (`git rev-parse --show-toplevel`) is the CURRENT tree's root — repo *content* (`.bflow/config`, the version script, PR templates: `main.rs`, `lifecycle::resolve_pr_template`/`resolve_landing_template`) must be read from here, because a linked worktree can have a different branch checked out than the main tree, and the main tree's copy would silently apply that other branch's policy. Rejected: one accessor for both — `repo_root` for everything is exactly what shipped that bug; `worktree_root` for everything would break folder naming and misdirect `remove_current_worktree`'s `-C`, both of which need the main tree regardless of where the command runs. - **`HostingPlatform` stays minimal (5 methods)**: `create_or_get_pr` is deliberately one method ("PR already open" is a normal resume outcome and the check-then-create dance is provider-specific); `merged_pr` reports the branch's most recent PR only when merged (open → still in play; closed-unmerged → a fresh PR, never cleanup) and carries the merged head SHA because squash merges break ancestor checks; `merged_pr_to` answers the base-filtered question ("did head land in *that* base") that protected-mode landing needs and `merged_pr` cannot answer (see Landing Modes & Version Script); `open_url` is a default method (provider-agnostic, but on the trait as a test seam); the `template` param is a *path* (only `az` needs the contents read; `gh` takes `--body-file`). - **`Prompter` is three methods, one actor.** `select` plus `prompt_name`/`prompt_line`, which differ in *shaping*, not validation (see Input shaping below); `prompt_name` owns its re-prompt loop so callers never receive an invalid name. Widened from one method so `menu::show_menu` — the interactive twin of `cli::resolve_action` — could be tested at all. The branch-type gating table is genuinely dual-encoded: `show_menu` owns the *offer* side (which actions a branch lists), `cli::resolve_action` owns the *reject* side (which it refuses, with its own terser wording). Neither derives from the other, so parity is a test obligation, not a structural guarantee — `every_work_kind_in_the_table_has_a_working_start_subcommand` in `cli_test.rs` pins the half that can drift silently. Rejected: a second trait for text input (same actor, same lifetime, same single impl — premature polymorphism). diff --git a/.claude/skills/bflow/SKILL.md b/.claude/skills/bflow/SKILL.md index 20809f7..d486ceb 100644 --- a/.claude/skills/bflow/SKILL.md +++ b/.claude/skills/bflow/SKILL.md @@ -113,7 +113,7 @@ bflow sync # merge release into develop (on release/* only) **Papercut**: version files can conflict on merge (hotfix vs. develop, major release vs. develop) — resolve manually, bflow does not auto-resolve. -`--no-checkout` hotfix creation skips the script (HEAD isn't on the new branch) and warns with the manual recovery steps. +`--no-checkout` hotfix creation still runs the script: via a temporary worktree of the new branch (commit lands there, worktree removed, user's checkout untouched). Only a failing script falls back to a warning with manual recovery steps. ### Tag Strategy diff --git a/README.md b/README.md index 0c153d8..2b844b4 100644 --- a/README.md +++ b/README.md @@ -216,7 +216,7 @@ bflow start hotfix-fix --name [--no-checkout] [--no-worktree] # must `--major` / `--minor` on `start release` skips the interactive prompt and forces the bump level. Useful for scripts and AI agents. -`--no-checkout` creates and pushes the branch without switching to it. You stay on your current branch. Designed for [git worktree](https://git-scm.com/docs/git-worktree) workflows. Not available for `start release`. +`--no-checkout` creates and pushes the branch without switching to it. You stay on your current branch. Prefer [worktree mode](#worktree-integration), which manages the worktree and editor for you — `--no-checkout` is the low-level escape hatch for users with their own worktree tooling. Not available for `start release`. `--no-worktree` skips the optional [worktree flow](#worktree-integration) for a single command when `bflow.worktree.enabled` is set. No effect otherwise. @@ -588,10 +588,12 @@ Because `develop` and an open hotfix or release branch can carry different versi #### Hotfix branches created without checkout -`bflow start hotfix-fix --no-checkout` (or an active [worktree](#worktree-integration) flow) creates the hotfix branch without switching to it, so bflow cannot safely commit version files there — HEAD stays on your current branch. bflow skips the script and warns instead of guessing: +`bflow start hotfix-fix --no-checkout` (or an active [worktree](#worktree-integration) flow) creates the hotfix branch without switching to it — HEAD stays on your current branch. The version script still runs: bflow adds a temporary git worktree for `hotfix/X.Y.Z`, runs the branch's own copy of the script there, commits, removes the worktree, and pushes the branch carrying its version commit. Your checkout and any local changes are never touched. + +Only if the script itself fails does bflow fall back to a warning with the manual recovery steps — a broken script never blocks an urgent hotfix: ``` -⚠ Version script not run: hotfix/1.2.1 was created without checkout, so bflow cannot commit version files there. +⚠ Version script failed in a temporary worktree: ... Recover manually: git switch hotfix/1.2.1, run set-version.sh 1.2.1, commit, and push. ```