[eslint-miner] eslint-factory: add prefer-structured-clone rule for actions/setup/js - #50530
Conversation
Flags JSON.parse(JSON.stringify(x)) deep-clone round-trips in actions/setup/js and suggests structuredClone(x) instead. The JSON round-trip silently drops undefined/function values, converts Date objects to strings, and throws on circular references, while structuredClone (available globally on the Node 24 runtime this action targets) clones plain/JSON-safe data directly and faster. Found 3 existing occurrences of this pattern in actions/setup/js: generate_safe_outputs_tools.cjs, pick_experiment.cjs, and safe_outputs_tools_loader.cjs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50530 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
Adds a custom ESLint rule recommending structuredClone over JSON round-trip cloning in setup scripts.
Changes:
- Implements and tests pattern detection and suggestions.
- Registers and enables the rule for setup JavaScript linting.
- Includes computed-property support.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/prefer-structured-clone.ts |
Implements the rule and suggestion. |
eslint-factory/src/rules/prefer-structured-clone.test.ts |
Tests accepted and reported patterns. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule as a warning. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Balanced
|
|
||
| const innerArg = node.arguments[0]; | ||
| if (innerArg.type !== AST_NODE_TYPES.CallExpression) return; | ||
| if (!isPlainJsonStringifyCall(innerArg)) return; |
| const isDirectAccess = !callee.computed && property.type === AST_NODE_TYPES.Identifier && property.name === "stringify"; | ||
| const isComputedAccess = callee.computed && property.type === AST_NODE_TYPES.Literal && property.value === "stringify"; | ||
| if (!isDirectAccess && !isComputedAccess) return false; | ||
| return node.arguments.length === 1; |
| { | ||
| messageId: "replaceWithStructuredClone", | ||
| fix(fixer: TSESLint.RuleFixer) { | ||
| return fixer.replaceText(node, `structuredClone(${clonedExpressionText})`); |
|
|
||
| describe("prefer-structured-clone", () => { | ||
| it("uses the correct docs URL", () => { | ||
| expect(preferStructuredCloneRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#prefer-structured-clone"); |
| // Custom reviver on parse changes semantics too; still matched today since only the | ||
| // parse call itself is checked for a single stringify argument — but the inner | ||
| // stringify call must have exactly one argument, so this is out of scope here. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The rule implementation is correct and well-scoped. Logic for detecting JSON.parse(JSON.stringify(x)) with exactly one argument at each level is sound, suggestion fix is properly wired, test coverage includes computed-property access and edge cases, and the integration into index.ts / eslint.config.cjs follows existing patterns.
One minor nit: the comment on lines 27–29 of prefer-structured-clone.test.ts ("still matched today since only the parse call itself is checked for a single stringify argument") is slightly misleading. The real reason JSON.parse(JSON.stringify(obj), reviver) is excluded is the node.arguments.length !== 1 check on the outer parse call. The comment implies the inner-stringify check does the excluding, which is inaccurate. Non-blocking — the test result is correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.4 AIC · ⌖ 9.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — one minor comment on a misleading test comment; otherwise the PR is solid.
📋 Key Themes & Highlights
Key Themes
- One inaccurate inline comment in the test file (lines 28–30) implies the rule fires on the reviver case when it actually correctly skips it. See inline comment above.
Positive Highlights
- ✅ Rule is narrowly scoped to the unambiguous
JSON.parse(JSON.stringify(x))shape — low false-positive risk - ✅ Computed-property access (
JSON["parse"]/JSON["stringify"]) is correctly handled and tested - ✅ Replacer/indent and reviver edge cases are considered and excluded with clear rationale
- ✅ Suggestion fixer produces clean
structuredClone(expr)output - ✅ Test structure is clear with separate valid/invalid describe blocks
- ✅ Rule description and error messages are accurate and actionable
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29 AIC · ⌖ 7.99 AIC · ⊞ 7.1K
Comment /matt to run again
| }); | ||
|
|
||
| it("invalid: JSON.parse(JSON.stringify(x)) is flagged and suggests structuredClone(x)", () => { | ||
| cjsRuleTester.run("prefer-structured-clone", preferStructuredCloneRule, { |
There was a problem hiding this comment.
[/tdd] The inline comment on lines 28–30 is misleading: it says "still matched today" but this case is in the valid array (i.e. it is not flagged). The phrase implies the rule fires on this input, which is the opposite of what happens — JSON.parse(JSON.stringify(obj), reviver) is correctly excluded because the outer parse call has 2 arguments.
💡 Suggested clarification
// Custom reviver: excluded because JSON.parse has 2 arguments —
// the node.arguments.length !== 1 guard in the rule rejects it cleanly.
`const clone = JSON.parse(JSON.stringify(obj), reviver);`,@copilot please address this.
There was a problem hiding this comment.
Verdict: COMMENT — narrow, low-risk rule with one real gap in autofix safety
Small, well-scoped addition (a new ESLint rule + tests + registration). The AST matching is deliberately conservative (bails on replacer/reviver args) and the tests cover direct and computed-property access.
💡 Themes / highlights
- Autofix safety gap: the rule matches
JSONpurely by identifier name with no scope resolution, so a shadowed/aliasedJSONwould be falsely flagged and thesuggestfixer would rewrite it to the real globalstructuredClone, changing behavior. Low likelihood in this codebase's.cjsscripts today, but worth hardening since this ships an automatic suggestion, not just a warning. - Minor test-comment inconsistency: one inline comment describing why the reviver-argument case is excluded is self-contradictory/misleading about the actual guard clause responsible.
- No blocking correctness bugs in the primary detection path (
JSON.parse(JSON.stringify(x)), single-arg constraint, computed-access handling) — the core rule logic is sound for its stated narrow scope.
🔎 Code quality review by PR Code Quality Reviewer · auto · 39.3 AIC · ⌖ 7.18 AIC · ⊞ 7.9K
Comment /review to run again
| function isJsonParseCall(node: TSESTree.CallExpression): boolean { | ||
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.MemberExpression) return false; | ||
| if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "JSON") return false; |
There was a problem hiding this comment.
This matches on the bare identifier name JSON with no scope/binding resolution, so a locally shadowed or aliased JSON will be falsely flagged and auto-fixed to structuredClone(...), silently changing runtime behavior.
💡 Details
isJsonParseCall/isPlainJsonStringifyCall only check callee.object.name === "JSON" textually, with no scope analysis. If any enclosing scope shadows the global JSON (e.g. a parameter, destructure, or test double named JSON), the rule still matches, and because this rule ships an auto-suggest fixer (not just a warning), applying it rewrites to the real global structuredClone, which is not equivalent to the shadowed value — actively breaking code rather than just warning about it.
Fix: resolve JSON via scope (e.g. check context.sourceCode.getScope(node) for a local binding named JSON and bail out) before reporting/fixing.
| messageId: "replaceWithStructuredClone", | ||
| output: `const runs = state.runs.map(run => structuredClone(run));`, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
This comment is self-contradictory: it claims the reviver case is "still matched today" but the test lists it under valid (i.e., not flagged) — the opposite claim.
💡 Details
The outer JSON.parse(JSON.stringify(obj), reviver) call has two arguments, so node.arguments.length !== 1 triggers an early return in the rule — it is never matched, for a different reason than the comment implies. The comment focuses on the inner stringify call's argument count, but the actual disqualifier here is the outer parse call's second reviver argument. Clarify the comment to state that the parse call itself is excluded due to its own arg count, rather than saying it is "matched" and excluded downstream.
|
🎉 This pull request is included in a new release. Release: |
Overview
Adds a new custom ESLint rule,
prefer-structured-clone, toeslint-factory(thegh-aw-customplugin) targetingactions/setup/jsscripts. The rule flags the commonJSON.parse(JSON.stringify(x))deep-clone idiom and suggests replacing it withstructuredClone(x).Rationale
The JSON round-trip clone pattern:
structuredCloneundefinedand function valuesDateobjects to stringsstructuredClone(available globally since Node ≥17, and in the Node 24 runtime targeted byactions/setup/js) clones plain/JSON-safe data correctly without these pitfalls.Changes
eslint-factory/eslint.config.cjsgh-aw-custom/prefer-structured-cloneatwarnseverity in the plugin's rule config.eslint-factory/src/index.tspreferStructuredCloneRuleinto the plugin's rules map.eslint-factory/src/rules/prefer-structured-clone.ts@typescript-eslint/utilsESLintUtils.RuleCreator. DetectsJSON.parse(JSON.stringify(x))via both direct (JSON.parse) and computed (JSON["parse"]) member access. The stringify-side match requires exactly one argument, intentionally excluding replacer/indent variants that change round-trip semantics. Reports with an auto-fixable suggestion (hasSuggestions: true) that replaces the full expression withstructuredClone(x).eslint-factory/src/rules/prefer-structured-clone.test.tsRuleTester-based test suite (83 lines).Test coverage
JSON.parse/JSON.stringifycalls, existingstructuredClone()usage,JSON.parse(JSON.stringify(obj, null, 2))(replacer/indent argument),JSON.parse(JSON.stringify(obj), reviver)(custom reviver).JSON.parse(JSON.stringify(tool))→structuredClone(tool); the same pattern inside an arrow-function/.map()callback; computed member accessJSON["parse"](JSON["stringify"](tool))→structuredClone(tool).Impact
New lint rule addition only — no breaking changes. Existing code is not modified beyond wiring the new rule into the plugin config; violations will surface as
warn-level lint findings for future review/fix.