Skip to content

Merge-and-finish: one action that merges a worktree branch and closes it out - #6

Merged
jbachorik merged 19 commits into
mainfrom
feat/merge-finish-followups
Jul 27, 2026
Merged

Merge-and-finish: one action that merges a worktree branch and closes it out#6
jbachorik merged 19 commits into
mainfrom
feat/merge-finish-followups

Conversation

@jbachorik

@jbachorik jbachorik commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Finish ▸ → "Merge into <base>" used to run git merge --no-ff, flash a four-second pill in the tab header, and leave the worktree, branch, session, tab and sidebar row all behind. It now merges and closes the work out — worktree removed, branch deleted when drydock owns it, session closed, tab and row gone — and reports what actually happened in a modal you dismiss. Conflicts hand off to the session's agent, scoped to the main checkout with git -C, then poll until the merge is genuinely confirmed.

Scope of this PR

22 commits, because local main had never been pushed — it was 20 ahead of origin/main. This PR therefore carries that backlog plus the feature. The feature itself is the 14-commit feat/merge-and-finish branch, already merged into local main as 0c9df53, plus two follow-ups on top.

The design decision that matters

A merge is never inferred from an exit code. git merge --no-ff is followed by an inspection of the repository, and only one thing counts as success: a commit on the expected base branch whose parents are the recorded pre-merge HEAD and the recorded branch tip.

Ancestry (merge-base --is-ancestor) is deliberately not the oracle. git merge -s ours, a bare git checkout <branch>, or a reset --hard all make ancestry true while the base tree contains none of the work — and every one of them is reachable by the agent that resolves conflicts, after which git branch -D would destroy the only copy. The parent-set check cannot be satisfied without a real merge commit of the tip that was verified.

The destructive step then re-reads the branch tip one last time and refuses git branch -D if it moved, or if the re-read itself failed. On the hand-off path minutes elapse with the agent sitting in the worktree; a commit landing there during that window used to be force-deleted while the modal reported "✓ Merged … branch deleted".

Structure

Three layers, so the parts worth testing are testable — no test in this repository touches JavaFX:

  • WorktreeService — pre-flight probe, the verified merge, BranchNotDeletedException so a failed branch delete is distinguishable from a failed worktree removal.
  • MergeFinishDecision, WorktreeSessionCleanup, BranchDeleteGate — FX-free. Every decision, every user-visible sentence, and the destructive sequence itself. This is where the safety properties are asserted, including a test that loops all twelve verdict × hand-off states to prove only a confirmed merge reaches cleanup.
  • MergeAndFinishFlow — a thin JavaFX shell: async plumbing and one modal.

WorktreeSessionCleanup is now the single implementation of "this worktree session is finished", used by both this flow and the panel's existing Delete, replacing an inline lambda that ignored deleteSession's failure. Its invariant: the session is deleted only if the worktree is actually gone, because a surviving worktree still holds files the user needs the session's terminal to deal with.

Also here

  • CI. The suite had no automation at all — 484 tests that ran only when someone typed the command. .github/workflows/tests.yml runs them on every push to main and every PR.

Known conflict

app/src/test/java/app/drydock/app/SessionManagerTest.java. The multi-agent refactor on origin/main (#2) deleted the Claude-specific resume-fallback tests; this branch predates that and still has them. The resolution is almost certainly to take origin/main's side, but that is a call about the provider abstraction rather than about this feature, so it is left unresolved here.

Everything else merges cleanly — including the ManagedClaudeSession → ManagedAgentSession rename, which git applied to the files this branch touches. :app:compileJava succeeds on the merged result, verified locally against origin/main.

Verification

  • 484 tests, 0 failures, on the branch. Also verified green in a clone with no submodule and no global git config, which is what the new CI job does.
  • Not yet verified: the JavaFX shell. No test here executes MergeAndFinishFlow or the controller wiring, by design. An 8-item manual script exists; the two that matter most are the tip-drift path (commit to the branch inside the worktree while the flow polls — the branch must survive and the modal must say so) and confirming the in-flight guard releases after a stop.

Deliberately deferred

  • An atomic git update-ref -d refs/heads/<b> <expected-oid> would close the last sub-second window between the tip re-read and the delete. The minutes-long window is closed; this is the residue.
  • Threading Optional<String> branch/base through the tab header, so the "(detached)" display sentinel never reaches flow logic (currently guarded, not cured).
  • GitStatusService still conflates interrupt with timeout; the sibling exception type now exists and the conversion is mechanical.

🤖 Generated with Claude Code

jbachorik and others added 19 commits July 27, 2026 12:05
@jbachorik @claude
Spec the merge-and-finish flow for worktree sessions
5712214 
Merging a worktree branch today leaves the worktree, branch, session and
tab behind, and reports success as a four-second pill in a tab header the
user is looking away from. Design one action that merges and then closes
the work out, with a modal that says what happened.

An adversarial review of the first draft found the merge's success oracle
was the weak part: git merge-base --is-ancestor passes for merge -s ours,
for a bare checkout of the branch, and for a detached main checkout --
every one of them reachable by the agent that resolves conflicts, and all
of them followed by git branch -D. The spec records a merge only when the
new HEAD is a real merge commit of the recorded branch tip on the
expected base branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Plan the merge-and-finish implementation
4e34206 
Six tasks, TDD, each with its own test cycle: the main-checkout pre-flight
probe, the merge whose verdict is verified rather than inferred, a distinct
failed-branch-deletion signal, the FX-free decision table that holds every
string the user sees, the shared destructive cleanup, and the JavaFX shell
that renders it.

The decision table and the cleanup are separate classes because no test in
this repository touches JavaFX -- putting "only a confirmed merge may reach
the cleanup" in an FX-free class is what makes that rule assertable.

Two spec corrections while planning: the session is now deleted only if its
worktree is gone (closing it would discard the terminal and conversation the
user needs to deal with the files that blocked removal), and the test plan
drops the faked-services flow test in favour of the two FX-free classes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Ignore the subagent-driven-development scratch directory
aceedce 
.superpowers/ holds per-plan execution scratch (ledger, task briefs, review
packages). It is disposable and must never reach a commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik
Probe the main checkout before merging a worktree branch
d85d7e5
@jbachorik
Verify what a worktree merge actually did instead of trusting its exi…
f40a193 
…t code
@jbachorik
Fix round 1: verify the merge oracle against reset --hard, give merge…
b1956d6 
…s their own timeout, and stop treating failed queries as empty

Adds a test proving the parent-set check rejects a reset --hard onto the
branch tip (manually confirmed it fails under a bare ancestor check),
gives git merge its own 2-minute timeout so a slow hook or large tree
no longer aborts verification, makes a failed diff/merge-base query
throw instead of silently reading as "no conflicts"/"not an ancestor",
rewords the Refused detail to cover the staged-but-uncommitted case,
and rejects a detached main checkout in verify() rather than relying on
an upstream caller to gate it.
@jbachorik
Fix round 2: give a merge interrupt its own exception type so it can'…
b6c54d9 
…t be mistaken for a timeout

Round 1 made a timed-out or IO-failed merge spawn fall through to verify()
instead of throwing, but caught the same GitCommandFailedException that
run() also threw for a genuine interrupt -- so an interrupt spuriously
fell through too, and verify()'s own first spawn then re-threw a second,
uncaught failure because Process.waitFor() rejects a thread that is
already interrupted.

Adds GitCommandInterruptedException as a sibling of GitCommandFailedException
under the sealed GitException hierarchy, has run(...) throw it specifically
for the InterruptedException case, and lets merge()'s catch (which only
names GitCommandFailedException) leave it uncaught -- an interrupt now
propagates with the thread's interrupt status intact and never reaches
verify(). Extracts merge()'s body into a package-private mergeBlocking(),
matching this class's existing Blocking-method convention, so a test can
interrupt the worker thread directly and confirm the propagation.
@jbachorik
Report a failed branch deletion separately from a failed worktree rem…
6cc531b 
…oval
@jbachorik
Decide and word the merge-and-finish flow outside JavaFX
63dbe59
@jbachorik
Fix round 1: pin the CleanUp invariant, the preflight order, and the …
595daec 
…hand-off prompt's scope

- Test the CleanUp-only-from-Merged/AlreadyMerged rule as one 12-cell invariant
  instead of two independent facts.
- Add multi-refusal preflight tests that actually pin the check order (the
  single-refusal tests didn't).
- Rewrite the conflict hand-off prompt to name the conflicted files as absolute
  paths under the main checkout and to prefix every forbidden git shortcut with
  `git -C <main>`, so an agent whose cwd is the worktree can't misread it.
- Rename CleanupOutcome's `detail` to `worktreeKeptReason` and reject
  worktreeRemoved=true paired with BranchResult.NOT_ATTEMPTED in a compact
  constructor.
- Make describe()'s NONE arm a loud IllegalArgumentException instead of
  ungrammatical unreachable copy.
@jbachorik
Extract the worktree-session cleanup so it reports per step
2152e6b 
Widen both BranchNotDeletedException's and WorktreeNotCleanException's
constructors to public so app.drydock.ui's cleanup tests can construct
them directly; both are already public final classes with public
accessors and the sidebar already branches on their types.
@jbachorik
Fix round 1: close the exceptional-completion and false-report gaps i…
f47c1cf 
…n worktree-session cleanup

- WorktreeSessionCleanup.run/closeSession now guard both collaborator
  calls so a synchronous throw or null return becomes a failed future
  instead of escaping run() or completing the returned future
  exceptionally -- both reachable at shutdown (executor rejection,
  Platform.runLater after the toolkit stops).
- WorktreeService.removeBlocking now wraps any GitException from the
  branch-delete stage as BranchNotDeletedException, so a timeout,
  spawn failure, or interrupt there can no longer be misreported as
  "worktree kept" when the worktree is actually already gone. The
  interrupt flag run() sets is left untouched by the wrap.
- WorktreeLockedException gets its own worktreeKeptReason wording
  ("it is locked (reason)") instead of its raw exception message, and
  its constructor is widened to public alongside the two from the
  previous round, for the same consistency reason.
- A blank branch name is no longer reported as BranchResult.DELETED.
@jbachorik @claude
Merge into base now finishes the worktree and says so
8084b82 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Report merge-and-finish stages honestly, and never as failures
5b43b57 
Fix round 1 of the Task 6 review:

- the modal-layer fallback no longer announces "finished" from a progress
  render (it fired up to three times per flow, first before the merge
  started); it now relays the outcome's own headline
- one home for the guard against a synchronous throw from an async call
  (AsyncCalls), applied to the Finish panel's three probes so a click
  during shutdown cannot strand the busy modal
- handoffDelete re-resolves the tab before touching a header, and drops a
  "Removed" pill that was negated in the same FX pulse
- a cleanup that never ran reports "Merged", not "✗": the merge commit is
  in the base branch either way
- the merge stages' copy moves into MergeFinishDecision, with tests
- the header shows a "Merging…" pill, so the in-flight guard has a
  visible reason

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Refuse the branch delete when the branch moved since the merge
b729d7e 
The merge oracle proves a merge commit of the tip recorded at pre-flight
sits on the base branch. It proves nothing about where refs/heads/<branch>
points by the time `git branch -D` runs -- and on the conflict hand-off
path minutes elapse in between, with the session's Claude running with the
worktree as its cwd. A commit landing on the branch in that window was
deleted with the ref: `-D` never refuses, `-D` drops the branch reflog and
the worktree removal drops the worktree's HEAD reflog, so the commit was
recoverable only through `git fsck --lost-found` -- while the modal
reported "✓ Merged feat/x into main · branch feat/x deleted".

The destructive step now re-reads the branch tip immediately before it
runs and compares it to the recorded one. On any difference -- including a
re-read that failed, which is treated as drift rather than as an
assumption of no drift -- the cleanup runs with a KEEP_MOVED plan: the
worktree is still removed (safe: the commits are on the branch, which
survives) and the report says "branch feat/x kept — it moved since the
merge" rather than borrowing KEPT_NOT_OURS's false "(already existed)".

WorktreeSessionCleanup takes a BranchDeletePlan instead of a boolean, so
the two reasons a branch is kept cannot collapse into one sentence. The
Finish panel's own Delete passes forRequestedDelete: it is what the user
asked for outright, with no merge oracle behind it to invalidate.

Tests: the decision's four arms and the new copy verbatim, the cleanup
never passing a moved branch to git, and one end-to-end test against real
git -- verified merge, another commit on the branch, cleanup -- asserting
the ref and the commit only it reaches both survive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Address the rest of the final whole-branch review
142a67f 
Items 2-12 of the final review, none of them behavioural risks on their own
but several of them lies the code was telling:

- The conflict hand-off's *success* path had no test: it is the only path
  where the merge commit comes from `git commit` rather than `git merge`.
- isWorktreeClean reported a git failure (worktree directory deleted
  outside the app, corrupt index) as "the worktree has uncommitted
  changes", defeating the Finish panel's deliberate default-to-clean one
  layer up. It now throws; `remove`'s internal check is unchanged, since
  there a failure legitimately means "don't claim it's clean".
- The locked-worktree comment claimed the string named a force escape
  hatch. It did not, and none is reachable for a worktree that still has
  its session, so the comment and the test name were corrected instead of
  inventing copy the user could not act on.
- Dismissing the progress modal with Esc made it pop back up at the next
  stage, which reads as broken. A dismissal is now remembered and only the
  terminal render may re-show.
- Three javadoc blocks still described the pre-branch behaviour ("merge and
  delete run directly; only PR creation hands off to Claude").
- The terminal modal's button label now comes from the decision -- "Close"
  for a stop, "Done" for a success -- and the tab-less hand-off refusal is
  the last piece of this flow's copy to move into MergeFinishDecision with
  a verbatim test. The spec's "Closing session ..." caption was dropped
  rather than rendered; the reasoning is in the spec and the report.
- dirty vs worktreeClean on the panel context are documented as the
  non-negations they are, so a future simplification cannot re-introduce
  the permanently-disabled Merge.
- The interrupt test ordered its interrupt with a fixed sleep; it now waits
  for a marker the fake git writes when the branch-delete stage begins,
  closes the WorktreeService it leaked, and pins the interrupt-flag
  preservation instead of leaving it to inspection.
- Finish button clicked on a header rebuilt mid-flow was swallowed
  silently; it now shows the "Merging..." pill.
- The "(detached)" header sentinel could reach user copy as if it were a
  branch name; it is refused up front with its own honest message.

Spec: corrected the handoffDelete pill/delay paragraph, recorded `merge
-s ours` as an accepted limitation of the parent-set check rather than
implying it defends against it, and added the tip-drift limitation item 1
fixed.

Report (untracked, .superpowers is gitignored):
.superpowers/sdd/2026-07-25-merge-and-finish/final-fix-report.md

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Pin "the branch tip is read again" with a test instead of a comment
b19e49e 
The gate in front of `git branch -D` compared a tip recorded at pre-flight
with one read at the destructive step, minutes later on the conflict
hand-off path. That the second tip is actually read again -- rather than the
recorded one being passed twice -- was the property keeping a commit made
during the hand-off from being force-deleted, and nothing tested it: the
decision's own tests supply both tips, and so does the end-to-end drift
test, so the substitution would have kept every one of them green.

Extract the step into BranchDeleteGate, FX-free like the two classes it
sits between, and test it with a probe that answers with a different tip
than the recorded one. Weakening the gate to compare the recorded tip with
itself now fails four of its six tests.

Also give the test fixtures a repo-local git identity. WorktreeService.merge
writes a merge commit, and the fixtures relied on the host's ambient one.
That is not the CI hazard it first looks like -- git derives an identity
from the OS account when none is configured, on a runner as much as on a
laptop -- but it does break under `user.useConfigOnly=true`, where six tests
fail with git exiting 128 and a message pointing nowhere near the cause.
Verified both directions against a clone with no global or system config.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Run the test suite on every push and pull request
f2562a1 
The suite had no automation: 484 tests that ran only when someone
remembered to type the command, on a branch that had just added a feature
which removes worktrees and force-deletes git branches.

macOS runner, because the product is macOS-only and the git-backed tests
spawn the same `git` the app does. No submodule checkout: third_party/
ghostty is read only by buildGhosttyNative, a lazily registered task that
:app:test never realises -- verified by configuring the build against an
empty submodule directory, then running the full suite green in a clone
that had none.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik @claude
Wrap the .app bundle into a real .dmg (Stage 4)
586d169 
`./gradlew dmg` was a registered no-op that failed with a "not
implemented yet" message. It now builds build/dist/Drydock.dmg: the
ad-hoc-signed Drydock.app staged next to an /Applications symlink and
compressed into a read-only UDZO image via hdiutil. It depends on
appImage, so the bundle is built and wrapped in one step.

DmgTask takes the .app bundle as its input rather than the dist root
that contains it. The root is where the .dmg is written, so declaring
it would make the task dirty its own input and never go up-to-date --
which is exactly what the first cut did (Gradle: "Input property
'distDir' file .../Drydock.dmg has changed"). The bundle is also all
the task actually reads.

Ad hoc signing only, so the image runs locally but still trips
Gatekeeper on another machine; Developer ID signing and notarization
(Stages 5-6) stay out of scope. Docs updated accordingly -- including
architecture.md, which still described appImage/macApp as no-ops after
Stage 3 had already landed.

Verified by hand (buildSrc has no test harness): dmg builds, second
run is UP-TO-DATE, hdiutil verify passes, and the image mounts with
Drydock.app plus the /Applications symlink, the bundle's ad-hoc
signature (io.btrace.drydock) intact after the copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbachorik
jbachorik force-pushed the feat/merge-finish-followups branch from 1dfab46 to 586d169 Compare July 27, 2026 10:09
@jbachorik
jbachorik merged commit 586d169 into main Jul 27, 2026
@jbachorik
jbachorik deleted the feat/merge-finish-followups branch July 27, 2026 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jbachorik