Fix PR creation logic to check PR existence instead of branch existence

This fixes a critical bug where PRs would fail to be created on subsequent runs
if the branch already existed from a previous failed PR creation attempt.

Changes:
- Check for open PR existence instead of branch existence in coana-fix
- Clean up stale branches that exist without open PRs
- Add detailed error handling for PR creation failures
- Return typed OpenPrResult instead of undefined on failures

Author: jdalton

src/commands/fix/coana-fix.mts

@@ -25,6 +25,7 @@ import {
   gitCommit,
   gitCreateBranch,
   gitDeleteBranch,
+  gitDeleteRemoteBranch,
   gitPushBranch,
   gitRemoteBranchExists,
   gitResetAndClean,
@@ -341,13 +342,33 @@ export async function coanaFix(
     const branch = getSocketFixBranchName(ghsaId)
 
     try {
-      // Check if branch already exists.
+      // Check if an open PR already exists for this GHSA.
       // eslint-disable-next-line no-await-in-loop
-      if (await gitRemoteBranchExists(branch, cwd)) {
-        debugFn('notice', `skip: remote branch "${branch}" exists`)
+      const existingOpenPrs = await getSocketFixPrs(
+        fixEnv.repoInfo.owner,
+        fixEnv.repoInfo.repo,
+        { ghsaId, states: GQL_PR_STATE_OPEN },
+      )
+
+      if (existingOpenPrs.length > 0) {
+        const prNum = existingOpenPrs[0]!.number
+        logger.info(`PR #${prNum} already exists for ${ghsaId}, skipping.`)
+        debugFn('notice', `skip: open PR #${prNum} exists for ${ghsaId}`)
         continue ghsaLoop
       }
 
+      // If branch exists but no open PR, delete the stale branch.
+      // This handles cases where PR creation failed but branch was pushed.
+      // eslint-disable-next-line no-await-in-loop
+      if (await gitRemoteBranchExists(branch, cwd)) {
+        logger.warn(
+          `Stale branch ${branch} found without open PR, cleaning up...`,
+        )
+        debugFn('notice', `cleanup: deleting stale branch ${branch}`)
+        // eslint-disable-next-line no-await-in-loop
+        await gitDeleteRemoteBranch(branch)
+      }
+
       debugFn('notice', `pr: creating for ${ghsaId}`)
 
       const details = ghsaDetails.get(ghsaId)
@@ -408,7 +429,7 @@ export async function coanaFix(
       )
 
       // eslint-disable-next-line no-await-in-loop
-      const prResponse = await openSocketFixPr(
+      const prResult = await openSocketFixPr(
         fixEnv.repoInfo.owner,
         fixEnv.repoInfo.repo,
         branch,
@@ -421,8 +442,8 @@ export async function coanaFix(
         },
       )
 
-      if (prResponse) {
-        const { data } = prResponse
+      if (prResult.ok) {
+        const { data } = prResult.pr
         const prRef = `PR #${data.number}`
 
         logger.success(`Opened ${prRef} for ${ghsaId}.`)
@@ -443,6 +464,29 @@ export async function coanaFix(
           logger.dedent()
           spinner?.dedent()
         }
+      } else {
+        // Handle PR creation failures.
+        if (prResult.reason === 'already_exists') {
+          logger.info(
+            `PR already exists for ${ghsaId} (this should not happen due to earlier check).`,
+          )
+        } else if (prResult.reason === 'validation_error') {
+          logger.error(
+            `Failed to create PR for ${ghsaId}:\n${prResult.details}`,
+          )
+        } else if (prResult.reason === 'permission_denied') {
+          logger.error(
+            `Failed to create PR for ${ghsaId}: Permission denied. Check SOCKET_CLI_GITHUB_TOKEN permissions.`,
+          )
+        } else if (prResult.reason === 'network_error') {
+          logger.error(
+            `Failed to create PR for ${ghsaId}: Network error. Please try again.`,
+          )
+        } else {
+          logger.error(
+            `Failed to create PR for ${ghsaId}: ${prResult.error.message}`,
+          )
+        }
       }
 
       // Reset back to base branch for next iteration.

src/commands/fix/pull-request.mts

@@ -35,13 +35,26 @@ export type OpenSocketFixPrOptions = {
   ghsaDetails?: Map<string, GhsaDetails> | undefined
 }
 
+export type OpenPrResult =
+  | { ok: true; pr: OctokitResponse<Pr> }
+  | { ok: false; reason: 'already_exists'; error: RequestError }
+  | {
+      ok: false
+      reason: 'validation_error'
+      error: RequestError
+      details: string
+    }
+  | { ok: false; reason: 'permission_denied'; error: RequestError }
+  | { ok: false; reason: 'network_error'; error: RequestError }
+  | { ok: false; reason: 'unknown'; error: Error }
+
 export async function openSocketFixPr(
   owner: string,
   repo: string,
   branch: string,
   ghsaIds: string[],
   options?: OpenSocketFixPrOptions | undefined,
-): Promise<OctokitResponse<Pr> | undefined> {
+): Promise<OpenPrResult> {
   const { baseBranch = 'main', ghsaDetails } = {
     __proto__: null,
     ...options,
@@ -59,25 +72,51 @@ export async function openSocketFixPr(
       body: getSocketFixPullRequestBody(ghsaIds, ghsaDetails),
     }
     debugDir('inspect', { octokitPullsCreateParams })
-    return await octokit.pulls.create(octokitPullsCreateParams)
+    const pr = await octokit.pulls.create(octokitPullsCreateParams)
+    return { ok: true, pr }
   } catch (e) {
-    let message = `Failed to open pull request`
-    const errors =
-      e instanceof RequestError
-        ? (e.response?.data as any)?.['errors']
-        : undefined
-    if (Array.isArray(errors) && errors.length) {
-      const details = errors
-        .map(
-          d =>
-            `- ${d.message?.trim() ?? `${d.resource}.${d.field} (${d.code})`}`,
+    // Handle RequestError from Octokit.
+    if (e instanceof RequestError) {
+      const errors = (e.response?.data as any)?.['errors']
+      const errorMessages = Array.isArray(errors)
+        ? errors.map(
+            d => d.message?.trim() ?? `${d.resource}.${d.field} (${d.code})`,
+          )
+        : []
+
+      // Check for "PR already exists" error.
+      if (
+        errorMessages.some(msg =>
+          msg.toLowerCase().includes('pull request already exists'),
         )
-        .join('\n')
-      message += `:\n${details}`
+      ) {
+        debugFn('error', 'Failed to open pull request: already exists')
+        return { ok: false, reason: 'already_exists', error: e }
+      }
+
+      // Check for validation errors (e.g., no commits between branches).
+      if (errors && errors.length > 0) {
+        const details = errorMessages.map(d => `- ${d}`).join('\n')
+        debugFn('error', `Failed to open pull request:\n${details}`)
+        return { ok: false, reason: 'validation_error', error: e, details }
+      }
+
+      // Check HTTP status codes.
+      if (e.status === 403 || e.status === 401) {
+        debugFn('error', 'Failed to open pull request: permission denied')
+        return { ok: false, reason: 'permission_denied', error: e }
+      }
+
+      if (e.status && e.status >= 500) {
+        debugFn('error', 'Failed to open pull request: network error')
+        return { ok: false, reason: 'network_error', error: e }
+      }
     }
-    debugFn('error', message)
+
+    // Unknown error.
+    debugFn('error', `Failed to open pull request: ${e}`)
+    return { ok: false, reason: 'unknown', error: e as Error }
   }
-  return undefined
 }
 
 export type GQL_MERGE_STATE_STATUS =