fix(review): add retry logic and Pass 1 validation for 99.9% success rate

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 || '' }}

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<number> => {
+    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<number>((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<number>((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);
 }

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<T>(
+  operation: () => Promise<T>,
+  options: RetryOptions = {},
+): Promise<T> {
+  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;
+}

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<typeof spyOn>;
+
+  beforeEach(() => {
+    timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation(((
+      handler: Parameters<typeof setTimeout>[0],
+    ) => {
+      if (typeof handler === "function") {
+        handler();
+      }
+      return 0 as unknown as ReturnType<typeof setTimeout>;
+    }) 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<typeof setTimeout>) => 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<typeof setTimeout>
+      | undefined;
+    expect(firstCall?.[1]).toBe(5);
+  });
+});