pelikhan · Sep 20, 2025 · Sep 20, 2025 · Sep 20, 2025 · Sep 20, 2025 · Sep 20, 2025
diff --git a/.github/workflows/ci-doctor.lock.yml b/.github/workflows/ci-doctor.lock.yml
diff --git a/.github/workflows/dev.lock.yml b/.github/workflows/dev.lock.yml
diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go
@@ -2325,7 +2325,7 @@ func (c *Compiler) generateSafeOutputsConfig(data *WorkflowData) string {
 		if data.SafeOutputs.CreatePullRequestReviewComments.Max > 0 {
 			prReviewCommentConfig["max"] = data.SafeOutputs.CreatePullRequestReviewComments.Max
 		}
-		safeOutputsConfig["create-pull-request-review0comment"] = prReviewCommentConfig
+		safeOutputsConfig["create-pull-request-review-comment"] = prReviewCommentConfig
 	}
 	if data.SafeOutputs.CreateCodeScanningAlerts != nil {
 		// Security reports typically have unlimited max, but check if configured

diff --git a/pkg/workflow/js/create_pull_request.test.cjs b/pkg/workflow/js/create_pull_request.test.cjs
@@ -16,7 +16,7 @@ const createTestableFunction = scriptContent => {
 
   // Create a testable function that has the same logic but can be called with dependencies
   return new Function(`
-    const { fs, crypto, execSync, github, core, context, process, console } = arguments[0];
+    const { fs, crypto, github, core, context, process, console } = arguments[0];
 
     return async function main() {
       ${mainFunctionBody}
@@ -36,6 +36,11 @@ describe("create_pull_request.cjs", () => {
     // Create testable function
     createMainFunction = createTestableFunction(scriptContent);
 
+    // Set up global exec mock
+    global.exec = {
+      exec: vi.fn().mockResolvedValue(0), // Return exit code directly
+    };
+
     // Set up mock dependencies
     mockDependencies = {
       fs: {
@@ -122,6 +127,13 @@ describe("create_pull_request.cjs", () => {
     };
   });
 
+  afterEach(() => {
+    // Clean up global exec mock
+    if (typeof global !== "undefined") {
+      delete global.exec;
+    }
+  });
+
   it("should throw error when GITHUB_AW_WORKFLOW_ID is missing", async () => {
     const mainFunction = createMainFunction(mockDependencies);
 
@@ -212,21 +224,14 @@ describe("create_pull_request.cjs", () => {
     await mainFunction();
 
     // Verify git operations (excluding git config which is handled by workflow)
-    expect(mockDependencies.execSync).toHaveBeenCalledWith(
-      "git checkout -b test-workflow-1234567890abcdef",
-      {
-        stdio: "inherit",
-      }
-    );
-    expect(mockDependencies.execSync).toHaveBeenCalledWith(
-      "git am /tmp/aw.patch",
-      { stdio: "inherit" }
+    expect(global.exec.exec).toHaveBeenCalledWith("git fetch origin");
+    expect(global.exec.exec).toHaveBeenCalledWith("git checkout main");
+    expect(global.exec.exec).toHaveBeenCalledWith(
+      "git checkout -b test-workflow-1234567890abcdef"
     );
-    expect(mockDependencies.execSync).toHaveBeenCalledWith(
-      "git push origin test-workflow-1234567890abcdef",
-      {
-        stdio: "inherit",
-      }
+    expect(global.exec.exec).toHaveBeenCalledWith("git am /tmp/aw.patch");
+    expect(global.exec.exec).toHaveBeenCalledWith(
+      "git push origin test-workflow-1234567890abcdef"
     );
 
     // Verify PR creation

diff --git a/pkg/workflow/js/push_to_pr_branch.test.cjs b/pkg/workflow/js/push_to_pr_branch.test.cjs
@@ -61,15 +61,15 @@ global.context = mockContext;
 describe("push_to_pr_branch.cjs", () => {
   let pushToPrBranchScript;
   let mockFs;
-  let mockExecSync;
+  let mockExec;
 
   // Helper function to execute the script with proper globals
   const executeScript = async () => {
     // Set globals just before execution
     global.core = mockCore;
     global.context = mockContext;
     global.mockFs = mockFs;
-    global.mockExecSync = mockExecSync;
+    global.exec = mockExec;
 
     // Execute the script
     return await eval(`(async () => { ${pushToPrBranchScript} })()`);
@@ -90,8 +90,21 @@ describe("push_to_pr_branch.cjs", () => {
       readFileSync: vi.fn(),
     };
 
-    // Create fresh mock for execSync
-    mockExecSync = vi.fn();
+    // Create fresh mock for exec
+    mockExec = {
+      exec: vi.fn().mockImplementation((command, args, options) => {
+        // Handle the gh pr view command specifically
+        if (command === "gh" && args && args[0] === "pr" && args[1] === "view") {
+          // Simulate the stdout listener being called with branch name
+          if (options && options.listeners && options.listeners.stdout) {
+            options.listeners.stdout(Buffer.from("feature-branch\n"));
+          }
+          return Promise.resolve(0); // Return exit code directly, not an object
+        }
+        // For other commands, just return success
+        return Promise.resolve(0);
+      }),
+    };
 
     // Reset mockCore calls
     mockCore.setFailed.mockReset();
@@ -105,12 +118,11 @@ describe("push_to_pr_branch.cjs", () => {
 
     // Modify the script to inject our mocks and make core available
     pushToPrBranchScript = pushToPrBranchScript.replace(
-      'async function main() {\n  /** @type {typeof import("fs")} */\n  const fs = require("fs");\n  const { execSync } = require("child_process");',
-      `async function main() {
-  const core = global.core;
-  const context = global.context || {};
-  const fs = global.mockFs;
-  const execSync = global.mockExecSync;`
+      /\/\*\* @type \{typeof import\("fs"\)\} \*\/\nconst fs = require\("fs"\);/,
+      `const core = global.core;
+const context = global.context || {};
+const fs = global.mockFs;
+const exec = global.exec;`
     );
   });
 
@@ -120,7 +132,7 @@ describe("push_to_pr_branch.cjs", () => {
       delete global.core;
       delete global.context;
       delete global.mockFs;
-      delete global.mockExecSync;
+      delete global.exec;
     }
   });
 
@@ -225,7 +237,6 @@ describe("push_to_pr_branch.cjs", () => {
       mockFs.readFileSync.mockReturnValue("");
 
       // Mock the git command to return a branch name
-      mockExecSync.mockReturnValue("feature-branch");
 
       // Execute the script
       await executeScript();
@@ -274,7 +285,6 @@ describe("push_to_pr_branch.cjs", () => {
       );
 
       // Mock the git commands that will be called
-      mockExecSync.mockReturnValue("feature-branch");
 
       // Execute the script
       await executeScript();
@@ -332,7 +342,6 @@ describe("push_to_pr_branch.cjs", () => {
       mockFs.readFileSync.mockReturnValue("some patch content");
 
       // Mock the git commands
-      mockExecSync.mockReturnValue("feature-branch");
 
       // Execute the script
       await executeScript();
@@ -383,7 +392,6 @@ describe("push_to_pr_branch.cjs", () => {
       mockFs.readFileSync.mockReturnValue(patchContent);
 
       // Mock the git commands that will be called
-      mockExecSync.mockReturnValue("feature-branch");
 
       // Execute the script
       await executeScript();
@@ -447,7 +455,6 @@ describe("push_to_pr_branch.cjs", () => {
       mockFs.readFileSync.mockReturnValue(patchContent);
 
       // Mock the git commands that will be called
-      mockExecSync.mockReturnValue("feature-branch");
 
       // Execute the script
       await executeScript();
@@ -478,7 +485,6 @@ describe("push_to_pr_branch.cjs", () => {
       mockFs.readFileSync.mockReturnValue(""); // Empty patch
 
       // Mock the git commands that will be called
-      mockExecSync.mockReturnValue("feature-branch");
 
       // Execute the script
       await executeScript();

diff --git a/pkg/workflow/js/setup_agent_output.cjs b/pkg/workflow/js/setup_agent_output.cjs
@@ -1,6 +1,10 @@
 const fs = require("fs");
+const crypto = require("crypto");
+
 function main() {
-  const outputFile = `/tmp/aw_output.txt`;
+  // Generate a unique filename using 16 random hex characters
+  const randomSuffix = crypto.randomBytes(8).toString("hex");
+  const outputFile = `/tmp/aw_output_${randomSuffix}.txt`;
   fs.mkdirSync("/tmp", { recursive: true });
   core.exportVariable("GITHUB_AW_SAFE_OUTPUTS", outputFile);
   core.setOutput("output_file", outputFile);

diff --git a/pkg/workflow/publish_assets_test.go b/pkg/workflow/publish_assets_test.go
@@ -15,7 +15,7 @@ func TestParseUploadAssetConfig(t *testing.T) {
 		{
 			name: "upload-asset config with custom values",
 			input: map[string]any{
-				"upload-asset": map[string]any{
+				"upload-assets": map[string]any{
 					"branch":       "my-assets/${{ github.event.repository.name }}",
 					"max-size":     5120,
 					"allowed-exts": []any{".jpg", ".png", ".txt"},

diff --git a/pkg/workflow/safe_outputs_mcp_server_test.go b/pkg/workflow/safe_outputs_mcp_server_test.go
@@ -29,6 +29,11 @@ func NewMCPTestClient(t *testing.T, outputFile string, config map[string]any) *M
 	// Set up environment
 	env := os.Environ()
 	env = append(env, fmt.Sprintf("GITHUB_AW_SAFE_OUTPUTS=%s", outputFile))
+
+	// Add required environment variables for upload_asset tool
+	env = append(env, "GITHUB_AW_ASSETS_BRANCH=test-assets")
+	env = append(env, "GITHUB_SERVER_URL=https://github.com")
+	env = append(env, "GITHUB_REPOSITORY=test/repo")
 
 	if config != nil {
 		configJSON, err := json.Marshal(config)
@@ -133,7 +138,7 @@ func TestSafeOutputsMCPServer_ListTools(t *testing.T) {
 		toolNames[i] = tool.Name
 	}
 
-	expectedTools := []string{"create-issue", "create-discussion", "missing-tool"}
+	expectedTools := []string{"create_issue", "create_discussion", "missing_tool"}
 	for _, expected := range expectedTools {
 		found := false
 		for _, actual := range toolNames {
@@ -445,13 +450,13 @@ func TestSafeOutputsMCPServer_PublishAsset(t *testing.T) {
 		t.Fatalf("Expected first content item to be text content, got %T", result.Content[0])
 	}
 
-	if !strings.Contains(textContent.Text, "published successfully") {
-		t.Errorf("Expected response to mention asset publishing, got: %s", textContent.Text)
+	if !strings.Contains(textContent.Text, "raw.githubusercontent.com") {
+		t.Errorf("Expected response to contain URL with raw.githubusercontent.com, got: %s", textContent.Text)
 	}
 
 	// Verify the output file contains the expected entry
-	if err := verifyOutputFile(t, tempFile, "upload_asset", map[string]any{
-		"type": "upload_asset",
+	if err := verifyOutputFile(t, tempFile, "upload-asset", map[string]any{
+		"type": "upload-asset",
 	}); err != nil {
 		t.Fatalf("Output file verification failed: %v", err)
 	}