fix(eslint-factory): prefer-core-logging suggestion unsafe for non-string arguments#47020
Conversation
…t for static strings
Only attach the `replaceWithCoreMethod` suggestion when the single argument
is statically known to be a string: a string Literal or a TemplateLiteral
with no expressions. Identifiers, object literals, numeric literals, and
other non-string nodes are now reported without a suggestion, because
`core.info` coerces non-strings via string coercion ("[object Object]")
while `console.log` uses `util.inspect`, degrading diagnostic output.
Also remove the now-unused `isInterpolatedTemplateLiteral` helper (the
new logic re-uses `getStaticStringValue` which already handles the
no-expression-template case).
Tests updated: the existing identifier test case (`console.log(foo)`)
is corrected to expect no suggestion, and a new test block asserts
report-only behaviour for identifiers, object literals, and numeric
literals.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0). |
There was a problem hiding this comment.
Pull request overview
Restricts prefer-core-logging suggestions to statically known strings, preventing unsafe non-string replacements.
Changes:
- Gates suggestions using
getStaticStringValue. - Removes the redundant template-literal helper.
- Adds regression coverage for non-string arguments.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/prefer-core-logging.ts |
Limits replacement suggestions to static strings. |
eslint-factory/src/rules/prefer-core-logging.test.ts |
Verifies non-string calls remain report-only. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly restricts the replaceWithCoreMethod suggestion to statically-known string arguments by gating on getStaticStringValue(arg) !== null. The removed isInterpolatedTemplateLiteral helper is cleanly superseded. Tests cover all key cases (identifier, object literal, numeric literal, direct number). No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 15.1 AIC · ⌖ 4.34 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (13 tests)
📋 Test Breakdown| Test Case | Classification | Coverage | 🔍 Edge Cases & AssertionsString arguments (autofix enabled):
Non-string arguments (report-only, no autofix):
Safe exemptions:
Test modifications in this PR:
Verdict✅ PASS. 85% design-driven tests (threshold: ≤30% implementation). Strengths:
Rationale for 88/100:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor cohesion note.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: gates the suggestion on
getStaticStringValue(arg) !== null, fixing the semantic mismatch betweenconsole.log(util.inspect) andcore.info(string coercion). - Dead code removed:
isInterpolatedTemplateLiteralis cleanly removed; its logic is already subsumed bygetStaticStringValue. - Existing test corrected:
console.log(foo)was wrongly expecting a suggestion — now correctly report-only. - New test coverage: four new cases cover identifier, object literal, numeric identifier, and numeric literal — all report-only.
Positive Highlights
- ✅ Fix is minimal and surgical — no unrelated changes.
- ✅ Comment in
canSuggestCoreReplacementclearly explains the coercion asymmetry. - ✅ Test coverage is thorough for the changed behaviour.
Minor Note
One inline comment on test structure cohesion — fully optional, does not block merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 32.5 AIC · ⌖ 4.52 AIC · ⊞ 6.6K
Comment /matt to run again
| }); | ||
| }); | ||
|
|
||
| it("invalid: console.log with non-string single argument is report-only (no autofix suggestion)", () => { |
There was a problem hiding this comment.
[/tdd] The new non-string test cases use a standalone it/ruleTester.run block, while existing invalid cases share a common block. Coverage is correct — this is a minor cohesion note only.\n\n
💡 Optional refactor
\n\nConsider merging the four new cases into the existinginvalid: block that already owns the single-identifier story (line 32). That keeps all "argument type" cases together and makes the test file easier to scan at a glance.\n\nFully optional — the current layout is readable.\n\nThere was a problem hiding this comment.
Kept this as-is. I considered merging those cases into the earlier block, but kept the dedicated non-string regression block to make the safety boundary explicit in one place.
There was a problem hiding this comment.
✅ Test Quality Sentinel: 88/100 APPROVED.
85% design-driven tests (threshold: ≤30% implementation). New test block directly addresses the safety invariant: autofix suggestions only for statically-known strings. All 4 non-string argument types covered (identifier, object literal, numeric variable, numeric literal). No violations.
There was a problem hiding this comment.
The fix is correct — gating suggestions on getStaticStringValue properly prevents silent output degradation when console.log(obj) would be replaced with core.info(obj). Two non-blocking issues worth fixing before merge:
### Findings
-
Redundant parse (
prefer-core-logging.tsline 86) —getStaticStringValueis called twice for the same node on the valid-string path (once incanSuggestCoreReplacement, then again insidehasConsoleFormatSpecifier). RefactoringhasConsoleFormatSpecifierto accept astringand passing the already-computed value in eliminates the redundancy. -
Missing positive test coverage (
prefer-core-logging.test.tsline 37) — the PR removes the only test that verified a string-literal argument produces a correct suggestion with the right output text. A regression that breaks the entire suggestion path would now go undetected. A singleconsole.log("hello")→core.info("hello")test case with full suggestion output would restore this coverage.
🔎 Code quality review by PR Code Quality Reviewer · 38.1 AIC · ⌖ 4.71 AIC · ⊞ 5.6K
Comment /review to run again
| return false; | ||
| } | ||
|
|
||
| return !hasConsoleFormatSpecifier(arg); |
There was a problem hiding this comment.
getStaticStringValue is called twice for the same node on every valid-string path.
💡 Details
canSuggestCoreReplacement (line 82) calls getStaticStringValue(arg) to gate the suggestion, then falls through to hasConsoleFormatSpecifier(arg) (line 86), which calls getStaticStringValue(arg) again internally (line 58). The value is parsed twice on every string-argument path.
Simpler and cheaper — pass the already-computed value forward:
function hasConsoleFormatSpecifier(value: string): boolean {
for (let i = 0; i < value.length - 1; i++) { ... }
return false;
}
function canSuggestCoreReplacement(node: TSESTree.CallExpression): boolean {
const arg = node.arguments[0];
if (node.arguments.length !== 1 || !arg) return false;
const value = getStaticStringValue(arg);
if (value === null) return false;
return !hasConsoleFormatSpecifier(value);
}This removes the redundant parse and makes the data flow obvious.
There was a problem hiding this comment.
Fixed in 813b76d. canSuggestCoreReplacement now computes getStaticStringValue(arg) once and passes the string value to hasConsoleFormatSpecifier, removing the duplicate parse.
| messageId: "preferCoreLogging", | ||
| data: { method: "log", replacement: "core.info" }, | ||
| suggestions: [{ messageId: "replaceWithCoreMethod", data: { replacement: "core.info", args: `foo` }, output: `const foo = "bar"; core.info(foo);` }], | ||
| suggestions: [], |
There was a problem hiding this comment.
No test covers the autofix happy path — the new suggestions: [] change removes the only positive coverage for the suggestion output.
💡 Details
The original test at line 37 had:
suggestions: [{ messageId: "replaceWithCoreMethod", data: { replacement: "core.info", args: `foo` }, output: `const foo = "bar"; core.info(foo);` }]This was the sole test verifying that a string-literal argument (foo) produces a working autofix with correct output text. Replacing it with suggestions: [] is correct for the fix (the argument foo is an identifier, not a string literal), but it leaves zero tests for the suggestion-present path.
Add a case that exercises a string literal:
{
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");`,
}],
},
],
},Without this, a future regression that breaks suggestion generation entirely would not be caught by tests.
There was a problem hiding this comment.
This is already covered: the existing invalid case for console.log("hello") asserts the replaceWithCoreMethod suggestion and expected autofix output. No additional change needed.
|
@copilot run pr-finisher skill |
…on gate Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/29826418446 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
canSuggestCoreReplacementoffered thereplaceWithCoreMethodautofix for any single-argumentconsole.*call, including identifiers, object literals, and numeric literals. This silently degrades output:console.log(obj)usesutil.inspect, butcore.info(obj)coerces to"[object Object]".Changes
canSuggestCoreReplacement— gates the suggestion ongetStaticStringValue(arg) !== null, restricting it to stringLiteraland no-expressionTemplateLiteralnodes. All other argument types (identifiers, objects, numbers) produce a report with no suggestion.isInterpolatedTemplateLiteralhelper (its logic is subsumed bygetStaticStringValue).console.log(foo)case (was wrongly expecting a suggestion); added a new block covering identifier, object literal, numeric identifier, and direct numeric literal — all report-only.