fix(upload_assets): resolve staged assets via a single GH_AW_ASSETS_DIR#40122
Conversation
…-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
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes recurring upload_assets job failures caused by path-prefix mismatches between where assets are staged at agent runtime (often under RUNNER_TEMP/gh-aw/...) and where downstream jobs download artifacts (often under /tmp/gh-aw/...). It updates the asset consumer to search across multiple candidate staging directories so asset publication is resilient to these prefix differences.
Changes:
- Update
upload_assets.cjsto build a de-duplicated list of candidatesafeoutputs/assetsdirectories and search them in order per asset. - Improve missing-asset warnings to include all searched directories while preserving fail-soft behavior (only fail when all declared assets are missing).
- Add a regression test covering the cross-prefix staging scenario.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/upload_assets.cjs | Switch from a single derived assetsDir to ordered multi-candidate asset resolution to avoid path-prefix mismatches. |
| actions/setup/js/upload_assets.test.cjs | Add regression coverage for assets staged under RUNNER_TEMP while GH_AW_AGENT_OUTPUT points elsewhere. |
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: 2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Adjusting this fix |
|
🧠 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. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40122 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Neither enforcement condition is met. |
…ching 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.
dsyme
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with suggestions on test coverage and long-term design.
📋 Key Themes & Highlights
Key Themes
/diagnose: The multi-candidate search is a clean structural fix vs. the prior single-path approach, and the PR description accurately traces the root cause. The underlying producer-consumer path contract is still implicit — worth tracking as a follow-up./tdd: The regression test is well-targeted and exercises real filesystem operations. Two gaps: (1) priority ordering between candidates is untested, (2) thecore.infodiagnostic log isn't asserted and could be silently removed.
Positive Highlights
- ✅ De-duplication via
new Setcorrectly handles the case whereagentOutputFile's parent is/tmp/gh-aw— no double-check of the same path. - ✅ Fail-soft semantics (warn per missing file, only fail when all are absent) are fully preserved.
- ✅ The new warning message now surfaces every directory searched, which is a meaningful improvement for diagnosability.
- ✅ The regression test uses real filesystem isolation (
mkdtempSync) and a properfinallycleanup. - ✅ 194/194 tests passing, including the new regression case.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 69.3 AIC · ⌖ 7.63 AIC · ⊞ 6.9K
| fs.existsSync(runnerTempBase) && fs.rmSync(runnerTempBase, { recursive: !0, force: !0 }); | ||
| fs.existsSync(path.join(process.cwd(), targetFile)) && fs.unlinkSync(path.join(process.cwd(), targetFile)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
[/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 case
Add a second it that stages the file in both the agent-output directory and RUNNER_TEMP, then asserts the agent-output-relative copy was the one used (e.g. via SHA or a sentinel byte). Without this, a future reordering of candidateBaseDirs would pass all existing tests silently.
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)
});| } | ||
| candidateBaseDirs.push("/tmp/gh-aw"); | ||
| // Build the per-directory assets paths, de-duplicated while preserving order. | ||
| const assetsDirs = [...new Set(candidateBaseDirs.map(dir => path.join(dir, "safeoutputs", "assets")))]; |
There was a problem hiding this comment.
[/diagnose] This is the third targeted fix for the producer-consumer path-prefix mismatch (after #39900 and #40062). The multi-candidate search is structurally sound, but the root coupling — producer and consumer never agreeing on a base path — remains.
💡 More permanent alternative
The MCP upload_asset handler already knows exactly where it staged each file. If it wrote the staging base directory (or the full resolved path per asset) into agent_output.json alongside the fileName, this consumer could read it directly — eliminating the need for candidate-list guessing and making the contract explicit.
This is a larger change, but it would permanently close the recurrence vector and remove the /tmp/gh-aw hardcoded fallback.
| }); | ||
| try { | ||
| await executeScript(); | ||
| expect(mockCore.setFailed).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[/tdd] The test asserts setFailed was not called and upload_count equals 1, which is the right outcome check. Consider also asserting that core.info was called with the expected candidate paths — this locks in the diagnostic logging that will help future debugging if the mismatch recurs.
💡 Example assertion
const infoSearchCall = mockCore.info.mock.calls.find(
([msg]) => msg.startsWith("Searching for staged assets in:")
);
expect(infoSearchCall).toBeDefined();
expect(infoSearchCall[0]).toContain(runnerTempBase);This also guards against accidental removal of the info log, which is valuable for diagnosing future environment-specific path mismatches.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 0 (
|
test