Commit aefd5bb
authored
Measure a worktree against where it lands, not the repo default (SPEC-51) (#160)
* Measure a worktree against where it lands, not the repo default (SPEC-51)
A worktree stacked on another worktree's branch reported its parent's work as
its own: `repo_service` handed the repo default to `diffStat`/`commitsAhead` for
every worktree, and the base the user picked at creation time was passed once to
`git worktree add` and then discarded. A child with 3 lines on top of a 20-line
parent showed +23.
Model the *target* — where the work lands — instead of the base. One field feeds
the diff (`target...HEAD`, i.e. what a PR would contain), the ahead-count
fallback, `gh pr create --base`, and the branch a wrap-up fast-forwards. Being
three-dot, git finds the merge base live, so no fork point is stored and the
diff self-heals once a parent lands.
The contract:
- resolveTargetBranch owns precedence: primary/detached → null, then an OPEN
PR's baseRefName (which inherits GitHub's auto-retargeting for free), then the
persisted choice, then the repo default. Default-last means upgrading moves
nobody's numbers until they choose.
- Renaming a branch repoints every worktree that lands in it.
- Wrapping up hands its target down, recursively, so a stack that lands
bottom-up collapses to where it actually landed.
- A target that vanishes without a wrap-up falls back through the chain to the
default and *says so*, until the user picks one explicitly.
- A live PR's base is adopted into the persisted value, so closing or reopening
no longer reverts to a pre-PR value.
DiffStat gains `targetResolved`, because the failure mode is not a zero: when
the committed leg fails, working-tree files still count, so an unresolvable
target yields a plausible *small* number. Clients suppress the pill rather than
publish a partial count.
The target is create-time config, so it spends no first-glance space. Three
disclosures share one picker: worktree actions (canonical — the only
per-worktree menu, and the only one that exists without a session), the `Ship
it` caret menu, and a `branch ≫ target` line in the detail sheet header. "Lands
in" is enabled where Rename is blocked, because retargeting an open PR is a
first-class operation.
`base` → `target` throughout, with two documented exceptions: `baseRefName`
(GitHub's own field) and one-release wire aliases on worktree.create/wrapUp.
Three bugs found only by driving the real app: the picker was dead in a repo
with no remote (the on-remote rule is vacuous without one), a merged PR kept
overriding the user's choice, and the retarget announcement stole the composer
strip's headline from an actionable fact.
Server 1386 tests, app 333 across the touched suites.
* Harden the lands-in feature against the review's edge cases (SPEC-51)
Address the failing CI guard and the ten open review threads on #160.
Failing check:
- lands_in_picker: post the retarget failure to the StatusCenter
(ref.status.failure) instead of a raw showSnackBar, so it lands on the
Activity record — satisfies the no-snackbar guard test.
Server:
- git.diffStat: zero the counts when targetResolved is false, so a consumer
that forgets the flag degrades to "nothing", not a plausible partial reading.
- git.closestAncestorBranch: one parallel `rev-list --left-right --count` per
candidate instead of two serial calls each (bounded), killing the O(2N)
serial subprocess fan-out on picker open.
- git.listRemoteBranchNames: scope to refs/remotes/origin — a branch that
exists only on another remote is not a valid `gh` PR base.
- repo_service.repairVanishedTargets: only touch THIS repo's worktree paths
(was corrupting other repos' persisted targets from the global store), and
treat origin branches as live so a just-adopted remote-only PR base is not
clobbered back to the default. Extracted the pure core, repointVanishedTargets.
- repo_service.listRepos: wire pruneTargets against the union of live worktree
paths, guarded against a transient git failure (a git repo reporting zero
worktrees aborts the sweep). pruneTargets was dead code.
- worktree-target-store.saveTargets/putTarget: return whether the write landed;
manager.setWorktreeTarget now throws on a failed persist instead of acking a
success the next snapshot contradicts.
- manager.removeWorktree: clear the target under the canonical (resolved) path.
- ws/commands/worktree: fix the misleading setTarget ordering comment, collapse
the nested target/base ternaries, drop the "" default; alias the shadowed
targetCandidates import in the manager.
App:
- pr_signals: gate the Ship it / Create PR CTA on targetResolved — an
unresolvable target has nowhere to land and would open the PR against the
wrong base.
- pr_detail._open: open the re-derived live PR url, not the stale widget field
(was throwing a null assertion when the live PR appeared).
- repo_chips onRun / worktree_actions: re-derive the live worktree at
invocation time so a remedy or picker acts on today's target, not the value
captured when the sheet opened.
* Close the second review round on the lands-in feature (SPEC-51)
Fix the failing status-lifetime guard and the six open review threads on #160.
Failing check + thread 6 (same root cause):
- lands_in_picker: hoist `final status = ref.status` above the picker await, so
a widget disposed while the picker is open can't throw on a defunct ref and
lose the target change. Reworded the comment to drop the word "await", which
was tripping the guard's own textual scan.
App:
- pr_detail: when the snapshot knows the worktree but its PR is now null, use
that null (no PR) instead of falling back to the stale open-time `this.pr` —
the old `??` resurrected a closed PR's title and GitHub link (a null-assert
waiting to happen). Regression test added.
- worktree_actions: wrap the bottom-sheet body in a Consumer so `ref.watch`
subscribes inside the sheet's own element — the outer WidgetRef rebuilt the
caller, never the open sheet, so its target/guards froze at open-time.
Server:
- repo_service.listRepos: revert the pruneTargets sweep. It was a write in a
read path that could delete real targets on a transient `isGitRepo`/worktree-
enumeration failure and race a concurrent create. Stale entries are already
harmless (removeWorktree clears, createWorktree overwrites a reused path, a
vanished target surfaces as targetResolved:false), so the sweep bought nothing
worth that risk. Removed collectLivePathsForPrune + its tests.
- manager._handDownTarget: include `origin` branches in the live set (like
repairVanishedTargets), so a wrap-up whose fetch didn't land doesn't drag every
child worktree onto the repo default instead of the branch its PR targets.
Regression test drives it through a remote-only landing branch.
* Scope target hand-down and rename to the repo that triggered them (SPEC-51)
Third review round (macroscope + coderabbit both High/Major): the worktree
target store is GLOBAL across every project, but two writers matched entries by
branch name alone. Branch names are not unique across repos (main, dev, develop,
a shared feature name), so:
- manager._handDownTarget wrapped up `develop` in one repo and silently
retargeted every `develop`-bound worktree in *other* repos — moving their diff
and future PR base, and stamping a bogus retargetedFrom announcement.
- worktree-target-store.renameTargetBranch (via renameWorktreeBranch) rewrote a
same-named target in unrelated repos on any branch rename.
Both now scope to the triggering repo's own worktree paths (`trees`, already
loaded). renameTargetBranch takes an optional `scope` set; _handDownTarget keeps
its cheap global pre-check but filters `affected` to this repo. Regression tests
seed a foreign-repo entry and assert it is left untouched (verified they fail
without the scoping).
* Clear the third review round in full — real findings, nits, and off-diff (SPEC-51)
Fixes everything valid across macroscope + coderabbit + open-code-review, plus
the pre-existing (off-diff) session-lifecycle bugs macroscope surfaced.
Staleness pattern (the round-2 fix, completed everywhere):
- session_pr_chip, pr_bar, desktop_sidebar: re-derive the live worktree/status/pr
at invocation time instead of the value captured when the sheet/menu opened.
- All re-derivation paths (repo_chips too) now guard `context.mounted` before
touching `ref`, so a row removed by a snapshot mid-sheet can't throw on a
defunct ref (and can't resurrect a stale PR: pr uses at.worktree.pr when known).
Target-store correctness:
- repairVanishedTargets: liveness is now local refs ∪ OPEN-PR bases, not every
refs/remotes/origin/* ref — a stale ref left after a merged branch is
auto-deleted no longer blocks the repair, while a remote-only PR base is still
protected. adoptLivePrTargets now skips `stale` PRs (no overwriting a fresh
user target with unverified data during a GitHub outage).
- _handDownTarget: liveness is local refs ∪ the explicit landedIn, for the same
reason (drops the all-remotes dependency).
- repointVanishedTargets: never persists a self-target (mutual-stack edge).
- Persistence failures are no longer swallowed: saveTargets’ result propagates
through clearTarget/renameTargetBranch/pruneTargets; createWorktree,
removeWorktree and _handDownTarget log when a best-effort persist fails.
- target_candidates: previews only SELECTABLE candidates (skip off-remote), so a
disabled local-only branch can't consume a preview slot.
Off-diff (pre-existing) session lifecycle:
- createSession kills a half-started adapter if start() rejects (was leaking a
live child, unlike the reattach path).
- toSessionListItem no longer reports a closed session as `attached`.
- attachPiSession reopens + relives a closed session instead of returning its
DetachedAdapter (which launched no process).
Nits: reattached two orphaned JSDoc blocks; hasStale→hasCandidates; cached the
double lastKnown lookup; lazy ListView.builder in the picker; ellipsis on the
menu's target-branch text; TODO(SPEC-51) markers on the baseBranch shims; doc +
naming-collision comments; import grouping; mockup `font: … inherit` → var(--sans).
Tests: cross-repo isolation already covered; added hasAnyRemote, the worktree
group, asymmetric hasPreview, an actual open-PR "gone" signal, the retarget
command assertion, a settle() deadline, and updated the repoint unit tests for
the new liveness + self-target guard. Server 1398 pass; flutter analyze clean.
* Fix post-merge HIGH threads: race safety, session lifecycle, hasOriginRemote
* Settle the target-liveness trade-off, coalesce revivals, honour the override
Second post-merge review round (macroscope HIGH ×2 + Medium, CodeRabbit Major).
- repo_service.repairVanishedTargets: `origin` refs are live again, and the
trade-off is now documented in place so it stops flip-flopping. Excluding them
silently REDIRECTS a worktree whose target lives only on the remote (open PR
into a remote-only `release` → PR closes → target rewritten to the default,
moving every future diff and PR base). Including them only DELAYS a repair
until the next `fetch --prune`, which surfaces as the honest
`targetResolved: false`. A delayed repair beats an unrecoverable redirect.
Regression test drives a remote-only target (verified it fails without the fix).
- manager.attachPiSession: reviving a closed session now goes through the same
`attachInFlight` dedupe as a fresh attach. `reopenSession` clears `closed`
before `reattachSession` finishes `start()`, so an uncoalesced second caller
saw `closed === false`, returned early, and could send to an uninitialised
adapter.
- target_candidates: takes the stored `defaultBranchOverride` and resolves via
`resolveDefaultBranch`, mirroring `repoSnapshot`. The picker was labelling and
ranking git's own answer as `default` while every diff and new worktree used
the override — the picker disagreeing with the app about what "default" means.
- lands_in_picker: `candidatesFuture.ignore()` before the modal opens. The
request is in flight before the route's first build and the user can dismiss
before it lands, so a rejection reached no listener and escaped to the zone
handler. Same pattern as `_NewWorktreeDialogState._loadPrs`; the FutureBuilder
still renders the error state.
Server 2143 pass; flutter analyze clean.
* Make a refused target-rename write distinguishable from a no-op
`renameTargetBranch` returned 0 both for "nothing pointed at the old name" (a
success) and for "the store is not writable" (a silent divergence where every
dependent worktree keeps aiming at a branch that no longer exists). It now
returns `number | null`, and `renameWorktreeBranch` logs the refusal — git has
already renamed the branch by then, so this cannot fail the operation, but it
must not pass unrecorded.
Adds the two test gaps CodeRabbit flagged: `scope` coverage (a rename in one repo
leaving another repo's same-named target alone) and the null-vs-0 contract.
* One live-PR predicate, a pinned test store, and a spec correction
- repo_service: extract `isLivePr` (a type guard, so it narrows) — two sites
encoded "exists && OPEN && not stale" independently, and a change to one would
silently diverge from the other.
- target_rules fixture: pin MAKIT_WORKTREE_TARGETS_FILE. `worktreeTargetsFile()`
prefers it over MAKIT_HOME, so a value inherited from another suite would point
these tests at a shared store.
- SPEC-51: the wire aliases read `||`, not `??` (an empty string must fall
through), and the wrapUp fallback is `defaultBranchFor()` post-merge.
* Handle the origin/-qualified default branch on both read paths
`resolveDefaultBranch` deliberately returns a remote-only default QUALIFIED
(`origin/release`) because git cannot resolve a bare name against
`refs/remotes/origin/`. Two consumers assumed the bare form:
- repo_service.repairVanishedTargets built `live` from `listRemoteBranchNames`,
which STRIPS the prefix — so `live` held `release` while `defaultBranch` was
`origin/release`. `resolveThroughChain` rejected a perfectly live default as
"gone" and skipped the repair, leaving the worktree on a broken target. It now
records both spellings.
- target_candidates built the candidate list from local branches only, so a
remote-only default was omitted entirely and NO candidate received the
`default` group — the picker could not offer the branch every diff and new
worktree measures against, recreating the exact disagreement the
override-threading was meant to fix. It is now offered, and marked `onRemote`
(it is on the remote by definition).
Coverage note: the picker test fails without its fix (verified). The
`repointVanishedTargets` unit tests pin the CONTRACT (the core honours a
qualified default; a default absent from `live` is never invented) but pass
`live` in by hand, so they do not by themselves cover the caller's `live`
construction — the existing "a target that still exists on origin is NOT repaired
away" test is what exercises that path.1 parent e69d750 commit aefd5bb
50 files changed
Lines changed: 7673 additions & 268 deletions
File tree
- .pnpm-store/v11
- projects
- app
- lib
- desktop/chat
- store
- ui
- home
- session
- widgets
- test
- desktop
- store
- ui
- home
- widgets
- tool
- docs/specs
- mockups
- server
- src
- ws/commands
- test/ws
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Binary file not shown.
This file was deleted.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| |||
647 | 648 | | |
648 | 649 | | |
649 | 650 | | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
| 663 | + | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
650 | 672 | | |
651 | 673 | | |
652 | 674 | | |
| |||
665 | 687 | | |
666 | 688 | | |
667 | 689 | | |
668 | | - | |
| 690 | + | |
| 691 | + | |
| 692 | + | |
669 | 693 | | |
670 | 694 | | |
671 | 695 | | |
| |||
836 | 860 | | |
837 | 861 | | |
838 | 862 | | |
839 | | - | |
840 | | - | |
| 863 | + | |
| 864 | + | |
| 865 | + | |
| 866 | + | |
| 867 | + | |
| 868 | + | |
841 | 869 | | |
842 | 870 | | |
843 | 871 | | |
| |||
856 | 884 | | |
857 | 885 | | |
858 | 886 | | |
| 887 | + | |
| 888 | + | |
| 889 | + | |
| 890 | + | |
| 891 | + | |
859 | 892 | | |
860 | 893 | | |
861 | 894 | | |
| |||
891 | 924 | | |
892 | 925 | | |
893 | 926 | | |
| 927 | + | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
| 937 | + | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
| 942 | + | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
| 948 | + | |
| 949 | + | |
| 950 | + | |
| 951 | + | |
| 952 | + | |
| 953 | + | |
| 954 | + | |
| 955 | + | |
| 956 | + | |
| 957 | + | |
| 958 | + | |
| 959 | + | |
| 960 | + | |
| 961 | + | |
| 962 | + | |
| 963 | + | |
| 964 | + | |
| 965 | + | |
| 966 | + | |
| 967 | + | |
| 968 | + | |
| 969 | + | |
| 970 | + | |
894 | 971 | | |
895 | 972 | | |
896 | 973 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
65 | | - | |
| 65 | + | |
66 | 66 | | |
67 | 67 | | |
68 | 68 | | |
| |||
81 | 81 | | |
82 | 82 | | |
83 | 83 | | |
84 | | - | |
| 84 | + | |
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
| |||
115 | 115 | | |
116 | 116 | | |
117 | 117 | | |
118 | | - | |
| 118 | + | |
119 | 119 | | |
120 | | - | |
| 120 | + | |
121 | 121 | | |
122 | 122 | | |
123 | 123 | | |
124 | 124 | | |
125 | 125 | | |
126 | 126 | | |
127 | 127 | | |
128 | | - | |
129 | | - | |
| 128 | + | |
| 129 | + | |
130 | 130 | | |
131 | 131 | | |
132 | 132 | | |
| |||
148 | 148 | | |
149 | 149 | | |
150 | 150 | | |
151 | | - | |
| 151 | + | |
152 | 152 | | |
153 | 153 | | |
154 | 154 | | |
| |||
200 | 200 | | |
201 | 201 | | |
202 | 202 | | |
203 | | - | |
| 203 | + | |
204 | 204 | | |
205 | 205 | | |
206 | 206 | | |
| |||
499 | 499 | | |
500 | 500 | | |
501 | 501 | | |
502 | | - | |
503 | | - | |
| 502 | + | |
| 503 | + | |
504 | 504 | | |
505 | 505 | | |
506 | 506 | | |
| |||
512 | 512 | | |
513 | 513 | | |
514 | 514 | | |
515 | | - | |
| 515 | + | |
516 | 516 | | |
517 | 517 | | |
518 | 518 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
| 7 | + | |
7 | 8 | | |
8 | 9 | | |
9 | 10 | | |
| |||
111 | 112 | | |
112 | 113 | | |
113 | 114 | | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
114 | 119 | | |
115 | 120 | | |
116 | 121 | | |
| |||
121 | 126 | | |
122 | 127 | | |
123 | 128 | | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
124 | 132 | | |
125 | 133 | | |
126 | 134 | | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | | - | |
136 | | - | |
137 | | - | |
138 | | - | |
139 | | - | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
140 | 163 | | |
141 | 164 | | |
142 | 165 | | |
| |||
269 | 292 | | |
270 | 293 | | |
271 | 294 | | |
272 | | - | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
273 | 302 | | |
274 | 303 | | |
275 | 304 | | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
276 | 310 | | |
277 | 311 | | |
278 | 312 | | |
| |||
316 | 350 | | |
317 | 351 | | |
318 | 352 | | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
319 | 360 | | |
320 | 361 | | |
321 | 362 | | |
| |||
0 commit comments