chore: sync direction wording by jopmiddelkamp · Pull Request #20 · Beans-BV/beans-gitflow

jopmiddelkamp · 2026-08-08T07:09:01Z

No description provided.

The sync operation is strictly release -> develop, but nothing at the CLI level said so. Wording-only change: command narration, clap help text, README, and the bflow skill now state the direction and why the reverse does not exist (the staging-tag gate). Also records the CR-01 rejection in the proposal (premise falsified: clap already prints a guiding similar-subcommand tip) and the planning lesson in tasks/lessons.md. No behavior change; pinned commit-message and PR-title strings untouched.

Copilot

Pull request overview

Clarifies that bflow sync operates from release to develop while documenting related UX proposals.

Changes:

Updates sync narration, help text, README, and skill guidance.
Adds UX improvement proposals and planning lessons.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/flows/finish_release.rs`	Clarifies sync narration.
`src/cli.rs`	Updates sync help text.
`README.md`	Documents one-way sync behavior.
`.claude/skills/bflow/SKILL.md`	Synchronizes workflow guidance.
`tasks/proposal-ux-improvements.md`	Adds UX change proposals.
`tasks/lessons.md`	Records proposal-validation guidance.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jopmiddelkamp · 2026-08-08T16:20:00Z

    let current = git.current_branch()?;

-    println!("Merging {release_branch} into develop...");
+    println!("Merging {release_branch} into develop (one-way — develop is never merged into a release)...");


Confirmed and fixed in b726520. sync_with_develop returned at the Mode::Protected check before reaching the narration, so protected repos got no direction statement at all.

Took the hoist but not the shape you suggested: adding a line before the branch while leaving the parenthetical on the free-mode Merging ... line would print the direction twice on that path. Instead the hoisted line is the single place the direction is stated, which made the Merging ... progress line redundant (the closing Develop synced with ... already reports the outcome), so it is gone. Free and protected now both open with:

Syncing release/2.6.0 into develop (one-way — develop is never merged into a release).

No test covers this: flows println! straight to stdout and the suite asserts git.calls(). Closing that needs an output port, which is not justified by a wording fix.

jopmiddelkamp · 2026-08-08T16:20:03Z

+**Proposed change.** Wording only, in three places:
+- Command narration: `Merging release/X.Y.Z into develop (one-way — develop
+  is never merged into a release)`.
+- clap `--help` text for the subcommand.
+- README + bflow skill: state the direction and the *why* (RC gate).


Confirmed and fixed in b726520. src/menu.rs:40 is now "sync release into develop", changed red-first — tests/menu_test.rs pins the label set, so the assertion went first and was watched fail.

Also updated the two README spots that print the menu (the release-branch menu block and the sequence-diagram note), and corrected the proposal to derive its surface list from architecture principle 8 rather than listing three places from memory. That planning miss is recorded in tasks/lessons.md.

jopmiddelkamp · 2026-08-08T16:20:19Z

+- [ ] 3.2 Wire into the protected paths of `finish` (release + hotfix),
+      `bump`, `sync`. Existing protected call-sequence tests must stay
+      green **byte-for-byte** — they pin the no-local-mutation guarantee.


Confirmed and fixed in b726520. Checked protected_hotfix_opens_main_pr_and_stops — it is an assert_eq!(git.calls(), vec![...]) over a three-call vector, so any added read breaks it. CR-03 step 3.2 asked for exactly that while also requiring the vectors stay byte-for-byte, which cannot both hold.

Resolved the way you propose: the plan now says to update the exact-sequence vectors deliberately and red-first, and to preserve the no-local-mutation assertions byte-for-byte instead — those are the separate !calls.iter().any(...) checks for checkout:, merge: and create_tag in the same test, and they are what actually pins the guarantee.

jopmiddelkamp · 2026-08-08T16:20:21Z

+**Precedent:** the worktree machinery and its `Git` primitives already
+exist (`worktree.rs`); this reuses them privately rather than adding
+surface.


Confirmed and fixed in b726520. Both primitives check out as you describe:

Git::remove_current_worktree — contract says it removes the worktree the process is standing in and "must be the LAST git operation of a flow", so it cannot clean up a temp worktree the flow created and wants to continue past.

ScriptCli::run — runs with .current_dir(&self.repo_root) fixed at construction, so it cannot be pointed at a temp worktree path.

So CR-04's stated precedent ("reuses them privately rather than adding surface") was false. The proposal now says plainly that CR-04 needs path-scoped worktree removal and a path-scoped version-script run — trait changes on Git and VersionScript plus their mocks — and that this is an explicit architectural decision to be made before implementation, not an assumption. CR-04 is marked blocked on that decision in tasks/todo.md.

jopmiddelkamp · 2026-08-08T16:05:32Z

Code review

Found 3 issues:

The interactive menu label is the one surface left unchanged, and it is the most bidirectional-sounding of the four. "sync with develop" reads exactly the way this PR is trying to stop users reading it, and a user picking from the menu never sees the clap --help text that was clarified. README documents the menu as the release-branch entry point, so this is the primary surface, not a secondary one. "sync release into develop" would close it (plus its assertion in tests/menu_test.rs).

beans-gitflow/src/menu.rs

Lines 38 to 42 in 2a3e84e

    
                   Self::StartReleaseFix => "start release fix", 
        
                   Self::BumpVersion => "bump version", 
        
                   Self::SyncWithDevelop => "sync with develop", 
        
               } 
        
           }

The new docs give the staging-tag gate a rationale it does not have, contradicting the one recorded 120 lines below. The added sentence says the gate "exists so unstaged develop content cannot ride into a release", and the skill doc says the one-way direction is something "the staging-tag gate depends on it". The gate has no notion of develop: it counts commits past the latest RC/patch tag with rev_list_count and refuses on any of them, whatever their provenance.

beans-gitflow/src/flows/finish_release.rs

Lines 293 to 298 in 2a3e84e

    
           if !git.is_ancestor(&release_branch, main_branch)? { 
        
               let latest_staged_tag = latest_staged.tag_name(); 
        
               let commits_past = git.rev_list_count(&latest_staged_tag, &release_branch)?; 
        
               if commits_past > 0 { 
        
                   return Err(past_staged_tag_error(&release_branch, main_branch, &latest_staged_tag, commits_past, cfg.bump_strategy)); 
        
               }

The recorded purpose is different, and stated twice — in this same README file ("Every commit merged to main must have been validated on staging via a tagged deploy") and in decisions.md:65 ("every commit reaching main was staging-validated under an RC deploy"). The claim is also wrong on its own terms: merging develop into a release would not be blocked by the gate — it would fire, tell the user to run bflow bump, and the develop content would ride in legally once tagged. The direction claim itself is correct; only the why is invented.

beans-gitflow/README.md

Lines 283 to 287 in 2a3e84e

    
           Both require being on a release branch. 
        
           `bflow sync` is strictly one-way: it merges the release branch into `develop`, never the reverse. Develop is deliberately never merged into a release — the staging-tag gate exists so unstaged develop content cannot ride into a release. 
        
           ## Worktree integration

Same claim in the skill doc:

beans-gitflow/.claude/skills/bflow/SKILL.md

Lines 97 to 101 in 2a3e84e

    
           ```bash 
        
           bflow bump # create next RC tag (on release/* only) 
        
           bflow sync # merge release into develop (on release/* only; one-way — develop is never merged into a release, the staging-tag gate depends on it) 
        
           ```

A 223-line forward-planning document for CR-03 and CR-04 is bundled into a copy-only chore PR (CLAUDE.md says "Minimal Impact: Changes should only touch what's necessary"). The CR-02 change is 7 lines across four files; the diff is +245/-5, so the planning doc is roughly 91% of it and specifies two changes that are not in this PR. The same channel flagged a one-line unrelated cleanup on feat: devops-support #13 and it was reverted to keep the diff scoped — feat: devops-support #13 (comment). The doc also embeds a spec change to the mutation-pinned hotfix_no_checkout_skips_script, which deserves its own review rather than approval-by-adjacency inside a wording PR. It additionally cites CLAUDE.md's "does NOT apply" list as a TDD exemption for copy-level .rs changes; that list lives under Architectural Decisions and Documentation Sync, while the TDD Policy says it "Applies to EVERY change to .rs files, Cargo.toml, or shell scripts — even one-liners".

beans-gitflow/tasks/proposal-ux-improvements.md

Lines 1 to 10 in 2a3e84e

    
           # bflow — Feature & Fix Request Proposal 
        
           | | | 
        
           |---|---| 
        
           | **Document** | Change request proposal — CLI UX improvements | 
        
           | **Date** | 2026-08-06 | 
        
           | **Source** | Full command-surface review of bflow from the end-user perspective (every command, flag, and configuration variation) | 
        
           | **Status** | Draft — awaiting approval | 
        
           | **Supersedes** | The interim plan in `tasks/todo.md` (same content, this document is the authoritative form) |

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

Review found the CR-02 change landed on some surfaces but not all, and justified itself with a mechanism that does not do what was claimed. - Menu label `sync with develop` -> `sync release into develop`. It is the one surface whose user never reads `--help`, and it was the most bidirectional-sounding of the four (architecture principle 8). - Narration hoisted above the `Mode::Protected` branch. Protected mode returns early, so the direction was stated only on the free path. Hoisting keeps one string in one place instead of duplicating it per mode, which also retires the now-redundant "Merging ..." progress line. - README/skill no longer claim the staging-tag gate exists to keep develop content out of a release. That gate counts commits past the latest RC/patch tag and has no notion of develop; the claim contradicted the guard blockquote in the same README and decisions.md. Replaced with the actual reason: a release branch freezes scope when it is cut from develop. - Proposal doc: corrected the same invented rationale, the surface list, the miscited TDD carve-out, CR-03's impossible byte-for-byte constraint, and CR-04's false claim that existing worktree/version-script primitives suffice (`remove_current_worktree` is standing-in-it only; `ScriptCli::run` is pinned to its construction-time root). - tasks/todo.md held a full second copy of the plan the proposal says it supersedes; it now tracks in-flight work and points at the proposal. Narration is not test-observable (flows println! to stdout; tests assert git.calls()), so 2.2's narration half ships uncovered. The menu label is pinned and was changed red-first.

jopmiddelkamp · 2026-08-08T16:20:52Z

Review response

All seven findings assessed. Six accepted and fixed in b726520; one rejected in part.

Accepted

Finding	Fix
Menu label still bidirectional (both reviewers)	`sync release into develop`, changed red-first against the pinned assertion in `tests/menu_test.rs`; the two README spots that print the menu updated with it
Narration unreachable in protected mode (Copilot)	Hoisted above the `Mode::Protected` branch as the single place the direction is stated
Invented staging-gate rationale	Replaced with the real reason — scope freeze at cut time
CR-03's byte-for-byte constraint is self-contradictory (Copilot)	Plan now updates the sequence vectors red-first and preserves the no-mutation assertions instead
CR-04's "primitives already exist" is false (Copilot)	Plan now names the required path-scoped `Git`/`VersionScript` surface as an explicit decision; CR-04 marked blocked on it
Proposal miscites CLAUDE.md's "does NOT apply" list as a TDD carve-out	Corrected — that list belongs to Architectural Decisions and Documentation Sync; the TDD Policy has no copy-level exemption

Rejected in part: "remove the proposal document from this PR"

That was my own finding and the scope half of it does not hold up. The #13 precedent it leaned on was about unrelated production code (src/menu.rs) inside a feature PR. A planning document in tasks/ that produced this very change is a different kind of thing, and tasks/ is where CLAUDE.md's Task Management section puts plans. Deleting it would also discard six considered-and-declined items written specifically so they are not re-litigated — a net loss. The "91% of the diff" framing was misleading: doc lines are not code lines.

What was legitimate in that finding is fixed instead: the document had real defects (three of them above), and it claimed to supersede tasks/todo.md while leaving a full second copy of the same plan in place — including the CR-01 premise this PR's own lessons.md entry records as falsified. todo.md now tracks in-flight work and points at the proposal, so the plan is written down once.

Known gap

The narration change ships uncovered. Flows println! directly to stdout and the suite asserts git.calls(), so no test can observe it. Closing that needs an output port, which a wording fix does not justify. Flagging it rather than claiming coverage it does not have.

Out of scope, worth a separate issue

src/cli.rs:31 documents bump as "Bump the patch version on the current release branch", but under the default bump-strategy=rc it cuts the next RC tag, not a patch version. Real, adjacent to a line this PR touches — and fixing it here would be the exact scope creep flagged above.

`bflow bump --help` said "Bump the patch version on the current release branch". Under the default bump-strategy=rc it cuts the next RC tag; the patch-version wording is only true under bump-strategy=patch. The line predates the bump-strategy feature (afa4546), so it was already wrong when bump only ever produced RC tags. README and the bflow skill already describe it correctly, so the clap help was the only surface stating it wrong; no docs change needed. Clap help text is asserted nowhere, so this is doc-comment-only with no test impact.

jopmiddelkamp · 2026-08-08T16:24:55Z

Correction to the "out of scope" note above — I folded it into this PR after a closer look.

Three things changed the call:

The line predates the bump-strategy feature (afa4546), so it was already wrong back when bump only ever cut RC tags. feat: bump strategy #19 did not break it; it made it conditionally right under the non-default patch strategy.
README and the bflow skill already describe bump correctly, so the clap help was the single surface stating it wrong. No docs change needed.
Clap help is asserted nowhere, so it is doc-comment-only with no behavior or test impact.

My earlier reasoning applied the scope rule mechanically rather than to what this PR is actually about — release-branch command help stating what the command does. Bump fails that same test on the line directly above Sync. The #13 precedent does not reach it: that was unrelated production code in a feature PR, not an adjacent instance of the same defect in a help-text PR.

Fixed in 684026b:

bump      Cut the next RC tag on the current release branch (the next patch tag under bump-strategy=patch)
sync      Sync the current release branch into develop (one-way; develop is never merged into a release)

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (7)

tasks/proposal-ux-improvements.md:104

The protected hotfix path does not re-derive the full plan on every run: it returns at the first pending target (for example, src/flows/finish_hotfix.rs:124-129 returns before querying develop or release legs). This also conflicts with line 142's correct note that full-plan derivation adds reads. Reword the premise to say the status is derivable, not already fully derived and discarded.

every run — *"where am I in this?"* — is information bflow already re-derives
internally from PR and tag state on every run, and then discards.

tasks/proposal-ux-improvements.md:220

This step still requires using the existing worktree primitives, directly contradicting lines 194-209, which establish that those primitives cannot remove a private temporary worktree or run the script there. Make the port/API decision and adapter/mock changes an explicit prerequisite in the implementation plan.

- [ ] 4.2 Implement in `flows/start.rs`'s hotfix-creation path using the
      existing `Git` worktree primitives. Every failure path removes the
      temp worktree; script failure produces the existing warning text
      (pinned by its own test).

src/cli.rs:33

These doc comments generate user-visible clap output, but tests/cli_test.rs only checks parsing and does not assert either changed description. The proposal's stdout limitation does not apply to clap help, which is directly testable. Add help-rendering assertions for the new Bump and Sync wording so these required surfaces cannot drift unnoticed.

    /// Cut the next RC tag on the current release branch (the next patch tag under bump-strategy=patch)
    Bump,
    /// Sync the current release branch into develop (one-way; develop is never merged into a release)

README.md:285

In protected mode, bflow sync opens or reuses a PR and exits; it does not perform the merge (src/flows/finish_release.rs:252-260). This new paragraph therefore contradicts protected-mode behavior. Distinguish the direct free-mode merge from the protected-mode PR while retaining the one-way explanation.

`bflow sync` is strictly one-way: it merges the release branch into `develop`, never the reverse. A release branch freezes its scope the moment it is cut from `develop`, while `develop` keeps collecting work for the *next* release. Merging `develop` back in would pull that unfinished work into a release that is being stabilized, so bflow has no command that does it.

README.md:406

This table still says bflow sync performs a merge, but protected mode only opens/reuses a PR and exits. Describe both landing modes so this command reference agrees with the protected-mode section.

| **bflow sync** | Merges release changes into `develop` for fixes needed immediately (one-way: develop is never merged into a release) |

.claude/skills/bflow/SKILL.md:99

This says the command merges, while the same skill's protected-mode section states that sync only opens/reuses a PR and bflow never merges it. Use mechanism-neutral wording or name both modes to avoid contradictory guidance.

bflow sync # merge release into develop (on release/* only; one-way — the reverse would pull next-release work into a stabilizing release)

tasks/proposal-ux-improvements.md:9

tasks/todo.md is not present in the repository, so the claim that it now tracks work and points here leaves a dead reference in this new proposal. Either add the tracked file or remove that claim.

This issue also appears on line 103 of the same file.

| **Supersedes** | The interim plan formerly in `tasks/todo.md`, which now tracks in-flight work only and points here |

jopmiddelkamp requested a balanced review from Copilot August 8, 2026 15:52

Copilot started reviewing on behalf of jopmiddelkamp August 8, 2026 15:53 View session

Copilot AI reviewed Aug 8, 2026

View reviewed changes

jopmiddelkamp requested a balanced review from Copilot August 8, 2026 16:27

Copilot started reviewing on behalf of jopmiddelkamp August 8, 2026 16:28 View session

Copilot AI reviewed Aug 8, 2026

View reviewed changes

jopmiddelkamp closed this Aug 10, 2026

jopmiddelkamp deleted the chore/sync-direction-wording branch August 10, 2026 07:18

Navigation Menu

Uh oh!

chore: sync direction wording - #20

chore: sync direction wording#20
jopmiddelkamp wants to merge 3 commits into
developfrom
chore/sync-direction-wording

jopmiddelkamp commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jopmiddelkamp Aug 8, 2026

Uh oh!

jopmiddelkamp Aug 8, 2026

Uh oh!

jopmiddelkamp Aug 8, 2026

Uh oh!

jopmiddelkamp Aug 8, 2026

Uh oh!

jopmiddelkamp commented Aug 8, 2026

Uh oh!

jopmiddelkamp commented Aug 8, 2026

Uh oh!

jopmiddelkamp commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jopmiddelkamp commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jopmiddelkamp Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

jopmiddelkamp Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

jopmiddelkamp Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

jopmiddelkamp Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

jopmiddelkamp commented Aug 8, 2026

Code review

Uh oh!

jopmiddelkamp commented Aug 8, 2026

Review response

Uh oh!

jopmiddelkamp commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants