diff --git a/AGENTS.md b/AGENTS.md index aa415c31..213a1d50 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,6 +17,9 @@ Rules: `================ [WHY] =================` `================ [NEXT] ================` - Show `2-5` candidates in `CANDIDATES`. +- Every candidate must be a **genuinely viable option** — something a reasonable engineer could actually pick. No strawmen, no filler options that obviously lose, no "do nothing" / "give up" padding just to reach a count. +- Candidates must be **maximally diverse approaches** to the same problem: different layers (backend vs UI), different mechanisms (fix vs redesign vs config), different scopes (targeted patch vs broader refactor) — not the same idea with cosmetic variations, and not "subset of the chosen plan" vs "the chosen plan". +- If only one reasonable approach exists, skip the `CANDIDATES` section entirely and say so in `DECISION` — a fake alternatives list is worse than none. - Mark the selected candidate with `(chosen)`. - Each candidate may use up to 5 short lines. - `CANDIDATES` is for concise option framing, not full justification. @@ -128,13 +131,15 @@ gh auth switch --user h0x91b 2>/dev/null || true This is a no-op for collaborators who don't have the `h0x91b` account — `gh` will fall back to whatever account they have configured. +**PRs are squash-merged.** To enable auto-merge, always pass the strategy flag: `gh pr merge --auto --squash `. A bare `gh pr merge --auto` fails non-interactively ("--merge, --rebase, or --squash required"). + ## Changelog policy **For every code change, create a changelog entry file.** This avoids merge conflicts when multiple agents work in parallel. **Path:** `change-logs/YYYY/MM/DD/-.md` -**The `YYYY/MM/DD` is the expected PR merge date — not the date you started.** If a task spans more than one day, do not leave the entry under the day you created it. Before opening/merging the PR, move (rename) the file/folder so the date matches the actual merge day. The changelog UI groups entries by ship date, so a stale start-date silently files the feature under the wrong day. +**The `YYYY/MM/DD` is the expected PR merge date — not the date you started.** If a task spans more than one day, do not leave the entry under the day you created it. Before opening/merging the PR, move (rename) the file/folder so the date matches the actual merge day (with auto-merge, that is normally the day you open the PR). The changelog UI groups entries by ship date, so a stale start-date silently files the feature under the wrong day. **Type prefixes:** `feature-`, `fix-`, `refactor-`, `docs-`, `chore-` @@ -244,7 +249,7 @@ Two-process model: The renderer and main process communicate via **Electrobun's built-in RPC** (IPC bridge). The schema is defined in `src/shared/types.ts` as `AppRPCSchema` with two channels: `bun` (main process) and `webview` (renderer). -- **Request/response:** Components call `api.request.METHOD(params)` (returns a Promise, 2-minute timeout). Handlers live in `src/bun/rpc-handlers/*.ts`, split by domain (`app-handlers`, `settings-config`, `task-lifecycle`, `git-operations`, `tmux-pty`, `notes-labels`, `remote-access`). The root `src/bun/rpc-handlers.ts` is a barrel re-exporter that merges them into a single `handlers` object. +- **Request/response:** Components call `api.request.METHOD(params)` (returns a Promise, 2-minute timeout). Handlers live in `src/bun/rpc-handlers/*.ts`, split by domain (`app-handlers`, `settings-config`, `task-lifecycle`, `git-operations`, `tmux-pty`, `notes-labels`, `remote-access`, `port-tunnels`, `scripts`; `shared.ts`/`shared-pure.ts` are cross-domain helpers, not domains). The root `src/bun/rpc-handlers.ts` is a barrel re-exporter that merges them into a single `handlers` object. - **Push messages:** The main process sends unsolicited updates via `pushMessage?.("eventName", payload)`. The renderer dispatches these as `CustomEvent`s (e.g., `rpc:taskUpdated`), which components listen to with `window.addEventListener()`. ### State management @@ -255,9 +260,9 @@ UI state uses React's **`useReducer`** pattern (no external state library). The - Components call `api.request.*` to fetch/mutate backend data, then `dispatch()` reducer actions to update local state. - Push messages from the main process trigger event listeners that dispatch actions to keep the UI in sync. -### HMR mechanism +### Renderer asset loading (dev-channel Vite fallback) -The main process checks if the Vite dev server is running on `localhost:5173`. If the app is on the `dev` channel and the server responds, it loads from Vite (HMR enabled). Otherwise it falls back to bundled assets via the `views://` protocol. +The main process checks if the Vite dev server is running on `localhost:5173`. If the app is on the `dev` channel and the server responds, it loads from Vite (HMR enabled). Otherwise it falls back to bundled assets via the `views://` protocol. This mechanism exists in the code for the `dev` channel, but agents must never run a Vite watch/HMR loop themselves — see the HMR ban in [Commands](#commands). ### Build pipeline @@ -288,10 +293,10 @@ Each project has three lifecycle scripts, configurable in Project Settings (`src | Field | When it runs | |---|---| | `setupScript` | After a new worktree is created for a task | -| `devScript` | When starting the dev server for the project (not yet wired up — reserved for future use) | +| `devScript` | When starting the task dev server (`dev3 dev-server start` or the UI button; runs in a tmux window — see `src/bun/rpc-handlers/tmux-pty.ts`) | | `cleanupScript` | Before a task worktree is removed after `completed` or `cancelled` (and `archived` once that status is added) | -All three are free-form shell scripts. They are saved via the `updateProjectSettings` RPC handler in `src/bun/rpc-handlers.ts`. +All three are free-form shell scripts. They are saved via the `updateProjectSettings` RPC handler in `src/bun/rpc-handlers/settings-config.ts`. ## Styling & design tokens @@ -383,7 +388,7 @@ Call with `t.plural("dashboard.projectCount", count)`. ### Adding a new locale -1. Create `src/mainview/i18n/translations/{locale}.ts` with type `TranslationRecord & Record` +1. Create a `src/mainview/i18n/translations/{locale}/` directory with domain files mirroring `en/`, plus a barrel `src/mainview/i18n/translations/{locale}.ts` that merges them (copy the structure of `ru.ts`); the barrel must satisfy `TranslationRecord` 2. Add the locale to `ALL_LOCALES` and `LOCALE_LABELS` in `src/mainview/i18n/types.ts` 3. Import and register in `src/mainview/i18n/context.tsx` (`translationSets`) 4. Add plural rules in `src/mainview/i18n/interpolate.ts` (`getPluralForm`) diff --git a/change-logs/2026/06/10/fix-merge-prompt-reliability.md b/change-logs/2026/06/10/fix-merge-prompt-reliability.md new file mode 100644 index 00000000..b42b5a1e --- /dev/null +++ b/change-logs/2026/06/10/fix-merge-prompt-reliability.md @@ -0,0 +1 @@ +Fixed several reliability issues in the "PR merged" completion popup: a prompt lost to an app restart was suppressed forever (now it re-appears after an hour if unanswered, while explicit dismissals stay permanent), tasks whose remote branch was pruned after merge (delete_branch_on_merge) were never detected (now verified via the GitHub merged-PR check), the popup could appear while uncommitted changes remained in the worktree, and it could fire when comparing against a non-base ref in the diff dropdown. Dismissed prompts also no longer burn git/gh calls on every poller tick. diff --git a/decisions/066-merge-prompt-retry-and-gating.md b/decisions/066-merge-prompt-retry-and-gating.md new file mode 100644 index 00000000..0684307e --- /dev/null +++ b/decisions/066-merge-prompt-retry-and-gating.md @@ -0,0 +1,22 @@ +# 066 — Merge-completion prompt: retry window, pruned-branch fallback, dirty/compare gating + +## Context + +The "PR merged — complete the task?" popup periodically misbehaved: it silently never appeared for some merged tasks, and could appear when it shouldn't. Live data showed tasks stuck with `mergeCompletionPrompt.promptedAt` set, `dismissedAt: null`, still in review — the popup was reserved but the user never saw it (app restart / undelivered push), and the precise-fingerprint suppression in `shouldSuppressMergePrompt` muted it forever. + +## Investigation + +Five independent problems were confirmed in `src/bun/rpc-handlers/git-operations.ts` and `src/mainview/components/task-info-panel/useTaskBranchStatus.ts`: +(A) prompt state is persisted *before* display, and precise fingerprints were suppressed permanently regardless of `dismissedAt`; (B) `getUnpushedCount === -1` (remote branch pruned by `delete_branch_on_merge`) skipped detection entirely; (C) no uncommitted-changes check before a popup that claims "no changes left"; (D) the UI prompt fired for `mergedByContent` computed against any user-selected compare ref; (E) dismissed tasks re-ran merge-tree/patch-id/gh checks every 60s forever. + +## Decision + +Permanent suppression now requires an explicit `dismissedAt`; an unanswered reservation only mutes re-prompts for 1h (`MERGE_PROMPT_RETRY_SUPPRESS_MS`), and the in-memory key set became a `Map` with the same TTL. Strategy 3 of `isContentMergedInto` was extracted into `git.isBranchMergedViaGitHubPR` (trusted only when the merged PR's `headRefOid` equals local HEAD) and is used by the poller when the remote branch is gone. The poller checks suppression first, then skips dirty worktrees *without* reserving; the UI prompt additionally requires a clean worktree and the default base compare ref. + +## Risks + +A user who ignores the popup (closes it without answering) will see it again an hour later — intentional, but a behavior change. The pruned-branch path adds a gh API call per never-pushed review task per minute; acceptable against the 5000/h limit and offset by the savings from (E). + +## Alternatives considered + +Persisting "popup actually displayed" acknowledgements from the renderer would remove the reserve-before-display race entirely, but requires a new RPC round-trip and still fails on renderer crash; the retry window is simpler and self-healing. Treating `unpushed === -1` as merged when content strategies pass was rejected: a never-pushed branch with zero commits trivially "matches" the base and would false-positive. diff --git a/src/bun/__tests__/git-merge-detection.test.ts b/src/bun/__tests__/git-merge-detection.test.ts index c71e5e89..6c6e8ec2 100644 --- a/src/bun/__tests__/git-merge-detection.test.ts +++ b/src/bun/__tests__/git-merge-detection.test.ts @@ -22,7 +22,7 @@ vi.mock("../spawn", async () => { return createSpawnMock(() => ghPrListResponse); }); -import { isContentMergedInto } from "../git"; +import { isBranchMergedViaGitHubPR, isContentMergedInto } from "../git"; import { createTestRepo, cleanup, makeTaskCommits, g, type TestRepo } from "./git-test-helpers"; // ─── Tests ─────────────────────────────────────────────────────────────────── @@ -255,3 +255,72 @@ describe("isContentMergedInto", () => { expect(result).toBe(false); }); }); + +describe("isBranchMergedViaGitHubPR", () => { + let repo: TestRepo; + + beforeEach(() => { + repo = createTestRepo(); + ghPrListResponse = "[]"; + }); + + afterEach(() => { + cleanup(repo); + }); + + it("returns true when a merged PR's head commit matches the current local HEAD", async () => { + // Simulates delete_branch_on_merge: the PR merged and origin/ + // was pruned, so content strategies are unavailable — gh is the source + // of truth. + g("git checkout -b task-branch", repo.local); + makeTaskCommits(repo.local); + + const headSha = g("git rev-parse HEAD", repo.local).trim(); + ghPrListResponse = JSON.stringify([{ number: 42, headRefOid: headSha }]); + + const result = await isBranchMergedViaGitHubPR(repo.local); + expect(result).toBe(true); + }); + + it("returns false when no merged PR exists for the branch", async () => { + g("git checkout -b task-branch", repo.local); + makeTaskCommits(repo.local); + + const result = await isBranchMergedViaGitHubPR(repo.local); + expect(result).toBe(false); + }); + + it("returns false when the merged PR's head commit does not match current HEAD (reused branch name)", async () => { + g("git checkout -b task-branch", repo.local); + makeTaskCommits(repo.local); + + ghPrListResponse = JSON.stringify([ + { number: 7, headRefOid: "0000000000000000000000000000000000000000" }, + ]); + + const result = await isBranchMergedViaGitHubPR(repo.local); + expect(result).toBe(false); + }); + + it("returns false on detached HEAD", async () => { + g("git checkout -b task-branch", repo.local); + makeTaskCommits(repo.local); + const headSha = g("git rev-parse HEAD", repo.local).trim(); + g(`git checkout --detach ${headSha}`, repo.local); + + ghPrListResponse = JSON.stringify([{ number: 42, headRefOid: headSha }]); + + const result = await isBranchMergedViaGitHubPR(repo.local); + expect(result).toBe(false); + }); + + it("returns false when gh output is not valid JSON", async () => { + g("git checkout -b task-branch", repo.local); + makeTaskCommits(repo.local); + + ghPrListResponse = "gh: command failed"; + + const result = await isBranchMergedViaGitHubPR(repo.local); + expect(result).toBe(false); + }); +}); diff --git a/src/bun/__tests__/rpc-handlers.test.ts b/src/bun/__tests__/rpc-handlers.test.ts index 7ebe67c7..7adc7ac1 100644 --- a/src/bun/__tests__/rpc-handlers.test.ts +++ b/src/bun/__tests__/rpc-handlers.test.ts @@ -64,6 +64,7 @@ vi.mock("../git", () => ({ getBranchDiffStats: vi.fn(), canRebaseCleanly: vi.fn(), isContentMergedInto: vi.fn(), + isBranchMergedViaGitHubPR: vi.fn(), cloneRepo: vi.fn(), extractRepoName: vi.fn(), getCurrentBranch: vi.fn(), @@ -6147,6 +6148,9 @@ describe("startMergeDetectionPoller / stopMergeDetectionPoller", () => { stopMergeDetectionPoller(); _resetMergePollerState(); vi.mocked(git.getHeadSha).mockResolvedValue("abc123"); + // clearAllMocks does not reset implementations left by earlier describe + // blocks (e.g. isWorktreeDirty -> true), so pin a clean worktree here. + vi.mocked(git.isWorktreeDirty).mockResolvedValue(false); vi.mocked(data.updateTask).mockImplementation(async (_project: Project, _taskId: string, patch: Partial) => makeTask(patch)); }); @@ -6467,6 +6471,242 @@ describe("startMergeDetectionPoller / stopMergeDetectionPoller", () => { expect(push).not.toHaveBeenCalled(); expect(git.isContentMergedInto).not.toHaveBeenCalled(); }); + + it("re-prompts a precise prompt that was reserved over an hour ago but never answered", async () => { + // App restart / undelivered push leaves promptedAt set with dismissedAt + // null — the popup was lost and must come back, not be muted forever. + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + mergeCompletionPrompt: { + fingerprint: "v1:dev3/task-test:abc123", + promptedAt: new Date(Date.now() - 2 * 60 * 60_000).toISOString(), + dismissedAt: null, + precise: true, + }, + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(0); + vi.mocked(git.isContentMergedInto).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(push).toHaveBeenCalledWith("branchMerged", expect.objectContaining({ + taskId: task.id, + fingerprint: "v1:dev3/task-test:abc123", + })); + }); + + it("does not re-prompt a precise unanswered prompt within the retry window", async () => { + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + mergeCompletionPrompt: { + fingerprint: "v1:dev3/task-test:abc123", + promptedAt: new Date(Date.now() - 30 * 60_000).toISOString(), + dismissedAt: null, + precise: true, + }, + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(0); + vi.mocked(git.isContentMergedInto).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(push).not.toHaveBeenCalledWith("branchMerged", expect.anything()); + }); + + it("never re-prompts a precise prompt the user dismissed, even after the retry window", async () => { + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + mergeCompletionPrompt: { + fingerprint: "v1:dev3/task-test:abc123", + promptedAt: new Date(Date.now() - 3 * 60 * 60_000).toISOString(), + dismissedAt: new Date(Date.now() - 3 * 60 * 60_000).toISOString(), + precise: true, + }, + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(0); + vi.mocked(git.isContentMergedInto).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(push).not.toHaveBeenCalledWith("branchMerged", expect.anything()); + }); + + it("checks suppression before running expensive merge detection for dismissed prompts", async () => { + // Dismissed tasks must not burn git/gh calls on every 60s tick. + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + mergeCompletionPrompt: { + fingerprint: "v1:dev3/task-test:abc123", + promptedAt: new Date(Date.now() - 10 * 60_000).toISOString(), + dismissedAt: new Date(Date.now() - 10 * 60_000).toISOString(), + precise: true, + }, + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(0); + vi.mocked(git.isContentMergedInto).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(git.isContentMergedInto).not.toHaveBeenCalled(); + expect(git.getUnpushedCount).not.toHaveBeenCalled(); + expect(push).not.toHaveBeenCalledWith("branchMerged", expect.anything()); + }); + + it("falls back to the GitHub merged-PR check when the remote branch is gone (unpushed === -1)", async () => { + // delete_branch_on_merge prunes origin/ after the PR lands; + // getUnpushedCount then returns -1 and the merged task was silently skipped. + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(-1); + vi.mocked(git.isBranchMergedViaGitHubPR).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(git.isBranchMergedViaGitHubPR).toHaveBeenCalledWith("/tmp/test-worktree", project); + expect(git.isContentMergedInto).not.toHaveBeenCalled(); + expect(push).toHaveBeenCalledWith("branchMerged", expect.objectContaining({ + taskId: task.id, + fingerprint: "v1:dev3/task-test:abc123", + })); + }); + + it("does not prompt when the remote branch is gone and GitHub has no matching merged PR", async () => { + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(-1); + vi.mocked(git.isBranchMergedViaGitHubPR).mockResolvedValue(false); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(push).not.toHaveBeenCalledWith("branchMerged", expect.anything()); + }); + + it("does not prompt or reserve while the worktree is dirty, then prompts once clean", async () => { + // The popup claims "no changes left" — a dirty worktree means changes + // DO remain, and completing the task would destroy them. + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(0); + vi.mocked(git.isContentMergedInto).mockResolvedValue(true); + vi.mocked(git.isWorktreeDirty).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(push).not.toHaveBeenCalledWith("branchMerged", expect.anything()); + expect(data.updateTask).not.toHaveBeenCalled(); + + vi.mocked(git.isWorktreeDirty).mockResolvedValue(false); + await vi.advanceTimersByTimeAsync(60_000); + + expect(push).toHaveBeenCalledWith("branchMerged", expect.objectContaining({ taskId: task.id })); + }); + + it("re-prompts in the same session when an unanswered in-memory reservation expires", async () => { + const project = makeProject(); + const task = makeTask({ + status: "review-by-user", + worktreePath: "/tmp/test-worktree", + branchName: "dev3/task-test", + }); + const push = vi.fn(); + + vi.mocked(data.loadProjects).mockResolvedValue([project]); + vi.mocked(data.loadTasks).mockResolvedValue([task]); + vi.mocked(git.fetchOrigin).mockResolvedValue(true); + vi.mocked(git.getCurrentBranch).mockResolvedValue("dev3/task-test"); + vi.mocked(git.getUnpushedCount).mockResolvedValue(0); + vi.mocked(git.isContentMergedInto).mockResolvedValue(true); + setPushMessage(push); + + startMergeDetectionPoller(); + await vi.advanceTimersByTimeAsync(60_000); + expect(push).toHaveBeenCalledTimes(1); + + // loadTasks keeps returning the task without persisted prompt state, + // so only the in-memory reservation suppresses re-prompts here. + await vi.advanceTimersByTimeAsync(62 * 60_000); + expect(push).toHaveBeenCalledTimes(2); + }); }); describe("startPRDetectionPoller / stopPRDetectionPoller", () => { diff --git a/src/bun/git.ts b/src/bun/git.ts index b55dc9e7..b1dc1961 100644 --- a/src/bun/git.ts +++ b/src/bun/git.ts @@ -1313,58 +1313,70 @@ export async function isContentMergedInto( // When both local strategies fail (main diverged before AND after the squash // on the same files), ask GitHub directly if a merged PR exists for this branch. // This is the definitive source of truth for GitHub-hosted repos. - // - // CRITICAL: a merged PR matching the head branch *name* is NOT enough. Branch - // names get reused — a previously merged PR can coexist with brand-new unmerged - // work pushed to the same branch, or an open PR for the same head. This bites - // PR-review tasks especially. We only trust the merged-PR signal when the PR's - // merged head commit (headRefOid) equals the current local HEAD; in every - // GitHub merge method (merge/squash/rebase) the head ref tip is left untouched, - // so a genuine merge always satisfies this, while stale/reused-name PRs do not. + if (await isBranchMergedViaGitHubPR(worktreePath, project)) { + return true; + } + + log.info("isContentMergedInto", { ref, mergeBase, merged: false }); + return false; +} + +// CRITICAL: a merged PR matching the head branch *name* is NOT enough. Branch +// names get reused — a previously merged PR can coexist with brand-new unmerged +// work pushed to the same branch, or an open PR for the same head. This bites +// PR-review tasks especially. We only trust the merged-PR signal when the PR's +// merged head commit (headRefOid) equals the current local HEAD; in every +// GitHub merge method (merge/squash/rebase) the head ref tip is left untouched, +// so a genuine merge always satisfies this, while stale/reused-name PRs do not. +export async function isBranchMergedViaGitHubPR( + worktreePath: string, + project?: Pick, +): Promise { const [branchResult, headShaResult] = await Promise.all([ run(["git", "rev-parse", "--abbrev-ref", "HEAD"], worktreePath), run(["git", "rev-parse", "HEAD"], worktreePath), ]); - if (branchResult.ok && branchResult.stdout && headShaResult.ok && headShaResult.stdout) { - const headSha = headShaResult.stdout.trim(); - try { - const ghResult = project - ? await github.runGitHub( - project, - worktreePath, - ["pr", "list", "--head", branchResult.stdout, "--state", "merged", "--json", "number,headRefOid", "--limit", "1"], - ) - : await run( - ["gh", "pr", "list", "--head", branchResult.stdout, "--state", "merged", "--json", "number,headRefOid", "--limit", "1"], - worktreePath, - ); - if (ghResult.ok && ghResult.stdout) { - try { - const prs = JSON.parse(ghResult.stdout); - if (Array.isArray(prs) && prs.length > 0) { - const pr = prs[0]; - if (pr?.headRefOid && pr.headRefOid === headSha) { - log.info("isContentMergedInto", { ref, method: "github-pr", pr: pr.number, merged: true }); - return true; - } - log.info("isContentMergedInto", { - ref, - method: "github-pr", - pr: pr?.number, - headRefOid: pr?.headRefOid, - headSha, - merged: false, - reason: "merged PR head does not match current HEAD", - }); + if (!branchResult.ok || !branchResult.stdout || !headShaResult.ok || !headShaResult.stdout) { + return false; + } + // Detached HEAD has no branch name to match PRs against. + if (branchResult.stdout === "HEAD") return false; + + const headSha = headShaResult.stdout.trim(); + try { + const ghResult = project + ? await github.runGitHub( + project, + worktreePath, + ["pr", "list", "--head", branchResult.stdout, "--state", "merged", "--json", "number,headRefOid", "--limit", "1"], + ) + : await run( + ["gh", "pr", "list", "--head", branchResult.stdout, "--state", "merged", "--json", "number,headRefOid", "--limit", "1"], + worktreePath, + ); + if (ghResult.ok && ghResult.stdout) { + try { + const prs = JSON.parse(ghResult.stdout); + if (Array.isArray(prs) && prs.length > 0) { + const pr = prs[0]; + if (pr?.headRefOid && pr.headRefOid === headSha) { + log.info("isBranchMergedViaGitHubPR", { method: "github-pr", pr: pr.number, merged: true }); + return true; } - } catch { /* ignore parse errors */ } - } - } catch { - // Ignore gh lookup/auth failures and fall back to the offline result. + log.info("isBranchMergedViaGitHubPR", { + method: "github-pr", + pr: pr?.number, + headRefOid: pr?.headRefOid, + headSha, + merged: false, + reason: "merged PR head does not match current HEAD", + }); + } + } catch { /* ignore parse errors */ } } + } catch { + // Ignore gh lookup/auth failures and report not-merged. } - - log.info("isContentMergedInto", { ref, mergeBase, merged: false }); return false; } diff --git a/src/bun/rpc-handlers/git-operations.ts b/src/bun/rpc-handlers/git-operations.ts index 045fad25..05f1279c 100644 --- a/src/bun/rpc-handlers/git-operations.ts +++ b/src/bun/rpc-handlers/git-operations.ts @@ -16,10 +16,14 @@ import { spawn } from "../spawn"; import { getPushMessage, log } from "./shared"; const gitOpPaneIds = new Map(); -const mergeNotifiedPromptKeys = new Set(); +// promptKey -> reservedAt (ms). A reservation only mutes re-prompts for +// MERGE_PROMPT_RETRY_SUPPRESS_MS: if the user never answers (app restart, +// undelivered push), the prompt must come back instead of being lost forever. +const mergeNotifiedPromptKeys = new Map(); const branchStatusInFlight = new Map>(); const prPromotedTasks = new Set(); const MERGE_PROMPT_FALLBACK_SUPPRESS_MS = 60 * 60 * 1000; +const MERGE_PROMPT_RETRY_SUPPRESS_MS = 60 * 60 * 1000; interface MergeCompletionFingerprint { fingerprint: string; @@ -36,9 +40,22 @@ function parseTime(value: string | null | undefined): number | null { return Number.isFinite(time) ? time : null; } +function isPromptKeyReserved(promptKey: string, nowMs: number): boolean { + const reservedAt = mergeNotifiedPromptKeys.get(promptKey); + if (reservedAt === undefined) return false; + if (nowMs - reservedAt > MERGE_PROMPT_RETRY_SUPPRESS_MS) { + mergeNotifiedPromptKeys.delete(promptKey); + return false; + } + return true; +} + function shouldSuppressMergePrompt(state: MergeCompletionPromptState | null | undefined, fingerprint: MergeCompletionFingerprint, nowMs: number): boolean { if (!state || state.fingerprint !== fingerprint.fingerprint) return false; - if (fingerprint.precise && state.precise) return true; + // Permanent suppression only for an explicit user dismissal of this exact + // head. A reserved-but-unanswered prompt (dismissedAt null) falls through + // to the time window below so a lost popup re-appears. + if (fingerprint.precise && state.precise && parseTime(state.dismissedAt) !== null) return true; const lastPromptTime = Math.max( parseTime(state.promptedAt) ?? 0, @@ -66,17 +83,17 @@ async function getMergeCompletionFingerprint(task: Pick { const promptKey = mergePromptKey(task.id, fingerprint.fingerprint); - if (mergeNotifiedPromptKeys.has(promptKey)) return false; - const nowMs = now.getTime(); + if (isPromptKeyReserved(promptKey, nowMs)) return false; + if (shouldSuppressMergePrompt(task.mergeCompletionPrompt, fingerprint, nowMs)) { - mergeNotifiedPromptKeys.add(promptKey); + mergeNotifiedPromptKeys.set(promptKey, nowMs); return false; } // Reserve the slot before awaiting so concurrent callers see the key immediately - // and cannot both pass the has() check above. - mergeNotifiedPromptKeys.add(promptKey); + // and cannot both pass the reservation check above. + mergeNotifiedPromptKeys.set(promptKey, nowMs); await data.updateTask(project, task.id, { mergeCompletionPrompt: { fingerprint: fingerprint.fingerprint, @@ -250,7 +267,7 @@ async function checkMergedBranches(): Promise { const tasks = await data.loadTasks(project); for (const task of tasks) allTaskIds.add(task.id); } - for (const key of mergeNotifiedPromptKeys) { + for (const key of [...mergeNotifiedPromptKeys.keys()]) { const taskId = key.slice(0, key.indexOf(":")); if (!allTaskIds.has(taskId)) mergeNotifiedPromptKeys.delete(key); } @@ -291,13 +308,34 @@ async function checkMergedBranches(): Promise { } const ref = `origin/${baseBranch}`; - const hasRemote = await git.getUnpushedCount(task.worktreePath!, branchName); - if (hasRemote === -1) continue; + // Cheap suppression first: a prompt already reserved/dismissed for + // this exact head must not burn merge-tree/patch-id/gh calls on + // every 60s tick. + const fingerprint = await getMergeCompletionFingerprint(task, branchName); + const promptKey = mergePromptKey(task.id, fingerprint.fingerprint); + const nowMs = Date.now(); + if (isPromptKeyReserved(promptKey, nowMs)) continue; + if (shouldSuppressMergePrompt(task.mergeCompletionPrompt, fingerprint, nowMs)) continue; + + // The popup claims "no changes left" — never prompt while the + // worktree has uncommitted changes. Skip WITHOUT reserving so a + // later clean tick prompts normally. + if (await git.isWorktreeDirty(task.worktreePath!)) continue; - const merged = await git.isContentMergedInto(task.worktreePath!, ref, project); + const unpushed = await git.getUnpushedCount(task.worktreePath!, branchName); + let merged: boolean; + if (unpushed === -1) { + // origin/ is gone: either never pushed, or pruned after + // the PR merged (delete_branch_on_merge). Content strategies are + // unsafe here (a never-pushed branch with zero commits would + // false-positive), but a merged PR whose head equals local HEAD + // is definitive. + merged = await git.isBranchMergedViaGitHubPR(task.worktreePath!, project); + } else { + merged = await git.isContentMergedInto(task.worktreePath!, ref, project); + } if (!merged) continue; - const fingerprint = await getMergeCompletionFingerprint(task, branchName); const shouldPrompt = await reserveMergeCompletionPrompt(project, task, fingerprint); if (!shouldPrompt) continue; @@ -317,7 +355,7 @@ async function checkMergedBranches(): Promise { } export function clearMergeNotification(taskId: string): void { - for (const key of mergeNotifiedPromptKeys) { + for (const key of [...mergeNotifiedPromptKeys.keys()]) { if (key.startsWith(`${taskId}:`)) mergeNotifiedPromptKeys.delete(key); } } diff --git a/src/mainview/__tests__/App.test.tsx b/src/mainview/__tests__/App.test.tsx index 4cb7a09a..9c23b61e 100644 --- a/src/mainview/__tests__/App.test.tsx +++ b/src/mainview/__tests__/App.test.tsx @@ -722,9 +722,13 @@ describe("App keyboard shortcuts", () => { await fireBranchMerged("t1", "p1", "fp-confirm-1"); - await waitFor(() => { - expect(screen.getByTestId("project-screen")).toBeInTheDocument(); - }); + // Slow CI runners can exceed the default 1s waitFor timeout here + await waitFor( + () => { + expect(screen.getByTestId("project-screen")).toBeInTheDocument(); + }, + { timeout: 5000 }, + ); expect(screen.queryByTestId("task-screen")).not.toBeInTheDocument(); expect(api.request.moveTask).toHaveBeenCalledWith( expect.objectContaining({ taskId: "t1", projectId: "p1", newStatus: "completed" }), diff --git a/src/mainview/components/__tests__/TaskInfoPanel.test.tsx b/src/mainview/components/__tests__/TaskInfoPanel.test.tsx index 519d88b8..a2fd1b1b 100644 --- a/src/mainview/components/__tests__/TaskInfoPanel.test.tsx +++ b/src/mainview/components/__tests__/TaskInfoPanel.test.tsx @@ -1796,6 +1796,86 @@ describe("TaskInfoPanel", () => { }); }); + it("does not offer auto-complete when comparing against a non-base ref", async () => { + // mergedByContent is computed against the user-selected compare ref; + // a custom ref must never trigger the "Branch Merged" popup. + const dispatch = vi.fn(); + const navigate = vi.fn(); + const task = makeTask({ status: "review-by-user" }); + const customCompareProject = { ...project, defaultCompareRef: "origin/develop" }; + mockedApi.request.getBranchStatus.mockResolvedValue({ + ...defaultBranchStatus, + mergedByContent: true, + mergeCompletionFingerprint: "v1:dev3/task-t1:abc123", + }); + vi.mocked(confirm).mockResolvedValue(true); + + await act(async () => { + renderPanel(task, { dispatch, navigate, project: customCompareProject }); + }); + + await waitFor(() => { + expect(mockedApi.request.getBranchStatus).toHaveBeenCalledWith({ + taskId: "t1", + projectId: "p1", + compareRef: "origin/develop", + }); + }); + expect(mockedApi.request.prepareMergeCompletionPrompt).not.toHaveBeenCalled(); + expect(vi.mocked(confirm)).not.toHaveBeenCalled(); + }); + + it("does not offer auto-complete while the worktree has uncommitted changes", async () => { + const dispatch = vi.fn(); + const navigate = vi.fn(); + const task = makeTask({ status: "review-by-user" }); + mockedApi.request.getBranchStatus.mockResolvedValue({ + ...defaultBranchStatus, + mergedByContent: true, + insertions: 5, + deletions: 2, + mergeCompletionFingerprint: "v1:dev3/task-t1:abc123", + }); + vi.mocked(confirm).mockResolvedValue(true); + + await act(async () => { + renderPanel(task, { dispatch, navigate }); + }); + + await waitFor(() => { + expect(mockedApi.request.getBranchStatus).toHaveBeenCalled(); + }); + expect(mockedApi.request.prepareMergeCompletionPrompt).not.toHaveBeenCalled(); + expect(vi.mocked(confirm)).not.toHaveBeenCalled(); + }); + + it("does not offer auto-complete after a merge while uncommitted changes remain", async () => { + const dispatch = vi.fn(); + const navigate = vi.fn(); + const task = makeTask({ status: "in-progress" }); + mockedApi.request.getBranchStatus.mockResolvedValue({ + ...defaultBranchStatus, + insertions: 5, + deletions: 2, + }); + vi.mocked(confirm).mockResolvedValue(true); + + await act(async () => { + renderPanel(task, { dispatch, navigate }); + }); + + await act(async () => { + window.dispatchEvent( + new CustomEvent("rpc:gitOpCompleted", { + detail: { taskId: "t1", operation: "merge", ok: true }, + }), + ); + }); + + expect(mockedApi.request.prepareMergeCompletionPrompt).not.toHaveBeenCalled(); + expect(vi.mocked(confirm)).not.toHaveBeenCalled(); + }); + it("does not show a second merge-complete dialog while one is already open", async () => { const dispatch = vi.fn(); const navigate = vi.fn(); diff --git a/src/mainview/components/task-info-panel/useTaskBranchStatus.ts b/src/mainview/components/task-info-panel/useTaskBranchStatus.ts index 1041e0f1..a17ec22d 100644 --- a/src/mainview/components/task-info-panel/useTaskBranchStatus.ts +++ b/src/mainview/components/task-info-panel/useTaskBranchStatus.ts @@ -101,6 +101,13 @@ export function useTaskBranchStatus({ let cancelled = false; let timer: ReturnType | null = null; + // mergedByContent is computed against whatever ref the user selected in + // the compare dropdown. The completion prompt is only meaningful against + // the task's real base branch — comparing against another ref must never + // trigger a "Branch Merged" popup. + const isDefaultBaseCompare = + !compareRef || compareRef === baseBranch || compareRef === `origin/${baseBranch}`; + const fetchStatus = async () => { try { const status = await api.request.getBranchStatus({ @@ -114,6 +121,11 @@ export function useTaskBranchStatus({ if ( status.mergedByContent && + isDefaultBaseCompare && + // The popup claims "no changes left" — uncommitted changes + // mean that's false, and completing would destroy them. + status.insertions === 0 && + status.deletions === 0 && MERGE_COMPLETE_ELIGIBLE_STATUSES.includes(task.status) && !mergeDialogShownRef.current ) { @@ -164,6 +176,7 @@ export function useTaskBranchStatus({ } }; }, [ + baseBranch, compareRef, completeTask, isTaskActive, @@ -222,6 +235,11 @@ export function useTaskBranchStatus({ } if (detail.operation === "merge" && detail.ok) { + // Completing the task destroys the worktree — never offer it while + // uncommitted changes remain. + if (refreshedStatus && (refreshedStatus.insertions > 0 || refreshedStatus.deletions > 0)) { + return; + } const promptState = await api.request.prepareMergeCompletionPrompt({ taskId: task.id, projectId: project.id,