fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js

[Copilot]

Three ESLint rule categories were firing across 18 files in actions/setup/js: improper failure signaling (core.error() + process exit instead of core.setFailed()), missing control transfer after core.setFailed(), coercive isNaN(), and interpolated exec command strings.

Failure signaling (no-core-error-then-process-exit, no-core-error-then-process-exitcode)

Replace core.error() + process.exit(1) / process.exitCode = 1 with core.setFailed():

// before
core.error(`Failed: ${err.message}`);
process.exit(1);

// after
core.setFailed(`Failed: ${err.message}`);
return;

Multi-error diagnostic blocks retain preceding core.error() calls for log output; core.setFailed() is inserted as the last statement before the control transfer.

Affected: convert_gateway_config_copilot.cjs, start_mcp_gateway.cjs, mcp_cli_bridge.cjs

Control transfer after core.setFailed() (require-return-after-core-setfailed)

Added explicit return after core.setFailed() calls that had no control transfer, preventing execution fallthrough:

Affected: lock-issue.cjs, log_parser_bootstrap.cjs, push_experiment_state.cjs, mcp_cli_bridge.cjs

Global isNaN() → Number.isNaN() (prefer-number-isnan)

// before
if (isNaN(retryCount)) { ... }
// after
if (Number.isNaN(retryCount)) { ... }

Affected: 11 files including expired_entity_cleanup_helpers.cjs, pr_helpers.cjs, unified_timeline.cjs, and others.

Interpolated exec commands (no-exec-interpolated-command)

Convert template-literal exec commands to static command + args array form:

// before
await exec.exec(`git push origin ${branch}`);

// after
await exec.exec("git", ["push", "origin", branch]);

Affected: create_pull_request.cjs (7 instances), push_to_pull_request_branch.cjs (3 instances)

• Fixes [eslint-monster] setup/js lint stream 3: standardize failure signaling and unsafe coercion patterns #47413

---

Generated by 👨‍🍳 PR Sous Chef · gpt54 5.75 AIC · ⌖ 6.96 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

---

Generated by 👨‍🍳 PR Sous Chef · gpt54 10 AIC · ⌖ 7.12 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

---

Generated by 👨‍🍳 PR Sous Chef · gpt54 4.81 AIC · ⌖ 6.76 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

---

Run URL: https://github.com/github/gh-aw/actions/runs/29984131428

Generated by 👨‍🍳 PR Sous Chef · gpt54 16.7 AIC · ⌖ 9.04 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0, file_count=18).

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. All 18 changed files are production code in actions/setup/js/ (ESLint fixes for standardized failure signaling and unsafe coercion patterns). Test Quality Sentinel skipped.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

Standardizes ESLint-compliant error handling, numeric validation, and Git command execution in setup scripts.

Changes:

• Uses core.setFailed() with explicit control transfer.
• Replaces coercive isNaN() checks with Number.isNaN().
• Passes Git arguments separately to exec.exec().

Show a summary per file

File	Description
actions/setup/js/update_project.cjs	Uses non-coercive number validation.
actions/setup/js/update_discussion.cjs	Validates parsed discussion numbers safely.
actions/setup/js/unified_timeline.cjs	Validates timestamps without coercion.
actions/setup/js/temporary_id.cjs	Validates resolved issue numbers safely.
actions/setup/js/templatable.cjs	Updates integer parsing validation.
actions/setup/js/start_mcp_gateway.cjs	Standardizes gateway failure signaling.
actions/setup/js/reply_to_pr_review_comment.cjs	Updates comment ID validation.
actions/setup/js/push_to_pull_request_branch.cjs	Uses argument-array Git execution.
actions/setup/js/push_experiment_state.cjs	Stops after terminal push failure.
actions/setup/js/pr_helpers.cjs	Updates pull request number validation.
actions/setup/js/parse_claude_log.cjs	Updates maximum-turn parsing validation.
actions/setup/js/mcp_cli_bridge.cjs	Standardizes bridge failure signaling.
actions/setup/js/mark_pull_request_as_ready_for_review.cjs	Updates pull request number validation.
actions/setup/js/log_parser_bootstrap.cjs	Stops processing after parser failures.
actions/setup/js/lock-issue.cjs	Adds control transfer after lock failure.
actions/setup/js/expired_entity_cleanup_helpers.cjs	Updates date validity checking.
actions/setup/js/create_pull_request.cjs	Uses argument-array Git execution.
actions/setup/js/convert_gateway_config_copilot.cjs	Standardizes conversion failure signaling.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 18/18 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

Review: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js

The changes are well-structured and consistent across all 18 files. The isNaN → Number.isNaN, require-return-after-core-setfailed, and no-exec-interpolated-command fixes look correct throughout.

One blocking issue found: assertNotSymlink in start_mcp_gateway.cjs was only half-fixed — core.error() was replaced with core.setFailed(), but process.exit(1) was not replaced with return. See inline comment.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 33.9 AIC · ⌖ 5.26 AIC · ⊞ 5K

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs — requesting changes on 3 correctness issues found.

📋 Key Themes & Highlights

Issues Found

• Dead code in lock-issue.cjs: core.setOutput("locked", "false") is now unreachable after the added return.
• Missed process.exit in assertNotSymlink: The symlink-attack guard in start_mcp_gateway.cjs still calls process.exit(1) and was not converted — and a simple return there won't work since it's a sync helper; it needs to throw or return a sentinel.
• Redundant process.exitCode = 1 in mcp_cli_bridge.cjs: Three error paths set both process.exitCode = 1 and core.setFailed(). The former is inconsistent with every other converted path in this PR.

Positive Highlights

• ✅ Thorough isNaN → Number.isNaN sweep across 11 files — no misses found.
• ✅ exec.exec interpolation fixes are correct and improve injection safety.
• ✅ The multi-error diagnostic-block pattern (keeping core.error() for context before core.setFailed()) is consistent and intentional.
• ✅ return insertions after core.setFailed() are correctly placed in all async main() functions.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 48.9 AIC · ⌖ 4.83 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

REQUEST_CHANGES — Two correctness bugs introduced by the conversion, one security-path inconsistency, and one child-process lifecycle risk.

### Blocking issues (4)

1. lock-issue.cjs — dead setOutput call (high): The return added before core.setOutput("locked", "false") makes that call unreachable. Downstream steps reading the locked output will get an empty string instead of "false" in the error path.

2. start_mcp_gateway.cjs — assertNotSymlink half-converted (high): core.setFailed() was added but process.exit(1) was left. This is the symlink-attack guard. The function is now inconsistent — remove process.exit(1) and ensure callers handle the void return, or add a comment marking it intentional.

3. start_mcp_gateway.cjs — orphaned gateway process on late-stage failures (medium): Post-spawn failure paths now return instead of process.exit(1). return does not kill child processes. Needs verification that cleanup logic covers all new early-return paths.

4. mcp_cli_bridge.cjs — global.core unguarded in fatal catch (medium): If main() throws before global.core is assigned, core.setFailed() itself throws and the job gets no failure annotation. Add a null guard with stderr fallback.

🔎 Code quality review by PR Code Quality Reviewer · sonnet46 87 AIC · ⌖ 5.11 AIC · ⊞ 5.7K
_{Comment /review to run again}

Comments that could not be inline-anchored

actions/setup/js/lock-issue.cjs:89

Dead code: core.setOutput("locked", "false") is unreachable — the return inserted on the previous line means this output is never set in the error path, so downstream steps reading locked will see an empty value instead of "false".

<details>
<summary>💡 Suggested fix</summary>

Move the setOutput call before setFailed()/return:

  } catch (error) {
    const errorMessage = getErrorMessage(error);
    core.error(`Failed to lock issue: ${errorMessage}`);
    core.setOutpu…

</details>

<details><summary>actions/setup/js/start_mcp_gateway.cjs:283</summary>

**Incomplete fix in `assertNotSymlink()`: `process.exit(1)` is still present after `core.setFailed()`**, leaving this security-critical path half-converted and fragile.

&lt;details&gt;
&lt;summary&gt;💡 Details&lt;/summary&gt;

The original `core.error()` was replaced with `core.setFailed()`, but `process.exit(1)` was left in place. Currently only the hard-exit prevents execution from continuing past the symlink check. If a future refactor removes `process.exit(1)` (to align with the rest of the PR), execution …

</details>

<details><summary>actions/setup/js/start_mcp_gateway.cjs:452</summary>

**`setFailed()` + `return` after child process has already been spawned may orphan the gateway process** — unlike `process.exit(1)`, returning from `main()` does not kill spawned child processes.

&lt;details&gt;
&lt;summary&gt;💡 Details&lt;/summary&gt;

Multiple late-stage failure paths in `start_mcp_gateway.cjs` now use `return` instead of `process.exit(1)`. These failures occur *after* the gateway child process was already spawned (e.g., failures to read output config, gateway returning error payload, MCP se…

</details>

<details><summary>actions/setup/js/mcp_cli_bridge.cjs:1432</summary>

**`global.core` in the fatal `.catch()` handler may be `undefined`** — if `main()` throws before `global.core` is assigned, `core.setFailed()` itself throws and the Actions step gets no failure annotation.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

```js
main().catch(err =&gt; {
  const core = global.core;
  const message = err instanceof Error ? err.stack || err.message : String(err);
  if (core) {
    core.setFailed(`mcp_cli_bridge fatal: ${message}`);
  } else {
    process.stderr.write(`m…

</details>

[gh-aw-bot]

@copilot please run the pr-finisher skill, address the unresolved blocking review feedback below, and rerun checks once the branch is up to date.

Unresolved review feedback:

• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (review)
• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (review)
• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (review)

Failed checks:

• JS Tests (shard 1/4)

Generated by 👨‍🍳 PR Sous Chef · gpt54 3.92 AIC · ⌖ 16.2 AIC · ⊞ 5.5K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address the unresolved blocking review feedback below, and rerun checks once the branch is up to date.

Unresolved review feedback:...

Addressed in fb691a5. The failing JS Tests shard was caused by the lock-issue.cjs regression from the earlier return; I restored the locked=false output, completed the follow-up failure-handling fixes in start_mcp_gateway.cjs and mcp_cli_bridge.cjs, and reran local validation (make fmt, make lint, targeted JS tests, TEST_UNIT_RUN_FULL=1 make test-unit, make test, and TEST_UNIT_RUN_FULL=1 make agent-report-progress). CI on this updated head is still unverified because agent pushes do not retrigger workflows, so a maintainer will need to re-run the checks.

[Copilot]

@copilot please run the pr-finisher skill, address the remaining unresolved blocking review feedback, and rerun checks once the branch is up to date.

Unresolved review feedback:...

The four linked github-actions threads are already addressed on the current head (fb691a5, 8e0e7de). I rechecked /home/runner/work/gh-aw/gh-aw/actions/setup/js/lock-issue.cjs, /home/runner/work/gh-aw/gh-aw/actions/setup/js/start_mcp_gateway.cjs, and /home/runner/work/gh-aw/gh-aw/actions/setup/js/mcp_cli_bridge.cjs, confirmed the old JS Tests shard failure was the earlier lock-issue regression, and reran local validation on the current head: make fmt, make lint, make test-unit, and make test all passed. I also attempted to resolve the outdated review threads directly, but GraphQL thread-resolution requests are blocked from this environment, so CI/thread state on GitHub still needs a maintainer pass and a CI re-trigger on this head.

[gh-aw-bot]

@copilot please run the pr-finisher skill, address the unresolved blocking review feedback below, resolve any fully addressed threads, and rerun checks once the branch is up to date.

Unresolved review feedback:

• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (review)
• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (review)
• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (review)

The PR already has follow-up replies on these items; please leave a short status update for anything still open.

Generated by 👨‍🍳 PR Sous Chef · gpt54 4.81 AIC · ⌖ 6.76 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address the unresolved blocking review feedback below, resolve any fully addressed threads, and rerun checks once the branch is up to date.

Unresolved rev...

The three linked github-actions reviews are already satisfied by the fixes on the current head: /home/runner/work/gh-aw/gh-aw/actions/setup/js/start_mcp_gateway.cjs, /home/runner/work/gh-aw/gh-aw/actions/setup/js/lock-issue.cjs, and /home/runner/work/gh-aw/gh-aw/actions/setup/js/mcp_cli_bridge.cjs contain the follow-up changes from fb691a5 and 8e0e7de, and I reran make fmt, make lint, make test-unit, make test, and make recompile successfully on this head. I also rechecked the prior failing CI job (29966465207 / 89078966615); it was the earlier lock-issue.cjs regression on 03288a8, not the current head. The only remaining open item is GitHub-side review/thread state: I still cannot resolve the already-addressed outdated threads from this environment because GraphQL thread-resolution requests are blocked here, so a maintainer will need to resolve them and re-trigger CI on the current head.

Done

[pelikhan]

@copilot add setFailed implementation in shim.cjs

[Copilot]

actions/setup/js/shim.cjs already has core.setFailed() on the current head via 1a1b36c (pulled in by merge 34a0668). I verified the shim sets process.exitCode = 1 and reran npx vitest run start_mcp_gateway.test.cjs successfully, so no additional code change was needed here.

[pelikhan]

@copilot ensure all the shims are added in shim.cjs so that node.js run still work

[Copilot]

@copilot ensure all the shims are added in shim.cjs so that node.js run still work

Addressed in 4c6c22a. actions/setup/js/shim.cjs now installs the broader github-script-style runtime for plain node execution: richer core helpers, extra context fields, and lazy shims for github/octokit/getOctokit/exec/glob/io, with focused coverage in actions/setup/js/shim.test.cjs.

[pelikhan]

@copilot revert 4c6c22a and only shim setFailed

[gh-aw-bot]

@copilot please run the pr-finisher skill, address the remaining unresolved review feedback below, resolve any fully addressed threads, and rerun checks once the branch is up to date.

Unresolved review feedback:

• pelikhan: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (comment)
• github-actions: fix(eslint): standardize failure signaling and unsafe coercion patterns in setup/js #47419 (comment)

The older github-actions threads on this PR already have follow-up replies; please resolve any that are fully addressed and leave a short status update for anything still open.

Generated by 👨‍🍳 PR Sous Chef · gpt54 16.7 AIC · ⌖ 9.04 AIC · ⊞ 7K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot revert 4c6c22a and only shim setFailed

Addressed in b553b2a. actions/setup/js/shim.cjs is back to the minimal plain-node shim with setFailed(), and the broader runtime expansion from 4c6c22a has been reverted.

[Copilot]

@copilot please run the pr-finisher skill, address the remaining unresolved review feedback below, resolve any fully addressed threads, and rerun checks once the branch is up to date.

Unresolved re...

No further code changes were needed on the current head. pelikhan's shim feedback is satisfied by b553b2a, and the github-actions note at /home/runner/work/gh-aw/gh-aw/actions/setup/js/start_mcp_gateway.cjs is already answered and intentional. I rechecked the PR state: the branch contains the current main base commit, the prior JS failure was the earlier lock-issue.cjs regression, and b553b2a was locally validated with make agent-report-progress before push. I still cannot resolve the already-addressed threads from this environment because GitHub thread resolution is blocked here, and CI on the agent head remains unverified until a maintainer re-triggers it.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.83.1