diff --git a/action.yml b/action.yml index 838724f..8009959 100644 --- a/action.yml +++ b/action.yml @@ -307,7 +307,7 @@ runs: - name: Run Droid Exec (validator) id: droid_validator - if: steps.prepare.outputs.contains_trigger == 'true' && steps.prepare.outputs.run_code_review == 'true' + if: steps.prepare.outputs.contains_trigger == 'true' && steps.prepare.outputs.run_code_review == 'true' && steps.prepare_validator.outputs.validator_should_run != 'false' shell: bash run: | @@ -345,7 +345,7 @@ runs: GITHUB_EVENT_NAME: ${{ github.event_name }} TRIGGER_COMMENT_ID: ${{ github.event.comment.id }} IS_PR: ${{ github.event.issue.pull_request != null || github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review_comment' }} - DROID_SUCCESS: ${{ (steps.prepare.outputs.run_code_review == 'true' && steps.droid_validator.outputs.conclusion == 'success') || (steps.prepare.outputs.run_code_review != 'true' && steps.droid.outputs.conclusion == 'success') }} + DROID_SUCCESS: ${{ (steps.prepare.outputs.run_code_review == 'true' && ((steps.prepare_validator.outputs.validator_should_run == 'false' && steps.droid.outputs.conclusion == 'success') || (steps.prepare_validator.outputs.validator_should_run != 'false' && steps.droid_validator.outputs.conclusion == 'success'))) || (steps.prepare.outputs.run_code_review != 'true' && steps.droid.outputs.conclusion == 'success') }} TRIGGER_USERNAME: ${{ github.event.comment.user.login || github.event.issue.user.login || github.event.pull_request.user.login || github.event.sender.login || github.triggering_actor || github.actor || '' }} PREPARE_SUCCESS: ${{ steps.prepare.outcome == 'success' }} PREPARE_ERROR: ${{ steps.prepare.outputs.prepare_error || '' }} diff --git a/base-action/src/run-droid.ts b/base-action/src/run-droid.ts index 015dc98..2446db0 100644 --- a/base-action/src/run-droid.ts +++ b/base-action/src/run-droid.ts @@ -3,9 +3,15 @@ import { exec, spawn } from "child_process"; import { promisify } from "util"; import { stat } from "fs/promises"; import { parse as parseShellArgs } from "shell-quote"; +import { retryWithBackoff } from "./utils/retry"; const execAsync = promisify(exec); +/** Redact inline `--env KEY=value` secrets before logging a command string. */ +function redactEnvSecrets(text: string): string { + return text.replace(/--env\s+(\S+?)=\S+/g, "--env $1=***"); +} + const BASE_ARGS = [ "exec", "--output-format", @@ -138,13 +144,6 @@ export async function runDroid(promptPath: string, options: DroidOptions) { .filter(Boolean) .join(" "); - // Remove existing server if present (ignore errors) - try { - await execAsync(`droid mcp remove ${name}`); - } catch (_) { - // Ignore - server might not exist - } - // Build env flags const envFlags = Object.entries(def.env || {}) .map(([k, v]) => `--env ${k}=${String(v)}`) @@ -153,14 +152,33 @@ export async function runDroid(promptPath: string, options: DroidOptions) { const addCmd = `droid mcp add ${name} "${cmd}" ${envFlags}`.trim(); try { - await execAsync(addCmd, { env: { ...process.env } }); + await retryWithBackoff( + async () => { + // Remove existing server if present (ignore errors) before each attempt + try { + await execAsync(`droid mcp remove ${name}`); + } catch (_) { + // Ignore - server might not exist + } + try { + await execAsync(addCmd, { env: { ...process.env } }); + } catch (err) { + // Redact inline --env secrets before they reach any log or rethrow. + const message = + err instanceof Error ? err.message : String(err); + throw new Error(redactEnvSecrets(message)); + } + }, + { maxAttempts: 3, initialDelayMs: 2000, maxDelayMs: 10000 }, + ); console.log(` ✓ Registered MCP server: ${name}`); - } catch (e: any) { + } catch (e) { + const message = e instanceof Error ? e.message : String(e); console.error( ` ✗ Failed to register MCP server ${name}:`, - e.message, + message, ); - throw e; + throw new Error(message); } } } @@ -219,19 +237,6 @@ export async function runDroid(promptPath: string, options: DroidOptions) { // Use custom executable path if provided, otherwise default to "droid" const droidExecutable = options.pathToDroidExecutable || "droid"; - const droidProcess = spawn(droidExecutable, config.droidArgs, { - stdio: ["ignore", "pipe", "inherit"], - env: { - ...process.env, - ...config.env, - }, - }); - - // Handle Droid process errors - droidProcess.on("error", (error) => { - console.error("Error spawning Droid process:", error); - }); - // Determine if full output should be shown // Show full output if explicitly set to "true" OR if GitHub Actions debug mode is enabled const isDebugMode = process.env.ACTIONS_STEP_DEBUG === "true"; @@ -247,74 +252,107 @@ export async function runDroid(promptPath: string, options: DroidOptions) { ); } - // Capture output for parsing execution metrics - let sessionId: string | undefined; - droidProcess.stdout.on("data", (data) => { - const text = data.toString(); - - // Try to parse as JSON and handle based on verbose setting - const lines = text.split("\n"); - lines.forEach((line: string, index: number) => { - if (line.trim() === "") return; - - try { - // Check if this line is a JSON object - const parsed = JSON.parse(line); - if (!sessionId && typeof parsed === "object" && parsed !== null) { - const detectedSessionId = parsed.session_id; - if ( - typeof detectedSessionId === "string" && - detectedSessionId.trim() - ) { - sessionId = detectedSessionId; - console.log(`Detected Droid session: ${sessionId}`); + // Run Droid Exec with retry for transient failures. Uses the shared + // retryWithBackoff so backoff timing lives in one place (3 total attempts, + // 5s then 10s delays). + let lastExitCode = 1; + + const runDroidOnce = (): Promise => { + const droidProcess = spawn(droidExecutable, config.droidArgs, { + stdio: ["ignore", "pipe", "inherit"], + env: { + ...process.env, + ...config.env, + }, + }); + + // Handle Droid process errors + droidProcess.on("error", (error) => { + console.error("Error spawning Droid process:", error); + }); + + // Capture output for parsing execution metrics + let sessionId: string | undefined; + droidProcess.stdout.on("data", (data) => { + const text = data.toString(); + + // Try to parse as JSON and handle based on verbose setting + const lines = text.split("\n"); + lines.forEach((line: string, index: number) => { + if (line.trim() === "") return; + + try { + // Check if this line is a JSON object + const parsed = JSON.parse(line); + if (!sessionId && typeof parsed === "object" && parsed !== null) { + const detectedSessionId = parsed.session_id; + if ( + typeof detectedSessionId === "string" && + detectedSessionId.trim() + ) { + sessionId = detectedSessionId; + console.log(`Detected Droid session: ${sessionId}`); + } } - } - const sanitizedOutput = sanitizeJsonOutput(parsed, showFullOutput); + const sanitizedOutput = sanitizeJsonOutput(parsed, showFullOutput); - if (sanitizedOutput) { - process.stdout.write(sanitizedOutput); - if (index < lines.length - 1 || text.endsWith("\n")) { - process.stdout.write("\n"); + if (sanitizedOutput) { + process.stdout.write(sanitizedOutput); + if (index < lines.length - 1 || text.endsWith("\n")) { + process.stdout.write("\n"); + } } - } - } catch (e) { - // Not a JSON object - if (showFullOutput) { - // In full output mode, print as is - process.stdout.write(line); - if (index < lines.length - 1 || text.endsWith("\n")) { - process.stdout.write("\n"); + } catch (e) { + // Not a JSON object + if (showFullOutput) { + // In full output mode, print as is + process.stdout.write(line); + if (index < lines.length - 1 || text.endsWith("\n")) { + process.stdout.write("\n"); + } } + // In non-full-output mode, suppress non-JSON output } - // In non-full-output mode, suppress non-JSON output - } + }); }); - }); - - // Handle stdout errors - droidProcess.stdout.on("error", (error) => { - console.error("Error reading Droid stdout:", error); - }); - // Wait for Droid Exec to finish - const exitCode = await new Promise((resolve) => { - droidProcess.on("close", (code) => { - resolve(code || 0); + // Handle stdout errors + droidProcess.stdout.on("error", (error) => { + console.error("Error reading Droid stdout:", error); }); - droidProcess.on("error", (error) => { - console.error("Droid process error:", error); - resolve(1); + // Wait for Droid Exec to finish + return new Promise((resolve) => { + droidProcess.on("close", (code) => { + resolve(code || 0); + }); + + droidProcess.on("error", (error) => { + console.error("Droid process error:", error); + resolve(1); + }); }); - }); + }; - // Set conclusion based on exit code - if (exitCode === 0) { + try { + await retryWithBackoff( + async () => { + lastExitCode = await runDroidOnce(); + if (lastExitCode !== 0) { + console.log(`Droid Exec exited with code ${lastExitCode}`); + throw new Error(`Droid Exec exited with code ${lastExitCode}`); + } + }, + { maxAttempts: 3, initialDelayMs: 5000, maxDelayMs: 20000 }, + ); core.setOutput("conclusion", "success"); return; + } catch (_) { + // All retry attempts exhausted + console.error( + `Droid Exec failed after 3 total attempts (exit code: ${lastExitCode})`, + ); + core.setOutput("conclusion", "failure"); + process.exit(lastExitCode); } - - core.setOutput("conclusion", "failure"); - process.exit(exitCode); } diff --git a/base-action/src/utils/retry.ts b/base-action/src/utils/retry.ts new file mode 100644 index 0000000..6455e65 --- /dev/null +++ b/base-action/src/utils/retry.ts @@ -0,0 +1,48 @@ +export type RetryOptions = { + maxAttempts?: number; + initialDelayMs?: number; + maxDelayMs?: number; + backoffFactor?: number; +}; + +/** + * Retry an async operation with exponential backoff. + * Used for transient failures (Droid Exec, MCP registration, network calls). + * + * Mirrors `src/utils/retry.ts`; base-action is published standalone and cannot + * import from the parent `src/`, so the helper is duplicated here rather than + * shared. + */ +export async function retryWithBackoff( + operation: () => Promise, + options: RetryOptions = {}, +): Promise { + const { + maxAttempts = 3, + initialDelayMs = 5000, + maxDelayMs = 20000, + backoffFactor = 2, + } = options; + + let delayMs = initialDelayMs; + let lastError: Error | undefined; + + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + console.log(`Attempt ${attempt} of ${maxAttempts}...`); + return await operation(); + } catch (error) { + lastError = error instanceof Error ? error : new Error(String(error)); + console.error(`Attempt ${attempt} failed:`, lastError.message); + + if (attempt < maxAttempts) { + console.log(`Retrying in ${delayMs / 1000} seconds...`); + await new Promise((resolve) => setTimeout(resolve, delayMs)); + delayMs = Math.min(delayMs * backoffFactor, maxDelayMs); + } + } + } + + console.error(`Operation failed after ${maxAttempts} attempts`); + throw lastError; +} diff --git a/base-action/test/retry.test.ts b/base-action/test/retry.test.ts new file mode 100644 index 0000000..d414987 --- /dev/null +++ b/base-action/test/retry.test.ts @@ -0,0 +1,72 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import { retryWithBackoff } from "../src/utils/retry"; + +describe("retryWithBackoff", () => { + let timeoutSpy: ReturnType; + + beforeEach(() => { + timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation((( + handler: Parameters[0], + ) => { + if (typeof handler === "function") { + handler(); + } + return 0 as unknown as ReturnType; + }) as unknown as typeof setTimeout); + }); + + afterEach(() => { + timeoutSpy.mockRestore(); + }); + + it("resolves when the operation succeeds on the first attempt", async () => { + const result = await retryWithBackoff(async () => "success"); + + expect(result).toBe("success"); + expect(timeoutSpy).not.toHaveBeenCalled(); + }); + + it("retries failed attempts until the operation succeeds", async () => { + let attempts = 0; + + const result = await retryWithBackoff( + async () => { + attempts += 1; + if (attempts < 3) { + throw new Error(`failure ${attempts}`); + } + return "ok"; + }, + { maxAttempts: 4, initialDelayMs: 10, backoffFactor: 3, maxDelayMs: 90 }, + ); + + expect(result).toBe("ok"); + expect(attempts).toBe(3); + expect(timeoutSpy).toHaveBeenCalledTimes(2); + const delays = timeoutSpy.mock.calls.map( + (call: Parameters) => call[1]! as number, + ); + expect(delays).toEqual([10, 30]); + }); + + it("throws the last error after exhausting all attempts", async () => { + let attempts = 0; + + await expect( + retryWithBackoff( + async () => { + attempts += 1; + throw new Error(`still failing ${attempts}`); + }, + { maxAttempts: 2, initialDelayMs: 5 }, + ), + ).rejects.toThrow("still failing 2"); + + expect(attempts).toBe(2); + expect(timeoutSpy).toHaveBeenCalledTimes(1); + const firstCall = timeoutSpy.mock.calls[0] as + | Parameters + | undefined; + expect(firstCall?.[1]).toBe(5); + }); +}); diff --git a/base-action/test/run-droid-mcp.test.ts b/base-action/test/run-droid-mcp.test.ts index 79ebb8a..b765caf 100644 --- a/base-action/test/run-droid-mcp.test.ts +++ b/base-action/test/run-droid-mcp.test.ts @@ -273,7 +273,7 @@ describe("MCP Server Registration", () => { }); describe("Error Handling", () => { - test("should fail fast when MCP server registration fails", async () => { + test("should fail when MCP server registration fails after retries", async () => { const mcpTools = JSON.stringify({ mcpServers: { failing_server: { @@ -298,7 +298,7 @@ describe("MCP Server Registration", () => { } finally { await cleanupTempDir(tempDir); } - }); + }, 30000); // Allow time for retry backoff (3 attempts x 2s+4s delays) test("should not attempt MCP registration when config is not provided", () => { const options: DroidOptions = {}; diff --git a/src/core/review/artifacts/types.ts b/src/core/review/artifacts/types.ts index 62843d0..558069b 100644 --- a/src/core/review/artifacts/types.ts +++ b/src/core/review/artifacts/types.ts @@ -23,3 +23,15 @@ export type ReviewArtifactContents = { comments: unknown; // JSON-serializable description: string; }; + +/** + * Naming convention for review-artifact files on disk. Each platform + * gets its own basename for the diff and description (pr.diff / + * mr.diff, pr_description.txt / mr_description.txt) but the existing + * comments file is platform-neutral. + */ +export type ReviewArtifactNames = { + diff: string; + comments: string; + description: string; +}; diff --git a/src/core/review/artifacts/write.ts b/src/core/review/artifacts/write.ts index 4befa76..fdbccd5 100644 --- a/src/core/review/artifacts/write.ts +++ b/src/core/review/artifacts/write.ts @@ -1,5 +1,10 @@ import * as fs from "fs/promises"; import * as path from "path"; +import type { + ReviewArtifactContents, + ReviewArtifactNames, + ReviewArtifactPaths, +} from "./types"; import type { ReviewArtifactContents, ReviewArtifactPaths } from "./types"; /** diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 46886d0..447888c 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -16,6 +16,7 @@ import { generateReviewCandidatesPrompt } from "../create-prompt/templates/revie import { generateSecurityCandidatesPrompt } from "../create-prompt/templates/security-review-prompt"; import { normalizeDroidArgs, parseAllowedTools } from "../utils/parse-tools"; import { resolveReviewConfig } from "../utils/review-depth"; +import { retryWithBackoff } from "../utils/retry"; async function run() { try { @@ -60,17 +61,25 @@ async function run() { `Checking out PR #${context.entityNumber} branch for diff computation...`, ); try { - execSync("git reset --hard HEAD", { encoding: "utf8", stdio: "pipe" }); - execSync(`gh pr checkout ${context.entityNumber}`, { - encoding: "utf8", - stdio: "pipe", - env: { ...process.env, GH_TOKEN: githubToken }, - }); - console.log( - `Successfully checked out PR branch: ${execSync("git rev-parse --abbrev-ref HEAD", { encoding: "utf8" }).trim()}`, + await retryWithBackoff( + async () => { + execSync("git reset --hard HEAD", { + encoding: "utf8", + stdio: "pipe", + }); + execSync(`gh pr checkout ${context.entityNumber}`, { + encoding: "utf8", + stdio: "pipe", + env: { ...process.env, GH_TOKEN: githubToken }, + }); + console.log( + `Successfully checked out PR branch: ${execSync("git rev-parse --abbrev-ref HEAD", { encoding: "utf8" }).trim()}`, + ); + }, + { maxAttempts: 3, initialDelayMs: 3000, maxDelayMs: 15000 }, ); } catch (e) { - console.error(`Failed to checkout PR branch: ${e}`); + console.error(`Failed to checkout PR branch after retries: ${e}`); throw new Error( `Failed to checkout PR #${context.entityNumber} branch for review`, ); diff --git a/src/entrypoints/gitlab-prepare-validator.ts b/src/entrypoints/gitlab-prepare-validator.ts index d71fb7e..7529e7b 100644 --- a/src/entrypoints/gitlab-prepare-validator.ts +++ b/src/entrypoints/gitlab-prepare-validator.ts @@ -63,6 +63,35 @@ async function run(): Promise { const descriptionPath = ensure(state.descriptionPath, "descriptionPath"); const headSha = ensure(state.headSha, "headSha"); + // Validate that Pass 1 produced a valid candidates JSON file. + // If invalid or missing, skip Pass 2 gracefully rather than failing. + try { + const content = await fs.readFile(candidatesPath, "utf8"); + const parsed = JSON.parse(content); + if ( + !parsed || + typeof parsed !== "object" || + !Array.isArray(parsed.comments) + ) { + throw new Error("Missing or invalid 'comments' array in candidates"); + } + console.log( + `Pass 1 candidates validated: ${parsed.comments.length} comments found`, + ); + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + console.error(`Pass 1 candidates JSON is invalid or missing: ${message}`); + console.error( + "Skipping Pass 2 (validator) to avoid a full pipeline failure", + ); + // Write a no-op prompt so droid exec exits cleanly + await fs.writeFile( + promptPath, + "No review findings to validate. Pass 1 candidates were invalid. Exit with success.", + ); + return; + } + const promptCtx: GitlabReviewPromptContext = { projectPath: state.projectPath, mrIid, diff --git a/src/entrypoints/prepare-validator.ts b/src/entrypoints/prepare-validator.ts index 1382f76..f0aa23b 100644 --- a/src/entrypoints/prepare-validator.ts +++ b/src/entrypoints/prepare-validator.ts @@ -1,6 +1,7 @@ #!/usr/bin/env bun import * as core from "@actions/core"; +import { readFile } from "fs/promises"; import { setupGitHubToken } from "../github/token"; import { createOctokit } from "../github/api/client"; import { parseGitHubContext, isEntityContext } from "../github/context"; @@ -14,6 +15,41 @@ async function run() { throw new Error("prepare-validator requires a pull request context"); } + // Validate that Pass 1 produced a valid candidates JSON file. + // If the file is missing or invalid, skip Pass 2 gracefully rather than + // failing the entire pipeline. This prevents the ~2% of reviews where + // Pass 1 had a transient issue from counting as full failures. + const candidatesPath = process.env.REVIEW_CANDIDATES_PATH || ""; + if (candidatesPath) { + try { + const content = await readFile(candidatesPath, "utf8"); + const parsed = JSON.parse(content); + if ( + !parsed || + typeof parsed !== "object" || + !Array.isArray(parsed.comments) + ) { + throw new Error("Missing or invalid 'comments' array in candidates"); + } + console.log( + `Pass 1 candidates validated: ${parsed.comments.length} comments found`, + ); + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + console.error( + `Pass 1 candidates JSON is invalid or missing: ${message}`, + ); + console.error( + "Skipping Pass 2 (validator) to avoid a full pipeline failure", + ); + core.setOutput("validator_should_run", "false"); + core.notice( + "Pass 1 candidates validation failed - skipping validator pass", + ); + return; + } + } + const githubToken = await setupGitHubToken(); const octokit = createOctokit(githubToken); @@ -30,6 +66,7 @@ async function run() { }); core.setOutput("github_token", githubToken); + core.setOutput("validator_should_run", "true"); if (result?.mcpTools) core.setOutput("mcp_tools", result.mcpTools); } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); diff --git a/src/github/data/review-artifacts.ts b/src/github/data/review-artifacts.ts index c7d7a4f..1e1d7f6 100644 --- a/src/github/data/review-artifacts.ts +++ b/src/github/data/review-artifacts.ts @@ -2,6 +2,7 @@ import { execSync } from "child_process"; import { writeFile, mkdir } from "fs/promises"; import type { Octokits } from "../api/client"; import type { ReviewArtifacts } from "../../create-prompt/types"; +import { retryWithBackoff } from "../../utils/retry"; const DIFF_MAX_BUFFER = 50 * 1024 * 1024; // 50MB buffer for large diffs @@ -59,15 +60,22 @@ export async function computeAndStoreDiff( }); } catch { // Fallback: use gh CLI to get the diff (works even with shallow clones) + // Retry since gh CLI can have transient rate-limit or network failures if (options?.githubToken && options?.prNumber) { console.log( "Git merge-base failed, falling back to gh pr diff for PR diff", ); - diff = execSync(`gh pr diff ${options.prNumber}`, { - encoding: "utf8", - maxBuffer: DIFF_MAX_BUFFER, - env: { ...process.env, GH_TOKEN: options.githubToken }, - }); + diff = await retryWithBackoff( + () => + Promise.resolve( + execSync(`gh pr diff ${options.prNumber}`, { + encoding: "utf8", + maxBuffer: DIFF_MAX_BUFFER, + env: { ...process.env, GH_TOKEN: options.githubToken }, + }), + ), + { maxAttempts: 3, initialDelayMs: 3000, maxDelayMs: 15000 }, + ); } else { throw new Error( "Git merge-base failed and no fallback credentials provided", diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index 22215f7..d5d92ac 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -12,6 +12,7 @@ import { generateReviewCandidatesPrompt } from "../../create-prompt/templates/re import type { Octokits } from "../../github/api/client"; import type { PrepareResult } from "../../prepare/types"; import { resolveReviewConfig } from "../../utils/review-depth"; +import { retryWithBackoff } from "../../utils/retry"; type ReviewCommandOptions = { context: GitHubContext; @@ -55,17 +56,23 @@ export async function prepareReviewMode({ `Checking out PR #${context.entityNumber} branch for diff computation...`, ); try { - execSync("git reset --hard HEAD", { encoding: "utf8", stdio: "pipe" }); - execSync(`gh pr checkout ${context.entityNumber}`, { - encoding: "utf8", - stdio: "pipe", - env: { ...process.env, GH_TOKEN: githubToken }, - }); - console.log( - `Successfully checked out PR branch: ${execSync("git rev-parse --abbrev-ref HEAD", { encoding: "utf8" }).trim()}`, + await retryWithBackoff( + async () => { + execSync("git reset --hard HEAD", { encoding: "utf8", stdio: "pipe" }); + execSync(`gh pr checkout ${context.entityNumber}`, { + encoding: "utf8", + stdio: "pipe", + env: { ...process.env, GH_TOKEN: githubToken }, + }); + const branchName = execSync("git rev-parse --abbrev-ref HEAD", { + encoding: "utf8", + }).trim(); + console.log(`Successfully checked out PR branch: ${branchName}`); + }, + { maxAttempts: 3, initialDelayMs: 3000, maxDelayMs: 15000 }, ); } catch (e) { - console.error(`Failed to checkout PR branch: ${e}`); + console.error(`Failed to checkout PR branch after retries: ${e}`); throw new Error( `Failed to checkout PR #${context.entityNumber} branch for review`, );