diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 48756fc6c3..2030d36b44 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/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, diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index 016bfaecaf..acac185b6a 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/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(); diff --git a/actions/setup/js/safe_outputs_tools_loader.cjs b/actions/setup/js/safe_outputs_tools_loader.cjs index 9a1b93b5bd..b13e940cec 100644 --- a/actions/setup/js/safe_outputs_tools_loader.cjs +++ b/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, };