Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/skills/agentic-workflows/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Load these files from `github/gh-aw` (they are not available locally).
- `.github/aw/debug-agentic-workflow.md`
- `.github/aw/dependabot.md`
- `.github/aw/deployment-status.md`
- `.github/aw/designer-mappings.md`
- `.github/aw/designer.md`
- `.github/aw/evals.md`
- `.github/aw/experiments.md`
Expand Down Expand Up @@ -100,4 +101,3 @@ After loading the matching workflow prompt or skill, follow it directly:
- Design long-running multi-agent research workflows: `.github/aw/multi-agent-research.md`

When the task involves OTEL, OTLP, traces, observability backends, or telemetry-driven analysis, also read and follow `skills/otel-queries/SKILL.md` after loading the matching workflow prompt or skill.

52 changes: 52 additions & 0 deletions eslint-factory/src/rules/prefer-structured-clone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,56 @@ describe("prefer-structured-clone", () => {
],
});
});

it("invalid: no suggestion when the cloned identifier carries function-valued properties", () => {
cjsRuleTester.run("prefer-structured-clone", preferStructuredCloneRule, {
valid: [],
invalid: [
{
// Shape of actions/setup/js/safe_outputs_tools_loader.cjs: the JSON round-trip
// intentionally drops `tool.handler`, which is re-attached afterwards.
// structuredClone would throw DataCloneError here.
code: [
`function attachHandlers(tools, handlers) {`,
` tools.forEach(tool => {`,
` tool.handler = args => handlers.defaultHandler(args);`,
` });`,
`}`,
`function register(tool) {`,
` const toolToRegister = JSON.parse(JSON.stringify(tool));`,
` if (tool.handler) {`,
` toolToRegister.handler = tool.handler;`,
` }`,
` return toolToRegister;`,
`}`,
].join("\n"),
errors: [{ messageId: "preferStructuredClone", suggestions: [] }],
},
{
code: [`function register(tool) {`, ` if (typeof tool.handler === "function") {`, ` }`, ` return JSON.parse(JSON.stringify(tool));`, `}`].join("\n"),
errors: [{ messageId: "preferStructuredClone", suggestions: [] }],
},
{
code: [`const config = { handler: () => {} };`, `const clone = JSON.parse(JSON.stringify(config));`].join("\n"),
errors: [{ messageId: "preferStructuredClone", suggestions: [] }],
},
{
// JSON-sourced tool (actions/setup/js/generate_safe_outputs_tools.cjs shape):
// no function-valued property anywhere, so the suggestion is still offered.
code: [`const tools = JSON.parse(readFileSync(path, "utf8"));`, `for (const tool of tools) {`, ` const enhancedTool = JSON.parse(JSON.stringify(tool));`, `}`].join("\n"),
errors: [
{
messageId: "preferStructuredClone",
suggestions: [
{
messageId: "replaceWithStructuredClone",
output: [`const tools = JSON.parse(readFileSync(path, "utf8"));`, `for (const tool of tools) {`, ` const enhancedTool = structuredClone(tool);`, `}`].join("\n"),
},
],
},
],
},
],
});
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The guard at prefer-structured-clone.ts:119 only suppresses suggestions when clonedExpression is an Identifier — non-identifier expressions (e.g. JSON.parse(JSON.stringify(obj.sub))) always get the suggestion. A test pinning this boundary would prevent a future regression if the guard logic is widened.

💡 Suggested test case to add inside this `it` block
{
  // member-expression clone: guard does NOT apply even though `tool` carries a function property
  code: [
    `function register(tool) {`,
    `  tool.handler = () => {};`,
    `  return JSON.parse(JSON.stringify(tool.data));`,
    `}`,
  ].join('\n'),
  errors: [{
    messageId: 'preferStructuredClone',
    suggestions: [{ messageId: 'replaceWithStructuredClone', output: ... }],
  }],
},

@copilot please address this.

90 changes: 75 additions & 15 deletions eslint-factory/src/rules/prefer-structured-clone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,31 @@ function isPlainJsonStringifyCall(node: TSESTree.CallExpression): boolean {
return node.arguments.length === 1;
}

/**
* Returns the property name of a non-computed member expression, or a string literal
* computed access, and undefined otherwise.
*/
function getStaticPropertyName(node: TSESTree.MemberExpression): string | undefined {
if (!node.computed && node.property.type === AST_NODE_TYPES.Identifier) return node.property.name;
if (node.computed && node.property.type === AST_NODE_TYPES.Literal && typeof node.property.value === "string") return node.property.value;
return undefined;
}

/**
* Returns true when the expression is a function literal (`function () {}` or `() => {}`).
*/
function isFunctionLiteral(node: TSESTree.Node): boolean {
return node.type === AST_NODE_TYPES.FunctionExpression || node.type === AST_NODE_TYPES.ArrowFunctionExpression;
}

export const preferStructuredCloneRule = createRule({
name: "prefer-structured-clone",
meta: {
type: "suggestion",
hasSuggestions: true,
docs: {
description:
"Prefer structuredClone(...) over JSON.parse(JSON.stringify(...)) for deep-cloning plain data in actions/setup/js scripts. The JSON round-trip is slower, silently drops values it cannot represent (undefined, functions, Date becomes a string), and throws on circular references, whereas structuredClone (Node >=17, available globally in the Node 24 runtime this action targets) clones plain objects and JSON-safe data directly.",
'Prefer structuredClone(...) over JSON.parse(JSON.stringify(...)) for deep-cloning plain data in actions/setup/js scripts. The JSON round-trip is slower, silently drops values it cannot represent (undefined, functions, Date becomes a string), and throws on circular references, whereas structuredClone (Node >=17, available globally in the Node 24 runtime this action targets) clones plain objects and JSON-safe data directly. The autofix suggestion assumes the cloned value never carries function-valued properties: JSON.stringify silently drops functions while structuredClone throws DataCloneError, so code that intentionally relies on the drop-and-reattach idiom must not be rewritten. The suggestion is therefore withheld when the cloned expression is an identifier whose properties are assigned function literals (or checked with `typeof x.prop === "function"`) anywhere in the same file; the diagnostic is still reported so the round trip can be reviewed by hand.',
},
schema: [],
messages: {
Expand All @@ -50,7 +67,41 @@ export const preferStructuredCloneRule = createRule({
create(context) {
const sourceCode = context.sourceCode;

// Identifiers observed carrying function-valued properties somewhere in the file.
// Cloning those with structuredClone would throw DataCloneError, so the suggestion
// is withheld for them (the diagnostic is still reported).
const identifiersWithFunctionProperties = new Set<string>();
const candidates: { node: TSESTree.CallExpression; clonedExpression: TSESTree.Node }[] = [];

return {
AssignmentExpression(node) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The AssignmentExpression visitor only detects direct identifier targets (x.prop = () => {}). Aliased or destructured parameters like const t = tool; t.handler = () => {} bypass the guard. This is an inherent heuristic limitation — consider adding a code comment acknowledging it so future contributors don't try to "fix" the check into something more complex than the risk warrants.

💡 Suggested comment
// Heuristic: only tracks direct identifier targets (x.prop = fn).
// Aliased or destructured variables (const t = tool; t.prop = fn) are not tracked.
// This is intentional — over-suppression is preferable to offering an unsafe suggestion.

@copilot please address this.

if (node.left.type !== AST_NODE_TYPES.MemberExpression) return;
if (node.left.object.type !== AST_NODE_TYPES.Identifier) return;
if (!isFunctionLiteral(node.right)) return;
identifiersWithFunctionProperties.add(node.left.object.name);
},

VariableDeclarator(node) {
// `const x = { handler: () => {} }` also carries a function-valued property.
if (node.id.type !== AST_NODE_TYPES.Identifier) return;
if (!node.init || node.init.type !== AST_NODE_TYPES.ObjectExpression) return;
const hasFunctionProperty = node.init.properties.some(property => property.type === AST_NODE_TYPES.Property && (isFunctionLiteral(property.value) || property.method));
if (hasFunctionProperty) identifiersWithFunctionProperties.add(node.id.name);
},

BinaryExpression(node) {
// `typeof x.prop === "function"` is strong evidence that `x` carries a function property.
if (node.operator !== "===" && node.operator !== "==" && node.operator !== "!==" && node.operator !== "!=") return;
const [typeofSide, literalSide] = node.left.type === AST_NODE_TYPES.UnaryExpression ? [node.left, node.right] : [node.right, node.left];
if (typeofSide.type !== AST_NODE_TYPES.UnaryExpression || typeofSide.operator !== "typeof") return;
if (literalSide.type !== AST_NODE_TYPES.Literal || literalSide.value !== "function") return;
const argument = typeofSide.argument;
if (argument.type !== AST_NODE_TYPES.MemberExpression) return;
if (argument.object.type !== AST_NODE_TYPES.Identifier) return;
if (getStaticPropertyName(argument) === undefined) return;
identifiersWithFunctionProperties.add(argument.object.name);
},

CallExpression(node) {
if (!isJsonParseCall(node)) return;
if (node.arguments.length !== 1) return;
Expand All @@ -59,21 +110,30 @@ export const preferStructuredCloneRule = createRule({
if (innerArg.type !== AST_NODE_TYPES.CallExpression) return;
if (!isPlainJsonStringifyCall(innerArg)) return;

const clonedExpressionText = sourceCode.getText(innerArg.arguments[0]);
candidates.push({ node, clonedExpression: innerArg.arguments[0] });
},

"Program:exit"() {
for (const { node, clonedExpression } of candidates) {
const clonedExpressionText = sourceCode.getText(clonedExpression);
const carriesFunctionProperties = clonedExpression.type === AST_NODE_TYPES.Identifier && identifiersWithFunctionProperties.has(clonedExpression.name);

context.report({
node,
messageId: "preferStructuredClone",
data: { arg: clonedExpressionText },
suggest: [
{
messageId: "replaceWithStructuredClone",
fix(fixer: TSESLint.RuleFixer) {
return fixer.replaceText(node, `structuredClone(${clonedExpressionText})`);
},
},
],
});
context.report({
node,
messageId: "preferStructuredClone",
data: { arg: clonedExpressionText },
suggest: carriesFunctionProperties
Comment on lines +121 to +125
? []
: [
{
messageId: "replaceWithStructuredClone",
fix(fixer: TSESLint.RuleFixer) {
return fixer.replaceText(node, `structuredClone(${clonedExpressionText})`);
},
},
],
});
}
},
};
},
Expand Down
Loading