Skip to content

Make unsafe prefer-core-logging rewrites report-only #46588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 19, 2026
Merged

Make unsafe prefer-core-logging rewrites report-only #46588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c49d173
Initial plan
Copilot Jul 19, 2026
419369f
Make unsafe prefer-core-logging rewrites report-only
Copilot Jul 19, 2026
72e18eb
Prevent lossy prefer-core-logging suggestions
Copilot Jul 19, 2026
056d1c5
Merge branch 'main' into copilot/eslint-factory-fix-argument-loss
pelikhan Jul 19, 2026
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
50 changes: 49 additions & 1 deletion eslint-factory/src/rules/prefer-core-logging.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ describe("prefer-core-logging", () => {
},
],
},
{
code: "console.log(`hello`);",
errors: [
{
messageId: "preferCoreLogging",
data: { method: "log", replacement: "core.info" },
suggestions: [{ messageId: "replaceWithCoreMethod", data: { replacement: "core.info", args: "`hello`" }, output: "core.info(`hello`);" }],
},
],
},
],
});
});
Expand Down Expand Up @@ -162,7 +172,45 @@ describe("prefer-core-logging", () => {
{
messageId: "preferCoreLogging",
data: { method: "log", replacement: "core.info" },
suggestions: [{ messageId: "replaceWithCoreMethod", data: { replacement: "core.info", args: `"value:", someVar` }, output: `const core = require("@actions/core"); const someVar = 1; core.info("value:", someVar);` }],
suggestions: [],
},
],
},
],
});
});

it("invalid: console.log with format specifier is report-only", () => {
ruleTester.run("prefer-core-logging", preferCoreLoggingRule, {
valid: [],
invalid: [
{
code: `const core = require("@actions/core"); console.log("%s processed");`,
errors: [
{
messageId: "preferCoreLogging",
data: { method: "log", replacement: "core.info" },
suggestions: [],
},
],
},
{
code: `const core = require("@actions/core"); const someVar = 1; console.log("value: %s", someVar);`,
errors: [
{
messageId: "preferCoreLogging",

@github-actions github-actions Bot Jul 19, 2026

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] Missing test for zero-argument console.log() — this is a valid call that canSuggestCoreReplacement should classify as report-only (since node.arguments.length !== 1). Without a test, a future refactor could accidentally start suggesting core.info() for it.

💡 Suggested test 
{
  code: `console.log();`,
  errors: [
    {
      messageId: "preferCoreLogging",
      data: { method: "log", replacement: "core.info" },
      suggestions: [],
    },
  ],
},

Add this to the "report-only" invalid test suite.

@copilot please address this.

data: { method: "log", replacement: "core.info" },
suggestions: [],
},
],
},
{
code: 'const core = require("@actions/core"); const someVar = 1; console.log(`value: ${someVar}`);',
errors: [
{
messageId: "preferCoreLogging",
data: { method: "log", replacement: "core.info" },
suggestions: [],
},
],
},
Expand Down
78 changes: 65 additions & 13 deletions eslint-factory/src/rules/prefer-core-logging.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils";
import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);

Expand Down Expand Up @@ -30,6 +30,58 @@ function getConsoleMethod(node: TSESTree.CallExpression): string | null {
return prop.name in CONSOLE_TO_CORE ? prop.name : null;
}

function getStaticStringValue(node: TSESTree.CallExpressionArgument): string | null {
if (node.type === AST_NODE_TYPES.Literal) {
return typeof node.value === "string" ? node.value : null;
}

if (node.type !== AST_NODE_TYPES.TemplateLiteral) {
return null;
}

if (node.expressions.length > 0) {
return null;
}

const parts: string[] = [];
for (const quasi of node.quasis) {
if (quasi.value.cooked === null) {
return null;
}
parts.push(quasi.value.cooked);
}

@github-actions github-actions Bot Jul 19, 2026

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 format specifier set "sdifojO" is missing %c (CSS styling), which is a valid Node.js console format specifier handled by util.format. Without it, console.log("%c bold", "font-weight:bold") would incorrectly receive a suggestion.

💡 Suggested fix 
if (value[i] === "%" && value[i - 1] !== "%" && "sdifojOc".includes(value[i + 1] ?? "")) {

Add c to the character set. While CSS styling is less common in CI/Node contexts, being conservative here aligns with the PR's stated goal of never losing runtime semantics.

@copilot please address this.


return parts.join("");
}

function hasConsoleFormatSpecifier(node: TSESTree.CallExpressionArgument | undefined): boolean {
const value = node ? getStaticStringValue(node) : null;

@github-actions github-actions Bot Jul 19, 2026

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] canSuggestCoreReplacement only guards against single-argument calls via node.arguments.length !== 1, but it never validates that the single argument is a SpreadElement. console.log(...args) has arguments.length === 1 yet the argument is a spread — the replacement core.info(...args) would still change runtime behaviour.

💡 Suggested fix 
function canSuggestCoreReplacement(node: TSESTree.CallExpression): boolean {
  const arg = node.arguments[0];
  if (node.arguments.length !== 1 || !arg) return false;
  if (arg.type === AST_NODE_TYPES.SpreadElement) return false; // single spread still multi-arg at runtime

  return !isInterpolatedTemplateLiteral(arg) && !hasConsoleFormatSpecifier(arg);
}

This is related to the existing Copilot comment on this PR but is the minimal fix that closes the gap.

@copilot please address this.

if (value === null) {
return false;
}

for (let i = 0; i < value.length - 1; i++) {
if (value[i] === "%" && value[i - 1] !== "%" && "sdifojO".includes(value[i + 1] ?? "")) {
return true;
}
}

return false;
}

function isInterpolatedTemplateLiteral(node: TSESTree.CallExpressionArgument): boolean {
return node.type === AST_NODE_TYPES.TemplateLiteral && node.expressions.length > 0;
}

function canSuggestCoreReplacement(node: TSESTree.CallExpression): boolean {
const arg = node.arguments[0];
if (node.arguments.length !== 1 || !arg) {
return false;
}
Comment on lines +78 to +80

return !isInterpolatedTemplateLiteral(arg) && !hasConsoleFormatSpecifier(arg);
}

export const preferCoreLoggingRule = createRule({
name: "prefer-core-logging",
meta: {
Expand Down Expand Up @@ -57,23 +109,23 @@ export const preferCoreLoggingRule = createRule({
if (!method) return;

const replacement = CONSOLE_TO_CORE[method]!;

// Build replacement argument text from original call
const argsText = node.arguments.map(arg => sourceCode.getText(arg)).join(", ");
const suggest = canSuggestCoreReplacement(node)
? [
{
messageId: "replaceWithCoreMethod" as const,
data: { replacement, args: sourceCode.getText(node.arguments[0]) },
fix(fixer: TSESLint.RuleFixer) {
return fixer.replaceText(node, `${replacement}(${sourceCode.getText(node.arguments[0])})`);
},
},
]
: undefined;

context.report({
node,
messageId: "preferCoreLogging",
data: { method, replacement },
suggest: [
{
messageId: "replaceWithCoreMethod",
data: { replacement, args: argsText },
fix(fixer) {
return fixer.replaceText(node, `${replacement}(${argsText})`);
},
},
],
suggest,
});
},
};
Expand Down
Loading