[jsweep] Clean harness_retry_config.cjs#45378
Conversation
- Add // @ts-check to enable type checking - Export parseRetryConfigNumber for testability - Create harness_retry_config.test.cjs with 18 test cases covering: - parseRetryConfigNumber: empty/missing env vars, valid integers, floats, exponential/hex rejection, minimum enforcement, whitespace trimming, logger callback - resolveRetryConfig: defaults, env var overrides, maxRetries capping, maxDelayMs clamping, process.env fallback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45378 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR refactors the JavaScript retry-config parsing harness used by actions/setup/js by enabling TypeScript checking and adding unit tests, while keeping runtime behavior unchanged.
Changes:
- Enabled
// @ts-checkonharness_retry_config.cjsfor stronger static validation. - Exported
parseRetryConfigNumberso it can be directly unit-tested. - Added a new Vitest suite covering
parseRetryConfigNumberandresolveRetryConfigbehaviors and edge cases.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/harness_retry_config.cjs | Enables @ts-check and exports parseRetryConfigNumber for direct testing without changing logic. |
| actions/setup/js/harness_retry_config.test.cjs | Adds a focused Vitest suite validating numeric parsing rules and retry-config resolution/clamping behavior. |
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: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: [jsweep] Clean harness_retry_config.cjs
One blocking issue found in the new test file.
Blocking: harness_retry_config.test.cjs uses ES module import syntax (lines 3-4), but .cjs files are treated as CommonJS by Node.js -- static import is a syntax error in CJS modules. This would cause tests to fail at runtime despite the PR description claiming all 18 tests pass.
The fix is to use require() calls instead of import, or rename the file to .mjs/.js with appropriate Vitest config.
The source file changes (// @ts-check`` and exporting parseRetryConfigNumber) are clean and correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.8 AIC · ⌖ 5.46 AIC · ⊞ 4.8K
| @@ -0,0 +1,115 @@ | |||
| // @ts-check | |||
|
|
|||
| import { describe, it, expect } from "vitest"; | |||
There was a problem hiding this comment.
Bug: ES module import in a .cjs file
import { describe, it, expect } from "vitest";
import { resolveRetryConfig, parseRetryConfigNumber } from "./harness_retry_config.cjs";.cjs files are treated as CommonJS by Node.js (and bundlers), which does not support static import declarations. This will throw a SyntaxError at runtime/test-time.
Fix — replace the imports with require:
const { describe, it, expect } = require("vitest");
const { resolveRetryConfig, parseRetryConfigNumber } = require("./harness_retry_config.cjs");Or rename the file to harness_retry_config.test.mjs / harness_retry_config.test.js (if the project allows ESM tests) and update the test runner config accordingly.
@copilot please address this.
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 53/100 — Needs improvement
📊 Metrics (18 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one minor finding, otherwise a solid PR.
📋 Key Themes & Highlights
Finding
- Import style inconsistency (line 4 of test file): the module under test is loaded via ES
importrather thanrequire(), which is the established pattern in every other.test.cjsin this directory.
Positive Highlights
- ✅ Comprehensive coverage: 18 test cases covering defaults, clamping, invalid formats, floats, whitespace trimming, and logger callbacks
- ✅ Exporting
parseRetryConfigNumberis the right seam to enable direct unit testing - ✅ All CI checks passed; no logic changes in production code
- ✅ Good use of inline JSDoc
@typeannotations in test bodies
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 27 AIC · ⌖ 4.43 AIC · ⊞ 6.6K
Comment /matt to run again
| // @ts-check | ||
|
|
||
| import { describe, it, expect } from "vitest"; | ||
| import { resolveRetryConfig, parseRetryConfigNumber } from "./harness_retry_config.cjs"; |
There was a problem hiding this comment.
[/tdd] Uses ES named import to load the CJS module under test, inconsistent with every other .test.cjs in this directory which uses require().
💡 Suggested fix
All sibling test files follow this pattern:
import { describe, it, expect } from "vitest";
const { resolveRetryConfig, parseRetryConfigNumber } = require("./harness_retry_config.cjs");Using import from a .cjs file works today via vitest's CJS interop, but it silently diverges from the established convention and could behave unexpectedly if the interop rules change. Keeping the pattern consistent also makes it easier for contributors to follow.
@copilot please address this.
|
@copilot run pr-finisher skill |
Sighthound Security Scan — PR #45378126 findings from automated scan (run 29319221906). After triage, most are false positives or low-risk in this codebase context. Summary by severity
Findings requiring attention1. Path Traversal —
|
| Finding type | Count | Reason |
|---|---|---|
| Unsafe Deserialization | 54 | yaml.Unmarshal on internal content — not a Java-style risk |
| Code Injection | 17 | eval in bundled test framework, not app code |
| SSRF in testdata | 6 | Intentional linter test fixtures |
| ReDoS, DOM XSS, postMessage | 5 | Low-risk docs/editor UI |
Scan performed by Sighthound. Triage by agent.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
awmgmcpg
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
Generated by 🛡️ Sighthound Security Scan for #45378 · 21.5 AIC · ⌖ 7.84 AIC · ⊞ 4.1K · ◷
….cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Changed line 4 to use |
Summary
Cleaned
actions/setup/js/harness_retry_config.cjsas part of the JavaScript unbloat initiative.Changes
harness_retry_config.cjs//@ts-check`` to enable TypeScript type checking on the fileparseRetryConfigNumberto make it directly testableContext type: Node.js
harness_retry_config.test.cjs(new file)Created a comprehensive test file with 18 test cases covering:
parseRetryConfigNumber:1e3) and hex (0x10)allowFloat: trueallowFloatis false (default)resolveRetryConfig:maxRetries,initialDelayMs,backoffMultiplier,maxDelayMsfrom envmaxRetriesto cap of 100 and logs warningmaxDelayMstoinitialDelayMswhen lower and logs warningprocess.env)Validation ✅
All checks passed:
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js✓ (18/18 tests pass)Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.