diff --git a/docs-ai/003-diff-window/000-plan.md b/docs-ai/003-diff-window/000-plan.md index 3f70f454..fa3dc14d 100644 --- a/docs-ai/003-diff-window/000-plan.md +++ b/docs-ai/003-diff-window/000-plan.md @@ -97,3 +97,6 @@ cut the embedded asset from 9.3 MB to 2.7 MB (−7 MB on the .app) (#45). (#540) — see [004-appearance-follows-app.md](004-appearance-follows-app.md) - Updated 2026-07-14: built-in outgoing changes for an identified pull request, using its target remote and merge-base semantics — see [005-outgoing-changes.md](005-outgoing-changes.md) +- Updated 2026-07-24: hardening plan for outgoing changes — fully-qualified + base refs, labeled no-PR fallback ladder, distinct resolution errors, + focus-refresh fix — see [006-outgoing-changes-hardening.md](006-outgoing-changes-hardening.md) diff --git a/docs-ai/003-diff-window/006-outgoing-changes-hardening.md b/docs-ai/003-diff-window/006-outgoing-changes-hardening.md new file mode 100644 index 00000000..d0ea0ff1 --- /dev/null +++ b/docs-ai/003-diff-window/006-outgoing-changes-hardening.md @@ -0,0 +1,220 @@ +# 003.006 — Outgoing Changes Hardening: Plan + +| | | +| --- | --- | +| **Status** | Implemented | +| **Anchor date** | 2026-07-24 | +| **Primary PRs** | #586 (base work, superseded), hardening PR TBD | +| **Related** | [005-outgoing-changes.md](005-outgoing-changes.md), `docs/components/diff-view.md` | + +## Background + +#586 added the Outgoing Changes view ([005-outgoing-changes.md](005-outgoing-changes.md)). +Review (4× changes-requested on `c4a5ab95`) plus a follow-up audit on a +fresh merge with `main` (branch `outgoing-changes-hardening`; focused suites +pass post-merge, `main` did not touch the diff/Git layers) confirmed four +problems worth fixing and one product-scope gap: + +1. **Short-ref ambiguity (correctness).** `outgoingChangesBaseRef` builds + `/` and feeds it to `rev-parse`/`merge-base`. Git's refname + disambiguation prefers `refs/heads/` over `refs/remotes/`, so a local + branch literally named `upstream/main` silently wins and the diff is + computed against the wrong base. +2. **No-PR scope gap (product).** The entry point requires a cached pull + request with `baseRefName`; fork issue #510's core scenario — previewing + what a PR *would* contain before opening one — errors out. Prowl is a + general-purpose tool; "no PR yet" is the common case, not an edge. +3. **Conflated failure reasons (UX).** No matching remote, multiple matching + remotes, and an unfetched base all collapse to `nil` → one "Fetch the + target remote" message, which is wrong guidance for the ambiguous case. +4. **First-refocus staleness (pre-existing).** `DiffWindowManager.show` + sets `skipNextFocusRefresh` before the `didBecomeKey` observer exists + (new window) or when no notification will fire (already-key window), so + the flag survives and swallows the first genuine refocus refresh. Same + defect exists on `main` for Show Diff; it is merely more visible in + outgoing mode where an agent commits in the background. +5. **Not keybindable (minor UX).** Show Diff has an `AppShortcuts.CommandID` + entry; Outgoing Changes cannot be bound at all. + +## Goals + +- Outgoing Changes never computes a diff against a wrong base ref. +- It works without a pull request, using an explicitly labeled base, and + falls back in a predictable, user-visible order. +- Every resolution failure states its actual reason and an actionable fix. +- The first refocus after leaving the window always refreshes (both modes). +- The action is bindable like Show Diff. +- The view is reachable from visible UI, not only menu/palette/shortcut. + +### Non-goals + +- **Interactive base picker.** Still deferred (as in 005): the labeled + fallback ladder below covers branches cut from the configured base, which + is the dominant case. A picker can later build on `remoteBranchRefs`. +- **NUL-safe `--name-status` parsing.** Special filenames (tab/quote/newline) + break `DiffChangedFile.parseNameStatus` for Show Diff and Outgoing alike; + fixing it only here would fork the parser. Separate follow-up for both paths. +- **Path-prefixed PR URL parsing.** PR URLs come from GitHub GraphQL and are + always `https://host/owner/repo/pull/N`; today's parser fails safe (error, + never a wrong diff). Accepted limitation, documented here. + +## Design / Approach + +**A. Typed base resolution** (`supacode/Clients/Git/GitClient.swift`, +`supacode/Clients/Git/GitClientTypes.swift`). Replace the `String?` returned +by `outgoingChangesBaseRef` with a resolution result: + +``` +OutgoingBaseResolution { ref: String // fully qualified, e.g. refs/remotes/upstream/main + displayName: String // upstream/main + source: .pullRequest | .repositorySetting | .automatic } +OutgoingBaseError: .noMatchingRemote(host, path) + .multipleMatchingRemotes([String]) + .baseRefNotFetched(remote, branch) + .noResolvableBase +``` + +Verification and `merge-base` always use `resolution.ref` (fully qualified), +killing the local-branch shadowing bug. `GitOutgoingChangesComparison` keeps +the qualified ref plus `displayName` for titles/messages. + +**B. Fallback ladder for the no-PR case** +(`supacode/Clients/ExternalDiff/OutgoingChangesClient.swift`, +`supacode/Features/App/Reducer/AppFeature+CommandPalette.swift`): + +1. Cached PR base (current behavior) — source `.pullRequest`. +2. Per-repo `RepositorySettings.worktreeBaseRef` + (`supacode/Features/Settings/Models/RepositorySettings.swift`) — the base + the user told Prowl to cut worktrees from; correct by construction for + Prowl-created worktrees. Source `.repositorySetting`. +3. `GitClient.automaticWorktreeBaseRef` (origin/HEAD → local default branch), + the same chain worktree creation already trusts. Source `.automatic`. +4. Nothing resolves → `.noResolvableBase` with guidance. + +The ladder advances only when a source is *absent*. A source that is present +but fails to resolve errors out with its own reason instead of cascading: a +stacked PR whose base `feature-a` is unfetched must never silently become a +diff against `origin/main` — that would count all of `feature-a`'s commits +as outgoing with only a label to notice it by. Explicit intent (a PR, a +configured `worktreeBaseRef`) is never silently bypassed. + +Every refresh re-runs the full ladder (same code path as the mode switch), +so a PR created, retargeted, or closed while the window is open moves the +base visibly on the next refresh — decided over pinning the base for the +window's lifetime, because the view answers "what would my PR contain", +not "what was the base when I opened this window". + +The window makes the guess explicit instead of hidden: title becomes +"Outgoing Changes — vs " and the file-list header/empty +state names the source ("pull request base" / "worktree base setting" / +"default branch"). 005's "no implicit default-branch fallback" decision is +narrowed, not reversed: no *silent* fallback; a labeled one is fine. + +**C. Distinct failure messages.** `OutgoingChangesClient` maps each +`OutgoingBaseError` case to its own message; only `.baseRefNotFetched` +suggests fetching, `.multipleMatchingRemotes` lists the conflicting remote +names. + +**D. Focus-refresh fix** (`supacode/Features/DiffView/DiffWindowManager.swift`). +Register the `didBecomeKey` observer before `makeKeyAndOrderFront`, and set +`skipNextFocusRefresh` only when the window is not already key (i.e. only +when the show itself will emit the notification the flag is meant to +swallow). Extract the skip/refresh decision into a small pure helper so it +is unit-testable without real windows. Fixes Show Diff too. + +**E. Keybinding registration** (`supacode/App/AppShortcuts.swift`). Add +`CommandID.outgoingChanges` with no default binding; wire menu + palette rows +so users can bind it. + +**F. In-window mode switcher as the primary UI trigger** +(`supacode/Features/DiffView/DiffWindowContentView.swift`, +`DiffWindowManager.swift`, `DiffWindowState.swift`). The diff window gains a +toolbar segmented control — segments "Uncommitted" and "Outgoing" (plain +language over git jargon; "Outgoing" matches the command name) — bound to +`DiffComparison`. Window titles: working-tree mode keeps "Changes — +"; outgoing mode uses "Outgoing Changes — vs ", with base provenance detailed in the file-list header/empty state, +not the title. Show Diff and Outgoing Changes become two initial modes of +the same window, and the current mutual-replacement behavior of the singleton +window turns into an explicit, visible switch. Switching to Outgoing runs the +same resolution ladder (B); a failure renders the in-window error state with +the reason from C while the switcher stays usable to flip back. To support +switching from a window opened in working-tree mode, `DiffWindowManager.show` +gains an injected outgoing-comparison resolver closure instead of requiring +callers to resolve up front. Secondary trigger: add "Show Diff" and +"Outgoing Changes" rows to the worktree context menu +(`supacode/Features/Repositories/Views/WorktreeRowsView.swift`, +`rowContextMenu`), which today lacks even Show Diff. + +**G. Bounded document loading** +(`supacode/Features/DiffView/DiffWindowState.swift`). `loadAllFiles` fans +out one task per changed file (two `git show` processes each) with no +concurrency bound — tolerable for typically-small working-tree diffs, +not for outgoing diffs of long-lived branches. Bound the task group to a +small fixed width for both modes. + +**Tests.** Real-git regression for the `refs/heads/upstream/main` shadow +(extends `supacodeTests/GitOutgoingChangesTests.swift`); ladder tests per +source and per error case; focus-decision helper tests; existing suites stay +green (`make test`, `make check`, `make build-app`). + +## Alternatives & decisions + +- **Labeled fallback vs. base picker first** — ladder chosen: reuses two + proven resolution sources, no new UI/persistence; picker remains open as a + later layer on top of the same `OutgoingBaseResolution`. +- **Repo setting above origin/HEAD** — the setting is explicit user intent + and matches how the worktree was actually created; origin/HEAD is only the + hosting default. +- **Fix focus refresh here vs. separate PR** — here: the outgoing mode is + what makes the stale window user-visible, and the fix is small and shared. +- **Keep `String` refs vs. typed resolution** — typed: the same struct + carries qualification, display, and provenance, which B and C both need. +- **Refresh re-resolves vs. pins the base** — re-resolve (run the ladder on + every refresh): the view answers "what would my PR contain now"; a visibly + moving base beats a silently stale one. Decided 2026-07-24. +- **Delivery: supersede #586** — this branch (merge of #586 + `main` + + hardening) ships as a new PR whose body maps each of the four pending + review findings to its resolution; #586 is closed with cross-references. + Decided 2026-07-24. +- **UI trigger placement** — in-window segmented switcher chosen over a + second sidebar badge (an outgoing line count would add per-worktree git + cost and row clutter) and over repurposing the PR tag (it already opens + the checks popover, and a PR-only trigger contradicts the no-PR ladder). + The context menu rows are a low-cost secondary path; the +/− badge tap + keeps opening working-tree mode, now one visible click away from Outgoing. + +## Implementation notes & deviations (2026-07-24) + +Implemented on branch `outgoing-changes-hardening` (#586 merged with `main`, +then hardened per this plan). Deviations from the plan text: + +- **Default keybinding instead of "no default binding".** `AppShortcut` has no + unbound representation, so `outgoing_changes` ships with `⌘⌥⇧Y` — the + option-modified sibling of Show Diff's `⌘⇧Y` — and is rebindable like any + configurable action. +- **`incompletePullRequest` error case added.** A cached pull request whose + `baseRefName` has not loaded yet is a present-but-unresolvable source and + errors out (strict-ladder rule) instead of falling through. +- The live pull-request read is wired in `supacodeApp.swift` via the existing + `SupacodeAppStoreBox` pattern (`OutgoingChangesClient.live(pullRequestInfo:)` + reading `store.withState`), the same shape `PullRequestRefreshCoordinator` + uses for store access outside TCA effects. + +Key files: `supacode/Clients/Git/GitClient.swift` (ladder, qualified refs), +`supacode/Clients/Git/GitClientTypes.swift` (`OutgoingBaseResolution`, +`OutgoingBaseResolutionError`), `supacode/Clients/ExternalDiff/OutgoingChangesClient.swift` +(resolver factory), `supacode/Features/DiffView/DiffWindowState.swift` +(`DiffMode`, resolver refresh, bounded loads), +`supacode/Features/DiffView/DiffWindowManager.swift` (`DiffWindowFocusPolicy`, +title sync), `supacode/Features/DiffView/DiffWindowContentView.swift` +(switcher, provenance), `supacode/App/AppShortcuts.swift`, +`supacode/Features/Repositories/Views/WorktreeRowsView.swift` (context menu). +Tests: `supacodeTests/GitOutgoingChangesTests.swift` (ladder + shadow-branch +regression), `supacodeTests/DiffWindowStateTests.swift` (mode switch, resolver +refresh, failure path), `supacodeTests/DiffWindowFocusPolicyTests.swift`, +`supacodeTests/AppFeatureCommandPaletteTests.swift`. + +## Amendments + +(none yet) diff --git a/docs/components/command-palette.md b/docs/components/command-palette.md index df3d993e..672c1a3b 100644 --- a/docs/components/command-palette.md +++ b/docs/components/command-palette.md @@ -59,9 +59,10 @@ selected worktree has a pull request). suggestions. - PR and Canvas entries appear/disappear as state changes (PR present, Canvas active, etc.). -- Outgoing Changes compares committed work against the selected worktree's pull - request base; it reports an error rather than guessing when Prowl cannot - resolve that base. +- Outgoing Changes compares committed work against a labeled base (pull + request base → worktree base setting → default branch); a base that exists + but cannot be resolved reports a specific error rather than cascading to a + guess. See [diff-view](diff-view.md). - In Canvas, worktree-scoped actions use the focused card as their context. - There are no user settings for the palette; ranking is automatic. diff --git a/docs/components/diff-view.md b/docs/components/diff-view.md index 64aa385b..76b31216 100644 --- a/docs/components/diff-view.md +++ b/docs/components/diff-view.md @@ -3,7 +3,7 @@ > A dedicated window showing what changed in a worktree vs HEAD — review an > agent's work before you commit. -**Keywords:** diff, diff view, outgoing changes, changes, review, working tree, HEAD, split, unified, line changes, ⌘⇧Y, show diff +**Keywords:** diff, diff view, outgoing changes, changes, review, working tree, HEAD, split, unified, line changes, ⌘⇧Y, ⌘⌥⇧Y, show diff, uncommitted, base branch **Related:** [repositories-and-worktrees](repositories-and-worktrees.md) · [github-pull-requests](github-pull-requests.md) · [command-palette](command-palette.md) @@ -13,26 +13,43 @@ The default Diff window shows all changes in the selected worktree's working directory compared against **HEAD** — exactly what an agent has modified. It's a fast way to review before committing or merging. -**Open:** click a worktree's diff badge, press `⌘⇧Y` (`show_diff`), or use -Command Palette → "Show Diff". +**Open:** click a worktree's diff badge, press `⌘⇧Y` (`show_diff`), use +Command Palette → "Show Diff", or right-click a worktree row → "Show Diff". ## Outgoing Changes -**Outgoing Changes** is a separate built-in view for the committed changes a -selected worktree contributes to its pull request. It does not change the -working-tree semantics of Show Diff or its line-change badge. - -**Open:** View → Outgoing Changes, or Command Palette → "Outgoing Changes". - -Prowl uses the pull request's target repository and base branch to find its -matching local remote, then compares `git diff ...HEAD`. It reads the -merge-base and `HEAD` snapshots, so staged, unstaged, and untracked files are -excluded. On each focus refresh it captures a new consistent comparison. - -Outgoing Changes requires a pull request with a resolvable, fetched target -base. If Prowl cannot determine one, it shows an error instead of guessing a -default branch. It always uses the built-in window; the external Diff Tool -setting applies only to Show Diff. +**Outgoing Changes** is the second mode of the same window: the committed +changes the worktree's branch would contribute to a pull request +(`git diff ...HEAD`). It does not change the working-tree semantics of +Show Diff or its line-change badge. + +**Open:** View → Outgoing Changes, press `⌘⌥⇧Y` (`outgoing_changes`), use +Command Palette → "Outgoing Changes", right-click a worktree row → +"Outgoing Changes", or flip the window's **Uncommitted | Outgoing** toolbar +switcher. + +The comparison base is resolved by a strict ladder and always shown in the +window title and file-list header (e.g. `vs origin/main · pull request base`): + +1. **Pull request base** — the PR's target repository is matched to exactly + one local remote; the comparison uses `refs/remotes//`. +2. **Worktree base setting** — the repository's configured + `worktreeBaseRef` (Settings → repository → Base Branch), when no PR exists. +3. **Default branch** — the automatic base (`origin/HEAD`, falling back to + the local default branch), when nothing is configured. + +A source that is present but unresolvable (e.g. an unfetched PR base, or a +configured base branch that no longer exists) produces a specific error with +guidance instead of silently falling through to the next source. Multiple +remotes matching the PR repository is reported as its own error, listing the +conflicting remote names. + +Prowl reads merge-base and `HEAD` snapshots, so staged, unstaged, and +untracked files are excluded. Every focus refresh re-runs the full base +resolution, so a pull request created, retargeted, or closed while the window +is open moves the base (visibly) on the next refresh. Outgoing Changes always +uses the built-in window; the external Diff Tool setting applies only to Show +Diff. For **Show Diff**, Prowl opens its built-in YiTong-based diff window by default. In Settings → General → Diff Tool, you can choose an external tool instead: @@ -85,8 +102,10 @@ Diff is a **git-only** feature — it's unavailable for plain (non-git) folders. - The diff is **working-tree vs HEAD**, not vs the base branch — it reflects uncommitted changes in that worktree. -- Outgoing Changes is **merge-base vs HEAD** for an identified pull request; - it excludes all uncommitted files and does not guess a base branch. +- Outgoing Changes is **merge-base vs HEAD** against a labeled base + (PR base → worktree base setting → default branch); it excludes all + uncommitted files. A present-but-unresolvable base errors out rather than + cascading to a guess. - External GUI tools receive snapshot folders so untracked files are included without changing the git index. - The Hunk integration runs in a terminal tab because Hunk is terminal-native. diff --git a/docs/reference/keyboard-shortcuts.md b/docs/reference/keyboard-shortcuts.md index b7ce289b..5038eac7 100644 --- a/docs/reference/keyboard-shortcuts.md +++ b/docs/reference/keyboard-shortcuts.md @@ -45,6 +45,7 @@ Symbols: **⌘** Command · **⇧** Shift · **⌥** Option · **⌃** Control | Select Previous Agent (in panel) | ⌥⌃↑ | `select_previous_active_agent` | yes | | Jump to Latest Unread | ⌘⌥U | `jump_to_latest_unread` | yes | | Show Diff | ⌘⇧Y | `show_diff` | yes | +| Outgoing Changes | ⌘⌥⇧Y | `outgoing_changes` | yes | | Toggle Canvas | ⌘⌥↩ | `toggle_canvas` | yes | | Toggle Shelf | ⌘⇧↩ | `toggle_shelf` | yes |