Guard setup JS sync fs/exec calls with contextual error handling#47425
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hardens setup scripts against synchronous filesystem and subprocess failures by adding contextual error handling.
Changes:
- Wraps sync I/O failures with actionable errors or warnings.
- Preserves existing fallback behavior where failures are non-fatal.
- Adds representative regression tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/artifact_client.cjs |
Handles upload metadata failures. |
actions/setup/js/build_checkout_manifest.cjs |
Adds context to GitHub CLI failures. |
actions/setup/js/check_workflow_timestamp.cjs |
Handles timestamp inspection failures. |
actions/setup/js/comment_memory_helpers.cjs |
Wraps directory-read failures. |
actions/setup/js/comment_memory_helpers.test.cjs |
Tests directory-read failure handling. |
actions/setup/js/generate_git_bundle.cjs |
Handles bundle metadata failures. |
actions/setup/js/generate_git_patch.cjs |
Handles workspace metadata failures. |
actions/setup/js/install_frontmatter_skills.cjs |
Handles skill-directory reads. |
actions/setup/js/merge_remote_agent_github_folder.cjs |
Wraps scanning and copy failures. |
actions/setup/js/push_experiment_state.cjs |
Handles state file and workspace failures. |
actions/setup/js/push_repo_memory.cjs |
Wraps recursive directory scanning. |
actions/setup/js/run_evals.cjs |
Handles agent-output metadata failures. |
actions/setup/js/runtime_import.cjs |
Refetches when cache inspection fails. |
actions/setup/js/safe_outputs_handlers.cjs |
Wraps validation, scanning, and staging I/O. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Tests staging-copy failure handling. |
actions/setup/js/setup_threat_detection.cjs |
Adds contextual artifact inspection handling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Medium
| try { | ||
| promptStats = fs.statSync(promptPath); | ||
| } catch (err) { | ||
| core.warning(`⚠️ ${ERR_VALIDATION}: Failed to inspect workflow prompt context at ${promptPath}: ${getErrorMessage(err)}. ` + "Threat detection will continue with fallback workflow context."); |
|
@copilot avoid emojis in error messages |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47425 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
References: §29981333959
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR makes good progress on hardening sync I/O calls with contextual error handling. One blocking issue was found (already flagged as an inline comment): setup_threat_detection.cjs uses getErrorMessage in four new catch blocks but the error_helpers.cjs import is missing — every catch path will throw ReferenceError: getErrorMessage is not defined instead of reporting the I/O failure.
All other changed files that use getErrorMessage have the correct import.
Blocking: Add const { getErrorMessage } = require("./error_helpers.cjs"); to the imports in setup_threat_detection.cjs.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 28.1 AIC · ⌖ 5.58 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes for a blocking correctness bug and two follow-on improvements.
📋 Key Themes & Highlights
Blocking issue
- Missing
getErrorMessageimport insetup_threat_detection.cjs— all four new catch blocks callgetErrorMessagewhich is not imported, so every error path crashes withReferenceErrorinstead of the intended diagnostic. The fix is onerequireline.
Follow-on improvements
- Ambiguous combined
try/catchincheck_workflow_timestamp.cjs— twostatSynccalls share a single handler; the error message cannot distinguish which file was missing. - Early-exit inside file-collection loop in
push_experiment_state.cjs— the firststatSyncfailure aborts the whole scan; if the intent is to report all bad files, use a collect-then-fail pattern; if it's intentional, a test would confirm and document it.
Positive highlights
- ✅ Consistent error-message format (
Failed to <verb> <resource>: <cause>) used throughout — easy to scan in logs. - ✅
{ cause: err }on re-thrown errors preserves the original stack trace for deep debugging. - ✅ New tests for
comment_memory_helpersandsafe_outputs_handlerscover the representative wrapped failure modes as stated in the PR description. - ✅ All success-path behaviour is preserved; only exceptional paths are touched.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 51.4 AIC · ⌖ 4.95 AIC · ⊞ 6.7K
Comment /matt to run again
| try { | ||
| agentOutputStats = fs.statSync(agentOutputPath); | ||
| } catch (err) { | ||
| core.setFailed(`${ERR_VALIDATION}: Failed to inspect agent output file ${agentOutputPath}: ${getErrorMessage(err)}`); |
There was a problem hiding this comment.
[/diagnosing-bugs] getErrorMessage is used at lines 55, 105, 125, and 141 but is never imported — every catch path in this file will throw ReferenceError: getErrorMessage is not defined instead of reporting the I/O failure.
💡 Fix: add the missing import
Add this near the top of the file alongside the other require calls:
const { getErrorMessage } = require("./error_helpers.cjs");All four catch blocks reference getErrorMessage(err) (lines 55, 105, 125, 141), so without this import every error case crashes with a confusing ReferenceError instead of the intended diagnostic message.
@copilot please address this.
| lockStat = fs.statSync(lockFile); | ||
| } catch (err) { | ||
| core.setFailed(`Failed to inspect workflow timestamps: ${getErrorMessage(err)}`); | ||
| return; |
There was a problem hiding this comment.
[/diagnosing-bugs] Both statSync calls share a single try/catch, so the error message can't tell the operator whether the workflow .md or the .lock.yml file was unreadable.
💡 Suggested fix: separate the guards
try {
workflowStat = fs.statSync(workflowMdFile);
} catch (err) {
core.setFailed(`Failed to inspect workflow source ${workflowMdFile}: ${getErrorMessage(err)}`);
return;
}
try {
lockStat = fs.statSync(lockFile);
} catch (err) {
core.setFailed(`Failed to inspect lock file ${lockFile}: ${getErrorMessage(err)}`);
return;
}The two files have different diagnostic meanings — one missing is a config problem, the other is a build artifact gap. Separate errors make triage unambiguous.
@copilot please address this.
| try { | ||
| fileInfo = fs.statSync(full); | ||
| } catch (err) { | ||
| core.setFailed(`Failed to inspect ${stateLabel} file ${full}: ${getErrorMessage(err)}`); |
There was a problem hiding this comment.
[/tdd] core.setFailed inside a loop followed by return silently skips inspecting the remaining candidate files — only the first failure is reported. If multiple state files are unreadable, operators only learn about the first one.
💡 Options to consider
- Collect all errors first, then fail once:
const errors = [];
for (const name of candidateFiles) {
...
try { fileInfo = fs.statSync(full); }
catch (err) { errors.push(`${full}: ${getErrorMessage(err)}`); continue; }
...
}
if (errors.length) { core.setFailed(`Failed to inspect state files:
${errors.join("
")}`); return; }- Or keep the early-exit but add a test that verifies only the first error surfaces — so the behaviour is at least intentional and documented.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one critical correctness bug must be fixed before merge; two non-blocking observations noted inline.
### Findings summary
🔴 Critical — setup_threat_detection.cjs: getErrorMessage called without being imported
This file calls getErrorMessage() at four catch sites (lines 55, 105, 125, 141) introduced by this PR, but never imports it from ./error_helpers.cjs. Every one of those catch paths will throw ReferenceError: getErrorMessage is not defined at runtime instead of reporting the original I/O failure. The PR's stated goal — converting raw crashes into contextual errors — is completely defeated for this file.
Fix: add const { getErrorMessage } = require('./error_helpers.cjs'); to the imports, as done correctly in the other changed files (check_workflow_timestamp.cjs, run_evals.cjs, comment_memory_helpers.cjs, etc.).
🟡 Medium — check_workflow_timestamp.cjs: combined try/catch loses file identity (see inline comment)
🔵 Low — runtime_import.cjs: empty if (!stats) dead branch (see inline comment)
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 97.7 AIC · ⌖ 4.82 AIC · ⊞ 5.7K
Comment /review to run again
| workflowStat = fs.statSync(workflowMdFile); | ||
| lockStat = fs.statSync(lockFile); | ||
| } catch (err) { | ||
| core.setFailed(`Failed to inspect workflow timestamps: ${getErrorMessage(err)}`); |
There was a problem hiding this comment.
Ambiguous error when either statSync fails: both file stats are batched in one try/catch, so the error message Failed to inspect workflow timestamps cannot identify whether the workflow .md or the .lock.yml was unreadable.
💡 Suggested fix
Wrap each call separately:
try {
workflowStat = fs.statSync(workflowMdFile);
} catch (err) {
core.setFailed(`Failed to inspect workflow file ${workflowMdFile}: ${getErrorMessage(err)}`);
return;
}
try {
lockStat = fs.statSync(lockFile);
} catch (err) {
core.setFailed(`Failed to inspect lock file ${lockFile}: ${getErrorMessage(err)}`);
return;
}The two files have different roles (source vs generated output), so a missing lock file should produce a different diagnostic than a missing source file. The combined catch makes triage harder.
| stats = null; | ||
| } | ||
| if (!stats) { | ||
| // fall through to refetch below |
There was a problem hiding this comment.
Dead if (!stats) branch adds noise without doing anything: the if (!stats) { // fall through } block has an empty body — it's a no-op that exists solely to introduce the else branch holding the real logic. This is unnecessarily hard to read and inverts the normal control-flow idiom.
💡 Suggested fix
Invert the condition so the happy path is the if branch:
if (stats) {
const ageInMs = Date.now() - stats.mtimeMs;
const oneHourInMs = 60 * 60 * 1000;
if (ageInMs < oneHourInMs) {
core.info(`Using cached content for URL: ${url}`);
try {
return fs.readFileSync(cacheFile, "utf8");
} catch (err) {
throw new Error(`Failed to read file ${cacheFile}: ${String(err)}`, { cause: err });
}
}
}
// fall through to refetchThe comment // fall through to refetch below belongs at the bottom of this block, not inside the empty branch.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Daily
lint:setup-jssurfaced a broad reliability gap inactions/setup/js: several synchronous filesystem and child-process calls could throw unhandled errors and terminate the action abruptly. This change wraps the flagged sync I/O sites so failures surface as contextual, actionable errors instead of raw crashes.Scope: targeted sync I/O hardening
try/catchguards around the flaggedfs.statSync,fs.readdirSync,fs.copyFileSync, andexecFileSynccall sites across the affected setup scripts.Failure reporting
Behavioral adjustments by script
Regression coverage
Example of the change pattern: