[jsweep] Clean upload_assets.cjs#38284
Conversation
- Add `@typedef` for `UploadAssetItem` to type the filtered items array properly
- Remove the awkward inline `/** @param {any} item */` annotation inside `filter()`
- Rewrite test file from minified/compact form to readable, well-structured format
- Expand test suite from 5 to 14 tests, adding coverage for:
- Missing `GH_AW_ASSETS_BRANCH` env var (ERR_CONFIG)
- Agent output load failures
- Empty upload-asset items list
- SHA mismatch (ERR_VALIDATION)
- Missing asset source file (ERR_SYSTEM)
- Missing required asset fields (ERR_VALIDATION)
- Skipping assets whose target file already exists
- Staged mode suppressing git push
- Non-staged mode calling git push
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38284 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (≤100 threshold, requires_adr_by_default_volume=false). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer was skipped during the skills-based review. |
|
❌ Test Quality Sentinel was skipped during test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR cleans up the actions/setup/js/upload_assets.cjs github-script helper and fully rewrites actions/setup/js/upload_assets.test.cjs from a compressed/minified format into readable, structured Vitest tests, substantially expanding coverage for validation and git-flow behaviors.
Changes:
- Added a JSDoc
@typedefand replaced an inlinefilter()param annotation with a type-cast for the filtered upload-asset items array. - Rewrote
upload_assets.test.cjsinto a maintainable structure and added many new targeted test cases (env validation, agent output handling, asset validation, staged vs non-staged push behavior).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/upload_assets.cjs | Introduces a typedef and cleans up typing around filtering upload-asset items. |
| actions/setup/js/upload_assets.test.cjs | Reformats and expands tests to cover more validation and upload/push behaviors. |
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
| afterEach(() => { | ||
| for (const f of tempFiles) { | ||
| if (fs.existsSync(f)) fs.unlinkSync(f); | ||
| } | ||
| }); |
|
@copilot run pr-finisher skill |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Three cleanup regressions need fixing before merge.
### Blocking issues (3)
Test file temp-file leaks (2 regressions from the old code):
setAgentOutput()— the old code tracked the agent-output JSON intempFilePathand deleted it inafterEach; the rewrite returns the path but no call site registers it, so every test leaves a file in/tmp.createAssetFile()— callers push only the localtargetFileName(CWD copy) intotempFiles, neverassetPath(the fixture in/tmp/gh-aw/safeoutputs/assets/). Stale ASSET_DIR fixtures can cause false positives on test retry or re-run (e.g. theERR_SYSTEM/ghost.pngtest).
mockCore dropped debug and notice: the previous mock included both; their absence will produce cryptic TypeError: core.xxx is not a function errors the moment either method is called in the script.
### Non-blocking (1)
upload_assets.cjs line 82: JSDoc type assertion is missing the required parentheses (/** @type {T} */ (expr)).
🔎 Code quality review by PR Code Quality Reviewer · 11 AIC · ⌖ 13.5 AIC
| const content = typeof data === "string" ? data : JSON.stringify(data); | ||
| fs.writeFileSync(filePath, content); | ||
| process.env.GH_AW_AGENT_OUTPUT = filePath; | ||
| return filePath; |
There was a problem hiding this comment.
setAgentOutput leaks temp files on every test invocation: the returned filePath is never pushed to tempFiles, so afterEach never removes it. The old code explicitly tracked this in a tempFilePath variable and cleaned it up — this PR silently drops that.
💡 Suggested fix
Either have callers push the return value, or have the helper register it directly:
function setAgentOutput(data) {
const filePath = path.join("/tmp", `test_agent_output_${Date.now()}_${Math.random().toString(36).slice(2)}.json`);
const content = typeof data === "string" ? data : JSON.stringify(data);
fs.writeFileSync(filePath, content);
process.env.GH_AW_AGENT_OUTPUT = filePath;
tempFiles.push(filePath); // register for cleanup
return filePath;
}Note: tempFiles is suite-scoped so setAgentOutput can’t access it directly as written. The simplest fix is for each call site to do tempFiles.push(setAgentOutput(...)), or pass tempFiles as a parameter.
| const assetPath = path.join(ASSET_DIR, fileName); | ||
| fs.writeFileSync(assetPath, content); | ||
| const sha = crypto.createHash("sha256").update(fs.readFileSync(assetPath)).digest("hex"); | ||
| return { assetPath, sha, size: content.length }; |
There was a problem hiding this comment.
ASSET_DIR fixture files are never cleaned up: createAssetFile writes to /tmp/gh-aw/safeoutputs/assets/<fileName> and returns assetPath, but callers only push the local targetFileName into tempFiles — never assetPath. The source fixtures accumulate across test runs.
💡 Why this matters
If the same runner executes the suite twice (retry, re-run, parallel shard), stale files linger in ASSET_DIR. Any test that expects a file to not exist there (e.g. the ERR_SYSTEM test for ghost.png) could false-pass if a previous run left a matching file behind — or vice versa.
Fix: push assetPath into tempFiles at every call site:
const { assetPath, sha, size } = createAssetFile("orphan-test.png");
tempFiles.push(assetPath); // ← was missingor have createAssetFile accept the tempFiles array and register itself.
| })); | ||
| })); | ||
| })); | ||
| }; |
There was a problem hiding this comment.
mockCore silently dropped debug and notice: the previous mock included both debug: vi.fn() and notice: vi.fn(). Removing them means any future core.debug() or core.notice() call in upload_assets.cjs will throw TypeError: core.debug is not a function rather than failing with a meaningful assertion.
💡 Suggested fix
Add the missing stubs to keep the mock surface complete:
mockCore = {
debug: vi.fn(), // ← restore
info: vi.fn(),
notice: vi.fn(), // ← restore
warning: vi.fn(),
error: vi.fn(),
setFailed: vi.fn(),
setOutput: vi.fn(),
summary: {
addRaw: vi.fn().mockReturnThis(),
write: vi.fn().mockResolvedValue(undefined),
},
};@actions/core exposes all of these publicly; the mock should match its shape.
|
|
||
| // Find all upload-asset items | ||
| const uploadItems = result.items.filter(/** @param {any} item */ item => item.type === "upload_asset"); | ||
| const uploadItems = /** @type {UploadAssetItem[]} */ result.items.filter(item => item.type === "upload_asset"); |
There was a problem hiding this comment.
JSDoc type assertion requires parentheses around the expression: TypeScript JSDoc mandates /** @type {T} */ (expr) with parens. Without them, TypeScript treats @type as a variable annotation on uploadItems (silencing inference) rather than a checked cast that validates result.items.filter(...) is actually assignable to UploadAssetItem[]. The file is // @ts-check``, so the distinction matters.
💡 Suggested fix
const uploadItems = /** `@type` {UploadAssetItem[]} */ (result.items.filter(item => item.type === "upload_asset"));The parenthesized form is the only form TypeScript recognises as a proper type assertion in JSDoc; see the [TypeScript JSDoc reference]((www.typescriptlang.org/redacted)
github-actions
Bot
added
the
closed:unknown
Summary
Cleaned
upload_assets.cjsas part of the daily jsweep run.Context
Execution context: github-script (uses
core,execglobals)Changes to
upload_assets.cjs@typedefforUploadAssetItemto properly type the filtered items array/**@param{any} item */annotation insidefilter()with a clean type-cast using the new typedefChanges to
upload_assets.test.cjsFull rewrite of the test file from minified/compressed form to readable, well-structured format.
Tests: 5 → 14 (+9 new test cases)
New tests added:
branch env var validation— fails withERR_CONFIGwhenGH_AW_ASSETS_BRANCHis not setagent output handling—upload_count=0when agent output fails to loadagent output handling—upload_count=0when items list has no upload-asset entriesasset validation— fails withERR_VALIDATIONon SHA mismatchasset validation— fails withERR_SYSTEMwhen source file is missingasset validation— fails withERR_VALIDATIONwhen required asset fields are absentfile upload behaviour— skips assets whose target file already existsfile upload behaviour— suppressesgit pushin staged modefile upload behaviour— callsgit push originin non-staged modeAll pre-existing tests were preserved in a cleaner, more readable form.
Validation ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js— 14 passed ✓