Merge pull request #105 from Factory-AI/dev

Merge dev into main: GitLab Phase 1, review refactor, and README auth docs

Author: factory-nizar

README.md

@@ -240,6 +240,47 @@ jobs:
           security_scan_days: 7
 ```
 
+## Authentication
+
+Droid needs two separate kinds of access: permission to run Droid, and permission to post on your pull requests. You set them up independently.
+
+### 1. Factory API key (run Droid)
+
+Droid runs using your Factory API key. Create one at [app.factory.ai/settings/api-keys](https://app.factory.ai/settings/api-keys) and save it as a `FACTORY_API_KEY` secret in your repository or organization. Pass it to the action on every run:
+
+```yaml
+- uses: Factory-AI/droid-action@main
+  with:
+    factory_api_key: ${{ secrets.FACTORY_API_KEY }}
+```
+
+This input is required.
+
+### 2. GitHub access (post reviews)
+
+To leave comments and approvals on your PRs, Droid needs a GitHub token. There are two ways to provide one:
+
+- **Factory Droid GitHub App (default, recommended).** If you don't pass a token, the action securely requests one for the installed Factory Droid GitHub App. For most teams this is all you need: install the app on your repositories from [app.factory.ai/settings/organization](https://app.factory.ai/settings/organization). It requires the `id-token: write` permission so the action can request the token:
+
+  ```yaml
+  permissions:
+    contents: write
+    pull-requests: write
+    issues: write
+    id-token: write # required for GitHub App auth
+  ```
+
+- **Your own token (override).** If you'd rather use a personal access token or your own GitHub App — for example on GitHub Enterprise, or to control which account posts comments — pass it as `github_token`. When set, Droid uses it directly and skips the app. The token needs write access to pull requests and repository contents.
+
+  ```yaml
+  - uses: Factory-AI/droid-action@main
+    with:
+      factory_api_key: ${{ secrets.FACTORY_API_KEY }}
+      github_token: ${{ secrets.MY_GITHUB_TOKEN }}
+  ```
+
+> On GitLab, the same two pieces apply: set `FACTORY_API_KEY` and `GITLAB_TOKEN` as CI/CD variables. See [`docs/gitlab-setup.md`](docs/gitlab-setup.md).
+
 ## Configuration
 
 ### Core Inputs

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;
+}