[jsweep] Clean workflow_metadata_helpers.cjs#37490
Conversation
- Improve JSDoc type for buildWorkflowRunUrl ctx parameter:
{any} → {{ serverUrl?: string, runId: number | string }}
- Add 4 new test cases (7 → 11 total):
- getWorkflowMetadata: prefers context.serverUrl over GITHUB_SERVER_URL
- getWorkflowMetadata: falls back to https://github.com with no sources
- buildWorkflowRunUrl: falls back to https://github.com with no sources
- buildWorkflowRunUrl: handles runId of 0 correctly
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37490 does not have the "implementation" label and has 0 new lines of code in business logic directories (≤100 threshold). No custom .design-gate.yml config present. |
There was a problem hiding this comment.
Pull request overview
This PR is part of the jsweep “JavaScript unbloat” initiative and focuses on tightening the contract and test coverage for workflow run URL construction used in github-script contexts.
Changes:
- Refines the JSDoc type for
buildWorkflowRunUrl(ctx, ...)to a precise structural type for stronger API clarity. - Adds test coverage for server URL precedence, default URL fallback, and
runId === 0handling acrossgetWorkflowMetadataandbuildWorkflowRunUrl.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/workflow_metadata_helpers.cjs | Narrows buildWorkflowRunUrl’s JSDoc ctx type to a more explicit structural contract. |
| actions/setup/js/workflow_metadata_helpers.test.cjs | Adds 4 tests covering server URL precedence and fallback behavior, plus runId edge cases. |
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
| it("should fall back to https://github.com when no server URL sources are available", () => { | ||
| const originalEnv = process.env.GITHUB_SERVER_URL; | ||
| delete process.env.GITHUB_SERVER_URL; | ||
|
|
||
| const url = buildWorkflowRunUrl({ runId: 1 }, { owner: "owner", repo: "repo" }); | ||
|
|
||
| expect(url).toBe("https://github.com/owner/repo/actions/runs/1"); | ||
|
|
||
| if (originalEnv === undefined) { | ||
| delete process.env.GITHUB_SERVER_URL; | ||
| } else { | ||
| process.env.GITHUB_SERVER_URL = originalEnv; | ||
| } | ||
| }); |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![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 — one minor robustness observation in the new tests; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test env isolation: the new
buildWorkflowRunUrlfallback test uses a manual save/restore pattern withouttry-finally. The fix is to addbeforeEach/afterEachhooks to thebuildWorkflowRunUrldescribe block, matching the pattern already used ingetWorkflowMetadata(inline comment on line 150).
Positive Highlights
- ✅ JSDoc type upgrade — replacing
{any}with{{ serverUrl?: string, runId: number | string }}is a clear API-contract improvement that benefits all ~20 callers with zero runtime risk. - ✅ Four well-targeted new tests — serverUrl priority ordering,
https://github.comfallback, andrunId: 0edge case were genuine coverage gaps; all now documented as specifications. - ✅
getWorkflowMetadatatests usebeforeEach/afterEachcorrectly — clean, robust, easy to extend. - ✅ All 11 tests pass; format, lint, and typecheck ✓.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 569K · 170.7 AIC · ⌖ 13.3 AIC
| expect(url).not.toContain("cross-repo"); | ||
| }); | ||
|
|
||
| it("should fall back to https://github.com when no server URL sources are available", () => { |
There was a problem hiding this comment.
[/zoom-out] This test manually saves/restores GITHUB_SERVER_URL, but the cleanup at lines 158–162 is not guarded by try-finally. If the assertion on line 155 throws, cleanup is skipped and the deleted env var leaks into subsequent tests in this block.
�� Suggested: add lifecycle hooks to the buildWorkflowRunUrl describe block
The getWorkflowMetadata describe block (lines 5–118) already establishes the right pattern with beforeEach/afterEach. Applying the same to buildWorkflowRunUrl would let individual tests drop their manual save/restore entirely:
describe("buildWorkflowRunUrl", () => {
let savedServerUrl;
beforeEach(() => {
savedServerUrl = process.env.GITHUB_SERVER_URL;
});
afterEach(() => {
if (savedServerUrl === undefined) {
delete process.env.GITHUB_SERVER_URL;
} else {
process.env.GITHUB_SERVER_URL = savedServerUrl;
}
});
// tests can now freely mutate process.env.GITHUB_SERVER_URL
});As a smaller improvement, the existing if (originalEnv === undefined) { delete process.env.GITHUB_SERVER_URL; } branch (line 158) is a no-op — the var was already deleted at line 152 — so the branch can be simplified to just else { process.env.GITHUB_SERVER_URL = originalEnv; }.
There was a problem hiding this comment.
One non-blocking finding in the new test code. The JSDoc improvement to buildWorkflowRunUrl is correct.
🔍 Review summary
What was checked
workflow_metadata_helpers.cjs: only the@param {any}→@param {{ serverUrl?: string, runId: number | string }}JSDoc change — clean improvement, no issues.workflow_metadata_helpers.test.cjs: 4 new test cases added.
Finding: env cleanup not failure-safe (medium, non-blocking)
The new buildWorkflowRunUrl test "should fall back to https://github.com when no server URL sources are available" restores GITHUB_SERVER_URL after the expect() call. A failing assertion skips the restore, leaving GITHUB_SERVER_URL deleted for subsequent tests. The existing test at line 127 has the same pattern and would benefit from the same fix. Inline comment posted.
Dropped (out of diff)
Several pre-existing concerns were surfaced (implicit context global, URL component encoding, runId undefined guard) but none of those lines were touched in this PR and so are out of scope for this review.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 349.9K · ⌖ 13.4 AIC
| delete process.env.GITHUB_SERVER_URL; | ||
| } else { | ||
| process.env.GITHUB_SERVER_URL = originalEnv; | ||
| } |
There was a problem hiding this comment.
Env-var cleanup runs after the assertion — state leaks on test failure.
If expect(url).toBe(...) throws, the if/else restore block below it is never reached and GITHUB_SERVER_URL stays deleted for all tests that run afterward in the same process.
�� Suggested fix — wrap in try/finally
it("should fall back to https://github.com when no server URL sources are available", () => {
const originalEnv = process.env.GITHUB_SERVER_URL;
delete process.env.GITHUB_SERVER_URL;
try {
const url = buildWorkflowRunUrl({ runId: 1 }, { owner: "owner", repo: "repo" });
expect(url).toBe("https://github.com/owner/repo/actions/runs/1");
} finally {
if (originalEnv === undefined) {
delete process.env.GITHUB_SERVER_URL;
} else {
process.env.GITHUB_SERVER_URL = originalEnv;
}
}
});Alternatively, add a beforeEach/afterEach to the buildWorkflowRunUrl describe block that snapshots and restores process.env (matching the pattern already used in the getWorkflowMetadata suite above). The existing test at line 127 has the same fragility and would benefit from the same fix.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed. I updated the workflow metadata helper tests to restore |
Summary
Cleans up
workflow_metadata_helpers.cjsas part of thejsweepinitiative: tightens a JSDoc type annotation and strengthens the associated test suite with additional coverage and reliable env-var cleanup.Changes
actions/setup/js/workflow_metadata_helpers.cjsctxparameter type forbuildWorkflowRunUrlis narrowed from the untyped{any}to the precise{{ serverUrl?: string, runId: number | string }}, making the function's contract explicit and improving IDE/type-checker support.actions/setup/js/workflow_metadata_helpers.test.cjscontext.serverUrltaking priority over theGITHUB_SERVER_URLenvironment variable.https://github.comas the fallback when no server URL source is present.runIdof0(falsy edge case).try/finallyfor reliable env-var cleanup, fixing a gap where a test failure could leave environment state polluted for subsequent tests.Impact
workflow_metadata_helpers.cjsworkflow_metadata_helpers.test.cjsType
Test plan
Existing and new unit tests in
workflow_metadata_helpers.test.cjscover the changed code paths. No production logic was altered; the JSDoc change is annotation-only.