[jsweep] Clean validate_secrets.cjs#45184
Conversation
Replace legacy https.request Promise wrappers in makeRequest and makePostRequest with native fetch + AbortSignal.timeout(). Remove the unused https require. Update tests to mock fetch instead of https.request. 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. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45184 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the actions/setup/js/validate_secrets.cjs secret-validation helper by replacing the legacy https.request-based request helpers with the native fetch API (including request timeouts), and updates the Vitest suite accordingly.
Changes:
- Replaced the internal GET/POST request helpers to use
fetch+AbortSignal.timeout(10000)instead ofhttps.requestand manual timeout/chunk handling. - Updated
makePostRequestunit tests to stubglobalThis.fetchinstead of mockinghttps.request. - Added/updated coverage for the timeout/abort error path in
makePostRequest.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/validate_secrets.cjs | Refactors HTTP request helpers to use fetch with abort-based timeouts. |
| actions/setup/js/validate_secrets.test.cjs | Updates request-helper tests to mock fetch and validate the new error path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Low
| async function makeRequest(hostname, path, headers) { | ||
| const res = await fetch(`https://${hostname}${path}`, { | ||
| method: "GET", | ||
| headers, | ||
| signal: AbortSignal.timeout(10000), | ||
| }); | ||
| const data = await res.text(); | ||
| return { statusCode: res.status, data }; | ||
| } |
| async function makePostRequest(hostname, path, headers, body) { | ||
| const res = await fetch(`https://${hostname}${path}`, { | ||
| method: "POST", | ||
| headers: { ...headers, "Content-Type": headers["Content-Type"] || "application/json" }, | ||
| body, | ||
| signal: AbortSignal.timeout(10000), | ||
| }); | ||
| const data = await res.text(); | ||
| return { statusCode: res.status, data }; | ||
| } |
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor. Replacing https.request with fetch + AbortSignal.timeout is correct and the tests cover success, network error, and abort paths. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 10.3 AIC · ⌖ 4.27 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two minor issues worth addressing before merge.
📋 Key Themes & Highlights
Issues Found
- Weak timeout assertion (
validate_secrets.test.cjsline 157):.rejects.toThrow()with no argument does not confirm theAbortErrorpath. A different error would pass silently. - Silent
Content-Typedefault (validate_secrets.cjsline 63): injectingapplication/jsonfor callers that omit the header could mislead the server on non-JSON POST bodies.
Positive Highlights
- ✅ Great simplification — ~50 lines dropped per function using
fetch+AbortSignal.timeout - ✅
mockFetchhelper is much cleaner than the previousEventEmittermock - ✅ JSDoc types upgraded from
ObjecttoRecord<string, string> - ✅
makeRequestcorrectly dropped the unusedmethodparameter
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 22.3 AIC · ⌖ 4.53 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| await expect(promise).rejects.toThrow("Request timeout"); | ||
| expect(mockRequest.destroy).toHaveBeenCalled(); | ||
| await expect(makePostRequest("api.example.com", "/v1/test", {}, "{}")).rejects.toThrow(); |
There was a problem hiding this comment.
[/tdd] The timeout test assertion is too weak — .rejects.toThrow() with no argument only confirms something is thrown, not that the abort path is exercised.
💡 Suggested improvement
Assert the specific error name:
const err = await makePostRequest("api.example.com", "/v1/test", {}, "{}").catch(e => e);
expect(err.name).toBe("AbortError");Without this, a future regression that causes a different error would still pass this test.
@copilot please address this.
| }); | ||
| const data = await res.text(); | ||
| return { statusCode: res.status, data }; | ||
| } |
There was a problem hiding this comment.
[/codebase-design] makePostRequest silently injects Content-Type: application/json when none is provided. Callers who POST non-JSON bodies (e.g., form-encoded data) will receive a misleading content-type header without any warning.
💡 Suggested fix
Either require callers to always pass Content-Type explicitly, or only spread headers without a default injection:
headers: { ...headers },If a default is genuinely desired, document it in the JSDoc so callers are aware.
@copilot please address this.
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 53/100 — Needs improvement
📊 Metrics (35 tests)
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
|
Addressed all three review threads:
Local validation: 35 tests pass, lint clean, typecheck clean. Note: push is currently failing with 403 — may need a maintainer to re-trigger or re-push. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed. The lint failure was in |
|
@copilot please run the
|
…ub leak Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All review feedback addressed in the latest commit:
35 tests pass, lint and typecheck clean. CI needs a human re-trigger to confirm. |
|
@copilot please run the
|
Addressed the |
|
Run: https://github.com/github/gh-aw/actions/runs/29280443144
|
Addressed both github-actions threads in commit 109d099:
The three 35 tests pass, format and lint clean. Push is blocked at 403 — needs a maintainer re-trigger to confirm CI. |
Summary
Modernizes
validate_secrets.cjsby replacing the legacyhttpsmodule with the nativefetchAPI.Changes
validate_secrets.cjsrequire("https")— no longer neededmakeRequest()(manualPromisewrapper aroundhttps.requestwithreq.setTimeout) withfetch+AbortSignal.timeout(10000)makePostRequest()similarly — removesContent-Lengthheader calculation and the manual chunked data accumulationvalidate_secrets.test.cjsimport httpsandEventEmitter— no longer neededmakePostRequesttest block to usevi.stubGlobal("fetch", ...)instead ofvi.spyOn(https, "request")Context
github-scriptmakePostRequestblock)Validation
✅ Formatting:
npm run format:cjs✓✅ Linting:
npm run lint:cjs✓✅ Type checking:
npm run typecheck✓✅ Tests:
npm run test:js✓ (35 tests passing)Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.