fix: copy upload_artifact files to staging in MCP server handler (#26157)

Author: Copilot

actions/setup/js/safe_outputs_handlers.cjs

@@ -890,9 +890,112 @@ function createHandlers(server, appendSafeOutput, config = {}) {
     };
   };
 
+  /**
+   * Recursively copy all regular files from srcDir into destDir, preserving the relative
+   * path structure under srcDir. Non-regular entries (sockets, devices, pipes, symlinks)
+   * are skipped silently.
+   * @param {string} srcDir - Absolute source directory path
+   * @param {string} destDir - Absolute destination directory path
+   */
+  function copyDirectoryRecursive(srcDir, destDir) {
+    if (!fs.existsSync(destDir)) {
+      fs.mkdirSync(destDir, { recursive: true });
+    }
+    for (const ent of fs.readdirSync(srcDir, { withFileTypes: true })) {
+      const srcPath = path.join(srcDir, ent.name);
+      const destPath = path.join(destDir, ent.name);
+      if (ent.isDirectory()) {
+        copyDirectoryRecursive(srcPath, destPath);
+      } else if (ent.isFile() && !ent.isSymbolicLink() && !fs.existsSync(destPath)) {
+        fs.copyFileSync(srcPath, destPath);
+      }
+      // Skip symlinks, sockets, pipes, block/char devices — non-regular file types.
+    }
+  }
+
+  /**
+   * Handler for upload_artifact tool.
+   *
+   * When the agent calls upload_artifact with an absolute path (e.g.,
+   * /tmp/gh-aw/python/charts/loc_by_language.png), the file lives only inside the
+   * sandboxed container.  After the container exits the file is gone, so the safe_outputs
+   * job running on a different runner cannot find it.
+   *
+   * This handler copies the file (or directory) to the staging directory
+   * ($RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/), which is bind-mounted rw into
+   * the container.  The agent job then uploads that staging directory as the
+   * safe-outputs-upload-artifacts artifact, and the safe_outputs job downloads it before
+   * processing.
+   *
+   * For path-based requests with an absolute path the handler also rewrites entry.path to
+   * the staging-relative basename so that upload_artifact.cjs on the safe_outputs runner
+   * resolves the file from staging rather than trying the (non-existent) absolute path.
+   *
+   * Relative paths and filter-based requests are passed through unchanged because the
+   * agent is expected to have placed those files in staging directly.
+   */
+  const uploadArtifactHandler = args => {
+    const entry = { ...(args || {}), type: "upload_artifact" };
+
+    if (typeof entry.path === "string" && path.isAbsolute(entry.path)) {
+      const filePath = entry.path;
+
+      if (!fs.existsSync(filePath)) {
+        throw {
+          code: -32602,
+          message: `${ERR_VALIDATION}: upload_artifact: file not found: ${filePath}`,
+        };
+      }
+
+      const stat = fs.lstatSync(filePath);
+      if (stat.isSymbolicLink()) {
+        throw {
+          code: -32602,
+          message: `${ERR_VALIDATION}: upload_artifact: symlinks are not allowed: ${filePath}`,
+        };
+      }
+
+      const stagingDir = path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "safeoutputs", "upload-artifacts");
+      if (!fs.existsSync(stagingDir)) {
+        fs.mkdirSync(stagingDir, { recursive: true });
+      }
+
+      const destName = path.basename(filePath);
+
+      if (stat.isDirectory()) {
+        copyDirectoryRecursive(filePath, path.join(stagingDir, destName));
+      } else {
+        const destPath = path.join(stagingDir, destName);
+        if (!fs.existsSync(destPath)) {
+          fs.copyFileSync(filePath, destPath);
+        }
+      }
+
+      // Rewrite to staging-relative path so upload_artifact.cjs resolves it from staging.
+      entry.path = destName;
+      server.debug(`upload_artifact: staged ${filePath} as ${destName}`);
+    }
+
+    appendSafeOutput(entry);
+
+    const temporaryId = entry.temporary_id || null;
+    return {
+      content: [
+        {
+          type: "text",
+          text: JSON.stringify({
+            result: "success",
+            ...(temporaryId ? { temporary_id: temporaryId } : {}),
+          }),
+        },
+      ],
+    };
+  };
+
   return {
     defaultHandler,
     uploadAssetHandler,
+    uploadArtifactHandler,
     createPullRequestHandler,
     pushToPullRequestBranchHandler,
     pushRepoMemoryHandler,

actions/setup/js/safe_outputs_handlers.test.cjs

@@ -279,6 +279,130 @@ describe("safe_outputs_handlers", () => {
     });
   });
 
+  describe("uploadArtifactHandler", () => {
+    let testStagingDir;
+
+    beforeEach(() => {
+      const testId = Math.random().toString(36).substring(7);
+      testStagingDir = `/tmp/test-staging-${testId}`;
+      process.env.RUNNER_TEMP = testStagingDir;
+    });
+
+    afterEach(() => {
+      delete process.env.RUNNER_TEMP;
+      try {
+        if (fs.existsSync(testStagingDir)) {
+          fs.rmSync(testStagingDir, { recursive: true, force: true });
+        }
+      } catch {
+        // Ignore cleanup errors
+      }
+    });
+
+    it("should copy absolute-path file to staging and rewrite path to basename", () => {
+      const srcFile = path.join(testWorkspaceDir, "chart.png");
+      fs.writeFileSync(srcFile, "png data");
+
+      const result = handlers.uploadArtifactHandler({ path: srcFile });
+
+      // File should be in staging
+      const stagedPath = path.join(testStagingDir, "gh-aw", "safeoutputs", "upload-artifacts", "chart.png");
+      expect(fs.existsSync(stagedPath)).toBe(true);
+      expect(fs.readFileSync(stagedPath, "utf8")).toBe("png data");
+
+      // JSONL entry should use the basename, not the absolute path
+      expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "upload_artifact", path: "chart.png" }));
+
+      // Response should be success
+      const responseData = JSON.parse(result.content[0].text);
+      expect(responseData.result).toBe("success");
+    });
+
+    it("should include temporary_id in response when provided", () => {
+      const srcFile = path.join(testWorkspaceDir, "plot.png");
+      fs.writeFileSync(srcFile, "png data");
+
+      const result = handlers.uploadArtifactHandler({ path: srcFile, temporary_id: "aw_test123" });
+
+      const responseData = JSON.parse(result.content[0].text);
+      expect(responseData.result).toBe("success");
+      expect(responseData.temporary_id).toBe("aw_test123");
+    });
+
+    it("should throw when absolute-path file does not exist", () => {
+      expect(() => handlers.uploadArtifactHandler({ path: "/tmp/nonexistent-file.png" })).toThrow(expect.objectContaining({ message: expect.stringContaining("file not found") }));
+    });
+
+    it("should throw when path is a symlink", () => {
+      const srcFile = path.join(testWorkspaceDir, "real.png");
+      fs.writeFileSync(srcFile, "data");
+      const linkPath = path.join(testWorkspaceDir, "link.png");
+      fs.symlinkSync(srcFile, linkPath);
+
+      expect(() => handlers.uploadArtifactHandler({ path: linkPath })).toThrow(expect.objectContaining({ message: expect.stringContaining("symlinks are not allowed") }));
+    });
+
+    it("should not overwrite existing staged file on duplicate call", () => {
+      const srcFile = path.join(testWorkspaceDir, "chart.png");
+      fs.writeFileSync(srcFile, "original");
+
+      // First call stages the file
+      handlers.uploadArtifactHandler({ path: srcFile });
+
+      const stagedPath = path.join(testStagingDir, "gh-aw", "safeoutputs", "upload-artifacts", "chart.png");
+      expect(fs.readFileSync(stagedPath, "utf8")).toBe("original");
+
+      // Second call with modified source should not overwrite
+      fs.writeFileSync(srcFile, "updated");
+      handlers.uploadArtifactHandler({ path: srcFile });
+      expect(fs.readFileSync(stagedPath, "utf8")).toBe("original");
+    });
+
+    it("should pass through relative path without copying to staging", () => {
+      // Relative paths reference files already in staging - no copy needed
+      const result = handlers.uploadArtifactHandler({ path: "already-staged.png" });
+
+      // Staging dir should NOT have been created/written by the handler
+      const stagingDir = path.join(testStagingDir, "gh-aw", "safeoutputs", "upload-artifacts");
+      const stagedFile = path.join(stagingDir, "already-staged.png");
+      expect(fs.existsSync(stagedFile)).toBe(false);
+
+      // JSONL entry should preserve the relative path as-is
+      expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "upload_artifact", path: "already-staged.png" }));
+
+      const responseData = JSON.parse(result.content[0].text);
+      expect(responseData.result).toBe("success");
+    });
+
+    it("should pass through filters-based request without file copy", () => {
+      const result = handlers.uploadArtifactHandler({ filters: { include: ["**/*.png"] } });
+
+      const stagingDir = path.join(testStagingDir, "gh-aw", "safeoutputs", "upload-artifacts");
+      expect(fs.existsSync(stagingDir)).toBe(false);
+
+      expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "upload_artifact", filters: { include: ["**/*.png"] } }));
+
+      const responseData = JSON.parse(result.content[0].text);
+      expect(responseData.result).toBe("success");
+    });
+
+    it("should recursively copy directory to staging", () => {
+      const srcDir = path.join(testWorkspaceDir, "charts");
+      fs.mkdirSync(path.join(srcDir, "sub"), { recursive: true });
+      fs.writeFileSync(path.join(srcDir, "a.png"), "a");
+      fs.writeFileSync(path.join(srcDir, "sub", "b.png"), "b");
+
+      handlers.uploadArtifactHandler({ path: srcDir });
+
+      const stagingBase = path.join(testStagingDir, "gh-aw", "safeoutputs", "upload-artifacts", "charts");
+      expect(fs.existsSync(path.join(stagingBase, "a.png"))).toBe(true);
+      expect(fs.existsSync(path.join(stagingBase, "sub", "b.png"))).toBe(true);
+
+      // Entry path should be the directory basename
+      expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "upload_artifact", path: "charts" }));
+    });
+  });
+
   describe("createPullRequestHandler", () => {
     it("should be defined", () => {
       expect(handlers.createPullRequestHandler).toBeDefined();
@@ -446,6 +570,7 @@ describe("safe_outputs_handlers", () => {
     it("should export all required handlers", () => {
       expect(handlers.defaultHandler).toBeDefined();
       expect(handlers.uploadAssetHandler).toBeDefined();
+      expect(handlers.uploadArtifactHandler).toBeDefined();
       expect(handlers.createPullRequestHandler).toBeDefined();
       expect(handlers.pushToPullRequestBranchHandler).toBeDefined();
       expect(handlers.pushRepoMemoryHandler).toBeDefined();

actions/setup/js/safe_outputs_tools_loader.cjs

@@ -76,6 +76,7 @@ function attachHandlers(tools, handlers) {
     push_to_pull_request_branch: handlers.pushToPullRequestBranchHandler,
     push_repo_memory: handlers.pushRepoMemoryHandler,
     upload_asset: handlers.uploadAssetHandler,
+    upload_artifact: handlers.uploadArtifactHandler,
     create_project: handlers.createProjectHandler,
     add_comment: handlers.addCommentHandler,
   };