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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs — no blocking issues, three moderate suggestions.
📋 Key Themes & Highlights
Key Themes
- Repeated IIFE try-catch pattern in
artifact_client.cjs: four identical wrappers that beg for a sharedparseURLhelper. - Loose union type for endpoints: the
{ route, params } | stringunion requirestypeofguards at every call site; a tagged union (kind: 'rest' | 'discussion') would be cleaner. - Log ordering quirk in
safe_output_handler_manager.cjs: the 'completed' info log fires unconditionally even when the deferredsetFailedis about to run.
Positive Highlights
- ✅ Clean, consistent refactor of all route interpolations to the typed placeholder form.
- ✅ Test suite updated in lock-step (315 passing) — the test assertions were also migrated to match exact typed routes, not loose string matchers.
- ✅
check_team_member.cjsfix correctly movessetOutputbeforesetFailedand addsreturn, fully satisfying the lint rule intent.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51.3 AIC · ⌖ 4.75 AIC · ⊞ 6.7K
Comment /matt to run again
| throw new Error("ACTIONS_RUNTIME_TOKEN is required for artifact upload"); | ||
| } | ||
| const url = new URL(`/twirp/${TWIRP_ARTIFACT_SERVICE}/${method}`, getResultsServiceOrigin()).toString(); | ||
| const url = (() => { |
There was a problem hiding this comment.
[/codebase-design] Four identical IIFE try-catch wrappers around new URL() are spread across this file — a shared parseURL(url, base?) helper would eliminate the repetition and give one place to improve error messages later.
💡 Suggested helper
function parseURL(url, base) {
try {
return base ? new URL(url, base) : new URL(url);
} catch {
throw new Error(`Invalid URL: ${url}${base ? ` (base: ${base})` : ""}`);
}
}Each call site then becomes a readable one-liner.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 3bba3d7. I added a shared parseURL(url, base, errorMessage) helper and replaced the repeated URL try/catch wrappers in this file with helper calls at each call site.
| * @param {string} repo - Repository name | ||
| * @param {Record<string, any>} payload - The event payload | ||
| * @returns {Promise<{reactionEndpoint: string, commentUpdateEndpoint: string} | null>} | ||
| * @returns {Promise<{reactionEndpoint: { route: string, params: Record<string, unknown> } | string, commentUpdateEndpoint: { route: string, params: Record<string, unknown> } | string} | null>} |
There was a problem hiding this comment.
[/codebase-design] The union return type { route: string, params: ... } | string leaks the discussion-special-case format into the type signature, causing defensive typeof endpoint === 'object' checks at every call site. Consider a tagged union to make the branching explicit and self-documenting.
💡 Suggested typed shape
// `@typedef` {{ kind: 'rest', route: string, params: Record<string, unknown> }} RestEndpoint
// `@typedef` {{ kind: 'discussion', nodeRef: string }} DiscussionEndpoint
// `@typedef` {RestEndpoint | DiscussionEndpoint} EventEndpointCall sites can then switch (endpoint.kind) instead of typeof checks.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 3bba3d7 (with cleanup in 90c2fe7). I added explicit runtime endpoint-shape guards (expectRestEndpoint) so we no longer rely on unchecked casts. This avoids silent destructuring failures and gives a clear validation error if endpoint shape regresses.
| core.setFailed(failedOutputsMessage); | ||
| return; | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The deferred core.setFailed() now runs after the finally block exits — but the finally block calls core.info('Safe Output Handler Manager completed') unconditionally. If the failure path is taken, that 'completed' log misleadingly appears before the failure message.
💡 Suggestion
Move the success-only log inside an else branch so it only fires when there is no failure:
if (failedOutputsMessage !== null) {
core.setFailed(failedOutputsMessage);
return;
} else {
core.info("Safe Output Handler Manager completed");
}@copilot please address this.
There was a problem hiding this comment.
Addressed in 3bba3d7. The completion log now runs only on success; when failedOutputsMessage is present we call setFailed(...) and return before emitting "Safe Output Handler Manager completed".
| core.info(`Comment endpoint: ${typeof commentUpdateEndpoint === "object" ? commentUpdateEndpoint.route : commentUpdateEndpoint}`); | ||
| await addCommentWithWorkflowLink(commentUpdateEndpoint, runUrl, eventName, invocationContext); | ||
| } catch (error) { | ||
| if (isLockedError(error)) { |
There was a problem hiding this comment.
[/tdd] The addReaction branch casts reactionEndpoint with /** @type {...} */ but no test covers the code path where reactionEndpoint is unexpectedly a plain string (e.g. from a future event type regression). Adding a test that passes a string here would catch silent destructuring failures early.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 3bba3d7. I added a regression test that exercises the unexpected string endpoint path via expectRestEndpoint(...) and verifies it throws a validation error instead of failing later during destructuring.
There was a problem hiding this comment.
REQUEST_CHANGES — Two correctness issues and one significant test-coverage regression must be addressed before merging.
### Blocking issues found
1. Deferred setFailed loses failure details on exception (safe_output_handler_manager.cjs)
The original code called core.setFailed(...) immediately after building the failure message. The new code defers it ~100 lines later. Any exception in the intervening export logic silences the N-safe-outputs-failed diagnostic — callers only see a generic validation error. See inline comment.
2. Unchecked type cast on reactionEndpoint (add_reaction_and_edit_comment.cjs)
The JSDoc union type (string | { route, params }) and a JS cast are doing the safety work instead of a runtime guard. If a new event type is added and the eventName guard is not updated, route and params will both be undefined and the error will be opaque. See inline comment.
3. Test assertions weakened: params no longer verified (add_workflow_run_comment.test.cjs)
Most add_workflow_run_comment tests dropped from asserting fully-resolved paths (which verified owner/repo/issue_number routing) to asserting only the route template string. This leaves cross-repo routing bugs invisible to the test suite. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 346.8 AIC · ⌖ 4.91 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/safe_output_handler_manager.cjs:1744
Deferred setFailed swallows processing-failure details on exception: if any exception fires in the ~100 lines between setting failedOutputsMessage and calling setFailed, the failure-count summary is silently lost.
<details>
<summary>💡 Details and suggested fix</summary>
In the original code, setFailed was called immediately after building failedItems, so the failure message was always reported. The new code defers it to line ~1741, after a large block of output-export logic. If…
actions/setup/js/add_reaction_and_edit_comment.cjs:312
Unsafe cast on reactionEndpoint: the JSDoc type allows string | { route, params } but the cast on line 312 unconditionally treats it as the object form — if resolveEventEndpoints ever returns a string here (e.g., a discussion event falls through), this will throw a destructuring error.
<details>
<summary>💡 Details and suggested fix</summary>
The resolveEventEndpoints return type is declared as:
// `@returns` {Promise<{reactionEndpoint: { route: string, params: ... } | stri…
</details>
<details><summary>actions/setup/js/add_workflow_run_comment.test.cjs:655</summary>
**Tests lost owner/repo/issue_number assertions**: the integration tests now only assert the route string (`"POST /repos/{owner}/{repo}/issues/{issue_number}/comments"`) but no longer verify that the correct `owner`, `repo`, or `issue_number` params are passed — exactly the bug this refactor was meant to prevent.
<details>
<summary>💡 Why this matters</summary>
Before, the tests matched a fully-resolved path like `"/repos/testowner/testrepo/issues/456/comments"`, which implicitly proved the r…
</details>
🤖 PR Triage
Rationale: ESLint lint violation fixes (route interpolation, URL calls, control flow) in actions/setup/js. AI reviewer commented + GHA approved. CI running. Batch with #46102.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
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 |
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.