fix(setup/js): numeric coercion, setOutput stringification, and async entrypoint cleanup#43637
Conversation
…, add .catch() to async entrypoints Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43637 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. No test files were added or modified in this PR. Test Quality Sentinel skipped. Changes are exclusively to setup/js action files (stringification, numeric coercion fixes, and async entrypoint cleanup). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR tightens correctness and consistency across actions/setup/js scripts by removing implicit numeric coercion pitfalls, making integer parsing deterministic, and ensuring async CLI entrypoints fail in a predictable way within GitHub Actions logs.
Changes:
- Replaced global
isNaN(...)checks withNumber.isNaN(...)where the values being checked are already numeric (or intended to be numeric). - Added explicit radix
10toparseInt(...)calls to avoid legacy/implicit parsing behavior. - Hardened script entrypoints by chaining
.catch(...)onto directmain()invocations, and stringified selectedcore.setOutput(...)values.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_output_validator.cjs | Uses Number.isNaN for safer numeric validation. |
| actions/setup/js/safe_output_type_validator.cjs | Uses Number.isNaN in positive-integer validation helpers. |
| actions/setup/js/safe_output_helpers.cjs | Uses Number.isNaN in max-count and target/issue parsing. |
| actions/setup/js/safe_output_handler_manager.cjs | Uses Number.isNaN for parsed message numbers; stringifies "0" output in early-return paths. |
| actions/setup/js/parse_token_usage.cjs | Adds .catch(...) to the direct-execution main() entrypoint. |
| actions/setup/js/parse_mcp_scripts_logs.cjs | Adds .catch(...) to the direct-execution main() entrypoint. |
| actions/setup/js/parse_mcp_gateway_log.cjs | Adds .catch(...) to the direct-execution main() entrypoint. |
| actions/setup/js/parse_codex_log.cjs | Adds explicit radix 10 to parseInt(...) calls while parsing logs. |
| actions/setup/js/merge_remote_agent_github_folder.cjs | Adds .catch(...) to surface unexpected async rejections via coreObj.setFailed(...). |
| actions/setup/js/github_rate_limit_logger.cjs | Uses Number.isNaN when parsing rate limit reset seconds. |
| actions/setup/js/generate_usage_activity_summary.cjs | Uses Number.isNaN when validating parsed HTTP status codes. |
| actions/setup/js/error_recovery.cjs | Uses Number.isNaN for retry-after/reset parsing logic. |
| actions/setup/js/ephemerals.cjs | Uses Number.isNaN to validate parsed expiration dates. |
| actions/setup/js/create_project_status_update.cjs | Uses Number.isNaN when validating parsed date inputs. |
| actions/setup/js/create_pr_review_comment.cjs | Uses Number.isNaN for PR number and line-number validation. |
| actions/setup/js/create_issue.cjs | Uses Number.isNaN when parsing parent issue numbers. |
| actions/setup/js/create_discussion.cjs | Uses Number.isNaN when validating min_body_length parsing. |
| actions/setup/js/create_code_scanning_alert.cjs | Uses Number.isNaN for line/column parsing; stringifies findings_count output. |
| actions/setup/js/close_pull_request.cjs | Uses Number.isNaN for PR number validation. |
| actions/setup/js/close_issue.cjs | Uses Number.isNaN for issue number validation. |
| actions/setup/js/close_entity_helpers.cjs | Uses Number.isNaN for entity-number parsing validation. |
| actions/setup/js/check_stop_time.cjs | Uses Number.isNaN for stop-time date validation. |
| actions/setup/js/check_skip_if_helpers.cjs | Uses Number.isNaN for skip-threshold parsing validation. |
| actions/setup/js/check_skip_if_check_failing.cjs | Uses Number.isNaN for run-id parsing validation. |
| actions/setup/js/autofix_code_scanning_alert.cjs | Uses Number.isNaN for alert number validation. |
| actions/setup/js/assign_to_agent.cjs | Uses Number.isNaN for max-count and issue/PR number validation. |
| actions/setup/js/assign_milestone.cjs | Uses Number.isNaN for milestone-number presence/validation. |
| actions/setup/js/assign_copilot_to_created_issues.cjs | Uses Number.isNaN for issue number validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 28/28 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.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness gaps.
📋 Key Themes & Highlights
Blocking Issues
- Broken test:
create_code_scanning_alert.test.cjs:275assertsfindings_countis the number1, but the fix stringifies it — the test will fail on merge. - Incomplete
setOutputfix:safe_output_handler_manager.cjs:1569still passes numericsuccessCountin the success-path while the two early-return paths were correctly fixed to"0"— inconsistent output type across branches.
Lower Priority
- No regression tests for
.catch()handlers: The 4 new entrypoint handlers are unexercised by the test suite. - No test file for
merge_remote_agent_github_folder.cjs: Only entrypoint file in the set with zero tests.
Positive Highlights
- ✅
Number.isNaNmigration is comprehensive across 23 files. - ✅
parseInt(..., 10)radix fixes inparse_codex_log.cjseliminate a latent octal-parsing hazard. - ✅
.catch()error handler implementation is consistent across all 4 entrypoints. - ✅ Clean, surgical changes with no unrelated modifications.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 100.6 AIC · ⌖ 7.48 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/create_code_scanning_alert.cjs:263
[/diagnosing-bugs] The test in create_code_scanning_alert.test.cjs:275 still asserts findings_count receives the number 1, but this fix changes the call to String(validFindings.length) — the test will now fail.
<details>
<summary>💡 Fix needed in the test file</summary>
In create_code_scanning_alert.test.cjs, line 275:
// Before (now incorrect after this fix)
expect(mockCore.setOutput).toHaveBeenCalledWith("findings_count", 1);
// After (should match the stringified valu…
</details>
<details><summary>actions/setup/js/safe_output_handler_manager.cjs:1569</summary>
**[/diagnosing-bugs]** Incomplete fix: the success-path `processed_count` still passes numeric `successCount` directly, while the two early-return paths above (lines 1382 and 1413) were correctly fixed to `"0"`.
<details>
<summary>💡 Apply the same string conversion here</summary>
```js
// Line 1569 — fix to match early-return paths
core.setOutput("processed_count", String(successCount));This is the hot path that runs on every successful execution. Without this fix, downstream steps tha…
actions/setup/js/parse_token_usage.cjs:238
[/tdd] The new .catch() handler is untested — the parse_token_usage.test.cjs error test (line 453) calls main() directly, so it exercises the internal setFailed path but not the entrypoint rejection path added here.
<details>
<summary>💡 Add a regression test for the entrypoint catch</summary>
it('entrypoint catch surfaces rejections via process.exitCode', async () => {
// force main() to throw before any internal try/catch
vi.spyOn(fs, 'existsSync').mockImplementation((…
</details>
<details><summary>actions/setup/js/merge_remote_agent_github_folder.cjs:438</summary>
**[/tdd]** `merge_remote_agent_github_folder.cjs` is the only one of the four fixed entrypoint files without a test file, so this `.catch()` regression is completely unverifiable by the test suite.
<details>
<summary>💡 Create a minimal test file</summary>
At minimum, a `merge_remote_agent_github_folder.test.cjs` that:
1. Verifies `parseAgentImportSpec` parses valid specs correctly (already exported)
2. Verifies that when `main()` rejects, `coreObj.setFailed` is called with the stack — this i…
</details>
There was a problem hiding this comment.
Review: fix(setup/js): numeric coercion, setOutput stringification, and async entrypoint cleanup
The changes are well-scoped and correct. The isNaN → Number.isNaN sweep, explicit parseInt radix additions, and async .catch() additions are all sound mechanical improvements.
One missed case:
safe_output_handler_manager.cjs line 1569 still passes the numeric successCount to core.setOutput("processed_count", successCount). The PR fixed the two early-return "0" paths above, but this main-path call was missed — it should be String(successCount) for consistency. See the inline comment for details.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 79.8 AIC · ⌖ 5.92 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
actions/setup/js/safe_output_handler_manager.cjs:1569
Incomplete setOutput stringification — the PR converts processed_count to "0" in the two early-return paths (lines 1382/1413), but the main success path at this line still passes the numeric successCount directly:
// Current (numeric – inconsistent with the two "0" fixes above)
core.setOutput("processed_count", successCount);
// Should be
core.setOutput("processed_count", String(successCount));@actions/core accepts strings for outputs; passing a number is technically…
There was a problem hiding this comment.
One blocking gap in the setOutput stringification fix.
Review summary
What's correct
The isNaN → Number.isNaN substitutions are all safe: every call site passes a result of parseInt, Number(), or .getTime() — values that are already numeric — so there is no behavioral difference. The parseInt radix additions are straightforward and correct. The .catch() additions to four async entrypoints are well-structured.
Blocking issue
The processed_count stringification in safe_output_handler_manager.cjs is incomplete. The PR fixes only the two early-return branches ("0"), but the main success path on line ~1569 still emits a raw integer:
core.setOutput("processed_count", successCount); // unchangedThis is the path that runs whenever messages are actually processed. The fix for zero-output paths without touching the non-zero path is at best confusing and at worst inconsistent with whatever enforcement prompted these changes.
Fix: core.setOutput("processed_count", String(successCount));
Non-blocking observations
process.exitCode = 1(withoutprocess.exit(1)) in the threeconsole.errorcatch handlers is deliberate and acceptable for log-parsing scripts that have no async side-effects aftermain()returns — but worth a comment if this pattern is intentional.- The
"0"string literal vsString(0)vsString(count)across the threesetOutputsites is inconsistent; unifying toString(...)throughout would be cleaner.
🔎 Code quality review by PR Code Quality Reviewer · 212.6 AIC · ⌖ 5.08 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/safe_output_handler_manager.cjs:1569
Incomplete setOutput stringification: the main success path still passes a raw number, making the early-return fixes dead code for the normal case.
The PR converts 0 → "0" only in the two early-return branches, but the normal success path — where messages were actually processed — is unchanged:
// line ~1569, UNCHANGED in this PR — raw integer still passed
core.setOutput("processed_count", successCount);successCount is incremented as a plain integer. This path runs in …
🤖 PR Triage
Score breakdown:
Recommendation: Batch review alongside #43638 — Both PRs address
|
|
@copilot please run the Unresolved review feedback to address:
Once those are in place, rerun the JS checks and refresh the PR state.
|
…tions, add setFailed coverage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
|
|
🎉 This pull request is included in a new release. Release: |
Remediates correctness/style warnings in
actions/setup/jsaround numeric APIs,core.setOutputvalue types, and unhandled async rejections.Changes
isNaN→Number.isNaN(23 files): GlobalisNaNcoerces non-numeric inputs before checking, masking invalid values. All checked values are already parsed numbers soNumber.isNaNis semantically equivalent and correct.parseIntexplicit radix (parse_codex_log.cjs, 5 calls): Added, 10to every bareparseInt(str)call to prevent implicit octal/hex detection.core.setOutputstringification (create_code_scanning_alert.cjs,safe_output_handler_manager.cjs): Numeric values passed tosetOutputare now explicitly converted —.lengthvalues viaString(...), numeric literals as string literals"0".Bare async entrypoint
.catch()(merge_remote_agent_github_folder.cjs,parse_mcp_gateway_log.cjs,parse_mcp_scripts_logs.cjs,parse_token_usage.cjs): Baremain()calls now chain.catch()so rejections surface predictably rather than producing unhandled promise rejections. Files withcoreObjusecoreObj.setFailed; files withoutcoreuseconsole.error+process.exitCode = 1.