[eslint-miner] ESLint Miner: add require-fs-chmod-try-catch rule - #59408
Conversation
Flags unguarded fs.chmodSync/fs.fchmodSync calls (throw on missing
files, invalid descriptors, or permission errors) with an auto-fix
suggestion that wraps them in try/catch and preserves the error as
{ cause }.
Mined from actions/setup/js: fs.chmodSync/fchmodSync is used 18 times
to lock down permissions on files/dirs that may hold secrets, but
2 call sites (create_prompt.cjs, start_mcp_gateway.cjs) were unguarded.
Adjacent methods (readFileSync/writeFileSync/statSync/etc.) already
have dedicated try/catch rules; chmod was a gap.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #59408; no review comments were submitted because the changed ESLint rule and tests did not present a high-confidence actionable issue in changed lines. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail pass found a few places to trim explanatory verbosity and generated TODO noise without changing behavior. The rule logic and coverage look fine; this is mostly about keeping lint rule source and autofix output leaner and easier to maintain.
net: -14 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #59408 · codex · gpt53codex · 6.1 AIC · ⌖ 4.22 AIC · ⊞ 13.4K
Comment /ponytail to run again
There was a problem hiding this comment.
Reviewed with focus on correctness/maintainability (Impeccable modes did not directly apply — this is a backend ESLint rule, not UI). The new require-fs-chmod-try-catch rule faithfully mirrors the existing sibling rules (require-fs-sync-try-catch, require-mkdirsync-try-catch, etc.), reusing the shared try-catch-rule-utils helpers (createFsSyncMethodResolver, isInsideTryBlock, buildTryCatchSuggestion) with no deviations in structure. Registration in index.ts and eslint.config.cjs is correct and consistent with sibling rules (same warn severity). Tests cover valid (inside try), ignored (other fs methods / non-fs identifiers), and invalid (outside try, both methods, after unrelated try block) cases, matching the pattern used by sibling rule tests. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 23.7 AIC · ⌖ 13.4 AIC · ⊞ 8.3K
There was a problem hiding this comment.
🟡 Changes recommended
The rule exposes a dead documentation link because its README entry and anchor are missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an ESLint rule requiring fs.chmodSync and fs.fchmodSync calls to use try/catch error handling.
Changes:
- Implements and registers the rule.
- Adds unit coverage and enables warn-level enforcement.
- The generated documentation link lacks a corresponding README section.
File summaries
| File | Description |
|---|---|
eslint-factory/src/rules/require-fs-chmod-try-catch.ts |
Implements detection and suggested fixes. |
eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts |
Tests valid and invalid calls. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule as a warning. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design (pattern-fit for a new custom ESLint rule) and /tdd (test coverage check). No blocking issues found.
📋 Key Themes & Highlights
Key Themes
- Consistent with sibling rules:
require-fs-chmod-try-catchreusescreateFsSyncMethodResolver/isInsideTryBlock/buildTryCatchSuggestionexactly likerequire-fs-io-try-catch,require-mkdirsync-try-catch, etc. Registration inindex.tsandeslint.config.cjsfollows the same shape as neighboring entries. - Test coverage is solid: valid (inside try, both methods), ignored (other fs methods, non-
fsidentifiers), and invalid (outside try, both methods, plus the "after an unrelated try block" edge case) are all covered — good use ofRuleTestermatching the sibling test file's structure. - Evidence-backed: PR description documents the 2 real unguarded call sites found (
create_prompt.cjs:85,start_mcp_gateway.cjs:276) and confirms other call sites already pass, giving confidence in low false-positive risk.
Positive Highlights
- ✅ Rule correctly reuses
allowUnboundFsIdentifier: true, matching the option used byrequire-fs-io-try-catch, so a barefs.chmodSync(...)without an explicit import binding is still flagged (this is exactly the case increate_prompt.cjsandstart_mcp_gateway.cjs). - ✅ Auto-fix suggestion preserves original statement text/indentation via
buildTryCatchSuggestion, consistent with sibling rules' fix behavior. - ✅ Doc comment on
FS_CHMOD_METHODSclearly explains the why (EPERM/EINVAL, secret-bearing files) — good context for future maintainers per/codebase-design.
No changes requested.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 33.2 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
🧪 Test Quality Sentinel ReportSummary✅ Test Quality Score: 100/100 — Excellent Changed Test Files: 1 new TypeScript test file Test Case Analysis5 Test Cases — All High Quality
Quality Signals✅ Best Practices:
✅ No Red Flags:
VerdictAll tests enforce genuine design invariants. The rule correctly:
No violations. Approve.
|
@copilot Please take the next forward-progress pass on PR #59408.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
@copilot Please take the next forward-progress pass on PR #59408.
|
Completed the forward-progress pass in dd5ca6b: addressed all actionable review feedback, validated locally, and confirmed the branch already contains the latest main commit. |
Rechecked current HEAD |
|
@copilot please refresh this PR branch and run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
…-try-catch-af2bcd2c3b9ee259 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed the branch with latest |
@copilot Please take the next forward-progress pass on PR #59408.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Forward-progress pass complete: |
@copilot Please take the next forward-progress pass on PR #59408.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Confirmed: all five addressed review threads are resolved. CI run 34251958255 on |
@copilot Please take the next forward-progress pass on PR #59408.
Failed checks: none currently reported on the compact queue.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Current HEAD |
Summary
Adds a new custom ESLint rule,
require-fs-chmod-try-catch, that flags unguardedfs.chmodSync/fs.fchmodSynccalls inactions/setup/jsand offers an auto-fix suggestion that wraps the call intry { ... } catch (err) { throw new Error(..., { cause: err }); }.Rationale
fs.chmodSync/fs.fchmodSyncthrow synchronously on missing files/descriptors, permission errors (EPERM), or unsupported filesystems. Inactions/setup/js, these calls are used 18 times to lock down permissions on files/directories that may contain secrets (staged config files, audit dirs, temp gateway output). Adjacent fs methods (readFileSync,writeFileSync,statSync,unlinkSync,renameSync,mkdirSync,rmSync,realpathSync,mkdtempSync, ...) already each have a dedicated try/catch rule ineslint-factory/src/rules, butchmodSync/fchmodSynchad no coverage — this closes that gap.Evidence
Scanned
actions/setup/js/*.cjsforchmodSync/fchmodSynccall sites (18 total). Running the new rule against the codebase surfaces 2 real unguarded call sites that should be wrapped:actions/setup/js/create_prompt.cjs:85—fs.fchmodSync(fd, 0o600)outside try/catchactions/setup/js/start_mcp_gateway.cjs:276—fs.chmodSync(dir, 0o700)outside try/catchOther call sites (e.g.
convert_gateway_config_shared.cjs,mcp_server_core.cjs,safe_outputs_handlers.cjs,upload_artifact.cjs) are already inside try/catch and pass cleanly, confirming low false-positive risk.Changes
eslint-factory/src/rules/require-fs-chmod-try-catch.ts— new rule (reuses the existingcreateFsSyncMethodResolver/isInsideTryBlock/buildTryCatchSuggestionutilities used by sibling fs rules).eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts— unit tests covering valid (inside try), ignored (other fs methods / non-fsidentifiers), and invalid (outside try, both methods, and after an unrelated try block) cases.eslint-factory/src/index.ts— registers the new rule in the plugin.eslint-factory/eslint.config.cjs— enablesgh-aw-custom/require-fs-chmod-try-catchas"warn", matching severity of sibling fs rules.Validation
All pass.
npm run lint:setup-jsreports 65 warnings total (0 errors), including the 2 new findings above, consistent with the existing warn-only baseline for this suite.Scope
No Go code, documentation-only files, or JavaScript outside
actions/setup/jswere touched.Run: https://github.com/github/gh-aw/actions/runs/34226625975
pr-sous-chef run https://github.com/github/gh-aw/actions/runs/34270348844
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.