Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 83 additions & 8 deletions apps/desktop/src/main/ai/runners/github/pr-creator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,56 @@ Write a professional PR description. Output ONLY the Markdown body — no preamb
}
}

// =============================================================================
// Auto-Commit Uncommitted Changes
// =============================================================================

/**
* Stage and commit any uncommitted changes in the worktree.
* Called before pushing to ensure the branch has commits to push.
* Returns an error string on failure, or undefined on success (or no changes).
*/
function autoCommitWorktreeChanges(
worktreePath: string,
gitPath: string,
specId: string,
): string | undefined {
try {
// Check for uncommitted changes (staged or unstaged, tracked or untracked)
const status = execFileSync(
gitPath,
['status', '--porcelain'],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
).trim();

if (!status) {
// No uncommitted changes — nothing to do
return undefined;
}

// Stage all changes
execFileSync(
gitPath,
['add', '.'],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
);

// Commit with a descriptive message
execFileSync(
gitPath,
['commit', '-m', `auto-claude: implement ${specId}`],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
);

return undefined;
} catch (err: unknown) {
const stderr = err instanceof Error && 'stderr' in err
? String((err as NodeJS.ErrnoException & { stderr?: string }).stderr)
: String(err);
return stderr || 'Auto-commit failed';
}
}

// =============================================================================
// Push Branch
// =============================================================================
Expand Down Expand Up @@ -296,7 +346,17 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>
thinkingLevel = 'low',
} = config;

// Step 1: Push the branch to origin
// Strip remote prefix from base branch once — used in all steps below
const effectiveBase = baseBranch.startsWith('origin/')
? baseBranch.slice('origin/'.length)
: baseBranch;

// Step 1a: Auto-commit any uncommitted changes in the worktree before pushing.
// This handles the case where the agent wrote files but didn't run `git commit`.
// A failure here is non-fatal — we still attempt the push in case prior commits exist.
autoCommitWorktreeChanges(worktreePath, gitPath, specId);

Comment on lines +354 to +358
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle auto-commit errors instead of discarding them.

autoCommitWorktreeChanges(...) can fail, but its return value is ignored. That can silently create a PR from stale commits while newly generated worktree changes remain uncommitted.

Proposed fix
-  autoCommitWorktreeChanges(worktreePath, gitPath, specId);
+  const autoCommitError = autoCommitWorktreeChanges(worktreePath, gitPath, specId);
+  if (autoCommitError) {
+    return {
+      success: false,
+      error: `Failed to auto-commit worktree changes: ${autoCommitError}`,
+    };
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts` around lines 354 -
358, autoCommitWorktreeChanges(...) is being called and its result ignored,
which can let PRs be created without new worktree changes; change the call to
await and handle failures: call await autoCommitWorktreeChanges(worktreePath,
gitPath, specId) and if it returns a falsy value or throws, log an error
including specId and either abort PR creation (return/throw) or surface the
error to the caller so the push/PR step does not proceed with stale commits;
update any surrounding control flow in the PR creator to propagate the failure
instead of silently continuing.

// Step 1b: Push the branch to origin
const pushError = pushBranch(worktreePath, gitPath, branchName);
if (pushError) {
// If it looks like the branch is already up-to-date, don't bail
Expand All @@ -307,6 +367,26 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>
}
}

// Step 1c: Verify there is at least one commit ahead of the base branch.
// Without this check, `gh pr create` fails with a confusing GraphQL error.
try {
const commitCount = execFileSync(
gitPath,
['rev-list', '--count', `origin/${effectiveBase}..HEAD`],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
).trim();
if (commitCount === '0') {
return {
success: false,
error: `No commits found between '${effectiveBase}' and '${branchName}'. ` +
'The agent may not have committed any changes. ' +
'Please verify the implementation completed successfully before creating a PR.',
};
}
} catch {
// Unable to count commits — proceed and let `gh pr create` surface the error
}
Comment on lines +372 to +388
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The rev-list check assumes the remote is always named origin and that the base branch can be reached via origin/${effectiveBase}. This will fail if the remote has a different name (e.g., upstream) or if the base branch already includes a remote prefix.

Consider using a fallback pattern similar to the one used in gatherPRContext to improve robustness.

  try {
    let commitCount: string;
    try {
      commitCount = execFileSync(
        gitPath,
        ['rev-list', '--count', `origin/${effectiveBase}..HEAD`],
        { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
      ).trim();
    } catch {
      // Fallback to using the base branch directly if origin/ prefix fails
      commitCount = execFileSync(
        gitPath,
        ['rev-list', '--count', `${baseBranch}..HEAD`],
        { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
      ).trim();
    }

    if (commitCount === '0') {
      return {
        success: false,
        error: `No commits found between '${effectiveBase}' and '${branchName}'. ` +
          'The agent may not have committed any changes. ' +
          'Please verify the implementation completed successfully before creating a PR.',
      };
    }
  } catch {
    // Unable to count commits — proceed and let `gh pr create` surface the error
  }


// Step 2: Gather context for AI description
const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, baseBranch);

Expand All @@ -324,12 +404,7 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>

const prBody = aiBody || extractSpecSummary(projectDir, specId);

// Step 4: Strip remote prefix from base branch if present
const effectiveBase = baseBranch.startsWith('origin/')
? baseBranch.slice('origin/'.length)
: baseBranch;

// Step 5: Build gh pr create command
// Step 4: Build gh pr create command
const ghArgs = [
'pr', 'create',
'--base', effectiveBase,
Expand All @@ -342,7 +417,7 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>
ghArgs.push('--draft');
}

// Step 6: Execute gh pr create with retry on network errors
// Step 5: Execute gh pr create with retry on network errors
for (let attempt = 0; attempt < 3; attempt++) {
try {
const output = execFileSync(ghPath, ghArgs, {
Expand Down
10 changes: 9 additions & 1 deletion apps/desktop/src/main/ipc-handlers/terminal-handlers.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ipcMain } from 'electron';
import { ipcMain, clipboard } from 'electron';
import type { BrowserWindow, IpcMainInvokeEvent } from 'electron';
import { IPC_CHANNELS } from '../../shared/constants';
import type { IPCResult, TerminalCreateOptions, ClaudeProfile, ClaudeProfileSettings, ClaudeUsageSnapshot, AllProfilesUsage } from '../../shared/types';
Expand Down Expand Up @@ -730,6 +730,14 @@ export function registerTerminalHandlers(
}
}
);

// Read clipboard text via main process
// navigator.clipboard.readText() in the renderer requires the "clipboard-read" permission
// which can be silently denied on Windows, causing paste to fail. Using Electron's
// clipboard module from the main process bypasses this permission requirement.
ipcMain.handle(IPC_CHANNELS.TERMINAL_READ_CLIPBOARD, (): string => {
return clipboard.readText();
});
}

/**
Expand Down
5 changes: 5 additions & 0 deletions apps/desktop/src/preload/api/terminal-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ export interface TerminalAPI {
projectPath: string,
orders: Array<{ terminalId: string; displayOrder: number }>
) => Promise<IPCResult>;
/** Read clipboard text via main process (avoids renderer permission issues on Windows) */
readClipboardText: () => Promise<string>;

// Terminal Worktree Operations (isolated development)
createTerminalWorktree: (request: CreateTerminalWorktreeRequest) => Promise<TerminalWorktreeResult>;
Expand Down Expand Up @@ -197,6 +199,9 @@ export const createTerminalAPI = (): TerminalAPI => ({
): Promise<IPCResult> =>
ipcRenderer.invoke(IPC_CHANNELS.TERMINAL_UPDATE_DISPLAY_ORDERS, projectPath, orders),

readClipboardText: (): Promise<string> =>
ipcRenderer.invoke(IPC_CHANNELS.TERMINAL_READ_CLIPBOARD),

// Terminal Worktree Operations (isolated development)
createTerminalWorktree: (request: CreateTerminalWorktreeRequest): Promise<TerminalWorktreeResult> =>
ipcRenderer.invoke(IPC_CHANNELS.TERMINAL_WORKTREE_CREATE, request),
Expand Down
18 changes: 15 additions & 3 deletions apps/desktop/src/renderer/components/terminal/useXterm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ export function useXterm({ terminalId, onCommandEnter, onResize, onDimensionsRea
// Cap paste size to prevent GPU/memory pressure from extremely large clipboard contents.
const MAX_PASTE_BYTES = 1_048_576; // 1 MB
const handlePasteFromClipboard = (): void => {
navigator.clipboard.readText()
// Use Electron's main-process clipboard API via IPC, which is reliable on all
// platforms including Windows (where navigator.clipboard.readText() may silently
// fail due to renderer permission restrictions, causing paste to appear and vanish).
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: A synchronous TypeError occurs if window.electronAPI is undefined, preventing the fallback to navigator.clipboard and breaking paste functionality in non-Electron contexts.
Severity: HIGH

Suggested Fix

Use optional chaining (?.) on window.electronAPI when calling readClipboardText(). This will prevent a synchronous error and allow the .catch() block to execute the fallback logic as intended.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: apps/desktop/src/renderer/components/terminal/useXterm.ts#L174

Potential issue: In non-Electron contexts where `window.electronAPI` is `undefined`, the
direct property access in `window.electronAPI.readClipboardText()` will throw a
synchronous `TypeError`. This error is not handled by the `.catch()` block, which only
catches promise rejections. As a result, the fallback logic that uses
`navigator.clipboard.readText()` is unreachable. This breaks the clipboard paste
functionality in any environment where the Electron IPC API is not available, which
contradicts the stated goal of supporting non-Electron web contexts.

Did we get this right? 👍 / 👎 to inform future reviews.

window.electronAPI.readClipboardText()
.then((text) => {
Comment on lines +172 to 176
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard window.electronAPI before invoking clipboard IPC.

This call can throw synchronously when preload API is unavailable, which bypasses the .catch fallback and breaks paste in web/test contexts.

💡 Proposed fix
     const MAX_PASTE_BYTES = 1_048_576; // 1 MB
     const handlePasteFromClipboard = (): void => {
+      const pasteText = (text: string) => {
+        if (!text) return;
+        if (text.length > MAX_PASTE_BYTES) {
+          console.warn(`[useXterm] Paste truncated from ${text.length} to ${MAX_PASTE_BYTES} bytes`);
+          xterm.paste(text.slice(0, MAX_PASTE_BYTES));
+        } else {
+          xterm.paste(text);
+        }
+      };
+
+      const readFromNavigator = () =>
+        navigator.clipboard.readText()
+          .then(pasteText)
+          .catch((err) => {
+            console.error('[useXterm] Failed to read clipboard:', err);
+          });
+
       // Use Electron's main-process clipboard API via IPC, which is reliable on all
       // platforms including Windows (where navigator.clipboard.readText() may silently
       // fail due to renderer permission restrictions, causing paste to appear and vanish).
-      window.electronAPI.readClipboardText()
-        .then((text) => {
-          if (text) {
-            if (text.length > MAX_PASTE_BYTES) {
-              console.warn(`[useXterm] Paste truncated from ${text.length} to ${MAX_PASTE_BYTES} bytes`);
-              xterm.paste(text.slice(0, MAX_PASTE_BYTES));
-            } else {
-              xterm.paste(text);
-            }
-          }
-        })
-        .catch(() => {
-          // Fall back to navigator.clipboard if IPC unavailable
-          navigator.clipboard.readText()
-            .then((text) => {
-              if (text) {
-                xterm.paste(text.length > MAX_PASTE_BYTES ? text.slice(0, MAX_PASTE_BYTES) : text);
-              }
-            })
-            .catch((err) => {
-              console.error('[useXterm] Failed to read clipboard:', err);
-            });
-        });
+      if (window.electronAPI?.readClipboardText) {
+        window.electronAPI.readClipboardText()
+          .then(pasteText)
+          .catch(() => {
+            // Fall back to navigator.clipboard if IPC is unavailable or fails
+            readFromNavigator();
+          });
+      } else {
+        readFromNavigator();
+      }
     };

Also applies to: 186-196

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/renderer/components/terminal/useXterm.ts` around lines 172 -
176, The call to window.electronAPI.readClipboardText() can throw synchronously
when the preload API is absent; update the paste logic in useXterm.ts to first
guard that window.electronAPI exists and that readClipboardText is a function
before invoking it, and fall back to navigator.clipboard.readText() (or other
existing fallback path) if the guard fails; apply the same protective
check/try-catch pattern to the other clipboard invocation in the same module
(the later readClipboardText usage) so both places avoid synchronous exceptions
in web/test contexts.

if (text) {
if (text.length > MAX_PASTE_BYTES) {
Expand All @@ -180,8 +183,17 @@ export function useXterm({ terminalId, onCommandEnter, onResize, onDimensionsRea
}
}
})
.catch((err) => {
console.error('[useXterm] Failed to read clipboard:', err);
.catch(() => {
// Fall back to navigator.clipboard if IPC unavailable
navigator.clipboard.readText()
.then((text) => {
if (text) {
xterm.paste(text.length > MAX_PASTE_BYTES ? text.slice(0, MAX_PASTE_BYTES) : text);
}
})
.catch((err) => {
console.error('[useXterm] Failed to read clipboard:', err);
});
});
};

Expand Down
1 change: 1 addition & 0 deletions apps/desktop/src/shared/constants/ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export const IPC_CHANNELS = {
TERMINAL_RESTORE_FROM_DATE: 'terminal:restoreFromDate',
TERMINAL_CHECK_PTY_ALIVE: 'terminal:checkPtyAlive',
TERMINAL_UPDATE_DISPLAY_ORDERS: 'terminal:updateDisplayOrders', // Persist terminal display order after drag-drop reorder
TERMINAL_READ_CLIPBOARD: 'terminal:readClipboard', // Read clipboard text via main process (avoids renderer permission issues on Windows)

// Terminal worktree operations (isolated development in worktrees)
TERMINAL_WORKTREE_CREATE: 'terminal:worktreeCreate',
Expand Down
Loading