Address Copilot review feedback on stash/cherry-pick

- Stash now includes untracked files (git stash push -u) so an
  untracked-only change doesn't silently no-op while the UI reports success.
- CherryPickConflictError carries an abortFailed flag and gives an accurate
  message (rather than claiming a clean rollback) when `cherry-pick --abort`
  itself fails.
- StashPanel: block Enter-to-create while a stash op is pending; disable
  every stash row's actions while any stash mutation is in flight, not just
  the one that was clicked (apply/pop/drop aren't safely concurrent since
  they share the same working tree/index); show an explicit error state
  instead of rendering "No stashes" when the list query fails.

The other flagged lines (packages/git/src/diff.ts, packages/git/src/staging.ts
trailing-trim) were already superseded by main's independent fix for the same
status-parsing bug, adopted during the merge — no longer applicable.

Author: liam-russell

packages/git/src/__tests__/cherry-pick.test.ts

@@ -64,7 +64,10 @@ describe('cherryPickCommit', () => {
     execSync('git add .', { cwd: repoPath, stdio: 'ignore' });
     execSync('git commit -m "main change"', { cwd: repoPath, stdio: 'ignore' });
 
-    await expect(cherryPickCommit(repoPath, featureSha)).rejects.toThrow(CherryPickConflictError);
+    const error = await cherryPickCommit(repoPath, featureSha).catch((e: unknown) => e);
+    expect(error).toBeInstanceOf(CherryPickConflictError);
+    expect((error as CherryPickConflictError).abortFailed).toBe(false);
+    expect((error as CherryPickConflictError).message).toContain('was rolled back');
 
     // The cherry-pick must be fully rolled back — no in-progress state, no
     // conflict markers left behind, working tree clean.

packages/git/src/__tests__/stash.test.ts

@@ -103,6 +103,22 @@ describe('stash', () => {
     expect(content).not.toContain('dropped');
   });
 
+  it('includes untracked files in the stash (not just tracked modifications)', async () => {
+    writeFileSync(join(repoPath, 'untracked.txt'), 'new file\n');
+
+    await createStash(repoPath, 'untracked file');
+
+    const status = execSync('git status --porcelain', { cwd: repoPath }).toString();
+    expect(status.trim()).toBe('');
+
+    const result = await listStashes(repoPath);
+    expect(result.stashes).toHaveLength(1);
+
+    await popStash(repoPath, 'stash@{0}');
+    const content = execSync('cat untracked.txt', { cwd: repoPath }).toString();
+    expect(content).toBe('new file\n');
+  });
+
   it('lists multiple stashes newest first, matching stash@{N} refs', async () => {
     writeFileSync(join(repoPath, 'README.md'), '# Test Repo\nfirst\n');
     await createStash(repoPath, 'first stash');

packages/git/src/cherry-pick.ts

@@ -1,9 +1,18 @@
 import { gitForPath } from './client.js';
 
-/** Thrown when a cherry-pick stops due to conflicts and is rolled back. */
+/**
+ * Thrown when a cherry-pick stops due to conflicts. `abortFailed` reflects
+ * whether `git cherry-pick --abort` itself succeeded — if it didn't, the
+ * worktree may still be mid-cherry-pick and needs manual attention, so the
+ * message must not claim a clean rollback happened.
+ */
 export class CherryPickConflictError extends Error {
-  constructor(public readonly sha: string) {
-    super(`Cherry-pick of ${sha.slice(0, 7)} stopped due to conflicts and was rolled back.`);
+  constructor(public readonly sha: string, public readonly abortFailed = false) {
+    super(
+      abortFailed
+        ? `Cherry-pick of ${sha.slice(0, 7)} stopped due to conflicts, and the automatic rollback failed. Run "git cherry-pick --abort" manually to restore a clean state.`
+        : `Cherry-pick of ${sha.slice(0, 7)} stopped due to conflicts and was rolled back.`
+    );
     this.name = 'CherryPickConflictError';
   }
 }
@@ -24,14 +33,13 @@ export async function cherryPickCommit(worktreePath: string, sha: string): Promi
   } catch (error) {
     if (!isCherryPickConflict(error)) throw error;
 
+    let abortFailed = false;
     try {
       await git.raw(['cherry-pick', '--abort']);
     } catch {
-      // Best-effort: if abort itself fails, surface the original conflict
-      // error below rather than the abort failure — the worktree may need
-      // manual attention either way, but the conflict is the actionable part.
+      abortFailed = true;
     }
-    throw new CherryPickConflictError(sha);
+    throw new CherryPickConflictError(sha, abortFailed);
   }
 }
 

packages/git/src/stash.ts

@@ -5,10 +5,15 @@ import { gitForPath } from './client.js';
  * Creates a stash of the current working-tree + index state.
  * Uses `stash push` (not the deprecated `stash save`) so an optional
  * message can be attached without relying on positional-arg parsing.
+ * Includes untracked files (`-u`) — the UI's "changes to stash" check counts
+ * untracked files too, so without `-u` a stash of an untracked-only change
+ * would silently no-op ("No local changes to save") while still reporting
+ * success.
  */
 export async function createStash(worktreePath: string, message?: string): Promise<void> {
   const git = gitForPath(worktreePath);
-  await git.stash(message ? ['push', '-m', message] : ['push']);
+  const args = message ? ['push', '-u', '-m', message] : ['push', '-u'];
+  await git.stash(args);
 }
 
 /**

packages/ui/src/components/StashPanel.tsx

@@ -46,7 +46,7 @@ export function StashPanel({
   const [message, setMessage] = useState('');
   const stashKey = ['stashList', worktreePath, refreshSignal] as const;
 
-  const { data: stashes = [] } = useQuery({
+  const { data: stashes = [], isError: stashesErrored } = useQuery({
     queryKey: stashKey,
     queryFn: async () => (await listStashes(worktreePath)).stashes,
     staleTime: 0,
@@ -97,6 +97,14 @@ export function StashPanel({
     onError: (err) => onToast?.(`Failed to drop stash: ${String(err)}`, 'error'),
   });
 
+  // Stash operations aren't safely parallelizable — apply/pop/drop all read
+  // and rewrite the same working tree + index, so running two at once (even
+  // against different stash entries) can race. Block every row's actions
+  // while any of the four mutations is in flight, not just the one the user
+  // clicked.
+  const anyStashOpPending =
+    createMutation.isPending || applyMutation.isPending || popMutation.isPending || dropMutation.isPending;
+
   const sectionHdr = 'flex items-center justify-between px-[10px] py-[5px] text-[11px] font-semibold text-(--sg-text-faint) uppercase tracking-[0.04em] shrink-0 bg-(--sg-surface)';
   const iconBtn = 'inline-flex items-center justify-center p-[3px] bg-transparent border-none cursor-pointer text-(--sg-text-faint) rounded-[4px] transition-colors hover:text-(--sg-text) hover:bg-(--sg-surface-raised) disabled:opacity-40 disabled:cursor-not-allowed';
 
@@ -124,13 +132,13 @@ export function StashPanel({
               placeholder="Stash message (optional)"
               value={message}
               onChange={e => setMessage(e.target.value)}
-              onKeyDown={e => { if (e.key === 'Enter' && hasChangesToStash) createMutation.mutate(message); }}
+              onKeyDown={e => { if (e.key === 'Enter' && hasChangesToStash && !anyStashOpPending) createMutation.mutate(message); }}
               data-testid="input-stash-message"
             />
             <button
               className="sg-btn--primary inline-flex shrink-0 items-center gap-1 rounded bg-(--sg-primary) px-2 py-1 text-[11px] font-medium text-white hover:bg-(--sg-primary-hover) disabled:cursor-not-allowed disabled:opacity-40 border-none cursor-pointer transition-colors"
               onClick={() => createMutation.mutate(message)}
-              disabled={!hasChangesToStash || createMutation.isPending}
+              disabled={!hasChangesToStash || anyStashOpPending}
               title={hasChangesToStash ? 'Stash current changes' : 'No changes to stash'}
               data-testid="btn-create-stash"
             >
@@ -139,9 +147,11 @@ export function StashPanel({
           </div>
 
           <div className="flex flex-col gap-0.5 max-h-40 overflow-y-auto">
-            {stashes.length === 0 && (
+            {stashesErrored ? (
+              <p className="px-0.5 py-1 text-[11px] text-(--sg-danger)">Failed to load stashes.</p>
+            ) : stashes.length === 0 ? (
               <p className="px-0.5 py-1 text-[11px] text-(--sg-text-faint) italic">No stashes</p>
-            )}
+            ) : null}
             {stashes.map(s => (
               <StashRow
                 key={s.ref}
@@ -150,6 +160,7 @@ export function StashPanel({
                 onApply={() => applyMutation.mutate(s.ref)}
                 onPop={() => popMutation.mutate(s.ref)}
                 onDrop={() => dropMutation.mutate(s.ref)}
+                disabled={anyStashOpPending}
                 applying={applyMutation.isPending && applyMutation.variables === s.ref}
                 popping={popMutation.isPending && popMutation.variables === s.ref}
                 dropping={dropMutation.isPending && dropMutation.variables === s.ref}
@@ -168,6 +179,7 @@ function StashRow({
   onApply,
   onPop,
   onDrop,
+  disabled,
   applying,
   popping,
   dropping,
@@ -177,11 +189,12 @@ function StashRow({
   onApply: () => void;
   onPop: () => void;
   onDrop: () => void;
+  /** True whenever *any* stash mutation (this row's or another's) is in flight. */
+  disabled: boolean;
   applying: boolean;
   popping: boolean;
   dropping: boolean;
 }) {
-  const busy = applying || popping || dropping;
   return (
     <div
       className="sg-stash-row flex items-center gap-1 text-[11px] py-1"
@@ -191,13 +204,13 @@ function StashRow({
       <span className="flex-1 overflow-hidden text-ellipsis whitespace-nowrap" title={entry.message}>
         {entry.ref} — {entry.message}
       </span>
-      <button className={iconBtnClass} onClick={onApply} disabled={busy} title="Apply (keep stash)" data-testid="btn-apply-stash">
+      <button className={iconBtnClass} onClick={onApply} disabled={disabled} title="Apply (keep stash)" data-testid="btn-apply-stash">
         {applying ? <Spinner size="sm" /> : <ArchiveRestore size={12} />}
       </button>
-      <button className={iconBtnClass} onClick={onPop} disabled={busy} title="Pop (apply and remove)" data-testid="btn-pop-stash">
+      <button className={iconBtnClass} onClick={onPop} disabled={disabled} title="Pop (apply and remove)" data-testid="btn-pop-stash">
         {popping ? <Spinner size="sm" /> : <Check size={12} />}
       </button>
-      <button className={iconBtnClass} onClick={onDrop} disabled={busy} title="Drop (delete without applying)" data-testid="btn-drop-stash">
+      <button className={iconBtnClass} onClick={onDrop} disabled={disabled} title="Drop (delete without applying)" data-testid="btn-drop-stash">
         {dropping ? <Spinner size="sm" /> : <Trash2 size={12} />}
       </button>
     </div>