fix(setup/js): normalize unsafe caught/rejected error property access#43638
Conversation
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 #43638 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (src/, lib/, pkg/, etc.). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #43638 ('fix(setup/js): normalize unsafe caught/rejected error property access') modified 61 production .cjs files only. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR normalizes handling of caught/rejected errors across actions/setup/js to satisfy ESLint rules by avoiding unsafe property access on unknown values and by consistently using the shared getErrorMessage() helper.
Changes:
- Replaced many
x instanceof Error ? x.message : String(x)patterns and directerr.messageusage withgetErrorMessage(err). - Added
getErrorMessageimports across scripts and updated several catch blocks to avoid unsafe access patterns (including.statuschecks viaany-typed locals). - Refactored a few multi-use catch blocks (e.g., safe-outputs handlers) to compute the error message once and reuse it.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/validate_secrets.cjs | Use getErrorMessage() for API test error messages. |
| actions/setup/js/validate_memory_files.cjs | Import + use getErrorMessage() in directory scan error handling. |
| actions/setup/js/update_network_allowed.cjs | Import + use getErrorMessage() for JSON/read/write failures. |
| actions/setup/js/test-live-github-api.cjs | Import + use getErrorMessage() for top-level error reporting. |
| actions/setup/js/start_mcp_gateway.cjs | Import + use getErrorMessage() for parsing/runtime failures. |
| actions/setup/js/staged_preview.cjs | Import + use getErrorMessage() when failing the step. |
| actions/setup/js/send_otlp_span.cjs | Use existing local getErrorMessage() for retry error text. |
| actions/setup/js/safe_outputs_handlers.cjs | Use getErrorMessage() and reuse extracted message in validation catches. |
| actions/setup/js/safe_output_summary.cjs | Import + use getErrorMessage() in summary write/read warnings. |
| actions/setup/js/safe_output_handler_manager.cjs | Use getErrorMessage() for handler exception strings. |
| actions/setup/js/runtime_import.cjs | Import + use getErrorMessage() for expression evaluation errors. |
| actions/setup/js/run_operation_update_upgrade.cjs | Avoid unsafe .status access by aliasing catch error to any. |
| actions/setup/js/push_to_pull_request_branch.cjs | Use getErrorMessage() in git/Octokit failure messages. |
| actions/setup/js/push_signed_commits.cjs | Import + use getErrorMessage() in push/backfill error reporting. |
| actions/setup/js/push_repo_memory.cjs | Import + use getErrorMessage() for JSON parse/format failures. |
| actions/setup/js/push_experiment_state.cjs | Use getErrorMessage() for fetch error message matching. |
| actions/setup/js/pre_activation_summary.cjs | Import + use getErrorMessage() when template read fails. |
| actions/setup/js/pick_experiment.cjs | Import + use getErrorMessage() for spec parse failure output. |
| actions/setup/js/pi_provider.cjs | Import + use getErrorMessage() for safe-outputs emission failures. |
| actions/setup/js/pi_agent_core_driver.cjs | Import getErrorMessage() and update log messages for parse/read errors. |
| actions/setup/js/patch_awf_chroot_config.cjs | Import + use getErrorMessage() when wrapping patch failure errors. |
| actions/setup/js/parse_firewall_logs.cjs | Import + use getErrorMessage() when failing the step. |
| actions/setup/js/otlp.cjs | Import + use getErrorMessage() for non-fatal export warning text. |
| actions/setup/js/mount_mcp_as_cli.cjs | Import + use getErrorMessage() across MCP CLI read/write/network warnings. |
| actions/setup/js/mcp-scripts-runner.cjs | Import + use getErrorMessage() for input parsing warnings. |
| actions/setup/js/mcp_dependencies_manager.cjs | Import + use getErrorMessage() when building install failure details. |
| actions/setup/js/mcp_cli_bridge.cjs | Import + use getErrorMessage() for audit dir/log and MCP call failures. |
| actions/setup/js/log_parser_bootstrap.cjs | Use getErrorMessage() for terminal error reporting. |
| actions/setup/js/load_experiment_state_from_repo.cjs | Import + use getErrorMessage(); avoid unsafe .status access. |
| actions/setup/js/install_frontmatter_skills.cjs | Import + use getErrorMessage() for read/write/install warnings. |
| actions/setup/js/git_helpers.cjs | Import + use getErrorMessage() in shallow-history probe warning. |
| actions/setup/js/generate_aw_info.cjs | Import + use getErrorMessage() for env parse warnings. |
| actions/setup/js/fuzz_update_body_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_template_substitution_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_template_branch_harness.cjs | Import + use getErrorMessage() for harness error output. |
| actions/setup/js/fuzz_sanitize_output_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_sanitize_label_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_sanitize_incoming_text_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_runtime_import_expressions_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_remove_xml_comments_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_mentions_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_markdown_code_region_balancer_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_is_safe_expression_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/fuzz_bash_command_parser_harness.cjs | Import + use getErrorMessage() in harness error outputs. |
| actions/setup/js/frontmatter_hash_pure.cjs | Import + use getErrorMessage() when wrapping GitHub read failures. |
| actions/setup/js/emit_outcome_spans.cjs | Import + use getErrorMessage() for non-fatal OTEL export failures. |
| actions/setup/js/dynamic_checkout.cjs | Import + use getErrorMessage() for checkout failure logging. |
| actions/setup/js/display_file_helpers.cjs | Import + use getErrorMessage() when file/dir display fails. |
| actions/setup/js/determine_automatic_lockdown.cjs | Import + use getErrorMessage() when policy resolution fails. |
| actions/setup/js/create_pull_request.cjs | Use getErrorMessage() across git/Octokit failures and fallbacks. |
| actions/setup/js/create_issue.cjs | Use getErrorMessage() when parent-comment creation fails. |
| actions/setup/js/create_forecast_issue.cjs | Import + use getErrorMessage() for report/error JSON read/parse failures. |
| actions/setup/js/copilot_sdk_session.cjs | Import getErrorMessage() and update SDK watchdog disconnect logging. |
| actions/setup/js/copilot_harness.cjs | Import getErrorMessage() and update unexpected error logging. |
| actions/setup/js/convert_gateway_config_copilot.cjs | Import + use getErrorMessage() when resolving output path fails. |
| actions/setup/js/collect_skill_install_failures.cjs | Import + use getErrorMessage() for failures file read warning. |
| actions/setup/js/collect_ndjson_output.cjs | Use getErrorMessage() when composing JSON repair failure details. |
| actions/setup/js/codex_harness.cjs | Import getErrorMessage() and update unexpected error logging. |
| actions/setup/js/claude_harness.cjs | Import getErrorMessage() and update unexpected error logging. |
| actions/setup/js/checkout_pr_branch.cjs | Avoid unsafe .status access by aliasing caught errors to any. |
| actions/setup/js/check_version_updates.cjs | Import + use getErrorMessage() for update config fetch failures. |
| actions/setup/js/check_team_member.cjs | Import + use getErrorMessage() for repository permission check warnings. |
| actions/setup/js/check_permissions_utils.cjs | Import + use getErrorMessage() for aw_context parse failures. |
| actions/setup/js/check_membership.cjs | Import + use getErrorMessage() for provenance validation errors. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 64/64 changed files
- Comments generated: 5
- 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 /codebase-design — requesting changes on two correctness issues (one semantic mismatch, one missed conversion) and a recurring JSDoc cast syntax bug.
📋 Key Findings & Highlights
Issues Found
-
send_otlp_span.cjs— semantic mismatch (correctness, low-probability): The localgetErrorMessageis an OTLP-domain function that returns""for non-object/non-string values. Using it in an HTTP retry catch block creates a latent silent-failure path. Either use the shared helper or document the intentional choice. -
JSDoc cast missing parentheses (type-safety, affects 3+ files):
const errAny = /**@type{any} */ erris not a valid JSDoc type assertion — it needs/**@type{any} */ (err). The correct form is used inerror_helpers.cjsline 28. Affects all newerrAny/userErrAny/errorAnypatterns in this PR. -
push_signed_commits.cjs— incomplete conversion: The samecatch (err)block has two lines correctly converted togetErrorMessage(err)but one adjacenterr.messageaccess left unconverted (thePushSignedCommitsPolicyViolationthrow). Creates an inconsistency within a single catch block.
Positive Highlights
- ✅ The
safe_outputs_handlers.cjsrefactor is well-done: extractingerrMsgonce and reusing it for both the guard and the response is exactly right. - ✅ Correct removal of the erroneous import in
send_otlp_span.cjs(the local function must not be shadowed). - ✅ Excellent breadth: 168 violations cleared systematically with consistent import hygiene across 64 files.
- ✅
getErrorMessageinerror_helpers.cjshandles HTML sanitization and status extraction — a real improvement over raw ternaries.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 125.8 AIC · ⌖ 7.34 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| const msg = getErrorMessage(err); |
There was a problem hiding this comment.
[/diagnosing-bugs] The local getErrorMessage in this file handles OTLP-domain structured entries { type, message, error } — for non-object/non-string thrown values it returns "" instead of a useful string. The shared error_helpers.cjs version falls back to String(error), which is safer for HTTP exception objects.
💡 Details
The local function (defined ~line 1658) only handles strings and { message: string } shapes, returning "" for everything else. Since this is an HTTP retry catch block, errors are almost always Error instances so the regression is edge-case only — but it leaves a latent bug where unexpected thrown values produce silent empty log entries.
The PR description notes the import was intentionally removed to avoid shadowing the local function — but the local function has a fundamentally different signature and semantics. Consider either:
- Using the shared helper and deleting the local function (if the local one is only used here)
- Documenting the intentional use of the local function with a comment explaining why
@copilot please address this.
| // or a real user that is not a collaborator. Disambiguate via users API. | ||
| // Real users resolve via users.getByUsername; app/bot actors return 404. | ||
| if (err.status === 404) { | ||
| const errAny = /** @type {any} */ err; |
There was a problem hiding this comment.
[/codebase-design] The /** @type {any} */ err cast lacks the parentheses required for a JSDoc type assertion in @ts-check files — the correct form is /** @type {any} */ (err). Without parens, the type annotation is a JSDoc comment on the const declaration itself, not a cast expression; TypeScript will still treat errAny as unknown and the .status access will fail type-checking.
💡 Suggested fix
// Before (missing parens)
const errAny = /** `@type` {any} */ err;
// After (correct JSDoc cast)
const errAny = /** `@type` {any} */ (err);Note: error_helpers.cjs uses the correct form at line 28: const errorAsAny = /** @type {any} */ (error);.
This pattern is introduced in multiple files in this PR (checkout_pr_branch.cjs, load_experiment_state_from_repo.cjs, run_operation_update_upgrade.cjs). All need the parentheses.
@copilot please address this.
| @@ -747,9 +746,9 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c | |||
| throw new Error(`pushSignedCommits: refusing unsigned push for branch '${branch}': ${err.message}`, { cause: err }); | |||
There was a problem hiding this comment.
[/diagnosing-bugs] Two adjacent lines — one throw at line 746 and one core.warning — were converted to getErrorMessage(err), but the throw on the unchanged line above (the PushSignedCommitsPolicyViolation branch at line 746 context) still uses err.message directly. This is inconsistent: if err is not an Error instance it will surface undefined in the thrown message.
💡 Details
In the same catch (err) block, the conversion was applied to 2 of 3 err.message accesses but not to:
throw new Error(`pushSignedCommits: refusing unsigned push for branch '${branch}': ${err.message}`, { cause: err });Since this is inside a typed catch block where err is unknown, accessing .message directly can produce "undefined" in the error string if a non-Error is thrown. This should be:
throw new Error(`pushSignedCommits: refusing unsigned push for branch '${branch}': ${getErrorMessage(err)}`, { cause: err });@copilot please address this.
| } catch (/** @type {any} */ err) { | ||
| // 404 means the branch or file does not exist yet – that is normal on first run. | ||
| if (err.status === 404) { | ||
| const errAny = /** @type {any} */ err; |
There was a problem hiding this comment.
| } catch (error) { | ||
| // Label may not exist when auto-upgrade is used without maintenance label creation. | ||
| if (error?.status === 422) { | ||
| const errorAny = /** @type {any} */ error; |
There was a problem hiding this comment.
REQUEST_CHANGES: Two categories of bugs introduced alongside the otherwise-correct ESLint normalization.
Blocking issues (must fix before merge)
1. Strict mode disabled in three production harness files
claude_harness.cjs, copilot_harness.cjs, and pi_agent_core_driver.cjs all have the same structural regression: the new require() import was placed at line 2 (before the JSDoc block), which terminates the directive prologue. The subsequent "use strict" was also converted to ("use strict") — a parenthesized expression that never activates strict mode in any JS engine. These three files silently drop strict-mode semantics: undeclared-variable assignments become silent global creation, and this in plain function calls becomes globalThis.
Fix for all three: move the getErrorMessage import to after the "use strict" directive (bare string, unparenthesized):
// `@ts-check`
"use strict";
/** ... */
const { getErrorMessage } = require("./error_helpers.cjs");2. JSDoc type casts missing required parentheses
checkout_pr_branch.cjs (lines 160, 166) and load_experiment_state_from_repo.cjs (line 98) all use /** @type {any} */ err without parentheses. TypeScript JSDoc type assertions require the target expression to be parenthesized: /** @type {any} */ (err). Without the parens the cast is silently ignored and tsc --checkJs will still flag .status access as a type error under useUnknownInCatchVariables: true.
Non-blocking observations
send_otlp_span.cjs: thegetErrorMessagecall at line 1117 correctly resolves to the file-local function (not the sharederror_helpers.cjshelper). The local version applies OTLP-specific truncation and structured-entry extraction. The change is fine as-is, but this implicit scoping means the file diverges silently if the two implementations drift — worth a comment noting that the local variant is intentional.- The bulk of the
getErrorMessageconversions are correct and thesafe_outputs_handlers.cjsrefactor (extractingerrMsgonce for both the guard and the response) is an improvement.
🔎 Code quality review by PR Code Quality Reviewer · 191.2 AIC · ⌖ 7.29 AIC · ⊞ 5.4K
Comment /review to run again
| */ | ||
|
|
||
| "use strict"; | ||
| ("use strict"); |
There was a problem hiding this comment.
Strict mode permanently disabled: is a parenthesized expression statement — it never activates strict mode, regardless of position.
💡 What happened and how to fix
Inserting at line 2 (a statement) before the JSDoc comment terminates the directive prologue. Any string literal that follows is just an expression statement, not a directive. Compounding the problem, was also rewritten to — a parenthesized expression — which Node.js/V8 never treats as a strict-mode directive.
This file was previously strict-mode. After this PR it silently runs in sloppy mode:
- Assignments to undeclared variables silently create globals instead of throwing
- in plain function calls is the global object instead of
Fix: move the import to after the directive:
The same regression exists in (line 41) and (line 38).
| */ | ||
|
|
||
| "use strict"; | ||
| ("use strict"); |
There was a problem hiding this comment.
Strict mode permanently disabled: ("use strict") is a parenthesized expression statement — it never activates strict mode, regardless of position.
💡 What happened and how to fix
Inserting const { getErrorMessage } = require("./error_helpers.cjs") at line 2 (a const statement) before the JSDoc block terminates the directive prologue. Any string literal that follows is just an expression statement, not a directive. Compounding the issue, "use strict" was also rewritten to ("use strict") — a parenthesized expression that Node.js/V8 never treats as a strict-mode directive.
This file was previously running in strict mode. After this PR it silently runs in sloppy mode:
- Assignments to undeclared variables silently create globals instead of throwing
ReferenceError thisin plain function calls isglobalThisinstead ofundefined
Fix: move the require import to after the directive and restore the bare string form:
// `@ts-check`
"use strict";
/**
* Claude Code CLI Harness...
*/
const { getErrorMessage } = require("./error_helpers.cjs");
const fs = require("fs");The same regression exists in copilot_harness.cjs (line 41) and pi_agent_core_driver.cjs (line 38).
| */ | ||
|
|
||
| "use strict"; | ||
| ("use strict"); |
There was a problem hiding this comment.
Strict mode silently dropped: same as claude_harness.cjs — the require() at line 2 breaks the directive prologue and the parenthesized ("use strict") at line 41 is never interpreted as a directive by the engine.
💡 Fix
Move the import to after "use strict":
// `@ts-check`
"use strict";
require("./shim.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");| */ | ||
|
|
||
| "use strict"; | ||
| ("use strict"); |
There was a problem hiding this comment.
Strict mode silently dropped: same root cause as claude_harness.cjs — const { getErrorMessage } = require(...) at line 2 precedes the JSDoc and ("use strict") at line 38, ending the directive prologue before the directive is reached. The parenthesized form ("use strict") never enables strict mode.
💡 Fix
Move the import below "use strict" (bare, unparenthesized):
// `@ts-check`
"use strict";
/**
* Pi Agent Core Driver (inner harness)
* ...
*/
const fs = require("fs");
const { getErrorMessage } = require("./error_helpers.cjs");| // or a real user that is not a collaborator. Disambiguate via users API. | ||
| // Real users resolve via users.getByUsername; app/bot actors return 404. | ||
| if (err.status === 404) { | ||
| const errAny = /** @type {any} */ err; |
There was a problem hiding this comment.
JSDoc type cast is missing required parentheses: /** @type {any} */ err does not apply the cast — TypeScript JSDoc assertions require the target expression to be parenthesized.
💡 Detail and fix
The TypeScript JSDoc type-assertion syntax requires wrapping the expression in parentheses:
// WRONG — cast is ignored; err is still typed as unknown
const errAny = /** `@type` {any} */ err;
// CORRECT
const errAny = /** `@type` {any} */ (err);Without the parens, tsc --checkJs (and checkers like Pyright) still see errAny as the original type of err. In a catch block with useUnknownInCatchVariables: true, err is unknown, so errAny.status remains a type error. Even if your tsconfig has useUnknownInCatchVariables: false today, the pattern is incorrect and will break if strict checking is tightened.
The same issue appears at line 166 with userErrAny. Both casts need parens.
const errAny = /** `@type` {any} */ (err);
if (errAny.status === 404) {
const userErrAny = /** `@type` {any} */ (userErr);
if (userErrAny.status === 404) {| } catch (/** @type {any} */ err) { | ||
| // 404 means the branch or file does not exist yet – that is normal on first run. | ||
| if (err.status === 404) { | ||
| const errAny = /** @type {any} */ err; |
There was a problem hiding this comment.
JSDoc type cast missing parens: /** @type {any} */ err does not apply the cast; parentheses are required by the TypeScript JSDoc assertion syntax.
💡 Fix
// was (ineffective cast):
const errAny = /** `@type` {any} */ err;
// should be:
const errAny = /** `@type` {any} */ (err);Without the parentheses the variable is still typed as the original catch-variable type. See the same issue in checkout_pr_branch.cjs line 160.
🤖 PR Triage
Score breakdown:
Recommendation: Batch review alongside #43637 — Both PRs address
|
|
@copilot please run the Unresolved review feedback to address:
After that, rerun the relevant checks and update anything still stale.
|
…err.message conversions
- Restore `"use strict"` as a valid directive prologue in 5 harness files
(pi_agent_core_driver, copilot_sdk_session, copilot_harness, codex_harness,
claude_harness) by moving the getErrorMessage import to after "use strict"
instead of before the JSDoc block comment.
- Parenthesize all JSDoc type-assertion casts added in this PR:
const errAny = /** @type {any} */ (err);
Fixes checkout_pr_branch.cjs (errAny, userErrAny),
load_experiment_state_from_repo.cjs (errAny), and
run_operation_update_upgrade.cjs (errorAny).
- Convert the two remaining direct err.message accesses in
push_signed_commits.cjs (PushSignedCommitsUnsupportedShape and
PushSignedCommitsPolicyViolation branches) to getErrorMessage(err)
for consistency with the rest of the catch block.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all items in commit
|
|
@copilot merge main and lint js |
…r-normalize-error-handling Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…hroot_config.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged main (45 files, including new JS from |
ESLint rules
no-unsafe-catch-error-property,no-unsafe-promise-catch-error-property, andprefer-get-error-messagefired 168 warnings across 64 files inactions/setup/jsdue to direct.message/.status/.nameaccess on unknown caught/rejected values and inline ternary patterns instead of the sharedgetErrorMessage()helper.Changes
prefer-get-error-message(139 instances, 50 files): Replaced allx instanceof Error ? x.message : String(x)ternaries withgetErrorMessage(x)fromerror_helpers.cjs.Direct property access on caught values (16 files):
.messageaccess →getErrorMessage(err).status/.namechecks that can't usegetErrorMessage→ typed local variable to escape the catch-variable tracking in the rule:safe_outputs_handlers.cjs: three catch blocks usingerr?.message?.startsWith(SENTINEL)+buildIntentErrorResponse(err.message)→ extract once withconst errMsg = getErrorMessage(err)then use for both guard and response.Imports (49 files): Added
const { getErrorMessage } = require("./error_helpers.cjs");where missing.send_otlp_span.cjs: Removed erroneously added import — file defines its owngetErrorMessagefor OTLP error entries; calls within the file correctly resolve to the local function.