Skip to content

fix: upload_assets looks in RUNNER_TEMP but artifact is downloaded to /tmp #40062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jun 18, 2026
+17 −12
Merged

fix: upload_assets looks in RUNNER_TEMP but artifact is downloaded to /tmp #40062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91df900
fix: derive asset dir from GH_AW_AGENT_OUTPUT instead of RUNNER_TEMP …
Copilot Jun 18, 2026
717be57
Merge branch 'main' into copilot/investigate-image-upload-issue
github-actions[bot] Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion actions/setup/js/upload_assets.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,13 @@ async function main() {

core.info(`Found ${uploadItems.length} upload-asset item(s)`);

const assetsDir = path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "safeoutputs", "assets");
// Derive the base directory from GH_AW_AGENT_OUTPUT when available.
// In the upload_assets job, the agent artifact (including safeoutputs/assets/)
// is downloaded to the same parent directory as agent_output.json, which may
// differ from RUNNER_TEMP when the download path is explicitly set to /tmp/gh-aw/.
const agentOutputFile = process.env.GH_AW_AGENT_OUTPUT;
const baseDir = agentOutputFile ? path.dirname(agentOutputFile) : path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw");

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile path derivation — wrong GH_AW_AGENT_OUTPUT location silently breaks all asset uploads: if the env var ever points to a path more than one level below the artifact root, baseDir will resolve to the wrong directory and every asset will appear missing, with no error distinguishing this failure from "no assets found".

💡 Details and suggestion

The fix works today because GH_AW_AGENT_OUTPUT is set to /tmp/gh-aw/agent_output.json, so path.dirname() correctly yields /tmp/gh-aw/. But this is an implicit structural contract: the code assumes safeoutputs/assets/ is always a direct sibling of agent_output.json. Anything that changes the download layout (a nested subdirectory, a renamed artifact path, a self-hosted runner with a non-standard workspace) will silently break uploads.

A more robust alternative is a dedicated env var, or at minimum a log warning when the resolved assetsDir does not exist:

// Or at minimum: log clearly when the dir is missing
if (!fs.existsSync(assetsDir)) {
  core.warning(`Assets directory not found: ${assetsDir} — verify GH_AW_AGENT_OUTPUT path layout`);
}

The per-item "missing asset" warning is too granular to diagnose a path resolution bug in CI logs.

const assetsDir = path.join(baseDir, "safeoutputs", "assets");
Comment on lines +93 to +95
let uploadCount = 0;
let missingAssetCount = 0;
let hasChanges = false;
Expand Down
21 changes: 10 additions & 11 deletions actions/setup/js/upload_assets.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,24 @@ import path from "path";
const mockCore = { debug: vi.fn(), info: vi.fn(), notice: vi.fn(), warning: vi.fn(), error: vi.fn(), setFailed: vi.fn(), setOutput: vi.fn(), summary: { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(void 0) } };
((global.core = mockCore),
describe("upload_assets.cjs", () => {
let uploadAssetsScript, mockExec, tempFilePath;
let uploadAssetsScript, mockExec, tempFilePath, tempBase;
const setAgentOutput = data => {
tempFilePath = path.join("/tmp", `test_agent_output_${Date.now()}_${Math.random().toString(36).slice(2)}.json`);
tempFilePath = path.join(tempBase, "agent_output.json");
const content = "string" == typeof data ? data : JSON.stringify(data);
(fs.writeFileSync(tempFilePath, content), (process.env.GH_AW_AGENT_OUTPUT = tempFilePath));
},
getAssetsDir = () => path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "safeoutputs", "assets"),
getAssetsDir = () => path.join(tempBase, "safeoutputs", "assets"),
executeScript = async () => ((global.core = mockCore), (global.exec = mockExec), await eval(`(async () => { ${uploadAssetsScript}; await main(); })()`));
(beforeEach(() => {
(vi.clearAllMocks(), delete process.env.GH_AW_ASSETS_BRANCH, delete process.env.GH_AW_AGENT_OUTPUT, delete process.env.GH_AW_SAFE_OUTPUTS_STAGED);
tempBase = fs.mkdtempSync(path.join("/tmp", "test-gh-aw-"));
const scriptPath = path.join(__dirname, "upload_assets.cjs");
((uploadAssetsScript = fs.readFileSync(scriptPath, "utf8")), (mockExec = { exec: vi.fn().mockResolvedValue(0) }));
}),
afterEach(() => {
tempFilePath && fs.existsSync(tempFilePath) && fs.unlinkSync(tempFilePath);
tempBase && fs.existsSync(tempBase) && fs.rmSync(tempBase, { recursive: !0, force: !0 });
tempBase = void 0;
tempFilePath = void 0;
}),
describe("git commit command - vulnerability fix", () => {
it("should not wrap commit message in extra quotes to prevent command injection", async () => {
Expand Down Expand Up @@ -138,13 +141,11 @@ const mockCore = { debug: vi.fn(), info: vi.fn(), notice: vi.fn(), warning: vi.f
fs.existsSync("test.png") && fs.unlinkSync("test.png"));
}));
describe("missing asset handling", () => {
it("should skip missing assets while uploading present assets from RUNNER_TEMP", async () => {
it("should skip missing assets while uploading present assets", async () => {

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RUNNER_TEMP fallback path is now untested: the renamed test only exercises the GH_AW_AGENT_OUTPUT-derived path; there is no test that verifies the fallback branch (process.env.RUNNER_TEMP || '/tmp') still works correctly.

💡 Details and suggestion

The old test ("should skip missing assets while uploading present assets from RUNNER_TEMP") explicitly set process.env.RUNNER_TEMP and verified asset lookup through that path. The new test removes that and only exercises the GH_AW_AGENT_OUTPUT branch.

The fallback is the code path that runs when GH_AW_AGENT_OUTPUT is unset — a misconfigured job, a manual invocation, or any environment that doesn't set that variable. Any regression in the fallback will now go undetected.

Suggest adding a dedicated test like:

it('should derive asset dir from RUNNER_TEMP when GH_AW_AGENT_OUTPUT is not set', async () => {
  const runnerTempDir = fs.mkdtempSync(path.join('/tmp', 'runner-temp-'));
  process.env.RUNNER_TEMP = runnerTempDir;
  delete process.env.GH_AW_AGENT_OUTPUT;

  const assetDir = path.join(runnerTempDir, 'gh-aw', 'safeoutputs', 'assets');
  fs.mkdirSync(assetDir, { recursive: true });
  // ... write a test asset and verify upload ...

  fs.rmSync(runnerTempDir, { recursive: true, force: true });
  delete process.env.RUNNER_TEMP;
});

process.env.GH_AW_ASSETS_BRANCH = "assets/test-workflow";
process.env.GH_AW_SAFE_OUTPUTS_STAGED = "false";
const runnerTempDir = fs.mkdtempSync(path.join("/tmp", "upload-assets-runner-temp-"));
process.env.RUNNER_TEMP = runnerTempDir;
const assetDir = path.join(runnerTempDir, "gh-aw", "safeoutputs", "assets");
fs.mkdirSync(assetDir, { recursive: !0 });
const assetDir = getAssetsDir();
fs.existsSync(assetDir) || fs.mkdirSync(assetDir, { recursive: !0 });
Comment thread
pelikhan marked this conversation as resolved.
const presentAssetSourcePath = path.join(assetDir, "present.png");
fs.writeFileSync(presentAssetSourcePath, "present content");
const crypto = require("crypto"),
Expand Down Expand Up @@ -172,8 +173,6 @@ const mockCore = { debug: vi.fn(), info: vi.fn(), notice: vi.fn(), warning: vi.f
uploadCountCall && expect(uploadCountCall[1]).toBe("1");
fs.existsSync(presentAssetSourcePath) && fs.unlinkSync(presentAssetSourcePath);
fs.existsSync(path.join(process.cwd(), presentTargetFile)) && fs.unlinkSync(path.join(process.cwd(), presentTargetFile));
fs.rmSync(runnerTempDir, { recursive: !0, force: !0 });
delete process.env.RUNNER_TEMP;
});
});
}));
Expand Down
Loading