fix(pr): use default provider for PR review tasks

[jschwxrz]

summary:

• PR review tasks were opening with codex because review task provider metadata was missing and the UI fell back to codex

fix:

• ensures PR review tasks are created/loaded with agentId set to the configured default provider and backfills missing agentId on existing PR review tasks when they are opened

fixes #1438

[vercel]

@jschwxrz is attempting to deploy a commit to the General Action Team on Vercel.

A member of the Team first needs to authorize it.

[greptile-apps]

Greptile Summary

This PR fixes a bug where PR review tasks were opened with the codex provider because agentId was never set on those tasks, causing the UI to fall back to its default. The fix sets agentId to the configured defaultProvider when creating new PR review tasks in the main process (githubIpc.ts) and backfills agentId on existing tasks that are loaded without one. The renderer (OpenPrsSection.tsx) is also updated to propagate the returned agentId into the in-memory Task object passed to onReviewPr.

Key observations:

• The core bug fix is correct — buildTaskInfo now always includes agentId, and the backfill path correctly persists the updated task to the database.
• The IPC type declarations in electron-api.d.ts (and global.d.ts) were not updated to include agentId in the task return shape, requiring a type cast in the renderer.
• The renderer-side rpc.db.saveTask call guarded by !result.task.agentId (line 77) is dead code after this PR — the main process now guarantees agentId is always set on the returned task.
• A rpc.appSettings.get() round-trip is added to the renderer before every PR review action, but defaultProvider is already resolved in the main process and the renderer's copy is never actually used as a fallback in any reachable code path.
• The nested if in githubIpc.ts (lines 313–321) is logically correct but harder to read than necessary.

Confidence Score: 4/5

• Safe to merge — the core bug fix is correct and no critical logic errors were found; a few dead-code and type-declaration clean-ups are recommended but non-blocking.
• The fix correctly ensures agentId is set in all code paths (new task, existing task with missing agentId, existing task with agentId). The only issues found are style-level: outdated IPC type declarations, a dead-code renderer save, an unnecessary extra RPC round-trip, and a mildly confusing nested conditional. None of these affect runtime correctness.
• The agentId field should be added to the task return type in src/renderer/types/electron-api.d.ts and src/renderer/types/global.d.ts to close the type-safety gap introduced by this PR.

Important Files Changed

Filename	Overview
src/main/ipc/githubIpc.ts	Correctly adds agentId from getAppSettings().defaultProvider to newly-built PR review tasks and backfills it for existing tasks that are missing it; nested conditional logic is functionally correct but unnecessarily complex.
src/renderer/components/OpenPrsSection.tsx	Properly threads agentId into the task object passed to onReviewPr, but introduces a redundant rpc.appSettings.get() round-trip and a dead-code renderer-side saveTask call that can never fire after the main-process changes.

Sequence Diagram

sequenceDiagram
    participant UI as OpenPrsSection (Renderer)
    participant RPC as rpc (IPC Bridge)
    participant IPC as githubIpc (Main)
    participant DB as DatabaseService
    participant Settings as getAppSettings()

    UI->>RPC: appSettings.get()
    RPC-->>UI: { defaultProvider }

    UI->>IPC: githubCreatePullRequestWorktree(...)
    IPC->>Settings: getAppSettings().defaultProvider
    Settings-->>IPC: reviewProvider (e.g. 'claude')

    alt Existing worktree found
        IPC->>DB: getTaskByPath(existing.path)
        DB-->>IPC: persistedTask (may have agentId=null)

        alt persistedTask missing agentId
            Note over IPC: Backfill: existingTask = {...persistedTask, agentId: reviewProvider}
            IPC->>DB: saveTask(existingTask)
        else persistedTask has agentId
            Note over IPC: No save needed
        end

        IPC-->>UI: { success, worktree, task: existingTask (agentId set) }
    else New worktree
        Note over IPC: buildTaskInfo() always sets agentId: reviewProvider
        IPC->>DB: saveTask(taskInfo)
        IPC-->>UI: { success, worktree, task: taskInfo (agentId set) }
    end

    UI->>UI: Build Task with agentId from result.task.agentId
    UI->>UI: onReviewPr(task)

Loading

Comments Outside Diff (2)

1. src/renderer/types/electron-api.d.ts, line 880-888 (link)

   agentId missing from task return type declaration

   The main process (githubIpc.ts) now always sets agentId on the returned task object, but neither the task type in this file (lines 880–888) nor the parallel declaration in global.d.ts has been updated to reflect this. This is why the renderer must use (result.task as { agentId?: string | null }).agentId — a cast that would be unnecessary if the declared type were accurate. The declaration should be updated so TypeScript can enforce the contract without casts:

   The same update is needed at line 1692–1700 in this file and in src/renderer/types/global.d.ts.

2. src/main/ipc/githubIpc.ts, line 313-321 (link)

   Redundant nested condition

   The inner if (persistedTask && !persistedTask.agentId) is logically implied by the outer if (!persistedTask || !persistedTask.agentId) — when !persistedTask is true the inner condition is always false (no-op), and when !persistedTask.agentId is true the inner condition is always true. The nesting adds cognitive overhead without guarding any additional case. A clearer equivalent:

   if (!persistedTask || !persistedTask.agentId) {
       if (persistedTask) {
           existingTask = { ...persistedTask, agentId: reviewProvider };
       }
       try {
           await databaseService.saveTask(existingTask);
       } catch (dbError) {
           log.warn('Failed to save existing PR review task to database:', dbError);
       }
   }

_{Last reviewed commit: 442e32e}

[arnestrickmann]

🫡