refactor: dry-kiss-solid-review-fixes - #14
Merged
Conversation
The '!starts_with("release-fix/")' guard was dead in all four copies:
a branch starting with 'release-fix/' can never start with 'release/'
('release-' != 'release/'), and the same holds for the hotfix analog.
Replace the four enumeration sites with one branches_with_prefix helper
in flows/mod.rs and drop the dead clause. finish_hotfix keeps its
sort+dedup at the call site (documented as deliberate in decisions.md).
Recorded mock call sequences unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The HashSet dedup returned branches in arbitrary order while MockGit returns its Vec verbatim — the mock guaranteed an ordering the real adapter didn't (LSP). Three .first() consumers in flows/start.rs were nondeterministic with 2+ open release/hotfix branches, a state finish_hotfix explicitly supports. Sort + dedup before returning, per decisions.md's own 'sorted, deduped for deterministic replay' rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decisions.md's own rule: branch/tag names are always generated from SemVer methods, never string-concatenated. finish_hotfix built the branch name by hand one line before constructing the SemVer; finish_release_fix/finish_hotfix_fix and FinishState::source_branch re-implemented the same formats. All four now delegate to release_branch()/hotfix_branch(). Output strings are identical, so recorded mock call sequences are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 'latest RC for this release line' rule (parse tags, filter to vX.Y.0-rc.N, take max) was computed twice, in bump_version and finish_release. One helper now owns the rule; each caller keeps its own error message (per-command UX, deliberately different). Recorded mock call sequences unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Git::stash_push / Git::stash_pop had zero production callers — the documented stash policy is 'stash by message, never blind pop', served by stash_push_with_message / find_stash_by_message / stash_pop_ref. Only tests/stash_test.rs pinned the mock's recording of them (ISP: every implementor carried methods no client uses). Removed from the trait, GitCli, and MockGit; the two pinning tests are deleted and the file is renamed to mock_contract_test.rs for its surviving test (decisions.md reference updated). rev_parse is only used internally by GitCli::is_pushed, so it is now a private inherent method. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 'editor value blank or none means skip opening' rule was written out twice (open_worktree and show_status). One helper owns it now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 'bflow template > first existing native template > empty body' chain was duplicated in github.rs and devops.rs. resolve_body_file now owns the precedence; the native path lists stay local to each provider (documented as intentionally not shared). Two inline tests pin the precedence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Any pr-view failure — including auth expiry or network errors — was treated as 'no existing PR', while the Azure DevOps adapter hard-errors in the same situation (divergent failure semantics behind one trait). Only gh's 'no pull requests found' is now treated as 'no PR'; every other failure propagates with the next command to run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The method has always run 'git merge --ff-only', never 'git pull' (no network I/O — bflow fetches once up front). The name misled readers of both the trait and the recorded test scripts. Deliberate test-expectation diff: recorded call strings change from 'pull:' to 'ff_merge:'; the underlying git command sequence is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(L2) show_select and read_raw_line repeated the cursor::Show + disable_raw_mode cleanup at seven exit points; missing one on a future edit would leak raw mode. A small Drop guard now enforces the documented 'no raw-mode leak' invariant structurally — every return path, including Ctrl-C/Esc aborts and mid-render errors, restores the terminal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The guarded merge-into-target, push-if-needed, tag-if-missing, push-tag-if-missing, and source-branch-cleanup blocks were near-verbatim duplicates across finish_release and finish_hotfix (the cleanup block was identical including its comment). Small named helpers now own the guard-skip-act shape; commit messages and conflict guidance stay caller-supplied. The two flows are deliberately NOT unified into a template — RC gate, propagation, and messaging genuinely differ — and the release→main merge stays inline so the RC gate keeps running inside the not-yet-merged branch. Recorded mock call sequences verified unchanged: all exact-sequence tests pass unmodified. Only narration changed: release tagging now prints 'Tagging: vX.Y.0' instead of 'Tagging main: vX.Y.0'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The feature/fix/chore/docs/refactor taxonomy was restated in ~10 places;
the dangerous copy was finish_work's parent-branch detection, where a
string array could silently drift from BranchType (wrong PR base, no
compile error). Now:
- git/branch.rs owns a WORK_TYPES table (kind → constructor) consumed by
parse(); work_kind() is its compile-checked inverse (no wildcard, so a
new BranchType variant fails compilation until its kind is decided);
commit_type() derives from the kind ('feat' special case only);
is_work_branch() derives from work_kind().
- detect_parent_branch classifies candidates via BranchType::parse
instead of a local prefix list.
- menu.rs builds its develop/work-branch menus from work_kinds();
DevelopOption and WorkBranchOption (each restating the taxonomy and
carrying unreachable!() branch_prefix arms) are deleted. ReleaseOption
stays — its variants map 1:1 to distinct actions. Clap's StartKind
variants stay typed on purpose (compile-enforced sites).
- decisions.md gains a 'new work-branch type' extension recipe.
Menu labels, actions, and recorded call sequences are unchanged; two new
unit tests pin the table/inverse round-trip and the commit-type rule.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Action enum is the single currency both interfaces resolve into, but it lived inside menu.rs — forcing cli.rs (and main) to import the terminal-UI module. It now has its own module alongside the shared validate_branch_name rule. Purely mechanical: no behavior change, only import paths (including in tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
write_state_for_action re-encoded the branch->finish-identity mapping that finish_identity already owns. It now derives the identity from that one mapping and only decides whether the action warrants writing state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rule was written out independently in all three start flows; they now share one effective_no_checkout helper (the in-flow derivation itself is test-pinned: worktree mode must force the no-checkout path even when no_checkout=false is passed). run_flow's parameter — which shadowed the per-action no_checkout fields while actually gating the pre-dispatch branch sync — is renamed to skip_current_branch_sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
template::resolve probed CWD-relative paths from inside finish_work — tests implicitly depended on the process working directory, and running bflow from a subdirectory silently skipped templates. Resolution now happens in main.rs (composition root), anchored to git.repo_root(), and flows receive the resolved path as a parameter (DIP: flows no longer touch the filesystem). Deliberate signature change: the finish_work flow fns gain a template parameter; existing tests pass None, and two new tests pin repo-root anchoring and verbatim pass-through to the hosting platform. Recorded git call sequences unchanged (repo_root is called in main, outside the tested flows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
start.rs and finish_work.rs called menu::show_select directly, coupling flows to the terminal UI (DIP/SRP; architecture rules 1, 8, 9) and making every prompting code path unreachable in tests. A minimal Prompter port (one select method) is now passed alongside git/hosting; menu::MenuPrompter is the real adapter, wired in main.rs, and a scripted MockPrompter joins the test mocks. finish_release_fix and finish_hotfix_fix never prompt, so they deliberately do not take one. The parent-branch candidate logic — previously untestable because reaching it raw-moded the terminal — is now covered: distance-ascending menu order, develop-then-alphabetical tie-break, child-branch exclusion with single-candidate auto-select. MockGit gained keyed merge_base/rev_list_count knobs to express per-candidate topology. The worktree wizard keeps calling menu directly — it IS interface layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unique-temp-dir mechanics (pid + atomic counter) were duplicated in state.rs and hosting/template.rs test modules. A #[cfg(test)] test_support module owns the mechanics; each test module keeps a one-line prefix wrapper. Test infrastructure is the one thing DAMP says to DRY. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decisions.md placed the stash → state-write → dispatch ordering and the
three-way stash-pop policy in main.rs, which tests/ cannot link — the
system's most safety-critical invariant ('state saved before the first
mutating git call') was enforced by a comment, and a resume/--base bug
already slipped through this layer. The lifecycle body of run() now
lives in lifecycle::run, taking &dyn Git / &dyn HostingPlatform /
&dyn Prompter / &dyn Editor; main.rs keeps adapter construction,
preflight, and the bflow worktree dispatch (which must stay ahead of
auth machinery).
This deliberately amends the documented decision 'orchestration stays
in main.rs, not the library' — the decisions.md entry is updated with
the new boundary and rationale.
New tests (tests/lifecycle_test.rs) pin the previously-untestable
contract: a crash mid-flow leaves a resumable state file; success
clears it; a dirty finish is rejected before any stash/state/mutation;
a dirty start stashes before the first mutation and pops on success; a
failed resume keeps state; abort discards state without touching the
repo even mid-merge; plus the two resolve_action_with_state regression
tests moved out of main.rs. MockGit gained a working_tree_clean knob.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each integration-test crate compiles its own copy of tests/common and rarely uses every mock, so cargo test emitted 11 (now 17, with the new MockPrompter/knobs) dead-code warnings that mean nothing. Module-wide allow(dead_code) — the standard treatment for shared test-helper modules — takes the suite to zero warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR applies a broad DRY/KISS/SOLID refactor to the bflow Rust codebase, primarily by extracting cross-cutting orchestration into a testable lifecycle module, introducing a Prompter port to decouple flows from the TTY menu implementation, and consolidating gitflow branch taxonomy/behavior behind shared helpers.
Changes:
- Introduces
lifecycle::run(library-side orchestration) plus new integration tests that pin ordering/state/stash behavior. - Adds a
Promptertrait withMenuPrompter+MockPrompter, and updates flows/tests to use the port instead of callingmenu::show_selectdirectly. - Refactors gitflow branch handling (work-kind taxonomy table, SemVer-derived branch names) and improves deterministic behavior (sorted/deduped branch listing).
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/stash_test.rs | Removes stash push/pop recording tests after trait cleanup. |
| tests/start_test.rs | Updates start-flow tests to pass a MockPrompter into start_release. |
| tests/mock_contract_test.rs | Adds a focused test to pin the mock call-recording contract. |
| tests/lifecycle_test.rs | Adds integration tests that pin lifecycle ordering, crash-safety state, and stash policy. |
| tests/finish_work_test.rs | Updates finish-work tests for Prompter + template plumbing; adds parent-candidate ordering tests. |
| tests/finish_release_test.rs | Updates expected call strings from pull to ff_merge. |
| tests/finish_hotfix_test.rs | Updates expected call strings from pull to ff_merge. |
| tests/common/mod.rs | Extends mocks (Prompter, per-merge-base counts, working-tree cleanliness) and removes unused Git methods. |
| tests/cli_test.rs | Updates Action import to new src/action.rs module. |
| tests/action_test.rs | Updates Action import to new src/action.rs module. |
| src/worktree.rs | Centralizes “editor disabled” rule via editor_disabled(). |
| src/test_support.rs | Adds shared tmp_dir helper for unit tests (cfg(test)). |
| src/state.rs | Uses SemVer helpers for source-branch naming; reuses shared tmp-dir helper in tests. |
| src/prompt.rs | Adds Prompter trait port for interactive selection. |
| src/menu.rs | Adds MenuPrompter adapter; introduces TerminalGuard RAII for raw-mode cleanup; simplifies menus via work_kinds(). |
| src/main.rs | Shrinks to composition root: constructs adapters and delegates to lifecycle::run. |
| src/lifecycle.rs | New library module implementing orchestration (resume/action resolution, stash/state lifecycle, dispatch). |
| src/lib.rs | Exposes new modules (action, lifecycle, prompt) and test_support for unit tests. |
| src/hosting/template.rs | Anchors bflow template resolution to repo root; updates tests accordingly. |
| src/hosting/mod.rs | Centralizes PR body-file precedence (resolve_body_file) and adds unit tests. |
| src/hosting/github.rs | Tightens error handling: only swallows “no PR found”, propagates auth/network failures with guidance. |
| src/hosting/devops.rs | Uses shared PR body precedence helper. |
| src/git/mod.rs | Renames pull→ff_merge, removes stash push/pop from port, makes branch matching deterministic (sort+dedup), demotes rev_parse. |
| src/git/branch.rs | Adds WORK_TYPES single source of truth + compile-checked inverse (work_kind); derives commit type and work-kind list. |
| src/flows/start.rs | Adds effective_no_checkout, introduces Prompter for release-type prompting, refactors branch listing via helper. |
| src/flows/mod.rs | Adds shared branch-filter helper and shared idempotent finish-step helpers (merge/push/tag/delete). |
| src/flows/finish_work.rs | Routes prompting through Prompter, resolves template at composition boundary, adjusts parent-branch detection inputs. |
| src/flows/finish_release.rs | Extracts latest_rc helper; adopts shared idempotent finish-step helpers; switches to ff_merge. |
| src/flows/finish_hotfix.rs | Adopts shared idempotent finish-step helpers; uses SemVer-derived hotfix branch name; keeps deterministic release propagation. |
| src/cli.rs | Moves Action + validate_branch_name import to src/action.rs. |
| src/action.rs | New shared Action enum + shared validate_branch_name. |
| .claude/skills/architecture/decisions.md | Updates architecture/testing decisions to reflect new ports and lifecycle refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
43
| let parsed = BranchType::parse(branch); | ||
| if parsed != BranchType::Develop && !parsed.is_work_branch() { | ||
| continue; | ||
| } | ||
| let base = match git.merge_base(current, branch) { |
Comment on lines
+292
to
+295
| add_candidate(&mut git, "feature/parent", "develop", "base-d", 4, 0); | ||
| // feature/stacked has MORE commits since divergence than we do — it | ||
| // branched from us, so it must not be offered as a PR target. | ||
| add_candidate(&mut git, "feature/parent", "feature/stacked", "base-s", 1, 6); |
Code reviewChecked for bugs and CLAUDE.md compliance. 1 issue found. CLAUDE.md — README.md Architecture section is staleThis PR adds three new public library modules — However,
The root CLAUDE.md
The PR body's justification ("no user-visible CLI behavior changed... README... no changes needed") covers the CLI-facing surface, but README.md has a dedicated Architecture section that documents internal module structure, and this PR's own architecture skill entry ( Suggested fix: Update the |
Address PR #14 review: the README Architecture tree omitted action.rs, cli.rs, lifecycle.rs, prompt.rs, and test_support.rs, and still credited main.rs with dispatch. The architecture skill's Layer Map and port list had the same gap (Prompter, lifecycle.rs). Also promote the sorted+deduped ordering guarantee of list_branches_matching from an impl comment to the Git trait contract, and explain why finish_hotfix still re-sorts locally (crash-safety invariant enforced in the flow, pinned by a deliberately unsorted mock). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed the review finding in ed43cff5be2c748edbe958d21bafb46be3778288. 1. README Architecture section Updated the 2. Same staleness in the architecture skill While fixing the README I found the same gap in 3. Ordering contract made explicit Related cleanup from the same review pass: the sorted+deduped guarantee of Build and tests are green: 237 passed, 0 warnings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves all findings from the DRY/KISS/SOLID review of bflow (baseline
0e29a7b). Every finding was independently re-verified against the code before fixing; all of them held up, so nothing was skipped. One logical fix per commit (20 commits),cargo build+cargo testgreen after each. Final state: 237 tests, 0 warnings (baseline 222 tests, 11 warnings).Fixed
High
SemVer::release_branch()/hotfix_branch()everywhere (finish_hotfix,finish_work,FinishState::source_branch); no hand-concatenation left.WORK_TYPEStable ingit/branch.rsconsumed byparse, with a compile-checked inversework_kind()(no wildcard — a new variant fails compilation until decided).commit_typederives from it;detect_parent_branchclassifies viaBranchTypeinstead of a local string array (the silent-failure site). decisions.md gained a "new work-branch type" recipe.Prompterport (oneselectmethod); flows no longer callmenu::show_select.MenuPrompteris the real adapter,MockPrompteris scripted in tests. The previously-unreachable parent-branch candidate logic is now tested: distance-ascending order, develop-then-alphabetical tie-break, child-branch exclusion. The worktree wizard still usesmenudirectly (it is interface layer).GitCli::list_branches_matchingsorts + dedups instead of draining aHashSet, honoring the deterministic-order contract the mock already implied (three.first()consumers were nondeterministic with 2+ open release/hotfix branches).Medium
Git::stash_push/stash_popremoved from trait,GitCli, andMockGit(stash policy is stash-by-message only);rev_parsedemoted to a privateGitClimethod.stash_test.rs→mock_contract_test.rsfor the surviving test.merge_into,push_if_needed,tag_if_missing,push_tag_if_missing,delete_source_branch) extracted intoflows/mod.rs; commit messages and conflict guidance stay caller-supplied. Recorded mock call sequences verified byte-identical — the exact-sequence tests passed unmodified. The two flows were deliberately not unified (RC gate / propagation / messaging genuinely differ), and the release→main merge stays inline so the RC gate keeps its resume-skip semantics.!starts_with("release-fix/")clause (never true afterstarts_with("release/")) removed via onebranches_with_prefixhelper;finish_hotfixkeeps its documented sort+dedup at the call site.latest_rchelper infinish_release.rs; each caller keeps its distinct error message.DevelopOption/WorkBranchOption(taxonomy restated twice +unreachable!()arms) deleted; menus build fromwork_kinds().ReleaseOptionand the typed clapStartKindvariants stay.Action+validate_branch_namemoved tosrc/action.rs;cli.rsno longer imports the terminal-UI module. Mechanical.git.repo_root(), and passed into the flows as a parameter — flows no longer probe the CWD filesystem, and resolution works from subdirectories.Low
Git::pull→Git::ff_merge(it always rangit merge --ff-only). Deliberate test-expectation diff: recorded stringspull:→ff_merge:; git commands unchanged.TerminalGuardreplaces 7 hand-writtencursor::Show+disable_raw_modecleanups; the "no raw-mode leak" invariant is now structural.effective_no_checkoutderivation in the flows (the in-flow rule is test-pinned);run_flow's shadowing param renamed toskip_current_branch_sync.editor_disabled()rule inworktree.rs.resolve_body_fileprecedence inhosting/mod.rs; per-provider path lists stay local (documented as intentional).write_state_for_actionderives its identity fromfinish_identityinstead of re-encoding the mapping.tmp_dirtest helper (src/test_support.rs,#[cfg(test)]); the integration-test crate keeps its own copy intests/common(separate compilation unit).Skipped
None — every finding was confirmed on inspection.
Documented decisions changed (decisions.md)
main.rs, not the library" is amended. The lifecycle now lives inlifecycle::run(library, all-&dynports) so the reject → stash → write-state → dispatch contract and the three-way stash-pop policy are mock-tested;main.rskeeps adapter construction, preflight, and thebflow worktreedispatch. Newtests/lifecycle_test.rspins: crash mid-flow leaves a resumable state file, success clears it, dirty finish rejected before any side effect, dirty start stashes before the first mutation and pops on success, failed resume keeps state, abort discards state without touching the repo even mid-merge (+ the tworesolve_action_with_stateregression tests moved frommain.rs). Rationale recorded in the decisions.md entry itself: the old boundary made the state-before-mutation invariant untestable, and a resume/--basebug had already slipped through this layer.Prompterplug-in row (H3),mock_contract_test.rsreference (M1).Not changed
Tagging: vX.Y.0instead ofTagging main: vX.Y.0.clap+crossterm, zero dev-deps); error model staysResult<T, String>and every new error names the next command.🤖 Generated with Claude Code