[jsweep] Clean upload_artifact.cjs#35635
Conversation
- Fix JSDoc `/** @type {string[]} candidateRelPaths */` inline comment to correct `/** @type {string[]} */` declaration form - Fix JSDoc type casts for `include`/`exclude` to wrap full expressions with parentheses - Simplify verbose mutual-exclusion check using `hasPath === hasFilters` - Simplify filter expression with single-line logical operators instead of if-return chain - Add 8 new tests: max-size-bytes enforcement, artifact name derivation (custom, sanitised, basename, slot-index fallback), slot index outputs (increment, file_count/size_bytes) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35635 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics & Test Classification (8 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
Pull request overview
Cleanup of actions/setup/js/upload_artifact.cjs (a safe-output handler for uploading artifacts via @actions/artifact) for clarity, plus expanded test coverage in the colocated Vitest file.
Changes:
- Refactor
resolveFilesmutual-exclusion check (hasPath === hasFilters) and inline the include/exclude filter into a single boolean expression; fix a non-standard JSDoc@typeannotation. - Add 8 new tests covering
max-size-bytesenforcement at and over the limit, artifact-name derivation (custom name, sanitisation, basename fallback, slot-index fallback), and slot index/count outputs across multiple uploads.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/upload_artifact.cjs | Minor readability refactors in resolveFiles (mutual-exclusion guard, filter predicate, JSDoc type annotation). |
| actions/setup/js/upload_artifact.test.cjs | Adds 3 new describe blocks (8 tests) for size limits, name derivation, and slot index outputs. |
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
| } | ||
|
|
||
| /** @type {string[]} candidateRelPaths */ | ||
| /** @type {string[]} */ |
![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 /zoom-out and /tdd — no blocking issues, one readability suggestion.
📋 Key Themes & Highlights
Key Themes
- Correctness: Both simplifications are semantically equivalent to the originals.
hasPath === hasFiltersis true iff both are absent or both are present — matching the original count ≠ 1 guard exactly. ✅ - Readability trade-off: The filter lambda collapse is logically fine but produces a 158-char line; see inline comment for a split that preserves the one-liner intent without hitting line-length limits.
- PR description vs diff: The description claims JSDoc casts for
include/excludewere fixed to wrap the full||expression, but those lines appear unchanged in the diff. Not a blocker, but worth aligning the description with what actually changed.
Positive Highlights
- ✅
hasPath === hasFiltersis a genuine clarity win over the numeric-sum approach. - ✅ 8 new tests cover meaningful boundary conditions (exact-limit size, special-char sanitisation, slot-index increments) — good
/tddhygiene. - ✅ JSDoc variable-name-in-type-comment fix (
/**@type{string[]} candidateRelPaths */→/**@type{string[]} */) is correct.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
| if (exclude.length > 0 && matchesAnyPattern(f, exclude)) return false; | ||
| return true; | ||
| }); | ||
| candidateRelPaths = allFiles.filter(f => (include.length === 0 || matchesAnyPattern(f, include)) && (exclude.length === 0 || !matchesAnyPattern(f, exclude))); |
There was a problem hiding this comment.
[/zoom-out] The simplified one-liner is correct, but at 158 characters it trades the original multiline readability for compactness.
💡 Suggested split
candidateRelPaths = allFiles.filter(
f =>
(include.length === 0 || matchesAnyPattern(f, include)) &&
(exclude.length === 0 || !matchesAnyPattern(f, exclude))
);This keeps the two guard conditions on separate lines, making the logic easier to scan.
There was a problem hiding this comment.
The production logic changes are correct — hasPath === hasFilters is a valid boolean equality guard and the filter one-liner is logically equivalent to the removed multi-line form. Two test coverage gaps worth addressing before merge:
### Findings
- No regression test for the
hasPath === hasFiltersguard (main production change): both-present and neither-present paths are untested — see inline comment. - Sanitization test asserts absence of 3 chars but not structural validity of the result — an empty or all-hyphen name would pass silently — see inline comment.
All other new tests (max-size-bytes boundary, slot index outputs, name derivation fallbacks) look correct.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.7M
| expect(name).toBe("my-custom-name"); | ||
| }); | ||
|
|
||
| it("sanitises special characters in custom name", async () => { |
There was a problem hiding this comment.
Sanitization test only checks absence of three chars — the result could be empty or invalid.
💡 Suggested fix
The test asserts name does not contain , (, or !, but does not verify the result is non-empty or structurally valid. If the sanitizer collapses everything to hyphens or returns "", the test still passes and the upstream artifact upload would fail silently.
const [name] = mockArtifactClient.uploadArtifact.mock.calls[0];
expect(name.length).toBeGreaterThan(0);
expect(name).toMatch(/^[a-zA-Z0-9._-]+$/);
expect(name).not.toContain(" ");
expect(name).not.toContain("(");
expect(name).not.toContain("!");This fully pins the sanitized output contract rather than only checking a handful of rejected characters.
| @@ -507,6 +507,98 @@ describe("upload_artifact.cjs", () => { | |||
| }); | |||
There was a problem hiding this comment.
No regression test for the hasPath === hasFilters guard — the main production change in this PR.
💡 Suggested fix
The refactoring of the mutual-exclusivity check (hasMutuallyExclusive !== 1 → hasPath === hasFilters) is the only production code change, but there are no tests exercising the error branches it guards: (1) both path and filters provided, and (2) neither provided. A future refactor could silently regress the guard.
describe("resolveFiles mutual-exclusivity guard", () => {
it("fails when both path and filters are provided", async () => {
writeStaging("file.json");
const results = await runHandler(buildConfig(), [
{ type: "upload_artifact", path: "file.json", filters: { include: ["**"] } },
]);
expect(results[0].success).toBe(false);
expect(results[0].error).toContain("exactly one of 'path' or 'filters'");
});
it("fails when neither path nor filters are provided", async () => {
const results = await runHandler(buildConfig(), [
{ type: "upload_artifact" },
]);
expect(results[0].success).toBe(false);
expect(results[0].error).toContain("exactly one of 'path' or 'filters'");
});
});
|
|
1 similar comment
|
|
PR Description
Summary
Refactors
upload_artifact.cjsfor clarity and correctness, and expands the test suite with new coverage for size enforcement and future test scaffolding.Changes
actions/setup/js/upload_artifact.cjs(modified)path/filtersfields with a cleaner boolean equality expressionactions/setup/js/upload_artifact.test.cjs(modified)max-size-bytesenforcement:describeblocks for:Impact