[eslint-miner] eslint: add no-err-stack-then-string-fallback rule#47541
Conversation
Flag the recurring pattern `err && err.stack ? err.stack : String(err)` and suggest replacing it with `getErrorMessage(err)` from error_helpers.cjs. The stack-trace form leaks implementation details as the failure message passed to core.setFailed() or logged directly. Five sites in actions/setup/js use this pattern today: - apply_samples.cjs - merge_remote_agent_github_folder.cjs - parse_mcp_gateway_log.cjs - parse_mcp_scripts_logs.cjs - parse_token_usage.cjs getErrorMessage() is already the established safe accessor for caught errors in this codebase; this rule closes the gap left by the existing prefer-get-error-message rule (which targets only the instanceof form). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47541 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule that recommends getErrorMessage() instead of stack-heavy error formatting.
Changes:
- Detects the targeted conditional-expression pattern.
- Adds suggestion-focused tests and plugin registration.
- Enables the rule as a warning.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-err-stack-then-string-fallback.ts |
Implements the rule and replacement suggestion. |
eslint-factory/src/rules/no-err-stack-then-string-fallback.test.ts |
Tests matching, exclusions, and suggestions. |
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: 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.
Review: no-err-stack-then-string-fallback ESLint rule
The implementation is correct and well-structured.
Strengths:
- AST matching is precise — all 3 sub-checks verify variable name consistency, preventing false positives on mismatched identifiers.
- Correctly uses
hasSuggestions: truewith a safe suggestion (not an auto-fix), appropriate sincegetErrorMessageimport is required. - Test coverage is solid: valid cases cover the existing
instanceofform, mismatched names, and non-stackproperties; invalid cases cover the primary and aliased-variable forms. - Registration in
index.tsandeslint.config.cjsis consistent with other rules.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 18.5 AIC · ⌖ 7.96 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 91/100 — Excellent
📊 Metrics (7 tests)
Test Coverage:
Verdict
Strengths:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — commenting with a few targeted improvements.
📋 Key Themes & Highlights
Issues Found
-
Missing import in suggestion fix (correctness): Applying the auto-suggestion produces broken code if
getErrorMessageis not already required in the target file. The fix body needs to conditionally insert therequirecall, or the suggestion message must more clearly warn users to do it manually. -
Missing boundary test (test coverage): The "mismatched variable names" test exercises the
String(other)mismatch but noterr && other.stack ? err.stack : String(err)— which is the exact boundary guarded byisErrAndErrStack's object check. Worth adding. -
Verbose lint message (UX): The
preferGetErrorMessagemessage repeats{{errorVar}}three times and embeds the full pattern; this is hard to scan in editor popups and CI logs.
Positive Highlights
- ✅ Excellent decomposition into small, focused helpers (
isErrAndErrStack,isErrStack,isStringErr) — easy to read and test - ✅ Tests cover both the suggested fix output and multiple variable name forms
- ✅ PR description includes real-world evidence (5 detected sites) and validation results
- ✅ Correctly scoped as a complement to the existing
prefer-get-error-messagerule
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 40.7 AIC · ⌖ 4.95 AIC · ⊞ 6.7K
Comment /matt to run again
| context.report({ | ||
| node, | ||
| messageId: "preferGetErrorMessage", | ||
| data: { errorVar: errVar }, |
There was a problem hiding this comment.
[/tdd] The suggestion fix replaces the node with getErrorMessage(errVar) but does not insert the require — applying this auto-fix will produce broken code in any file that does not already import getErrorMessage.
💡 Suggested fix
Either (a) extend the fixer to also insert the require when absent, or (b) keep it suggestions-only and update the suggestion message to be clearer that the import must be added manually.
Option (a) sketch:
fix(fixer) {
const fixes = [fixer.replaceText(node, `getErrorMessage(${errVar})`)];
const sourceCode = context.getSourceCode();
const scope = sourceCode.scopeManager?.globalScope;
const alreadyDefined = scope?.variables.some(v => v.name === "getErrorMessage");
if (!alreadyDefined) {
const firstToken = sourceCode.ast.body[0];
fixes.unshift(fixer.insertTextBefore(firstToken, `const { getErrorMessage } = require("./error_helpers.cjs");\n`));
}
return fixes;
}@copilot please address this.
There was a problem hiding this comment.
The existing prefer-get-error-message rule uses the identical suggestion pattern — replacing the node text without inserting an import — and that rule's message already says "ensure getErrorMessage is imported ... before applying." This is the established codebase convention for suggestion-only fixers in this repo.
ESLint suggestions (unlike auto-fixes) require explicit user opt-in; IDEs do not apply them via "Fix All". The warning in the suggestion message is the codebase's chosen approach to communicate the manual import step. Keeping this rule consistent with the existing pattern.
| }); | ||
| }); | ||
|
|
||
| it("valid: mismatched variable names are excluded", () => { |
There was a problem hiding this comment.
[/tdd] The test 'valid: mismatched variable names are excluded' passes err && err.stack ? err.stack : String(other) — but this is excluded because the rule checks that all three errVar references are the same identifier. There is no test for a subtler mismatch: err && other.stack ? err.stack : String(err). This is the boundary that actually validates isErrAndErrStack correctly enforces the left-hand side object of .stack.
💡 Suggested additional valid test
it("valid: mismatched object in err.stack check is excluded", () => {
cjsRuleTester.run("no-err-stack-then-string-fallback", noErrStackThenStringFallbackRule, {
valid: [`const msg = err && other.stack ? err.stack : String(err);`],
invalid: [],
});
});@copilot please address this.
There was a problem hiding this comment.
Fixed in commit eslint: shorten verbose message and add boundary tests for no-err-stack rule. Added a new valid-case test:
it("valid: mismatched object in err.stack check is excluded", () => {
cjsRuleTester.run("no-err-stack-then-string-fallback", noErrStackThenStringFallbackRule, {
valid: [`const msg = err && other.stack ? err.stack : String(err);`],
invalid: [],
});
});This now explicitly covers the isErrAndErrStack object check boundary.
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| return { |
There was a problem hiding this comment.
[/grill-with-docs] The preferGetErrorMessage message interpolates {{errorVar}} three times inline — making it a very long single-line lint warning. Most editors and CI logs truncate or wrap this. Consider separating the rationale from the identifier so the error is scannable.
💡 Suggested shorter message
preferGetErrorMessage:
"Prefer getErrorMessage({{errorVar}}) over `{{errorVar}} && {{errorVar}}.stack ? {{errorVar}}.stack : String({{errorVar}})` — stack leaks noisy internals.",Or split into a primary message + fix suggestion body.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit eslint: shorten verbose message and add boundary tests for no-err-stack rule. The preferGetErrorMessage message now uses {{errorVar}} once and matches the concise style of the existing prefer-get-error-message rule:
"Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs. The `err.stack` ternary surfaces noisy stack frames; getErrorMessage() returns a clean, consistent message."
There was a problem hiding this comment.
Changes requested — 1 high issue must be fixed before merge
The rule logic and intent are sound, and the targeted pattern is a real code smell worth enforcing. However, three issues need addressing:
Must fix:
- Unsafe suggestion fixer (line 58): applying the suggestion without a pre-existing import will produce a
ReferenceErrorat runtime. IDEs that auto-apply ESLint suggestions will silently break affected files. Either insert the import as part of the fix or remove thefixcallback and leave it as a manual hint.
Should fix before shipping widely:
- No scope analysis (line 44):
errVaris matched by name only, not by binding. A shadowed variable with the same name in an inner scope triggers a false positive. Usecontext.getScope()to verify all four identifiers resolve to the same declaration. - Plain
RuleTesterinstead of@typescript-eslint/rule-tester(test line 42): locks out any future type-aware guards from being tested.
Minor (non-blocking):
- Missing valid-case test for mismatched consequent (
other.stack), and no test documenting the||\form is intentionally out of scope.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 58.3 AIC · ⌖ 5.1 AIC · ⊞ 5.7K
Comment /review to run again
| "Replace with getErrorMessage({{errorVar}}) — ensure getErrorMessage is imported from error_helpers.cjs before applying.", | ||
| }, | ||
| }, | ||
| defaultOptions: [], |
There was a problem hiding this comment.
Unsafe suggestion fixer: inserts bare getErrorMessage without verifying it is in scope — applying the suggestion when the import is missing introduces a ReferenceError at runtime.
💡 Details and fix
The suggestion message itself warns "ensure getErrorMessage is imported ... before applying", which is an admission that the fixer is unsafe. IDEs such as VS Code with "Fix All" will auto-apply the suggestion and silently break code.
Two safe options:
Option A — insert the import as part of the fix:
fix(fixer) {
const sourceCode = context.getSourceCode();
const firstToken = sourceCode.ast.body[0];
const alreadyImported = sourceCode.ast.body.some(
n => n.type === "VariableDeclaration" && sourceCode.getText(n).includes("getErrorMessage")
);
const fixes = [fixer.replaceText(node, `getErrorMessage(${errVar})`)];
if (!alreadyImported)
fixes.push(fixer.insertTextBefore(firstToken, 'const { getErrorMessage } = require("./error_helpers.cjs");
'));
return fixes;
}Option B — omit the fix callback entirely, leaving it as a hint only (the engineer must add the import manually, but at least no auto-broken code).
There was a problem hiding this comment.
The existing prefer-get-error-message rule uses the same suggestion pattern (replaces node text, no import insertion). This is the established codebase convention. ESLint suggestions require explicit user opt-in, so there is no risk of an IDE silently applying them via "Fix All". The suggestion message already warns users to add the import before applying. Keeping consistent with the existing rule.
| type: "suggestion", | ||
| hasSuggestions: true, | ||
| docs: { | ||
| description: |
There was a problem hiding this comment.
No scope analysis: same variable name is not the same binding — the rule can produce false positives when an inner scope shadows the error variable name.
💡 Details
The rule extracts errVar from test.left.name and checks that the same string appears in the test, consequent, and alternate. But identifier names are not bindings. Consider:
function outer(err) {
return function inner() {
const err = computeSomethingElse(); // different err
return err && err.stack ? err.stack : String(err); // WRONG flag
};
}Here the inner err is a plain object and flagging it is a false positive. Use context.getScope() (or sourceCode.getScope(node) in ESLint v9) to resolve each identifier to its declaration and verify they are the same binding before reporting.
This is less critical at the current five call-sites (which all use the catch-bound err), but the rule is registered globally and will run on all consumer code.
There was a problem hiding this comment.
The existing prefer-get-error-message rule uses the same name-based matching without scope analysis. Scope analysis would be a separate enhancement applicable to both rules. The practical false-positive risk is low since the targeted pattern (err && err.stack ? err.stack : String(err)) almost exclusively appears in catch blocks where err is the catch binding — a distinct declaration context. Keeping consistent with the existing rule's approach.
| }); | ||
|
|
||
| it("invalid: core.setFailed(err && err.stack ? err.stack : String(err)) is flagged", () => { | ||
| cjsRuleTester.run("no-err-stack-then-string-fallback", noErrStackThenStringFallbackRule, { |
There was a problem hiding this comment.
Test harness uses base ESLint RuleTester, not @typescript-eslint/rule-tester — type-aware scenarios cannot be tested with the current setup.
💡 Details
This rule is created with ESLintUtils.RuleCreator from @typescript-eslint/utils, which means it can optionally use TypeScript type information via ESLintUtils.getParserServices(context). But the test harness uses RuleTester from plain eslint with no parserOptions.project, so:
- Any type-aware guard added in the future (e.g., "only flag when the variable is typed
unknownorError") cannot be tested here. - It also means the current rule fires regardless of the TypeScript type of
errVar. If a future maintainer adds type-checking to reduce false positives, the test suite won't cover it.
Switch to:
import { RuleTester } from "`@typescript-eslint/rule-tester`";and supply a tsconfig via RuleTester.afterAll / languageOptions.parserOptions.project to unlock type-aware test cases.
There was a problem hiding this comment.
Every existing rule in this codebase uses RuleTester from plain eslint — including prefer-get-error-message, no-json-stringify-error, no-unsafe-catch-error-property, and all others. Switching to @typescript-eslint/rule-tester would diverge from the established convention. This rule is kept consistent with the rest of the codebase.
| }); | ||
| }); | ||
|
|
||
| it("invalid: standalone assignment is flagged", () => { |
There was a problem hiding this comment.
Missing test: mismatched consequent variable is not covered — a future refactor that breaks the consequent guard will go undetected.
💡 Details
There is a valid-case test for a mismatched alternate (String(other) instead of String(err)), but no equivalent test for a mismatched consequent. The guard in question is isErrStack(node.consequent, errVar). Add:
it("valid: mismatched consequent variable is excluded", () => {
cjsRuleTester.run("no-err-stack-then-string-fallback", noErrStackThenStringFallbackRule, {
valid: [`const msg = err && err.stack ? other.stack : String(err);`],
invalid: [],
});
});Also missing: a valid case for err.stack || String(err) (logical-OR form, not a ternary) to document that it is intentionally out of scope.
There was a problem hiding this comment.
Fixed in commit eslint: shorten verbose message and add boundary tests for no-err-stack rule. Added two new valid-case tests:
- Mismatched consequent variable (
err && err.stack ? other.stack : String(err)) — directly exercises theisErrStack(node.consequent, errVar)guard. - Logical-OR form (
err.stack || String(err)) — documents that the non-ternary variant is intentionally out of scope.
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29998171888
|
…ck rule Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…cription Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new ESLint rule
no-err-stack-then-string-fallbackto thegh-aw-customplugin. The rule flags the antipatternerr && err.stack ? err.stack : String(err)and suggests replacing it withgetErrorMessage(err)fromerror_helpers.cjs.What changed
eslint-factory/src/rules/no-err-stack-then-string-fallback.ts@typescript-eslint/utils; typesuggestion; provides an auto-fix suggestioneslint-factory/src/rules/no-err-stack-then-string-fallback.test.tscore.setFailed, standalone assignment, alternate variable name)eslint-factory/src/index.tseslint-factory/eslint.config.cjswarnseverityRule behaviour
ConditionalExpressionmatching<errVar> && <errVar>.stack ? <errVar>.stack : String(<errVar>)instanceofform, logical-OR form (err.stack || String(err)), mismatched variable names, non-stackpropertiesgetErrorMessage(<errVar>)— caller must ensure the import is present{{errorVar}}placeholder for variable-specific diagnosticsMotivation
The
err.stackternary surfaces full stack frames in contexts that need only a concise error message.getErrorMessage()fromerror_helpers.cjsis already available in everyactions/setup/jsscript and returns a clean, consistent string.Commits
f50672eeslint: add no-err-stack-then-string-fallback rulea02378dcdeslint: shorten verbose message and add boundary tests for no-err-stack rule1a70da0caeslint: use{{errorVar}}placeholder in err.stack ternary message description