fix: upload_assets looks in RUNNER_TEMP but artifact is downloaded to /tmp#40062
Conversation
…in upload_assets Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40062 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the upload_assets post-processing job by aligning upload_assets.cjs’s asset lookup directory with the actual artifact download location (co-located with GH_AW_AGENT_OUTPUT), and updates the associated tests to use a per-test temp base directory that mirrors the production layout.
Changes:
- Derive
assetsDirfrompath.dirname(GH_AW_AGENT_OUTPUT)(with a fallback) instead of assumingRUNNER_TEMP. - Update
upload_assets.test.cjsto create assets under a per-test temp directory aligned with the agent output location.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/upload_assets.cjs | Fixes asset discovery by basing safeoutputs/assets on the agent output parent directory. |
| actions/setup/js/upload_assets.test.cjs | Adjusts tests to create assets alongside GH_AW_AGENT_OUTPUT in a per-test temp base directory. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| const agentOutputFile = process.env.GH_AW_AGENT_OUTPUT; | ||
| const baseDir = agentOutputFile ? path.dirname(agentOutputFile) : path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw"); | ||
| const assetsDir = path.join(baseDir, "safeoutputs", "assets"); |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
Go: 0 ( Scoring breakdown:
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
COMMENT — Fix is correct; two non-blocking maintainability concerns.
Review themes
Implicit path coupling in baseDir derivation
The new logic extracts the asset directory from path.dirname(GH_AW_AGENT_OUTPUT), creating a structural dependency: it only works correctly if safeoutputs/assets/ is a direct sibling of agent_output.json. No validation or clear error distinguishes a path mismatch from genuinely absent assets — both produce per-item warnings and a silently low upload_count. See inline comment on upload_assets.cjs:94.
RUNNER_TEMP fallback is now untested
The old "from RUNNER_TEMP" test specifically exercised the fallback path. The replacement test only exercises the GH_AW_AGENT_OUTPUT branch. The fallback code is live but untested. See inline comment on upload_assets.test.cjs:144.
🔎 Code quality review by PR Code Quality Reviewer
| // 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"); |
There was a problem hiding this comment.
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.
| })); | ||
| 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 () => { |
There was a problem hiding this comment.
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;
});
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — no blocking issues; changes are a clean, well-scoped fix.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed: the path derivation now follows
GH_AW_AGENT_OUTPUTinstead ofRUNNER_TEMP, which is exactly where the artifact lands at runtime. - Fallback is dead code: because
loadAgentOutput()returns{success: false}whenGH_AW_AGENT_OUTPUTis unset andmain()returns early, line 94's ternary false-branch is unreachable in practice. The existing review comment on line 95 covers this well — consider apath.resolve()and/or removing the fallback to make the invariant explicit. - Test isolation significantly improved: migrating from manual
RUNNER_TEMPenv manipulation to per-testmkdtempSync+rmSync(recursive)inafterEachis a meaningful quality improvement — no more global state leakage between tests.
Positive Highlights
- ✅ Minimal blast radius: 2 files, +17/-12 lines, no API surface changes
- ✅ Test rename removes the
RUNNER_TEMPimplementation detail from the test name — good spec hygiene - ✅
tempFilePathcleanup inafterEachis correctly handled by deleting the wholetempBasedir, avoiding double-unlink
One nit
- The
fs.existsSync(assetDir) ||guard in the missing-asset test is redundant (see inline comment) — minor clarity issue only.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
|
@copilot run pr-finisher skill |
|
``
|
Ran pr-finisher triage on HEAD
Hand-off: please re-trigger CI from a maintainer action (close/reopen PR, workflow_dispatch, or maintainer push) before merge. |
…IR (#40122) * fix(upload_assets): search all candidate staging dirs to resolve path-prefix mismatch (#39885) The upload_assets (Push assets) job was failing with ERR_SYSTEM: Asset file not found: .../safeoutputs/assets/<file>.png even though the agent job succeeded, because assets were staged under a different base prefix than the job was reading from. Prior fixes (#39900, #40062) aligned individual paths, but the consumer still derived a single assetsDir — so any remaining producer/consumer prefix disagreement (e.g. RUNNER_TEMP=/home/runner/work/_temp vs the artifact download path /tmp/gh-aw) still hard-failed the whole job. Fix: build a de-duplicated list of candidate staging directories 1. parent of GH_AW_AGENT_OUTPUT (where the artifact was downloaded) 2. RUNNER_TEMP/gh-aw (where the MCP handler staged the file at runtime) 3. /tmp/gh-aw (canonical fallback) The first candidate that contains the file wins. The existing fail-soft behaviour (warn per missing file, only fail when all are missing) is preserved. The missing-file warning now lists every directory searched. Adds a regression test for the cross-prefix case. Closes #39885 * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * refactor(upload_assets): read single GH_AW_ASSETS_DIR instead of searching Replace the multi-candidate staging-dir search with a single source of truth. The download-artifact step in the upload_assets job writes the safe-outputs assets to a fixed path, and the Go generator now passes that exact path to the consumer via GH_AW_ASSETS_DIR. Add constants.TmpGhAwAssetsDir so the download path and the env var can never drift. This removes the GH_AW_AGENT_OUTPUT-derived path indirection that caused the phantom-asset failures (#39885): GH_AW_AGENT_OUTPUT belongs to a separate artifact and its directory is decoupled from where assets are downloaded. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The
upload_assetsjob downloads thesafe-outputs-assetsartifact to/tmp/gh-aw/safeoutputs/assets/, butupload_assets.cjsconstructed the lookup path fromRUNNER_TEMP(/home/runner/work/_tempon Actions runners) — causing all assets to appear missing and the job to fail.Changes
upload_assets.cjs: deriveassetsDirbase frompath.dirname(GH_AW_AGENT_OUTPUT)(already set to/tmp/gh-aw/agent_output.jsonin the job), falling back toRUNNER_TEMPwhen unsetupload_assets.test.cjs: replace ad-hocRUNNER_TEMP-based asset paths with a per-testtempBasedir created inbeforeEach, so asset paths in tests are always co-located withGH_AW_AGENT_OUTPUT— matching the new production layoutpr-sous-chef: requested branch refresh for run https://github.com/github/gh-aw/actions/runs/27762885695