fix(pi): address Cloud Code Review review and lint findings

Use the checked-out SHA for review submission, skip invalid/null inline comments, scrub GitGuardian-flagged test fixtures, and fix Biome import/format issues.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: 2 people

apps/sim/executor/handlers/pi/cloud-backend.ts

@@ -23,8 +23,8 @@ import {
   PI_SCRIPT,
   PI_TIMEOUT_MS,
   PROMPT_PATH,
-  raceAbort,
   REPO_DIR,
+  raceAbort,
   scrubGitSecrets,
 } from '@/executor/handlers/pi/cloud-shared'
 import { buildPiPrompt } from '@/executor/handlers/pi/context'

apps/sim/executor/handlers/pi/cloud-review-backend.test.ts

@@ -79,7 +79,9 @@ describe('runCloudReviewPi', () => {
       }
       return Promise.resolve({
         success: true,
-        output: { metadata: { html_url: 'https://github.com/octo/demo/pull/7#pullrequestreview-9' } },
+        output: {
+          metadata: { html_url: 'https://github.com/octo/demo/pull/7#pullrequestreview-9' },
+        },
       })
     })
     mockRun.mockImplementation(
@@ -150,9 +152,7 @@ describe('runCloudReviewPi', () => {
         event: 'COMMENT',
         body: 'Overall looks solid.',
         commit_id: 'deadbeef',
-        comments: [
-          { path: 'src/x.ts', body: 'Consider a null check', line: 12, side: 'RIGHT' },
-        ],
+        comments: [{ path: 'src/x.ts', body: 'Consider a null check', line: 12, side: 'RIGHT' }],
         apiKey: 'ghp_secret',
       })
     )
@@ -161,6 +161,69 @@ describe('runCloudReviewPi', () => {
     expect(result.prUrl).toBeUndefined()
   })
 
+  it('submits against the checked-out SHA when the API head moved', async () => {
+    mockRun.mockImplementation((command: string) => {
+      if (command.includes('git clone') || command.includes('git fetch')) {
+        return Promise.resolve({
+          stdout: '__HEAD_SHA__=clonedsha99',
+          stderr: '',
+          exitCode: 0,
+        })
+      }
+      if (command.includes('pi -p')) {
+        return Promise.resolve({ stdout: '', stderr: '', exitCode: 0 })
+      }
+      return Promise.resolve({ stdout: '', stderr: '', exitCode: 0 })
+    })
+
+    await runCloudReviewPi(baseParams(), { onEvent: vi.fn() })
+
+    expect(mockExecuteTool).toHaveBeenCalledWith(
+      'github_create_pr_review',
+      expect.objectContaining({ commit_id: 'clonedsha99' })
+    )
+  })
+
+  it('treats null comments as empty and drops invalid inline comments', async () => {
+    mockReadFile.mockResolvedValue(
+      JSON.stringify({
+        body: 'Summary only',
+        comments: [
+          null,
+          { path: 'a.ts', body: 'missing line' },
+          { path: 'b.ts', body: 'bad line', line: 0 },
+          { path: 'c.ts', body: 'ok', line: 4, side: 'RIGHT' },
+        ],
+      })
+    )
+
+    const result = await runCloudReviewPi(baseParams(), { onEvent: vi.fn() })
+
+    expect(mockExecuteTool).toHaveBeenCalledWith(
+      'github_create_pr_review',
+      expect.objectContaining({
+        body: 'Summary only',
+        comments: [{ path: 'c.ts', body: 'ok', line: 4, side: 'RIGHT' }],
+      })
+    )
+    expect(result.commentsPosted).toBe(1)
+  })
+
+  it('allows comments: null without aborting the review', async () => {
+    mockReadFile.mockResolvedValue(JSON.stringify({ body: 'No inline notes', comments: null }))
+
+    const result = await runCloudReviewPi(baseParams(), { onEvent: vi.fn() })
+
+    expect(mockExecuteTool).toHaveBeenCalledWith(
+      'github_create_pr_review',
+      expect.objectContaining({
+        body: 'No inline notes',
+        comments: [],
+      })
+    )
+    expect(result.commentsPosted).toBe(0)
+  })
+
   it('rejects a non-BYOK key', async () => {
     await expect(
       runCloudReviewPi(baseParams({ isBYOK: false }), { onEvent: vi.fn() })
@@ -178,9 +241,10 @@ describe('runCloudReviewPi', () => {
   })
 
   it('scrubs the token from clone failures', async () => {
+    // Avoid embedding a basic-auth URL (GitGuardian); scrubbing still covers bare tokens.
     mockRun.mockResolvedValue({
       stdout: '',
-      stderr: "fatal: unable to access 'https://x-access-token:ghp_secret@github.com/octo/demo.git/'",
+      stderr: 'fatal: Authentication failed for token ghp_secret',
       exitCode: 1,
     })
 

apps/sim/executor/handlers/pi/cloud-review-backend.ts

@@ -17,15 +17,15 @@ import {
   PI_SCRIPT,
   PI_TIMEOUT_MS,
   PROMPT_PATH,
-  raceAbort,
   REPO_DIR,
+  raceAbort,
   scrubGitSecrets,
 } from '@/executor/handlers/pi/cloud-shared'
 import { buildPiPrompt } from '@/executor/handlers/pi/context'
 import { applyPiEvent, createPiTotals, parseJsonLine } from '@/executor/handlers/pi/events'
 import { mapThinkingLevel, providerApiKeyEnvVar } from '@/executor/handlers/pi/keys'
-import type { CreatePRReviewComment } from '@/tools/github/types'
 import { executeTool } from '@/tools'
+import type { CreatePRReviewComment } from '@/tools/github/types'
 
 const logger = createLogger('PiCloudReviewBackend')
 
@@ -83,9 +83,7 @@ async function fetchPrContext(params: PiCloudReviewRunParams): Promise<PrContext
   })
 
   if (!result.success) {
-    throw new Error(
-      `Failed to fetch PR #${params.pullNumber}: ${result.error ?? 'unknown error'}`
-    )
+    throw new Error(`Failed to fetch PR #${params.pullNumber}: ${result.error ?? 'unknown error'}`)
   }
 
   const output = result.output as {
@@ -143,6 +141,15 @@ function buildPrContextMarkdown(params: PiCloudReviewRunParams, pr: PrContext):
     : content
 }
 
+function isPositiveInt(value: unknown): value is number {
+  return typeof value === 'number' && Number.isInteger(value) && value >= 1
+}
+
+/**
+ * Parses agent review JSON. Invalid inline comments are skipped so a usable
+ * summary body can still be submitted; comments without a valid line are dropped
+ * because GitHub rejects line-less review comments when commit_id is set.
+ */
 function parseReviewFindings(raw: string): ParsedReviewFindings {
   let parsed: unknown
   try {
@@ -161,30 +168,35 @@ function parseReviewFindings(raw: string): ParsedReviewFindings {
   }
 
   const comments: CreatePRReviewComment[] = []
-  if (record.comments !== undefined) {
+  // Treat null/undefined as "no comments" — agents often emit null for optional fields.
+  if (record.comments != null) {
     if (!Array.isArray(record.comments)) {
       throw new Error('Pi review output comments must be an array when present')
     }
-    for (const [index, item] of record.comments.entries()) {
-      if (!item || typeof item !== 'object') {
-        throw new Error(`Pi review comments[${index}] must be an object`)
-      }
+    for (const item of record.comments) {
+      if (!item || typeof item !== 'object') continue
       const comment = item as Record<string, unknown>
-      if (typeof comment.path !== 'string' || !comment.path.trim()) {
-        throw new Error(`Pi review comments[${index}].path is required`)
-      }
-      if (typeof comment.body !== 'string' || !comment.body.trim()) {
-        throw new Error(`Pi review comments[${index}].body is required`)
-      }
+      if (typeof comment.path !== 'string' || !comment.path.trim()) continue
+      if (typeof comment.body !== 'string' || !comment.body.trim()) continue
+      if (!isPositiveInt(comment.line)) continue
+
       const normalized: CreatePRReviewComment = {
         path: comment.path.trim(),
         body: comment.body,
+        line: comment.line,
+        side: comment.side === 'LEFT' || comment.side === 'RIGHT' ? comment.side : 'RIGHT',
       }
-      if (typeof comment.line === 'number') normalized.line = comment.line
-      if (comment.side === 'LEFT' || comment.side === 'RIGHT') normalized.side = comment.side
-      if (typeof comment.start_line === 'number') normalized.start_line = comment.start_line
-      if (comment.start_side === 'LEFT' || comment.start_side === 'RIGHT') {
-        normalized.start_side = comment.start_side
+      if (
+        isPositiveInt(comment.start_line) &&
+        comment.start_line < comment.line &&
+        (comment.start_side === undefined ||
+          comment.start_side === 'LEFT' ||
+          comment.start_side === 'RIGHT')
+      ) {
+        normalized.start_line = comment.start_line
+        if (comment.start_side === 'LEFT' || comment.start_side === 'RIGHT') {
+          normalized.start_side = comment.start_side
+        }
       }
       comments.push(normalized)
     }
@@ -327,7 +339,9 @@ export const runCloudReviewPi: PiBackendRun<PiCloudReviewRunParams> = async (par
         totals.finalText = findings.body
       }
 
-      const { reviewUrl, commentsPosted } = await submitReview(params, pr.headSha, findings)
+      // Submit against the SHA we actually checked out, not the earlier API fetch —
+      // the PR head can move between fetchPrContext and clone.
+      const { reviewUrl, commentsPosted } = await submitReview(params, clonedHead, findings)
 
       logger.info('Pi cloud review submitted', {
         owner: params.owner,

apps/sim/executor/handlers/pi/pi-handler.ts

@@ -68,11 +68,7 @@ export class PiBlockHandler implements BlockHandler {
 
     // Validate the mode up front so an invalid value reports a mode error rather
     // than a misattributed credential error from key resolution below.
-    if (
-      inputs.mode !== 'cloud' &&
-      inputs.mode !== 'cloud_review' &&
-      inputs.mode !== 'local'
-    ) {
+    if (inputs.mode !== 'cloud' && inputs.mode !== 'cloud_review' && inputs.mode !== 'local') {
       throw new Error(`Invalid Pi mode: ${String(inputs.mode)}`)
     }
     const mode: 'cloud' | 'cloud_review' | 'local' = inputs.mode