refactor: dry-kiss-solid-review-fixes

.claude/skills/architecture/SKILL.md

@@ -11,7 +11,7 @@ Broad, global rules only. Implementation-level decisions, extension recipes, and
 
 ## Principles
 
-1. **Ports & adapters.** Business logic lives in `flows/` and depends only on traits (`Git`, `HostingPlatform`, `Editor`). Concrete adapters are wired in `main.rs` — the only composition root. No subprocess calls outside adapter impls.
+1. **Ports & adapters.** Business logic lives in `flows/` and depends only on traits (`Git`, `HostingPlatform`, `Editor`, `Prompter`). Concrete adapters are wired in `main.rs` — the only composition root. No subprocess calls outside adapter impls.
 2. **Plug-and-play integrations.** Hosting providers are auto-detected from the remote and swappable; adding a provider or editor never touches flows.
 3. **Integrate through the user's own CLIs** (`git`, `gh`, `az`) — never link libraries or call APIs directly. bflow inherits the user's auth, config, hooks, and signing.
 4. **Minimal dependency budget.** Two crates today. A new dependency needs a very good reason; prefer hand-rolling small things.
@@ -26,8 +26,9 @@ Broad, global rules only. Implementation-level decisions, extension recipes, and
 
 | Module | Responsibility |
 |---|---|
-| `main.rs` | Composition root: builds adapters, preflight, cross-cutting lifecycle (stash/state/resume), dispatches `Action` once |
-| `cli.rs` / `menu.rs` | The two interfaces; both resolve to `Action` |
+| `main.rs` | Composition root: builds adapters, preflight, hands off to `lifecycle::run` |
+| `lifecycle.rs` | Cross-cutting lifecycle (stash/state/resume ordering), dispatches `Action` once |
+| `cli.rs` / `menu.rs` | The two interfaces; both resolve to `Action` (`action.rs`) |
 | `flows/` | Business logic per workflow |
-| `git/`, `hosting/`, `editor.rs` | Ports (traits) + adapters (CLI impls); `hosting/detect.rs` picks the provider |
+| `git/`, `hosting/`, `editor.rs`, `prompt.rs` | Ports (traits) + adapters (CLI impls); `hosting/detect.rs` picks the provider |
 | `state.rs`, `worktree.rs`, `version.rs` | Finish state, worktree feature, SemVer |

.claude/skills/architecture/decisions.md

@@ -7,6 +7,7 @@ Catalog of deliberate choices — each entry: the choice, why, and the rejected
 - **New git operation:** add method to `Git` trait → impl in `GitCli` → add to `MockGit` in `tests/common/mod.rs`.
 - **New hosting provider:** new file in `hosting/` implementing `HostingPlatform` → `Provider` variant + parse arm in `detect.rs` → preflight arm in `main.rs::create_hosting`. Reuse `hosting::run_cli`.
 - **New command/flow:** clap variant in `cli.rs` → `Action` variant + menu entry gated by `BranchType` → flow fn in `flows/` taking `&dyn` deps → dispatch arm in `run_flow`.
+- **New work-branch type** (e.g. `perf/`): variant on `BranchType` → entry in the `WORK_TYPES` table + arm in `work_kind` in `git/branch.rs` (the compiler forces both — `work_kind` has no wildcard) → clap `StartKind` variant + arms in `cli.rs::resolve_action` → `pr_template_keys` arm. Menus, parent-branch detection, and `commit_type` (kind name, unless special-cased like feature→feat) follow from the table automatically.
 
 ## Plug-in Points
 
@@ -15,6 +16,7 @@ Catalog of deliberate choices — each entry: the choice, why, and the rejected
 | `Git` | `GitCli`; `MockGit` in tests | Built once in `main.rs` |
 | `HostingPlatform` | `GitHub` (`gh`), `AzureDevOps` (`az`); `MockHosting` | Auto-detected from origin remote URL (`hosting/detect.rs`) |
 | `Editor` | `CommandEditor` (any command); `MockEditor` | `bflow.worktree.editor` git config |
+| `Prompter` | `MenuPrompter` (interactive select); `MockPrompter` (scripted) | Built once in `main.rs` |
 
 User-configurable without code changes: the worktree flow (`bflow.worktree.*` git config, local/global scope, `bflow worktree` wizard) and PR templates (`.github/pr-templates/bflow-<key>.md`, most-specific-first, falling back to the repo's native template).
 
@@ -70,15 +72,15 @@ User-configurable without code changes: the worktree flow (`bflow.worktree.*` gi
 - **`Git` is ~45 fine-grained primitives** so ordering logic lives in flows and mocks can record exact sequences. Rejected: coarse `finish_release()`-style methods (untestable ordering).
 - **`HostingPlatform` stays minimal (3 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); `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`).
 - **Detection: pure core, thin shell.** `resolve`/`parse_remote` are pure functions unit-tested without a repo (same pattern in `template.rs` and `devops.rs` helpers). Unknown host → GitHub (preserves pre-detection behavior for GitLab/Enterprise users), but an *explicit* `devops` override with an unparseable remote is a hard error — asymmetric on purpose. `Provider::AzureDevOps` carries `{org, project, repo}` so detection and construction are one step. ADO PR URLs are synthesized from parsed coordinates (`az`'s `webUrl` is unreliable); `validate_pr_id` rejects `az`'s literal `"None"`.
-- **Library crate + thin binary exists solely so `tests/` can link the flows.** Consequence: visibility falls exactly on the test boundary — `pub mod` at the top, tight function-level privacy inside (`run_cli`, parsers, runners all private). Orchestration (ordering, stash, state lifecycle) stays in `main.rs`, not the library.
+- **Library crate + thin binary exists solely so `tests/` can link the flows.** Consequence: visibility falls exactly on the test boundary — `pub mod` at the top, tight function-level privacy inside (`run_cli`, parsers, runners all private). Orchestration (ordering, stash, state lifecycle) lives in `lifecycle::run` in the library, taking `&dyn` ports, so the reject → stash → write-state → dispatch contract and the three-way stash-pop policy are mock-tested (`tests/lifecycle_test.rs`); `main.rs` keeps only adapter construction, preflight, and the `bflow worktree` dispatch. *(Amended: this previously read "orchestration stays in main.rs" — that made the state-before-mutation invariant untestable, and a resume/`--base` bug slipped through this layer.)*
 - **PR templates are repo files** (`.github/pr-templates/bflow-<key>.md`, specific → group → default → the repo's own native template → empty). The fix family shares one `bflow-fix.md` group key. Native-template path lists are per-provider knowledge, intentionally not shared.
 
 ## Testing Strategy
 
 - **Call-recording mock: one `Vec<String>` of `format!`-encoded calls; assertions are exact sequences** (23-element scripts in `finish_release_test.rs`) — a reorder of merge-before-tag is a test failure. Strings, not a typed `Call` enum: expectations read as a runnable script and diff legibly.
 - **One configurable `MockGit` with knob fields (state axes of git: what exists, what's merged, what's pushed), not a mock family.** Targeted failure injection (`fail_nth_merge` distinguishes "main merge conflicted" from "develop merge conflicted"); `RefCell` interior mutability keeps the trait `&self` so test bookkeeping never infects production signatures. Stateful where flows read back their own writes (stash save→find), dumb constants where nothing branches on the value.
 - **Negative assertions prove guards fire before damage** (no `checkout:main` in the log after a gate error; `rev_list_count_result = 99` proves the RC gate didn't even run on resume). Idempotent-resume tests build a fully-completed world and assert *zero* mutating calls.
-- **No real git anywhere in tests** (flows push/delete/PR — a sandbox would need a fake remote and stubs, and be slow on Windows CI). The one filesystem exception is `state.rs` round-trip tests. Tests split by *reachability*, not dogma: public behavior in `tests/` (one file per flow), private logic inline `#[cfg(test)]`. Scenario builders stay local per test file (DAMP over DRY); the tiny `stash_test.rs` deliberately pins the mock's own recording contract.
+- **No real git anywhere in tests** (flows push/delete/PR — a sandbox would need a fake remote and stubs, and be slow on Windows CI). The one filesystem exception is `state.rs` round-trip tests. Tests split by *reachability*, not dogma: public behavior in `tests/` (one file per flow), private logic inline `#[cfg(test)]`. Scenario builders stay local per test file (DAMP over DRY); the tiny `mock_contract_test.rs` deliberately pins the mock's own recording contract.
 
 ## Delivery & CI
 

README.md

@@ -568,8 +568,11 @@ jobs:
 
 ```
 src/
-├── main.rs              — Entry point, preflight checks, dispatch
+├── main.rs              — Composition root: builds adapters, preflight, hands off to lifecycle
 ├── lib.rs               — Library root, re-exports all modules
+├── action.rs            — Action enum: the single currency both interfaces resolve into
+├── cli.rs               — CLI subcommands (clap); resolves to Action
+├── lifecycle.rs         — Resume lookup, stash/state ordering contract, dispatch
 ├── git/
 │   ├── mod.rs           — Git trait + CLI implementation
 │   └── branch.rs        — Branch type detection and parsing
@@ -585,12 +588,14 @@ src/
 │   └── finish_hotfix.rs — Finish hotfix with auto-tag, propagate to open releases (idempotent)
 ├── state.rs             — Persisted finish state for conflict recovery
 ├── version.rs           — SemVer parsing and bumping
-├── menu.rs              — Interactive menus via crossterm
+├── menu.rs              — Interactive menus via crossterm; implements Prompter
+├── prompt.rs            — Prompter trait: interactive selection as a port
 ├── editor.rs            — Editor trait for opening worktrees
-└── worktree.rs          — Worktree config, path resolution, and setup wizard
+├── worktree.rs          — Worktree config, path resolution, and setup wizard
+└── test_support.rs      — Shared helpers for inline unit tests (test builds only)
 ```
 
-The `Git` and `HostingPlatform` traits enable multiple hosting providers (GitHub and Azure DevOps today, GitLab/Bitbucket-ready) and testability.
+The `Git`, `HostingPlatform`, `Editor`, and `Prompter` traits keep every flow fully mockable and make hosting providers swappable (GitHub and Azure DevOps today, GitLab/Bitbucket-ready).
 
 ## License
 

src/action.rs

@@ -0,0 +1,75 @@
+//! The single currency both interfaces resolve into: the menu (`menu.rs`) and
+//! the CLI subcommands (`cli.rs`) each produce an `Action`, and nothing
+//! downstream knows which interface ran. Lives in its own module so neither
+//! interface has to import the other.
+
+use crate::flows::start::ReleaseType;
+
+#[derive(Debug, PartialEq)]
+pub enum Action {
+    StartWorkBranch { prefix: String, name: String, from: String, no_checkout: bool, no_worktree: bool },
+    StartRelease(Option<ReleaseType>),
+    StartReleaseFix { name: String, no_checkout: bool, no_worktree: bool },
+    StartHotfixFix { name: String, no_checkout: bool, no_worktree: bool },
+    FinishWorkBranch { breaking: Option<bool>, base: Option<String> },
+    FinishReleaseFix,
+    FinishRelease,
+    FinishHotfix,
+    FinishHotfixFix,
+    AbortFinish,
+    BumpVersion,
+    SyncWithDevelop,
+}
+
+impl Action {
+    pub fn is_start(&self) -> bool {
+        matches!(
+            self,
+            Action::StartWorkBranch { .. }
+                | Action::StartRelease(_)
+                | Action::StartReleaseFix { .. }
+                | Action::StartHotfixFix { .. }
+        )
+    }
+
+    pub fn no_checkout(&self) -> bool {
+        match self {
+            Action::StartWorkBranch { no_checkout, .. } => *no_checkout,
+            Action::StartReleaseFix { no_checkout, .. } => *no_checkout,
+            Action::StartHotfixFix { no_checkout, .. } => *no_checkout,
+            _ => false,
+        }
+    }
+
+    /// Whether this action is a named-work-branch start eligible for the worktree
+    /// flow. Deliberately excludes `StartRelease`, unlike `is_start`.
+    pub fn worktree_eligible(&self) -> bool {
+        matches!(
+            self,
+            Action::StartWorkBranch { .. }
+                | Action::StartReleaseFix { .. }
+                | Action::StartHotfixFix { .. }
+        )
+    }
+
+    pub fn no_worktree(&self) -> bool {
+        match self {
+            Action::StartWorkBranch { no_worktree, .. } => *no_worktree,
+            Action::StartReleaseFix { no_worktree, .. } => *no_worktree,
+            Action::StartHotfixFix { no_worktree, .. } => *no_worktree,
+            _ => false,
+        }
+    }
+}
+
+/// Branch-name validation shared by the CLI (`--name`) and the interactive
+/// prompt — same rules, same message, whichever interface ran.
+pub fn validate_branch_name(input: &str) -> Result<(), String> {
+    if input.is_empty() {
+        return Err("Name cannot be empty".to_string());
+    }
+    if input.contains("..") || input.contains('~') || input.contains('^') || input.contains(':') || input.contains('\\') {
+        return Err("Invalid branch name. Avoid special characters (.. ~ ^ : \\)".to_string());
+    }
+    Ok(())
+}

src/cli.rs

@@ -1,7 +1,7 @@
 use clap::{Args, Subcommand};
 use crate::git::branch::BranchType;
 use crate::flows::start::ReleaseType;
-use crate::menu::{self, Action};
+use crate::action::{validate_branch_name, Action};
 
 #[derive(Subcommand)]
 pub enum Commands {
@@ -141,7 +141,7 @@ pub enum StartKind {
 }
 
 fn start_work_branch(prefix: &str, name: String, base: String, no_checkout: bool, no_worktree: bool) -> Result<Action, String> {
-    menu::validate_branch_name(&name)?;
+    validate_branch_name(&name)?;
     Ok(Action::StartWorkBranch { prefix: prefix.to_string(), name, from: base, no_checkout, no_worktree })
 }
 
@@ -180,14 +180,14 @@ pub fn resolve_action(command: Commands, branch_type: &BranchType, worktree_enab
                 Ok(Action::StartRelease(release_type))
             }
             StartKind::ReleaseFix { name, opts } => {
-                menu::validate_branch_name(&name)?;
+                validate_branch_name(&name)?;
                 if !auto_discovers_target(&opts, worktree_enabled) {
                     require_release_branch(branch_type)?;
                 }
                 Ok(Action::StartReleaseFix { name, no_checkout: opts.no_checkout, no_worktree: opts.no_worktree })
             }
             StartKind::HotfixFix { name, opts } => {
-                menu::validate_branch_name(&name)?;
+                validate_branch_name(&name)?;
                 if !auto_discovers_target(&opts, worktree_enabled)
                     && !matches!(branch_type, BranchType::Main | BranchType::Hotfix { .. })
                 {