-
Notifications
You must be signed in to change notification settings - Fork 425
fix(upload_assets): resolve staged assets via a single GH_AW_ASSETS_DIR #40122
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,8 +13,9 @@ const mockCore = { debug: vi.fn(), info: vi.fn(), notice: vi.fn(), warning: vi.f | |
| 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); | ||
| (vi.clearAllMocks(), delete process.env.GH_AW_ASSETS_BRANCH, delete process.env.GH_AW_AGENT_OUTPUT, delete process.env.GH_AW_ASSETS_DIR, delete process.env.GH_AW_SAFE_OUTPUTS_STAGED); | ||
| tempBase = fs.mkdtempSync(path.join("/tmp", "test-gh-aw-")); | ||
| process.env.GH_AW_ASSETS_DIR = path.join(tempBase, "safeoutputs", "assets"); | ||
| const scriptPath = path.join(__dirname, "upload_assets.cjs"); | ||
| ((uploadAssetsScript = fs.readFileSync(scriptPath, "utf8")), (mockExec = { exec: vi.fn().mockResolvedValue(0) })); | ||
| }), | ||
|
|
@@ -175,5 +176,39 @@ const mockCore = { debug: vi.fn(), info: vi.fn(), notice: vi.fn(), warning: vi.f | |
| fs.existsSync(path.join(process.cwd(), presentTargetFile)) && fs.unlinkSync(path.join(process.cwd(), presentTargetFile)); | ||
| }); | ||
| }); | ||
| describe("staging directory resolution", () => { | ||
| it("should read assets from the GH_AW_ASSETS_DIR directory", async () => { | ||
| process.env.GH_AW_ASSETS_BRANCH = "assets/test-workflow"; | ||
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "false"; | ||
| // Point GH_AW_ASSETS_DIR at a custom directory (distinct from the | ||
| // agent-output dir) to confirm the consumer reads exactly the | ||
| // directory the download step wrote to — no search, no derivation. | ||
| const customAssetsDir = fs.mkdtempSync(path.join("/tmp", "test-gh-aw-assets-")); | ||
| process.env.GH_AW_ASSETS_DIR = customAssetsDir; | ||
| const assetSourcePath = path.join(customAssetsDir, "chart.png"); | ||
| fs.writeFileSync(assetSourcePath, "chart content"); | ||
| const crypto = require("crypto"), | ||
| fileContent = fs.readFileSync(assetSourcePath), | ||
| targetFile = "chart-uploaded.png"; | ||
| setAgentOutput({ | ||
| items: [{ type: "upload_asset", fileName: "chart.png", sha: crypto.createHash("sha256").update(fileContent).digest("hex"), size: fileContent.length, targetFileName: targetFile, url: "https://example.com/chart.png" }], | ||
| }); | ||
| mockExec.exec.mockImplementation(async (command, args) => { | ||
| const fullCommand = Array.isArray(args) ? `${command} ${args.join(" ")}` : command; | ||
| if (fullCommand.includes("rev-parse")) throw new Error("Branch does not exist"); | ||
| return 0; | ||
| }); | ||
| try { | ||
| await executeScript(); | ||
| expect(mockCore.setFailed).not.toHaveBeenCalled(); | ||
| const uploadCountCall = mockCore.setOutput.mock.calls.find(call => "upload_count" === call[0]); | ||
| expect(uploadCountCall).toBeDefined(); | ||
| uploadCountCall && expect(uploadCountCall[1]).toBe("1"); | ||
| } finally { | ||
| fs.existsSync(customAssetsDir) && fs.rmSync(customAssetsDir, { recursive: !0, force: !0 }); | ||
| fs.existsSync(path.join(process.cwd(), targetFile)) && fs.unlinkSync(path.join(process.cwd(), targetFile)); | ||
| } | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The regression test covers the core fix, but the priority order (agent-output dir → RUNNER_TEMP → /tmp) is an implicit contract with no test to guard it. 💡 Suggested additional test caseAdd a second it("should prefer agent-output dir over RUNNER_TEMP when both contain the asset", async () => {
// stage one file in tempBase/safeoutputs/assets/ (agent output dir)
// stage a different file at runnerTempBase/gh-aw/safeoutputs/assets/
// assert the agent-output-dir version was uploaded (check its SHA)
}); |
||
| }); | ||
| })); | ||
| })); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The test asserts
setFailedwas not called andupload_countequals1, which is the right outcome check. Consider also asserting thatcore.infowas called with the expected candidate paths — this locks in the diagnostic logging that will help future debugging if the mismatch recurs.💡 Example assertion
This also guards against accidental removal of the info log, which is valuable for diagnosing future environment-specific path mismatches.