fix: patch_format runtime validation security and correctness improvements

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f35fc6e6-684c-42d5-82d5-3e2ccca5b4b5

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Author: Copilot

actions/setup/js/safe_outputs_handlers.cjs

@@ -319,10 +319,12 @@ function createHandlers(server, appendSafeOutput, config = {}) {
 
     // Determine transport format: "bundle" uses git bundle (preserves merge topology),
     // "am" (default) uses git format-patch / git am (good for linear histories).
-    const patchFormat = prConfig["patch_format"] || config["patch_format"] || "am";
+    // Use ?? (nullish coalescing) so an empty-string resolved value is preserved and
+    // rejected below rather than silently falling back to "am".
+    const patchFormat = prConfig["patch_format"] ?? config["patch_format"] ?? "am";
     const validPatchFormats = ["am", "bundle"];
     if (!validPatchFormats.includes(patchFormat)) {
-      const errorMsg = `Invalid patch_format: "${patchFormat}". Must be one of: ${validPatchFormats.join(", ")}`;
+      const errorMsg = `Invalid patch_format in configuration. Must be one of: ${validPatchFormats.join(", ")}`;
       server.debug(`create_pull_request: ${errorMsg}`);
       return {
         content: [
@@ -547,10 +549,12 @@ function createHandlers(server, appendSafeOutput, config = {}) {
 
     // Determine transport format: "bundle" uses git bundle (preserves merge topology),
     // "am" (default) uses git format-patch / git am (good for linear histories).
-    const pushPatchFormat = pushConfig["patch_format"] || config["patch_format"] || "am";
+    // Use ?? (nullish coalescing) so an empty-string resolved value is preserved and
+    // rejected below rather than silently falling back to "am".
+    const pushPatchFormat = pushConfig["patch_format"] ?? config["patch_format"] ?? "am";
     const validPushPatchFormats = ["am", "bundle"];
     if (!validPushPatchFormats.includes(pushPatchFormat)) {
-      const errorMsg = `Invalid patch_format: "${pushPatchFormat}". Must be one of: ${validPushPatchFormats.join(", ")}`;
+      const errorMsg = `Invalid patch_format in configuration. Must be one of: ${validPushPatchFormats.join(", ")}`;
       server.debug(`push_to_pull_request_branch: ${errorMsg}`);
       return {
         content: [

actions/setup/js/safe_outputs_handlers.test.cjs

@@ -603,12 +603,33 @@ describe("safe_outputs_handlers", () => {
       const responseData = JSON.parse(result.content[0].text);
       expect(responseData.result).toBe("error");
       expect(responseData.error).toContain("Invalid patch_format");
-      expect(responseData.error).toContain("invalid-format");
       expect(responseData.error).toContain("am");
       expect(responseData.error).toContain("bundle");
+      // Must not echo the raw resolved value (could be a secret expression result)
+      expect(responseData.error).not.toContain("invalid-format");
       // Must not have appended any safe output
       expect(mockAppendSafeOutput).not.toHaveBeenCalled();
     });
+
+    it("should fail closed when patch_format resolves to an empty string", async () => {
+      handlers = createHandlers(mockServer, mockAppendSafeOutput, {
+        create_pull_request: {
+          patch_format: "",
+        },
+      });
+
+      const result = await handlers.createPullRequestHandler({
+        branch: "feature-branch",
+        title: "Test PR",
+        body: "Test description",
+      });
+
+      expect(result.isError).toBe(true);
+      const responseData = JSON.parse(result.content[0].text);
+      expect(responseData.result).toBe("error");
+      expect(responseData.error).toContain("Invalid patch_format");
+      expect(mockAppendSafeOutput).not.toHaveBeenCalled();
+    });
   });
 
   describe("pushToPullRequestBranchHandler", () => {
@@ -818,12 +839,31 @@ describe("safe_outputs_handlers", () => {
       const responseData = JSON.parse(result.content[0].text);
       expect(responseData.result).toBe("error");
       expect(responseData.error).toContain("Invalid patch_format");
-      expect(responseData.error).toContain("invalid-format");
       expect(responseData.error).toContain("am");
       expect(responseData.error).toContain("bundle");
+      // Must not echo the raw resolved value (could be a secret expression result)
+      expect(responseData.error).not.toContain("invalid-format");
       // Must not have appended any safe output
       expect(mockAppendSafeOutput).not.toHaveBeenCalled();
     });
+
+    it("should fail closed when patch_format resolves to an empty string", async () => {
+      handlers = createHandlers(mockServer, mockAppendSafeOutput, {
+        push_to_pull_request_branch: {
+          patch_format: "",
+        },
+      });
+
+      const result = await handlers.pushToPullRequestBranchHandler({
+        branch: "feature-branch",
+      });
+
+      expect(result.isError).toBe(true);
+      const responseData = JSON.parse(result.content[0].text);
+      expect(responseData.result).toBe("error");
+      expect(responseData.error).toContain("Invalid patch_format");
+      expect(mockAppendSafeOutput).not.toHaveBeenCalled();
+    });
   });
 
   describe("handler structure", () => {