[jsweep] Clean upload_assets.cjs#41371
Conversation
- Add @typedef for UploadAssetItem to replace inline @param {any} annotation - Replace minified test file with readable, well-structured tests - Extract makeAsset and mockBranchMissing helpers to reduce duplication - Add 7 new test cases (14 total, up from 7): - Missing GH_AW_ASSETS_BRANCH env var - No upload-asset items in output - SHA mismatch detection - Invalid asset entry (missing required fields) - All declared assets missing - Staged mode does not push - Non-upload_asset item types are ignored Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41371 does not have the 'implementation' label and has 0 new lines of code in business logic directories. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the actions/setup/js/upload_assets.cjs github-script helper to use a named JSDoc typedef for upload-asset items, and rewrites the corresponding Vitest suite into a clearer, helper-based structure while expanding coverage across key validation and edge cases.
Changes:
- Added a JSDoc
@typedef(UploadAssetItem) and used it to type the filtereduploadItemsarray inupload_assets.cjs. - Rewrote
upload_assets.test.cjsfrom a minified style into an idiomatic Vitest structure with reusable helpers. - Expanded the test matrix to cover env validation, type filtering, SHA mismatch, invalid entries, “all missing” failure, staged-mode no-push behavior, and assets-dir resolution.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/upload_assets.cjs | Introduces a named upload-item typedef and applies it to the filtered upload-assets list for improved readability/type clarity under @ts-check. |
| actions/setup/js/upload_assets.test.cjs | Full test rewrite for readability + new/expanded cases validating branch/env behavior, SHA validation, missing/invalid asset handling, staged-mode push suppression, and assets dir sourcing. |
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: 0
![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 /tdd and /zoom-out — approving with minor suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Excellent test refactor: readable structure replacing the comma-expression minified style is a clear win for maintainability
- ✅
makeAsset()andmockBranchMissing()helpers eliminate boilerplate cleanly — exactly the right DRY move - ✅ 7 new test cases covering the important failure paths (missing env var, SHA mismatch, invalid entries, staged mode)
- ✅
cryptopromoted to a top-levelimportinstead of repeatedrequire()inside each test body - ✅
@typedefforUploadAssetItemis a good type-safety improvement over the previous@param {any}workaround
Suggestions (non-blocking)
@typecast syntax (upload_assets.cjs:82) — wrap the expression in parentheses for idiomatic JSDoc casting- Zombie
tempFilePathvariable (upload_assets.test.cjs:24) — never read after assignment; safe to remove - Vague assertion (
upload_assets.test.cjs:271) —stringContaining("missing")could pass on unrelated error messages; tighten to"declared assets were missing" - Cleanup without
try/finally(multiple tests) — if an assertion throws,process.cwd()temp files leak; theassets dir resolutiontest already models the right pattern - Helper underuse in staged-mode test (
upload_assets.test.cjs:292) — duplicates therev-parsethrow frommockBranchMissing()instead of composing with it
All issues are style/robustness level — no correctness or security problems found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 83.8 AIC · ⌖ 7.63 AIC · ⊞ 6.5K
|
|
||
| // 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.
[/zoom-out] JSDoc cast is missing parentheses around the expression — in a @ts-check file, the cast should wrap the full expression.
💡 Fix
// Current — cast may bind only to `result.items`, not the full call chain
const uploadItems = /** `@type` {UploadAssetItem[]} */ result.items.filter(item => item.type === "upload_asset");
// Idiomatic — cast wraps the entire expression
const uploadItems = /** `@type` {UploadAssetItem[]} */ (result.items.filter(item => item.type === "upload_asset"));Without parentheses, TypeScript's JSDoc parser treats the annotation as targeting result.items rather than the full .filter() return value. The parenthesised form is the canonical cast syntax (TypeScript handbook — JSDoc casts).
There was a problem hiding this comment.
Addressed in 5eddc6e by switching uploadItems to a formatter-stable variable-level @type {UploadAssetItem[]} annotation.
| let uploadAssetsScript; | ||
| let mockExec; | ||
| let tempBase; | ||
| let tempFilePath; |
There was a problem hiding this comment.
[/zoom-out] tempFilePath is a zombie variable — it is assigned inside setAgentOutput and reset to undefined in afterEach, but it is never read anywhere. Since afterEach already recursively removes tempBase (which contains the file), tempFilePath provides no cleanup value and can be dropped.
💡 Suggested removal
Remove the declaration on line 24, the assignment on line 29, and the tempFilePath = undefined; in afterEach. The GH_AW_AGENT_OUTPUT env var is already cleaned up by delete process.env.GH_AW_AGENT_OUTPUT in beforeEach.
There was a problem hiding this comment.
Addressed in 5eddc6e by removing the unused tempFilePath state; setAgentOutput() now keeps that path local.
| mockBranchMissing(); | ||
|
|
||
| await executeScript(); | ||
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("missing")); |
There was a problem hiding this comment.
[/tdd] The assertion stringContaining("missing") is too broad — any error message containing "missing" would satisfy it, including unrelated failures like SHA-mismatch messages or file-not-found warnings.
💡 More specific assertion
The actual error from upload_assets.cjs line 178 is:
All X declared assets were missing; no assets published.
A tighter assertion makes the test act as a specification:
expect(mockCore.setFailed).toHaveBeenCalledWith(
expect.stringContaining("declared assets were missing")
);
// or
expect(mockCore.setFailed).toHaveBeenCalledWith(
expect.stringContaining("no assets published")
);This ensures the test fails when the message wording changes and prevents false-passes from unrelated error paths.
There was a problem hiding this comment.
Addressed in 848f950 by asserting the all-missing path with a fixture-derived count instead of a broad "missing" substring.
| expect(orphanBranchCreated).toBe(true); | ||
| expect(mockCore.setFailed).not.toHaveBeenCalled(); | ||
| if (fs.existsSync(path.join(process.cwd(), "test.png"))) fs.unlinkSync(path.join(process.cwd(), "test.png")); | ||
| if (fs.existsSync(assetPath)) fs.unlinkSync(assetPath); |
There was a problem hiding this comment.
[/tdd] These fs.unlinkSync calls are outside a try/finally block — if any assertion before line 143 fails, the files in process.cwd() will leak between test runs. The assets dir resolution test (line 354) correctly uses try/finally; other tests should follow the same pattern.
💡 Suggested pattern
await executeScript();
try {
expect(orphanBranchCreated).toBe(true);
expect(mockCore.setFailed).not.toHaveBeenCalled();
} finally {
if (fs.existsSync(path.join(process.cwd(), "test.png"))) fs.unlinkSync(path.join(process.cwd(), "test.png"));
if (fs.existsSync(assetPath)) fs.unlinkSync(assetPath);
}Alternatively, move all process.cwd() cleanup into afterEach by tracking the produced target filenames there.
There was a problem hiding this comment.
Addressed in 5eddc6e by centralizing cwd artifact cleanup in afterEach, so failed assertions no longer leak files.
| if (fullCommand.includes("push")) pushCalled = true; | ||
| if (fullCommand.includes("rev-parse")) throw new Error("Branch does not exist"); | ||
| return 0; | ||
| }); |
There was a problem hiding this comment.
[/tdd] This mockImplementation duplicates the rev-parse throw logic from the newly-introduced mockBranchMissing() helper. Since the test only needs to additionally intercept push, it could compose with the helper instead of rewriting it.
💡 Approach using the helper
let pushCalled = false;
mockBranchMissing(); // sets the rev-parse behaviour
// Override to also track push while preserving rev-parse throwing
mockExec.exec.mockImplementation(async (command, args) => {
const fullCommand = Array.isArray(args) ? `${command} ${args.join(" ")}` : command;
if (fullCommand.includes("push")) pushCalled = true;
if (fullCommand.includes("rev-parse")) throw new Error("Branch does not exist");
return 0;
});This is a small duplication but stands out because mockBranchMissing() was introduced specifically to avoid it.
There was a problem hiding this comment.
Addressed in 5eddc6e by reusing mockBranchMissing() in the staged-mode case instead of duplicating the rev-parse behavior.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics & Test Classification (14 tests analyzed)
Go: 0; JavaScript: 14 ( Verdict
References:
|
There was a problem hiding this comment.
Non-blocking observations on a net-positive cleanup.
🔍 Findings summary
upload_assets.cjs
- JSDoc cast syntax (line 82):
/**@type{T} */ exprwithout parentheses is a variable annotation, not an inline cast. TypeScript accepts it silently here becauseresultisany-typed, but ifloadAgentOutputis ever typed more strictly the intent will be lost. Parenthesise:/**@type{UploadAssetItem[]} */ (result.items.filter(...)).
upload_assets.test.cjs — three new tests have weak assertions or fragile cleanup
stringContaining("missing")is too broad (line 271): matches env-var failures and other unrelated paths; use the actual message substring.fullCommand.includes("push")is too broad (line 289): any argument/path containing "push" as a substring satisfies it; checkcommand === "git" && args[0] === "push"instead.- cwd file cleanup not in
try/finally(line 142, and 6 other sites): if an assertion throws the cleanup is skipped and working-tree files persist across test runs. The "assets dir resolution" test already does this right.
No correctness, security, or CI-breaking issues found in the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · 93.9 AIC · ⌖ 7.49 AIC · ⊞ 5.2K
|
|
||
| // 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.
Malformed JSDoc cast — type assertion may not apply where intended.
💡 Details and fix
Per the TypeScript JSDoc spec, an inline cast requires the target expression to be parenthesised:
// ✗ current — `@type` is parsed as a cast of `result` alone
const uploadItems = /** `@type` {UploadAssetItem[]} */ result.items.filter(...);
// ✓ correct — `@type` applies to the full filter expression
const uploadItems = /** `@type` {UploadAssetItem[]} */ (result.items.filter(item => item.type === "upload_asset"));Without parentheses TypeScript reads the comment as a cast of result to UploadAssetItem[], and .items.filter() is called on that wrongly-typed value. This is silently accepted only because result is any-typed, meaning the annotation provides weaker type-safety than intended and will mislead anyone who later tightens the return type of loadAgentOutput.
There was a problem hiding this comment.
Addressed in 5eddc6e by replacing the inline cast with a stable variable-level @type {UploadAssetItem[]} annotation for uploadItems.
| mockBranchMissing(); | ||
|
|
||
| await executeScript(); | ||
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("missing")); |
There was a problem hiding this comment.
Assertion is too vague to prove the all-assets-missing code path ran.
💡 Details and fix
expect.stringContaining("missing") would also match a missing-env-var failure, a missing-required-fields failure, or any future error message that happens to contain the word. So the test cannot distinguish whether the specific "all declared assets missing" path was triggered.
The production code at line 177 emits a distinctive message:
core.setFailed(`All ${missingAssetCount} declared assets were missing; no assets published.`);Assert against that:
expect(mockCore.setFailed).toHaveBeenCalledWith(
expect.stringContaining("All 1 declared assets were missing")
);There was a problem hiding this comment.
Addressed in 848f950 by deriving the expected missing-asset count from the fixture and asserting that specific failure path.
| let pushCalled = false; | ||
| mockExec.exec.mockImplementation(async (command, args) => { | ||
| const fullCommand = Array.isArray(args) ? `${command} ${args.join(" ")}` : command; | ||
| if (fullCommand.includes("push")) pushCalled = true; |
There was a problem hiding this comment.
Substring match on "push" is too broad for a push-guard assertion.
💡 Details and fix
fullCommand.includes("push") would also fire on any git subcommand argument, file path, or commit message fragment that contains the substring. The test would incorrectly declare pushCalled = true (and then the expect(pushCalled).toBe(false) would fail) for unrelated matches.
Assert on the actual git push command shape instead:
mockExec.exec.mockImplementation(async (command, args) => {
if (command === "git" && Array.isArray(args) && args[0] === "push") pushCalled = true;
if (Array.isArray(args) && args.includes("rev-parse")) throw new Error("Branch does not exist");
return 0;
});There was a problem hiding this comment.
Addressed in 5eddc6e by narrowing the staged-mode check to the actual git push command shape.
| await executeScript(); | ||
| expect(orphanBranchCreated).toBe(true); | ||
| expect(mockCore.setFailed).not.toHaveBeenCalled(); | ||
| if (fs.existsSync(path.join(process.cwd(), "test.png"))) fs.unlinkSync(path.join(process.cwd(), "test.png")); |
There was a problem hiding this comment.
cwd file cleanup runs after assertions — leaked files if a test fails.
💡 Details and fix
This cleanup of process.cwd() artefacts is not guarded by try/finally. If expect(orphanBranchCreated).toBe(true) throws, the unlinkSync calls never run and test.png is left in the working tree. The same pattern repeats at lines 168, 194–195, 255–256, 297–298, and 336–337.
Either wrap the test body:
try {
await executeScript();
expect(...);
} finally {
if (fs.existsSync(path.join(process.cwd(), "test.png"))) fs.unlinkSync(...);
}or track targetFileName in a shared variable and clean it up in afterEach, mirroring the tempBase pattern.
The "assets dir resolution" test already uses try/finally correctly — apply the same pattern here.
There was a problem hiding this comment.
Addressed in 5eddc6e by moving cwd artifact cleanup into shared afterEach handling instead of post-assertion unlinkSync calls.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher pass. The review-thread fixes are in 5eddc6e and 848f950, local validation ( |
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
test