Measure a worktree against where it lands, not the repo default (SPEC-51) - #160
Conversation
…-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.
📝 WalkthroughWalkthroughThe PR introduces persisted worktree target branches across the server and Flutter app. It adds target resolution, candidate ranking, retargeting commands, unresolved-target handling, live PR state refreshes, and “Lands in” picker surfaces with compatibility aliases for legacy fields. ChangesWorktree target branch flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LandsInPicker
participant WebSocket
participant SessionManager
participant RepositorySnapshot
User->>LandsInPicker: select target branch
LandsInPicker->>WebSocket: worktree.setTarget
WebSocket->>SessionManager: validate and persist target
SessionManager-->>WebSocket: updated target
WebSocket->>RepositorySnapshot: broadcast snapshot
RepositorySnapshot-->>LandsInPicker: refreshed target and diff state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5512f7be-c067-4711-8071-cff1429fd7ac) |
|
@macroscope-app review |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Review in progress. Results will be posted as check runs when complete:
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 10 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ec166cd9-ae97-499b-96f6-37db881c09f4) |
|
All ten review threads addressed in d284fde: Server
App
Plus the failing no-snackbar guard: |
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.
|
Second review round addressed in 83eb2e7: Failing check + thread 6 (same root cause) — pr_detail (thread 1) — a worktree whose PR went null now shows no PR instead of falling back to the stale worktree_actions (thread 5) — the bottom-sheet body is wrapped in a repo_service prune (threads 2 & 3) — reverted the _handDownTarget (thread 4) — now includes Server 1393 tests + |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f58cb9f8-b23e-4980-93c0-4be7f765957d) |
…EC-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).
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3513d322-de52-4f08-92a9-ce4d8ac399e7) |
…iff (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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_055c5399-0a29-44f9-8353-107dc64cfcc2) |
|
Ran macroscope + coderabbit + open-code-review again and fixed all remaining valid findings, nits, and pre-existing off-diff issues in fbcad33:
The two still-open macroscope threads are outdated and addressed here: |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7122efcc-d6e7-4d67-bc0b-df52800e9540) |
069666e to
239150a
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bbb32578-73ad-40c6-9a83-c783f6c30363) |
|
Macroscope has since reviewed this pull request. An earlier review was skipped by a cost limit; a review has now completed, so that notice no longer applies. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ca25be9b-a17d-47e6-b33a-19c4ec26900b) |
Fixed all 4 post-merge HIGH threads (commit b732934)Thread 1 (toSessionListItem): Closed sessions now correctly show Thread 2 (ensureLive): Swallows Thread 3 (hasAnyRemote): Renamed to Thread 4 (race condition): Implemented compare-and-set in Tests: +5 new regression tests; 2142 server tests pass, app analyzes clean. Ready to merge. |
|
All four post-merge HIGH findings fixed in
Also in this round: Server 2142 tests pass; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/ui/widgets/pr_signals.dart (1)
303-320: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRename
PrResidue.targetBranch/targetBehindto name the primary checkout.
PrResidue.targetBranchholds the primary checkout's branch.prStatus(targetBranch:)holds this worktree's landing target. The two are different concepts with one name in one file. The comment at Lines 849-853 exists only to defend that collision. A future edit can wirew.targetBranchintoPrResidue.targetBranchand produce a wrong " is N behind" fact with no compile error.Rename the fields to
primaryBranchandprimaryBehind. Update the construction at Lines 854-855, the signal at Lines 402-405, and the fixtures inapp/tool/pr_bar_demo.dartLines 153-168.♻️ Proposed rename
const PrResidue({ this.sessions = 0, - this.targetBranch, - this.targetBehind = 0, + this.primaryBranch, + this.primaryBehind = 0, });/// The branch checked out in the primary checkout, for the fact's wording. /// Null when there is no primary worktree in the snapshot, or it is detached. - final String? targetBranch; + final String? primaryBranch; /// How far that branch trails its upstream. - final int targetBehind; + final int primaryBehind;Then at Lines 402-405:
- if (residue.targetBranch != null && residue.targetBehind > 0) + if (residue.primaryBranch != null && residue.primaryBehind > 0) PrSignal( - '${residue.targetBranch} is ' - '${_plural(residue.targetBehind, 'commit')} behind', + '${residue.primaryBranch} is ' + '${_plural(residue.primaryBehind, 'commit')} behind', PrTone.quiet, ),And at Lines 849-855 the naming caveat comment becomes unnecessary:
- // NB: `PrResidue.targetBranch` is NOT the same concept as the - // `prStatus(targetBranch:)` argument above. That one is THIS - // worktree's own landing target (drives the "nowhere to land" - // signal); this is the PRIMARY checkout's branch, named only so the - // "<main> is N behind" residue fact reads naturally. - targetBranch: primary?.branch, - targetBehind: primary?.behindCount ?? 0, + primaryBranch: primary?.branch, + primaryBehind: primary?.behindCount ?? 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/ui/widgets/pr_signals.dart` around lines 303 - 320, Rename PrResidue.targetBranch and PrResidue.targetBehind to primaryBranch and primaryBehind, updating all reads and writes including the signal, construction logic, and pr_bar_demo.dart fixtures. Preserve the existing primary-checkout semantics, and remove the naming-collision caveat comment that becomes obsolete after the rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/ui/widgets/lands_in_picker.dart`:
- Around line 207-211: Update the error handling around status.failure to import
status_event.dart directly and replace the StatusSources reference with
StatusSources.worktree, ensuring the worktree source resolves without relying on
status_providers.dart re-exports.
- Around line 178-198: Attach an error handler to the future returned by
store.targetCandidates before constructing or opening the modal, following the
existing future.ignore() pattern used by _NewWorktreeDialogState._loadPrs.
Preserve the same future for _LandsInPickerBody so FutureBuilder continues to
render its result or error state.
In `@app/test/desktop/desktop_sidebar_test.dart`:
- Around line 1046-1074: Extend the test “selecting it opens the picker (dialog,
not a sheet)” to retain the _FakeStore returned by _pumpWithStore, use a
worktree target different from the candidate (for example feat/parent), tap the
landsInCandidate-main row, and assert store.retargets records the expected
setWorktreeTarget call and path.
In `@app/test/store/pull_request_model_test.dart`:
- Around line 136-160: Add a WrapUpReport.fromJson test covering payloads
containing conflicting targetBranch/baseBranch aliases, asserting the intended
targetBranch value takes precedence and that corresponding target fields remain
consistent. Rename the existing tests around the targetUpdated false case
(including the later test near line 167) to use “target” rather than “base,”
matching the fields they verify.
In `@app/test/store/target_candidate_test.dart`:
- Around line 68-70: Expand the “a candidate without a branch is rejected” test
to assert the omitted-field defaults from TargetCandidate.fromJson: onRemote is
true, isSelf is false, and selectable is true. Preserve the existing rejection
assertion while locking these wire-contract defaults.
In `@app/test/ui/home/worktree_actions_test.dart`:
- Around line 388-414: Add a widget test alongside the existing picker test
using a worktree whose target branch already matches the selected candidate,
then confirm that `store.retargeted` remains empty after selection. Update the
fake’s `targetCandidates` implementation to record or expose its `projectId` and
`worktreePath` arguments, and assert the picker requests candidates for the
expected worktree.
In `@docs/specs/2026-08-11-SPEC-51-target-branch.md`:
- Around line 68-76: Update Rule 1 in the specification to document that
worktree.create and worktree.wrapUp read env.targetBranch || env.baseBranch,
matching the server implementation. Preserve the stated fallback behavior for
explicit empty strings and the existing one-release alias requirements.
In `@server/src/manager.ts`:
- Around line 967-975: Handle the return value from renameTargetBranch at the
call site after building scope, and log a warning when it indicates persistence
failed. Preserve the existing scoped rename behavior, and use the same
warning/logging convention as other target writes in this file so refused saves
are recorded.
In `@server/src/repo_service.test.ts`:
- Around line 256-277: Add a test case to the existing `resolveTargetBranch:
only an OPEN pull request outranks the persisted value` test that sets
`prBaseRefName` equal to `args.branch` with `prState: "OPEN"`, and assert the
function returns the worktree branch value. Cover this live PR-base path without
changing the existing merged and closed expectations.
In `@server/src/repo_service.ts`:
- Around line 107-118: Extract a shared live-PR predicate for the condition that
a PR exists, is not stale, and has an OPEN state, then reuse it in the
persistence loop and repairVanishedTargets instead of duplicating the checks.
Keep resolveTargetBranch’s bare-state check separate because it has no stale
metadata, while preserving its OPEN-state behavior.
In `@server/src/target_candidates.test.ts`:
- Around line 223-237: Add a test alongside “with no remote configured, every
branch is selectable” that creates a repository with only a non-origin remote
such as “upstream” and verifies every branch remains selectable. Run it first to
capture the failure, then update targetCandidates and its remote-state handling
so a non-origin-only remote is treated correctly rather than marking all
candidates onRemote: false; preserve existing origin and no-remote behavior.
In `@server/src/target_candidates.ts`:
- Around line 117-166: The remote check in target candidate construction must be
scoped to origin, matching listRemoteBranchNames, rather than any configured
remote. In server/src/target_candidates.ts lines 117-166, replace hasAnyRemote
with an origin-specific check so upstream-only repositories treat candidates as
onRemote; in server/src/target_candidates.test.ts lines 223-237, add a sibling
test configuring only upstream and assert no candidate is blocked.
In `@server/src/target_rules.test.ts`:
- Around line 186-200: Update the healthy-repository test around the existing
loadTargets assertion to capture the worktree targets file contents before and
after the manager operations using readFileSync, then assert the bytes are
identical. Retain the existing target and retargetedFrom assertions, and add
readFileSync to the node:fs imports.
- Around line 33-66: Update the fixture() setup to redirect
MAKIT_WORKTREE_TARGETS_FILE to a file under the temporary home directory, saving
its previous value alongside prevHome and prevWt. Restore or delete that
environment variable in cleanup so worktreeTargetsFile() cannot access the
developer’s persisted target store.
In `@server/src/worktree-target-store.test.ts`:
- Around line 279-293: Add unit coverage in the renameTargetBranch test near the
existing same-name target setup by passing a scope argument and asserting only
worktrees within that scope are repointed. Keep an identically named target from
an unrelated scope unchanged, directly pinning renameTargetBranch’s scope guard
in the store contract.
In `@server/src/worktree-target-store.ts`:
- Around line 110-126: Update saveTargets to fsync the temporary file’s contents
before renameSync, using the existing tmp-file write flow and ensuring the file
descriptor is properly closed before the rename. Preserve the current atomic
rename, cleanup behavior, and boolean error handling.
---
Outside diff comments:
In `@app/lib/ui/widgets/pr_signals.dart`:
- Around line 303-320: Rename PrResidue.targetBranch and PrResidue.targetBehind
to primaryBranch and primaryBehind, updating all reads and writes including the
signal, construction logic, and pr_bar_demo.dart fixtures. Preserve the existing
primary-checkout semantics, and remove the naming-collision caveat comment that
becomes obsolete after the rename.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cea4a73-e5e3-4ac0-97fb-0e5002470b02
📒 Files selected for processing (48)
app/lib/desktop/chat/desktop_sidebar.dartapp/lib/desktop/chat/new_worktree_dialog.dartapp/lib/desktop/chat/pr_bar.dartapp/lib/store/models.dartapp/lib/store/store.dartapp/lib/ui/home/new_session_sheet.dartapp/lib/ui/home/repo_card.dartapp/lib/ui/home/repo_chips.dartapp/lib/ui/home/start_session.dartapp/lib/ui/home/worktree_actions.dartapp/lib/ui/home/worktree_row.dartapp/lib/ui/session/session_pr_chip.dartapp/lib/ui/widgets/lands_in_picker.dartapp/lib/ui/widgets/pr_detail.dartapp/lib/ui/widgets/pr_signals.dartapp/lib/ui/widgets/wrap_up.dartapp/test/desktop/desktop_sidebar_test.dartapp/test/desktop/keymap_scope_test.dartapp/test/desktop/new_worktree_dialog_test.dartapp/test/store/pull_request_model_test.dartapp/test/store/target_candidate_test.dartapp/test/store/worktree_target_test.dartapp/test/ui/home/repo_card_test.dartapp/test/ui/home/worktree_actions_test.dartapp/test/ui/home/worktree_row_target_diff_test.dartapp/test/ui/widgets/pr_detail_live_test.dartapp/test/ui/widgets/pr_signals_target_test.dartapp/test/ui/widgets/pr_signals_test.dartapp/tool/pr_bar_demo.dartdocs/specs/2026-08-11-SPEC-51-target-branch.mdmockups/base-branch.htmlserver/src/git.test.tsserver/src/git.tsserver/src/manager.test.tsserver/src/manager.tsserver/src/pr_watcher.test.tsserver/src/protocol.tsserver/src/repo_service.test.tsserver/src/repo_service.tsserver/src/target_candidates.test.tsserver/src/target_candidates.tsserver/src/target_rules.test.tsserver/src/worktree-target-store.test.tsserver/src/worktree-target-store.tsserver/src/ws/commands/worktree.tsserver/test/ws/auto_mirror.test.tsserver/test/ws/pr_commands.test.tsserver/test/ws/worktree_set_target.test.ts
| status.failure( | ||
| 'Could not change where this lands', | ||
| error: e, | ||
| source: 'worktree', | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Resolve the StatusSources.worktree constant and compare it with the literal used here.
fd -t f 'status_.*\.dart' app/lib | xargs rg -n -C2 'class StatusSources|worktree\s*='
rg -nP --type=dart -C2 "source:\s*'worktree'|source:\s*StatusSources\.worktree" app/libRepository: leduckhc/makit
Length of output: 4377
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- imports and failure path ---'
sed -n '1,35p;185,218p' app/lib/ui/widgets/lands_in_picker.dart
printf '%s\n' '--- StatusSources declaration ---'
sed -n '30,46p' app/lib/status/status_event.dart
printf '%s\n' '--- Status source consumers and filtering ---'
rg -n -C3 --type=dart 'StatusSources\.worktree|source\s*==|source:' app/lib/status app/lib/ui/widgets/lands_in_picker.dartRepository: leduckhc/makit
Length of output: 9246
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- status_providers.dart declarations and exports ---'
sed -n '1,80p' app/lib/status/status_providers.dart
printf '%s\n' '--- StatusSources references and imports in related files ---'
sed -n '1,25p' app/lib/desktop/chat/desktop_sidebar.dart
sed -n '1,25p' app/lib/ui/home/repo_card.dart
rg -n --type=dart "import .*status_event\.dart|export .*status_event\.dart|StatusSources" app/lib/status/status_providers.dart app/lib/desktop/chat/desktop_sidebar.dart app/lib/ui/home/repo_card.dartRepository: leduckhc/makit
Length of output: 6414
Import status_event.dart and use StatusSources.worktree. status_providers.dart does not re-export StatusSources, so the direct import is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/ui/widgets/lands_in_picker.dart` around lines 207 - 211, Update the
error handling around status.failure to import status_event.dart directly and
replace the StatusSources reference with StatusSources.worktree, ensuring the
worktree source resolves without relying on status_providers.dart re-exports.
| testWidgets('selecting it opens the picker (dialog, not a sheet)', ( | ||
| tester, | ||
| ) async { | ||
| await _pumpWithStore( | ||
| tester, | ||
| repos: [ | ||
| _repo( | ||
| 'p1', | ||
| 'alpha', | ||
| worktrees: [ | ||
| _worktree( | ||
| 'wt-feat', | ||
| branch: 'feat/login', | ||
| targetBranch: 'main', | ||
| sessionIds: ['s1'], | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| sessions: [_session('s1', 'p1', 'work', 'pi')], | ||
| ); | ||
|
|
||
| await _openWorktreeMenu(tester, 'feat/login'); | ||
| await tester.tap(find.text('Lands in')); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| // The picker fetched its candidate list and rendered a row for it. | ||
| expect(find.byKey(const Key('landsInCandidate-main')), findsOneWidget); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the chosen candidate reaches setWorktreeTarget.
_FakeStore.retargets is recorded at line 112 but no test reads it. This test stops at "the picker rendered a row", so the menu → picker → command leg stays unproven: a picker that never calls setWorktreeTarget, or one that sends the wrong path, still passes.
💚 Proposed test extension
- await _openWorktreeMenu(tester, 'feat/login');
+ final store = await _pumpWithStore(
...
await tester.tap(find.text('Lands in'));
await tester.pumpAndSettle();
// The picker fetched its candidate list and rendered a row for it.
expect(find.byKey(const Key('landsInCandidate-main')), findsOneWidget);
+
+ // ...and choosing it persists the target for THIS worktree.
+ await tester.tap(find.byKey(const Key('landsInCandidate-main')));
+ await tester.pumpAndSettle();
+ expect(store.retargets, [
+ (path: '/tmp/wt/wt-feat', target: 'main'),
+ ]);Capture the return value of _pumpWithStore at line 1049 to obtain store.
Note: the fixture at line 1059 sets targetBranch: 'main', and showLandsInPicker returns early at line 199 of app/lib/ui/widgets/lands_in_picker.dart when the choice equals the current target. Give the worktree a different current target (for example feat/parent) so the selection actually dispatches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/test/desktop/desktop_sidebar_test.dart` around lines 1046 - 1074, Extend
the test “selecting it opens the picker (dialog, not a sheet)” to retain the
_FakeStore returned by _pumpWithStore, use a worktree target different from the
candidate (for example feat/parent), tap the landsInCandidate-main row, and
assert store.retargets records the expected setWorktreeTarget call and path.
| test('still decodes the legacy base* keys', () { | ||
| // A server that predates the base->target rename. Without these aliases the | ||
| // report silently loses which branch was caught up and whether it moved, | ||
| // which turns "tidied and caught up" into "tidied, base untouched". | ||
| final r = WrapUpReport.fromJson({ | ||
| 'branchDeleted': 'feat/x', | ||
| 'baseBranch': 'main', | ||
| 'baseUpdated': false, | ||
| 'baseReason': 'main has local commits that are not on origin/main', | ||
| 'baseUpdated': true, | ||
| 'baseReason': 'nope', | ||
| }); | ||
| expect(r.targetBranch, 'main'); | ||
| expect(r.targetUpdated, isTrue); | ||
| expect(r.targetReason, 'nope'); | ||
| }); | ||
|
|
||
| test('says the base was left alone when it was not fast-forwardable', () { | ||
| final r = WrapUpReport.fromJson({ | ||
| 'branchDeleted': 'feat/x', | ||
| 'targetBranch': 'main', | ||
| 'targetUpdated': false, | ||
| 'targetReason': 'main has local commits that are not on origin/main', | ||
| }); | ||
| expect(r.summary, 'Removed feat/x · main unchanged'); | ||
| expect(r.baseReason, contains('local commits')); | ||
| expect(r.targetReason, contains('local commits')); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a precedence test for the target/base alias overlap.
The new test covers base-only payloads. It does not cover a payload that carries both targetBranch and baseBranch with different values. A server in the one-release compatibility window can emit both. Without a locked precedence, a later change to WrapUpReport.fromJson can flip the winner and report the wrong branch with no failing test.
Also, the test name at Line 151 still says "the base was left alone" while the fields are target*. Use the target term there and at Line 167 so the names match the fields they assert.
💚 Proposed additional test
test('still decodes the legacy base* keys', () {
// A server that predates the base->target rename. Without these aliases the
// report silently loses which branch was caught up and whether it moved,
// which turns "tidied and caught up" into "tidied, base untouched".
final r = WrapUpReport.fromJson({
'branchDeleted': 'feat/x',
'baseBranch': 'main',
'baseUpdated': true,
'baseReason': 'nope',
});
expect(r.targetBranch, 'main');
expect(r.targetUpdated, isTrue);
expect(r.targetReason, 'nope');
});
+ test('the target* keys win when a server sends both spellings', () {
+ // The alias window lets one payload carry both. The new name is the
+ // authority; the alias is only a fallback.
+ final r = WrapUpReport.fromJson({
+ 'branchDeleted': 'feat/x',
+ 'targetBranch': 'release/2.0',
+ 'targetUpdated': true,
+ 'baseBranch': 'main',
+ 'baseUpdated': false,
+ 'baseReason': 'stale',
+ });
+ expect(r.targetBranch, 'release/2.0');
+ expect(r.targetUpdated, isTrue);
+ expect(r.targetReason, isNull);
+ });
+
- test('says the base was left alone when it was not fast-forwardable', () {
+ test('says the target was left alone when it was not fast-forwardable', () {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/test/store/pull_request_model_test.dart` around lines 136 - 160, Add a
WrapUpReport.fromJson test covering payloads containing conflicting
targetBranch/baseBranch aliases, asserting the intended targetBranch value takes
precedence and that corresponding target fields remain consistent. Rename the
existing tests around the targetUpdated false case (including the later test
near line 167) to use “target” rather than “base,” matching the fields they
verify.
| test('a candidate without a branch is rejected', () { | ||
| expect(TargetCandidate.fromJson({'group': 'other'}), isNull); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect TargetCandidate.fromJson defaults for onRemote and isSelf.
set -euo pipefail
fd -t f 'models.dart' app/lib/store | xargs -r rg -nP -C 4 'class TargetCandidate|onRemote|isSelf|selectable|blockedReason|hasPreview'Repository: leduckhc/makit
Length of output: 4035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- TargetCandidate implementation ---'
sed -n '992,1050p' app/lib/store/models.dart
printf '%s\n' '--- Relevant tests ---'
sed -n '1,130p' app/test/store/target_candidate_test.dart
printf '%s\n' '--- fromJson call sites and candidate payload references ---'
rg -n -C 3 'TargetCandidate\.fromJson|onRemote|isSelf' app/lib app/testRepository: leduckhc/makit
Length of output: 16587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("app/lib/store/models.dart").read_text()
required = [
"onRemote: j['onRemote'] != false,",
"isSelf: j['isSelf'] == true,",
"bool get selectable => !isSelf && onRemote;",
]
for text in required:
assert text in source, f"missing source rule: {text}"
def parse(payload):
on_remote = payload.get("onRemote") is not False
is_self = payload.get("isSelf") is True
return on_remote, is_self, (not is_self and on_remote)
for payload, expected in [
({"branch": "b", "group": "other"}, (True, False, True)),
({"branch": "b", "group": "other", "onRemote": False}, (False, False, False)),
({"branch": "b", "group": "other", "isSelf": True}, (True, True, False)),
]:
actual = parse(payload)
assert actual == expected, (payload, actual, expected)
print(payload, "=>", actual)
PY
printf '%s\n' '--- Candidate producers and payload construction across the repository ---'
rg -n -C 4 --glob '!app/lib/store/models.dart' --glob '!app/test/store/target_candidate_test.dart' \
'candidates|onRemote|isSelf|TargetCandidate' .Repository: leduckhc/makit
Length of output: 50370
Lock the omitted-field defaults in a test.
TargetCandidate.fromJson maps omitted onRemote to true and omitted isSelf to false, which makes the candidate selectable. Add assertions for both fields and selectable to guard the wire contract. The server currently emits both fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/test/store/target_candidate_test.dart` around lines 68 - 70, Expand the
“a candidate without a branch is rejected” test to assert the omitted-field
defaults from TargetCandidate.fromJson: onRemote is true, isSelf is false, and
selectable is true. Preserve the existing rejection assertion while locking
these wire-contract defaults.
| test("a healthy repo is never repaired, so the store is not churned", async () => { | ||
| const f = await fixture(); | ||
| try { | ||
| const parent = await branchWorktree(f, "parent", "main"); | ||
| const repos1 = await f.manager.listRepos({ includePrs: false }); | ||
| const parentBranch = repos1[0]!.worktrees.find((w) => w.path === parent)!.branch!; | ||
| const child = await branchWorktree(f, "child", parentBranch); | ||
| await f.manager.listRepos({ includePrs: false }); | ||
| const stored = loadTargets(worktreeTargetsFile())[child]; | ||
| assert.equal(stored?.target, parentBranch, "an intact target must be left alone"); | ||
| assert.equal(stored?.retargetedFrom, undefined); | ||
| } finally { | ||
| f.cleanup(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the file is unchanged, so the test matches its name.
The name says "the store is not churned", but the assertions only check the stored values. A repair that rewrote the entry with an identical target would pass. worktree-target-store.test.ts already uses byte comparison for the equivalent no-op claim; use the same check here.
💚 Proposed test change
const child = await branchWorktree(f, "child", parentBranch);
+ const before = readFileSync(worktreeTargetsFile(), "utf8");
await f.manager.listRepos({ includePrs: false });
const stored = loadTargets(worktreeTargetsFile())[child];
assert.equal(stored?.target, parentBranch, "an intact target must be left alone");
assert.equal(stored?.retargetedFrom, undefined);
+ assert.equal(
+ readFileSync(worktreeTargetsFile(), "utf8"),
+ before,
+ "a healthy repo must not rewrite the store on every snapshot",
+ );Add readFileSync to the node:fs import at Line 18.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("a healthy repo is never repaired, so the store is not churned", async () => { | |
| const f = await fixture(); | |
| try { | |
| const parent = await branchWorktree(f, "parent", "main"); | |
| const repos1 = await f.manager.listRepos({ includePrs: false }); | |
| const parentBranch = repos1[0]!.worktrees.find((w) => w.path === parent)!.branch!; | |
| const child = await branchWorktree(f, "child", parentBranch); | |
| await f.manager.listRepos({ includePrs: false }); | |
| const stored = loadTargets(worktreeTargetsFile())[child]; | |
| assert.equal(stored?.target, parentBranch, "an intact target must be left alone"); | |
| assert.equal(stored?.retargetedFrom, undefined); | |
| } finally { | |
| f.cleanup(); | |
| } | |
| }); | |
| test("a healthy repo is never repaired, so the store is not churned", async () => { | |
| const f = await fixture(); | |
| try { | |
| const parent = await branchWorktree(f, "parent", "main"); | |
| const repos1 = await f.manager.listRepos({ includePrs: false }); | |
| const parentBranch = repos1[0]!.worktrees.find((w) => w.path === parent)!.branch!; | |
| const child = await branchWorktree(f, "child", parentBranch); | |
| const before = readFileSync(worktreeTargetsFile(), "utf8"); | |
| await f.manager.listRepos({ includePrs: false }); | |
| const stored = loadTargets(worktreeTargetsFile())[child]; | |
| assert.equal(stored?.target, parentBranch, "an intact target must be left alone"); | |
| assert.equal(stored?.retargetedFrom, undefined); | |
| assert.equal( | |
| readFileSync(worktreeTargetsFile(), "utf8"), | |
| before, | |
| "a healthy repo must not rewrite the store on every snapshot", | |
| ); | |
| } finally { | |
| f.cleanup(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/target_rules.test.ts` around lines 186 - 200, Update the
healthy-repository test around the existing loadTargets assertion to capture the
worktree targets file contents before and after the manager operations using
readFileSync, then assert the bytes are identical. Retain the existing target
and retargetedFrom assertions, and add readFileSync to the node:fs imports.
| test("renameTargetBranch repoints every worktree that landed in the old name", () => { | ||
| const { dir, file } = tmpFile(); | ||
| try { | ||
| putTarget(file, "/wt/a", "feat/parent"); | ||
| putTarget(file, "/wt/b", "feat/parent"); | ||
| putTarget(file, "/wt/c", "main"); | ||
| const moved = renameTargetBranch(file, "feat/parent", "feat/renamed"); | ||
| assert.equal(moved, 2); | ||
| assert.equal(targetOf(file, "/wt/a"), "feat/renamed"); | ||
| assert.equal(targetOf(file, "/wt/b"), "feat/renamed"); | ||
| assert.equal(targetOf(file, "/wt/c"), "main", "unrelated targets are untouched"); | ||
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Cover renameTargetBranch's scope argument.
No test here passes scope. That parameter is the guard against rewriting a same-named branch target in an unrelated repo, and the store is global across projects. target_rules.test.ts covers the guard end-to-end through the manager, but the unit contract stays unpinned, so a refactor that drops the scope check would still pass this file.
💚 Proposed test
+test("renameTargetBranch only rewrites paths inside the given scope", () => {
+ const { dir, file } = tmpFile();
+ try {
+ putTarget(file, "/repo-a/wt", "feat/parent");
+ putTarget(file, "/repo-b/wt", "feat/parent");
+ // Branch names are not unique across repos, so a rename in repo A must not
+ // touch repo B's identically named target.
+ const moved = renameTargetBranch(file, "feat/parent", "feat/renamed", new Set(["/repo-a/wt"]));
+ assert.equal(moved, 1);
+ assert.equal(targetOf(file, "/repo-a/wt"), "feat/renamed");
+ assert.equal(targetOf(file, "/repo-b/wt"), "feat/parent");
+ } finally {
+ rmSync(dir, { recursive: true, force: true });
+ }
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("renameTargetBranch repoints every worktree that landed in the old name", () => { | |
| const { dir, file } = tmpFile(); | |
| try { | |
| putTarget(file, "/wt/a", "feat/parent"); | |
| putTarget(file, "/wt/b", "feat/parent"); | |
| putTarget(file, "/wt/c", "main"); | |
| const moved = renameTargetBranch(file, "feat/parent", "feat/renamed"); | |
| assert.equal(moved, 2); | |
| assert.equal(targetOf(file, "/wt/a"), "feat/renamed"); | |
| assert.equal(targetOf(file, "/wt/b"), "feat/renamed"); | |
| assert.equal(targetOf(file, "/wt/c"), "main", "unrelated targets are untouched"); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }); | |
| test("renameTargetBranch repoints every worktree that landed in the old name", () => { | |
| const { dir, file } = tmpFile(); | |
| try { | |
| putTarget(file, "/wt/a", "feat/parent"); | |
| putTarget(file, "/wt/b", "feat/parent"); | |
| putTarget(file, "/wt/c", "main"); | |
| const moved = renameTargetBranch(file, "feat/parent", "feat/renamed"); | |
| assert.equal(moved, 2); | |
| assert.equal(targetOf(file, "/wt/a"), "feat/renamed"); | |
| assert.equal(targetOf(file, "/wt/b"), "feat/renamed"); | |
| assert.equal(targetOf(file, "/wt/c"), "main", "unrelated targets are untouched"); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }); | |
| test("renameTargetBranch only rewrites paths inside the given scope", () => { | |
| const { dir, file } = tmpFile(); | |
| try { | |
| putTarget(file, "/repo-a/wt", "feat/parent"); | |
| putTarget(file, "/repo-b/wt", "feat/parent"); | |
| // Branch names are not unique across repos, so a rename in repo A must not | |
| // touch repo B's identically named target. | |
| const moved = renameTargetBranch(file, "feat/parent", "feat/renamed", new Set(["/repo-a/wt"])); | |
| assert.equal(moved, 1); | |
| assert.equal(targetOf(file, "/repo-a/wt"), "feat/renamed"); | |
| assert.equal(targetOf(file, "/repo-b/wt"), "feat/parent"); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/worktree-target-store.test.ts` around lines 279 - 293, Add unit
coverage in the renameTargetBranch test near the existing same-name target setup
by passing a scope argument and asserting only worktrees within that scope are
repointed. Keep an identically named target from an unrelated scope unchanged,
directly pinning renameTargetBranch’s scope guard in the store contract.
| export function saveTargets(file: string, targets: TargetMap): boolean { | ||
| const tmp = `${file}.tmp`; | ||
| try { | ||
| mkdirSync(dirname(file), { recursive: true }); | ||
| writeFileSync(tmp, JSON.stringify({ targets }, null, 2) + "\n"); | ||
| renameSync(tmp, file); | ||
| return true; | ||
| } catch (e) { | ||
| log.warn(`[makit] failed to write worktree targets ${file}: ${(e as Error).message}`); | ||
| try { | ||
| if (existsSync(tmp)) unlinkSync(tmp); | ||
| } catch { | ||
| // Best-effort cleanup; the write already failed and we must not throw. | ||
| } | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Consider an fsync before the rename.
The rename makes the swap atomic with respect to readers. It does not force the temp file's data to disk. On a crash shortly after the write, some filesystems can expose a renamed but zero-length file. loadTargets then degrades every worktree to "no target", which is the exact loss this store documents as unacceptable.
♻️ Proposed durability fix
-import { existsSync, mkdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs";
+import {
+ closeSync,
+ existsSync,
+ fsyncSync,
+ mkdirSync,
+ openSync,
+ readFileSync,
+ renameSync,
+ unlinkSync,
+ writeFileSync,
+} from "node:fs"; mkdirSync(dirname(file), { recursive: true });
writeFileSync(tmp, JSON.stringify({ targets }, null, 2) + "\n");
+ // Force the bytes out before the swap: `rename` orders visibility, not
+ // durability, so a crash can otherwise expose an empty file.
+ const fd = openSync(tmp, "r+");
+ try {
+ fsyncSync(fd);
+ } finally {
+ closeSync(fd);
+ }
renameSync(tmp, file);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function saveTargets(file: string, targets: TargetMap): boolean { | |
| const tmp = `${file}.tmp`; | |
| try { | |
| mkdirSync(dirname(file), { recursive: true }); | |
| writeFileSync(tmp, JSON.stringify({ targets }, null, 2) + "\n"); | |
| renameSync(tmp, file); | |
| return true; | |
| } catch (e) { | |
| log.warn(`[makit] failed to write worktree targets ${file}: ${(e as Error).message}`); | |
| try { | |
| if (existsSync(tmp)) unlinkSync(tmp); | |
| } catch { | |
| // Best-effort cleanup; the write already failed and we must not throw. | |
| } | |
| return false; | |
| } | |
| } | |
| export function saveTargets(file: string, targets: TargetMap): boolean { | |
| const tmp = `${file}.tmp`; | |
| try { | |
| mkdirSync(dirname(file), { recursive: true }); | |
| writeFileSync(tmp, JSON.stringify({ targets }, null, 2) + "\n"); | |
| // Force the bytes out before the swap: `rename` orders visibility, not | |
| // durability, so a crash can otherwise expose an empty file. | |
| const fd = openSync(tmp, "r+"); | |
| try { | |
| fsyncSync(fd); | |
| } finally { | |
| closeSync(fd); | |
| } | |
| renameSync(tmp, file); | |
| return true; | |
| } catch (e) { | |
| log.warn(`[makit] failed to write worktree targets ${file}: ${(e as Error).message}`); | |
| try { | |
| if (existsSync(tmp)) unlinkSync(tmp); | |
| } catch { | |
| // Best-effort cleanup; the write already failed and we must not throw. | |
| } | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/worktree-target-store.ts` around lines 110 - 126, Update
saveTargets to fsync the temporary file’s contents before renameSync, using the
existing tmp-file write flow and ensuring the file descriptor is properly closed
before the rename. Preserve the current atomic rename, cleanup behavior, and
boolean error handling.
…verride 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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d2b6acd1-dab3-476b-834f-85ecf16489a5) |
`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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3fdf552a-d738-42a8-87cb-a1524bda7ac6) |
|
Second post-merge round done — macroscope HIGH — remote-only target repaired away ( macroscope HIGH — uncoalesced revival ( macroscope Medium — picker ignored the default-branch override ( CodeRabbit Major — unhandled CodeRabbit Minor — failed rename persist ( Two of CodeRabbit's Majors ( Server 2145 pass; flutter analyze clean. |
- 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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_447db64f-4ebd-4f2f-9d28-10938065bc82) |
`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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3b9c50d0-8849-4a31-b266-eebdcecc67a9) |
|
Third post-merge round —
On the I'm leaving the remaining ~10 Trivial threads (extra test permutations, an Server 2148 pass; flutter analyze clean. |
Addressing open threads#1, #2, #5, #6 (Trivial code quality)
#3, #4 (already fixed in commit c0357e8)
#7 (Minor – fsync added in 7a56138)
#8 (Trivial – StatusSources.failure)
#9 (Minor – default-branch override handling)
#10 (Minor – write ordering)
All substantial findings have been implemented. The trivial coverage gaps are acceptable for a first iteration of the feature. |
1 participant
The bug
A worktree stacked on another worktree's branch reported its parent's work as its own.
repo_servicehanded the repo's default branch todiffStat()/commitsAhead()for every worktreeregardless of what that worktree was destined for, and the base the user picked at creation time was
passed once to
git worktree addand then discarded:A child with 3 lines of its own on top of a 20-line parent showed
+23. Three code paths disagreedabout what "the base" even was: the pill used the repo default, creation collected a value and threw
it away, and wrap-up used the PR's
baseRefName.The reframe
We stop modelling base (a fact about the past, which can be deleted and then poisons everything
downstream) and model target — where this branch's work lands. One field feeds four consumers:
git diff <target>...HEAD— what a pull request into it would containgh pr create --base/gh pr edit --baseThree-dot means git finds the merge base live, so no fork point is stored, and the diff
self-heals: once a parent lands,
maincontains its commits and the child's number drops to its owndelta with no intervention.
The contract
resolveTargetBranch()is the single owner of precedence — primary/detached →null, then anOPEN PR's
baseRefName(which inherits GitHub's automatic PR retargeting for free), then thepersisted choice, then the repo default. Default-last is deliberate: it reproduces today's behaviour,
so upgrading moves nobody's numbers until they choose.
Four rules on top:
DiffStatgainstargetResolved, because the failure mode is not a zero: when the committed legfails, working-tree files still count, so an unresolvable target yields a plausible small number
that reads as "barely diverged". Clients suppress the pill rather than publish a partial count.
The UI
The target is create-time config, so it spends no first-glance space — the worktree row and the
composer strip are unchanged. Three disclosures share one picker:
a worktree has no session
Ship it ⌄menu, bottom group, printing its value inlinebranch ≫ target, the only place head and target appear together(with a PR,
status.identityis#<number>, so it is the only place the branch appears at all)"Lands in" is enabled where Rename is blocked (open PR), because retargeting an open PR is a
first-class operation while renaming would orphan the PR's head.
Vocabulary
base→targetacross server and app, with two documented exceptions:baseRefName(GitHub's ownfield, at the gateway) and one-release wire aliases on
worktree.create/worktree.wrapUpso aclient on either side of the rename keeps working.
How it was tested
tsc --noEmitclean.flutter analyze --fatal-infosanddart format --set-exit-if-changedclean;flutter build macos --debugsucceeds.server/test/ws/worktree_set_target.test.ts):asserts the child drops from
+23to+3on the very next snapshot, which is what proves thepersist-before-broadcast ordering.
server/src/target_rules.test.ts), including athree-deep chain where the middle link is already gone.
itself live with no user action, the picker's ranked groups and previews, and the
≫ targetheader.Three bugs were found only by driving the real app, none visible to unit tests:
without one, and it disabled every row;
1 commit unpushed—PrStatus.loudis literallysignals.first, so list position is priority.Two of the three were fixture blind spots: the tests had no remote and only asserted positive cases.
Notes for review
docs/specs/2026-08-11-SPEC-51-target-branch.md. Numbered 51 because 48/49 were taken whilethis branch was in flight and 50 is claimed by profiles.
mockups/base-branch.htmlcarries the rejected directions, the review findings(B1–B7) and an AS BUILT card reconciling it with what shipped.
syncBaseBranch/BaseSyncResultare our own names and should be
target, andbaseRefNameleaks past the gateway intoPullRequestInfo/PullRequestDTO/DartPullRequest/wrap_up.dart— which also forces a futureForgejo gateway to impersonate GitHub's schema. ~14 sites, mechanical, deliberately left out of
this change.
wrapUpWorktree's?? detectDefaultBranch()turns "the callerdidn't say where this landed" into "I'll pick main and report success". That misfires with no
version skew at all — a quota-shed PR lookup hits it too. It is not destructive (
syncBaseBranchis fast-forward-only throughout, so the worst case is pulling a branch that didn't need it plus a
misleading report), but it should report "could not tell" instead of guessing.
Note
Medium Risk
Touches worktree diff measurement, PR base selection, and wrap-up fast-forward paths; wrong-target mistakes are irreversible. Mitigated by one-release wire aliases and broad tests, but still a wide behavioral change across create/ship/wrap-up.
Overview
Fixes stacked worktrees reporting a parent's diff as their own by introducing a persistent target branch — where a worktree's work lands — shared by diffs, PR base, and wrap-up.
Models & store.
WorktreegainstargetBranch,targetResolved, andretargetedFrom, plusshowsDiffso partial working-tree-only counts are suppressed when the target cannot be resolved. Renamesbase*→target*on create/wrap-up/WrapUpReport/PrResidue, with one-release wire aliases so mixed client/server versions cannot fast-forward the wrong branch. AddstargetCandidatesandsetWorktreeTargetcommands."Lands in" UI. A shared picker (sheet on touch, dialog on desktop) with ranked candidates and diff previews. Reachable from worktree action menus, the Ship-it caret menu, and the PR detail header (
branch ≫ target). Retargeting stays enabled for open PRs (unlike rename).PR status. Unresolved targets surface as a blocking "nowhere to land" fact and withdraw Ship it / Create PR. Automatic retargets are announced quietly. Detail sheets and remedy runners re-derive from the live snapshot so an in-sheet retarget cannot act on stale facts.
Reviewed by Cursor Bugbot for commit c0357e8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Measure worktree diffs against a resolved target branch instead of the repo default
diffStatnow measures committed deltas against the resolvedtargetBranchand exposestargetResolved; when the target ref is missing, committed counts are zeroed to prevent misleading partial readings.closestAncestorBranchandtargetCandidatesto rank and preview candidate landing branches, exposed via newworktree.targetCandidatesandworktree.setTargetWebSocket commands.WorktreeDTOgainstargetBranch,targetResolved, andretargetedFrom; the UI surfaces a 'Lands in' picker in worktree menus, action sheets, and the PR detail sheet, and suppresses diff pills when the target is unresolved.targetResolvedis false; an informational note appears when a worktree was automatically retargeted.baseBranch→targetBranchthroughout the API, models, and UI with one-release backward-compatibility aliases on both client and server.Macroscope summarized c0357e8.
Summary by CodeRabbit