Fix push_to_pull_request_branch generating bad patch on follow-up issue_comment runs

actions/setup/README.md

@@ -62,11 +62,10 @@ This action copies files from `actions/setup/`, including:
 - Safe output scripts (safe_outputs_*, safe_inputs_*, messages, etc.)
 - Utility scripts (sanitize_*, validate_*, generate_*, etc.)
 
-### Shell Scripts (7 files from `sh/`)
+### Shell Scripts (6 files from `sh/`)
 - `create_gh_aw_tmp_dir.sh` - Creates temporary directory structure
 - `start_safe_inputs_server.sh` - Starts safe-inputs HTTP server
 - `print_prompt_summary.sh` - Prints prompt summary to logs
-- `generate_git_patch.sh` - Generates git patches
 - `create_cache_memory_dir.sh` - Creates cache-memory directory
 - `create_prompt_first.sh` - Creates prompt directory
 - `validate_multi_secret.sh` - Validates that at least one secret from a list is configured

actions/setup/js/generate_git_patch.cjs

@@ -8,19 +8,140 @@ const { getBaseBranch } = require("./get_base_branch.cjs");
 const { getErrorMessage } = require("./error_helpers.cjs");
 const { execGitSync } = require("./git_helpers.cjs");
 
+const PATCH_PATH = "/tmp/gh-aw/aw.patch";
+
+/**
+ * Resolves the base ref to use for patch generation against a named branch.
+ * Preference order:
+ *   1. Remote tracking ref  refs/remotes/origin/<branch>  (already fetched)
+ *   2. Fresh fetch of origin/<branch>                     (gh pr checkout path)
+ *   3. merge-base with origin/<defaultBranch>             (brand-new branch)
+ * @param {string} branchName
+ * @param {string} defaultBranch
+ * @param {string} cwd
+ * @returns {string} baseRef
+ */
+function resolveBaseRef(branchName, defaultBranch, cwd) {
+  try {
+    execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd });
+    const baseRef = `origin/${branchName}`;
+    core.info(`[generate_git_patch] using remote tracking ref as baseRef="${baseRef}"`);
+    return baseRef;
+  } catch {
+    // Remote tracking ref not found (e.g. after gh pr checkout which doesn't set tracking refs).
+    // Try fetching the branch from origin so we use only the NEW commits as the patch base.
+    core.info(`[generate_git_patch] refs/remotes/origin/${branchName} not found; fetching from origin`);
+  }
+
+  try {
+    execGitSync(["fetch", "origin", branchName], { cwd });
+    const baseRef = `origin/${branchName}`;
+    core.info(`[generate_git_patch] fetch succeeded, baseRef="${baseRef}"`);
+    return baseRef;
+  } catch (fetchErr) {
+    // Branch doesn't exist on origin yet (new branch) – fall back to merge-base
+    core.warning(`[generate_git_patch] fetch of origin/${branchName} failed (${getErrorMessage(fetchErr)}); falling back to merge-base with "${defaultBranch}"`);
+  }
+
+  execGitSync(["fetch", "origin", defaultBranch], { cwd });
+  const baseRef = execGitSync(["merge-base", `origin/${defaultBranch}`, branchName], { cwd }).trim();
+  core.info(`[generate_git_patch] merge-base baseRef="${baseRef}"`);
+  return baseRef;
+}
+
+/**
+ * Writes a patch file for the range base..tip and returns whether it succeeded.
+ * @param {string} base - commit-ish for the base (exclusive)
+ * @param {string} tip  - commit-ish for the tip (inclusive)
+ * @param {string} cwd
+ * @returns {boolean} true if the patch was written with content
+ */
+function writePatch(base, tip, cwd) {
+  const commitCount = parseInt(execGitSync(["rev-list", "--count", `${base}..${tip}`], { cwd }).trim(), 10);
+  core.info(`[generate_git_patch] ${commitCount} commit(s) between ${base} and ${tip}`);
+
+  if (commitCount === 0) {
+    return false;
+  }
+
+  const patchContent = execGitSync(["format-patch", `${base}..${tip}`, "--stdout"], { cwd });
+  if (!patchContent || !patchContent.trim()) {
+    core.warning(`[generate_git_patch] format-patch produced empty output for ${base}..${tip}`);
+    return false;
+  }
+
+  fs.writeFileSync(PATCH_PATH, patchContent, "utf8");
+  core.info(`[generate_git_patch] patch written: ${patchContent.split("\n").length} lines, ${Math.ceil(Buffer.byteLength(patchContent, "utf8") / 1024)} KB`);
+  return true;
+}
+
 /**
- * Generates a git patch file for the current changes
+ * Strategy 1: generate a patch from the known remote state of branchName to
+ * its local tip, capturing only commits not yet on origin.
+ * @param {string} branchName
+ * @param {string} defaultBranch
+ * @param {string} cwd
+ * @returns {boolean} true if a patch was written
+ */
+function tryBranchStrategy(branchName, defaultBranch, cwd) {
+  core.info(`[generate_git_patch] Strategy 1: branch-based patch for "${branchName}"`);
+  try {
+    execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
+  } catch (err) {
+    core.info(`[generate_git_patch] local branch "${branchName}" not found: ${getErrorMessage(err)}`);
+    return false;
+  }
+
+  const baseRef = resolveBaseRef(branchName, defaultBranch, cwd);
+  return writePatch(baseRef, branchName, cwd);
+}
+
+/**
+ * Strategy 2: generate a patch from GITHUB_SHA to the current HEAD, capturing
+ * commits made by the agent after checkout.
+ * @param {string|undefined} githubSha
+ * @param {string} cwd
+ * @returns {{ generated: boolean, errorMessage: string|null }}
+ */
+function tryHeadStrategy(githubSha, cwd) {
+  const currentHead = execGitSync(["rev-parse", "HEAD"], { cwd }).trim();
+  core.info(`[generate_git_patch] Strategy 2: HEAD="${currentHead}" GITHUB_SHA="${githubSha || ""}"`);
+
+  if (!githubSha) {
+    const msg = "GITHUB_SHA environment variable is not set";
+    core.warning(`[generate_git_patch] ${msg}`);
+    return { generated: false, errorMessage: msg };
+  }
+
+  if (currentHead === githubSha) {
+    core.info(`[generate_git_patch] HEAD matches GITHUB_SHA – no new commits`);
+    return { generated: false, errorMessage: null };
+  }
+
+  try {
+    execGitSync(["merge-base", "--is-ancestor", githubSha, "HEAD"], { cwd });
+  } catch {
+    core.warning(`[generate_git_patch] GITHUB_SHA is not an ancestor of HEAD – repository state has diverged`);
+    return { generated: false, errorMessage: null };
+  }
+
+  const generated = writePatch(githubSha, "HEAD", cwd);
+  return { generated, errorMessage: null };
+}
+
+/**
+ * Generates a git patch file for the current changes.
  * @param {string} branchName - The branch name to generate patch for
  * @returns {Object} Object with patch info or error
  */
 function generateGitPatch(branchName) {
-  const patchPath = "/tmp/gh-aw/aw.patch";
   const cwd = process.env.GITHUB_WORKSPACE || process.cwd();
   const defaultBranch = process.env.DEFAULT_BRANCH || getBaseBranch();
   const githubSha = process.env.GITHUB_SHA;
 
-  // Ensure /tmp/gh-aw directory exists
-  const patchDir = path.dirname(patchPath);
+  core.info(`[generate_git_patch] branchName="${branchName || ""}" GITHUB_SHA="${githubSha || ""}" defaultBranch="${defaultBranch}"`);
+
+  const patchDir = path.dirname(PATCH_PATH);
   if (!fs.existsSync(patchDir)) {
     fs.mkdirSync(patchDir, { recursive: true });
   }
@@ -29,105 +150,49 @@ function generateGitPatch(branchName) {
   let errorMessage = null;
 
   try {
-    // Strategy 1: If we have a branch name, check if that branch exists and get its diff
     if (branchName) {
-      // Check if the branch exists locally
-      try {
-        execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
-
-        // Determine base ref for patch generation
-        let baseRef;
-        try {
-          // Check if origin/branchName exists
-          execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd });
-          baseRef = `origin/${branchName}`;
-        } catch {
-          // Use merge-base with default branch
-          execGitSync(["fetch", "origin", defaultBranch], { cwd });
-          baseRef = execGitSync(["merge-base", `origin/${defaultBranch}`, branchName], { cwd }).trim();
-        }
-
-        // Count commits to be included
-        const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${branchName}`], { cwd }).trim(), 10);
-
-        if (commitCount > 0) {
-          // Generate patch from the determined base to the branch
-          const patchContent = execGitSync(["format-patch", `${baseRef}..${branchName}`, "--stdout"], { cwd });
-
-          if (patchContent && patchContent.trim()) {
-            fs.writeFileSync(patchPath, patchContent, "utf8");
-            patchGenerated = true;
-          }
-        }
-      } catch (branchError) {
-        // Branch does not exist locally
-      }
+      patchGenerated = tryBranchStrategy(branchName, defaultBranch, cwd);
+    } else {
+      core.info(`[generate_git_patch] Strategy 1: skipped (no branchName)`);
     }
 
-    // Strategy 2: Check if commits were made to current HEAD since checkout
     if (!patchGenerated) {
-      const currentHead = execGitSync(["rev-parse", "HEAD"], { cwd }).trim();
-
-      if (!githubSha) {
-        errorMessage = "GITHUB_SHA environment variable is not set";
-      } else if (currentHead === githubSha) {
-        // No commits have been made since checkout
-      } else {
-        // Check if GITHUB_SHA is an ancestor of current HEAD
-        try {
-          execGitSync(["merge-base", "--is-ancestor", githubSha, "HEAD"], { cwd });
-
-          // Count commits between GITHUB_SHA and HEAD
-          const commitCount = parseInt(execGitSync(["rev-list", "--count", `${githubSha}..HEAD`], { cwd }).trim(), 10);
-
-          if (commitCount > 0) {
-            // Generate patch from GITHUB_SHA to HEAD
-            const patchContent = execGitSync(["format-patch", `${githubSha}..HEAD`, "--stdout"], { cwd });
-
-            if (patchContent && patchContent.trim()) {
-              fs.writeFileSync(patchPath, patchContent, "utf8");
-              patchGenerated = true;
-            }
-          }
-        } catch {
-          // GITHUB_SHA is not an ancestor of HEAD - repository state has diverged
-        }
-      }
+      const result = tryHeadStrategy(githubSha, cwd);
+      patchGenerated = result.generated;
+      errorMessage = result.errorMessage;
     }
   } catch (error) {
     errorMessage = `Failed to generate patch: ${getErrorMessage(error)}`;
+    core.warning(`[generate_git_patch] ${errorMessage}`);
   }
 
-  // Check if patch was generated and has content
-  if (patchGenerated && fs.existsSync(patchPath)) {
-    const patchContent = fs.readFileSync(patchPath, "utf8");
+  if (patchGenerated && fs.existsSync(PATCH_PATH)) {
+    const patchContent = fs.readFileSync(PATCH_PATH, "utf8");
     const patchSize = Buffer.byteLength(patchContent, "utf8");
     const patchLines = patchContent.split("\n").length;
 
     if (!patchContent.trim()) {
-      // Empty patch
       return {
         success: false,
         error: "No changes to commit - patch is empty",
-        patchPath: patchPath,
+        patchPath: PATCH_PATH,
         patchSize: 0,
         patchLines: 0,
       };
     }
 
     return {
       success: true,
-      patchPath: patchPath,
+      patchPath: PATCH_PATH,
       patchSize: patchSize,
       patchLines: patchLines,
     };
   }
 
-  // No patch generated
   return {
     success: false,
     error: errorMessage || "No changes to commit - no commits found",
-    patchPath: patchPath,
+    patchPath: PATCH_PATH,
   };
 }
 

actions/setup/js/generate_git_patch.test.cjs

@@ -1,9 +1,26 @@
-import { describe, it, expect, beforeEach, afterEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
+
+// Mock the global objects that GitHub Actions provides
+const mockCore = {
+  debug: vi.fn(),
+  info: vi.fn(),
+  notice: vi.fn(),
+  warning: vi.fn(),
+  error: vi.fn(),
+  setFailed: vi.fn(),
+  setOutput: vi.fn(),
+};
+
+// Set up global mocks before importing the module
+global.core = mockCore;
 
 describe("generateGitPatch", () => {
   let originalEnv;
 
   beforeEach(() => {
+    // Reset mocks before each test
+    vi.clearAllMocks();
+
     // Save original environment
     originalEnv = {
       GITHUB_SHA: process.env.GITHUB_SHA,

actions/setup/js/safe-outputs-mcp-server.cjs

@@ -5,6 +5,10 @@
 // This is the main entry point script for the safe-outputs MCP server
 // It starts the HTTP server on the configured port
 
+// Load core shim before any other modules so that global.core is available
+// for modules that rely on it (e.g. generate_git_patch.cjs).
+require("./shim.cjs");
+
 const { createLogger } = require("./mcp_logger.cjs");
 const logger = createLogger("safe-outputs-entry");
 

actions/setup/js/safe_inputs_mcp_server_http.cjs

@@ -18,6 +18,10 @@
  *   --log-dir <path>   Directory for log files
  */
 
+// Load core shim before any other modules so that global.core is available
+// for modules that rely on it.
+require("./shim.cjs");
+
 const http = require("http");
 const { randomUUID } = require("crypto");
 const { MCPServer, MCPHTTPTransport } = require("./mcp_http_transport.cjs");

actions/setup/js/safe_outputs_handlers.test.cjs

@@ -4,13 +4,29 @@ import fs from "fs";
 import path from "path";
 import { createHandlers } from "./safe_outputs_handlers.cjs";
 
+// Mock the global objects that GitHub Actions provides
+const mockCore = {
+  debug: vi.fn(),
+  info: vi.fn(),
+  notice: vi.fn(),
+  warning: vi.fn(),
+  error: vi.fn(),
+  setFailed: vi.fn(),
+  setOutput: vi.fn(),
+};
+
+// Set up global mocks before importing the module
+global.core = mockCore;
+
 describe("safe_outputs_handlers", () => {
   let mockServer;
   let mockAppendSafeOutput;
   let handlers;
   let testWorkspaceDir;
 
   beforeEach(() => {
+    vi.clearAllMocks();
+
     mockServer = {
       debug: vi.fn(),
     };

actions/setup/js/shim.cjs

@@ -0,0 +1,29 @@
+// @ts-check
+
+/**
+ * shim.cjs
+ *
+ * Provides a minimal `global.core` shim so that modules written for the
+ * GitHub Actions `github-script` context (which rely on the built-in `core`
+ * global) work correctly when executed as plain Node.js processes, such as
+ * inside the safe-outputs and safe-inputs MCP servers.
+ *
+ * When `global.core` is already set (i.e. running inside `github-script`)
+ * this module is a no-op.
+ */
+
+if (!global.core) {
+  global.core = {
+    debug: /** @param {string} message */ message => console.debug(`[debug] ${message}`),
+    info: /** @param {string} message */ message => console.info(`[info] ${message}`),
+    notice: /** @param {string} message */ message => console.info(`[notice] ${message}`),
+    warning: /** @param {string} message */ message => console.warn(`[warning] ${message}`),
+    error: /** @param {string} message */ message => console.error(`[error] ${message}`),
+    setFailed: /** @param {string} message */ message => {
+      console.error(`[error] ${message}`);
+    },
+    setOutput: /** @param {string} name @param {unknown} value */ (name, value) => {
+      console.info(`[output] ${name}=${value}`);
+    },
+  };
+}