fix(eslint): resolve route/URL/control-flow lint violations in actions/setup/js#46100
Conversation
…up/js Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…etup/js Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 10 + Urgency 7 + Quality 13 Rationale: Resolves route/URL/control-flow ESLint violations in 10 files in
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46100 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
Updates JavaScript action helpers to satisfy custom ESLint rules while preserving GitHub API behavior.
Changes:
- Replaces interpolated REST endpoints with typed routes and parameters.
- Guards URL construction and adds explicit errors.
- Adds control flow after
core.setFailed()and updates tests.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/release.lock.yml |
Refreshes generated action metadata. |
actions/setup/js/add_reaction.cjs |
Uses typed reaction routes. |
actions/setup/js/add_reaction.test.cjs |
Updates reaction route tests. |
actions/setup/js/add_reaction_and_edit_comment.cjs |
Uses typed reaction/comment routes. |
actions/setup/js/add_reaction_and_edit_comment.test.cjs |
Updates combined-action tests. |
actions/setup/js/add_workflow_run_comment.cjs |
Uses typed comment routes. |
actions/setup/js/add_workflow_run_comment.test.cjs |
Updates comment route tests. |
actions/setup/js/artifact_client.cjs |
Guards artifact URL construction. |
actions/setup/js/check_team_member.cjs |
Returns after failure handling. |
actions/setup/js/generate_history_link.cjs |
Guards history URL construction. |
actions/setup/js/mcp_cli_bridge.cjs |
Rejects malformed bridge URLs. |
actions/setup/js/mount_mcp_as_cli.cjs |
Rejects malformed mounted-server URLs. |
actions/setup/js/route_slash_command.cjs |
Uses typed immediate-reaction routes. |
actions/setup/js/route_slash_command.test.cjs |
Updates slash-command route tests. |
actions/setup/js/safe_output_handler_manager.cjs |
Defers failure until outputs are exported. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Medium
| const url = (() => { | ||
| try { | ||
| return new URL(`/twirp/${TWIRP_ARTIFACT_SERVICE}/${method}`, getResultsServiceOrigin()).toString(); | ||
| } catch { | ||
| throw new Error(`Failed to construct twirp URL for method: ${method}`); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Fixed in 3bba3d7 and refined in 90c2fe7. twirpRequest now resolves getResultsServiceOrigin() before constructing the Twirp URL, so ACTIONS_RESULTS_URL validation errors remain visible instead of being masked by the construction guard.
| let failedOutputsMessage = null; | ||
| if (failureCount > 0) { | ||
| core.warning(`${failureCount} message(s) failed to process`); | ||
| const failedItemLines = fatalFailures.map(r => ` - ${r.type}: ${r.error || "Unknown error"}`); | ||
| const failedItems = failedItemLines.join("\n"); | ||
| core.setFailed(`${failureCount} safe output(s) failed:\n${failedItems}`); | ||
| failedOutputsMessage = `${failureCount} safe output(s) failed:\n${failedItems}`; |
There was a problem hiding this comment.
Addressed in 3bba3d7. I moved failedOutputsMessage to outer scope and updated the catch path to include it when later export logic throws, so primary safe-output failures are preserved alongside handler errors.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: ESLint lint-violation fixes
The changes correctly address all three lint violation categories:
Typed github.request() routes — resolveRestEndpoint / resolveEventEndpoints now return { route, params } objects. The typed route strings use {placeholder} syntax and params are spread at the call site. Cascade updates to callers and tests are consistent.
Guarded new URL() — All 8 call sites in artifact_client.cjs, generate_history_link.cjs, mcp_cli_bridge.cjs, and mount_mcp_as_cli.cjs are wrapped in try/catch.
Control transfer after core.setFailed() — check_team_member.cjs adds return after setFailed. safe_output_handler_manager.cjs correctly defers setFailed until after all outputs are exported, then returns — preventing any subsequent output after failure.
No correctness, security, or logic issues found. 315 tests pass.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 26.7 AIC · ⌖ 5.21 AIC · ⊞ 5K
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Addressed the unresolved feedback in commit Changes include:
I also reviewed the failing check logs:
I can’t re-run CI from this cloud-agent push path; please re-trigger checks from a maintainer context (close/reopen PR, workflow_dispatch, or maintainer push). |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
Ran
I still can’t re-run GitHub checks from this cloud-agent path, and unresolved-thread state itself is not writable through the available tool path here. Please have a maintainer re-trigger CI (close/reopen PR, workflow_dispatch, or maintainer push) and resolve threads if the existing follow-ups are satisfactory. |
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in Merged |
|
@copilot lint js and fix js tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in I ran JS lint and fixed the JS type/test breakage by making Validated locally:
|
|
@copilot please run the The latest branch update says JS lint and impacted JS tests were fixed on current HEAD. Please confirm all previously-raised review threads are actually resolved or answered against the new head commit, then leave a concise maintainer handoff. Failed checks:
|
|
🎉 This pull request is included in a new release. Release: |
Three categories of custom ESLint rule violations across 10 files in
actions/setup/js: interpolatedgithub.request()routes, unguardednew URL()calls, andcore.setFailed()without immediate control transfer.Typed
github.request()routes (no-github-request-interpolated-route)Replaced whole-route interpolations and template-literal routes with typed placeholder form. This required refactoring
resolveRestEndpoint/addReactionto return{ route, params }objects instead of path strings, cascading throughadd_reaction_and_edit_comment.cjsandadd_workflow_run_comment.cjs.Files:
add_reaction.cjs,add_reaction_and_edit_comment.cjs,add_workflow_run_comment.cjs,route_slash_command.cjsGuarded
new URL()(require-new-url-try-catch)Wrapped
new URL(...)in try/catch in all flagged call sites.Files:
artifact_client.cjs(4 sites),generate_history_link.cjs,mcp_cli_bridge.cjs,mount_mcp_as_cli.cjsControl transfer after
core.setFailed()(require-return-after-core-setfailed)check_team_member.cjs: movedsetOutputbeforesetFailed, addedreturnsafe_output_handler_manager.cjs: captured failure message in a variable, deferredsetFailed+returnto after all output exports completeTest updates
All affected test files updated to match the new
{ route, params }call signatures. 315 tests pass across 8 test files.