[eslint-miner] eslint-factory: add no-string-fallback-for-non-string-message rule - #55052
Conversation
Detects the pattern: typeof <chain>.message === "string" ? <chain>.message : String(<container>) where the String() fallback stringifies a different (container) expression than the .message chain under test. When .message exists but is non-string, this silently produces "[object Object]" instead of coercing the message value itself. This mirrors the real bug reported in issue #55014 (error_helpers.cjs getErrorMessage()) and the rule additionally flags 4 live occurrences of the same anti-pattern in actions/setup/js: dispatch_workflow.cjs, log_parser_shared.cjs, route_slash_command.cjs, and safeoutputs_cli.cjs. Registered at warn severity in eslint.config.cjs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for the contribution! This is a well-structured eslint rule with solid test coverage and clear documentation. However, there's an important process issue: 🚫 This PR violates the contribution process — According to our CONTRIBUTING.md, traditional pull requests from non-core team members are not enabled. The project uses agentic development primarily via Copilot coding agents. Instead of opening a PR directly, please:
This process ensures alignment with the project's agentic workflow and helps the team prioritize contributions effectively. Note: The PR content itself looks solid — the rule is well-designed, tests are comprehensive, and the scope is clear. Once you open an issue, the core team can take it from there!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55052 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new rule has a real matcher hole: it only recognizes one specific && operand ordering, so a trivial guard rewrite makes the bug pattern invisible again.
Details
The implementation only descends into node.right for logical && tests. That means the rule misses equivalent conditionals where typeof <chain>.message === "string" is not the rightmost operand, and the current tests do not cover that case. Please make the matcher walk the whole && tree and add a regression test for a reordered guard.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 8.22 AIC · ⌖ 8.83 AIC · ⊞ 7K
Comment /review to run again
| // Support `<guard> && typeof <chain>.message === "string"` by checking the | ||
| // rightmost operand of a right-associated `&&` chain. | ||
| if (node.type === AST_NODE_TYPES.LogicalExpression && node.operator === "&&") { | ||
| return getTypeofMessageCheckChainKey(node.right); |
There was a problem hiding this comment.
This matcher only inspects the right-hand side of an && chain, so it misses the same bug shape when the typeof x.message === "string" check is followed by another guard (for example typeof err.message === "string" && err.message.length > 0 ? ... : String(err)). That makes the rule fragile against trivial reordering and undermines the claim that it prevents this recurring pattern.
💡 Why this matters and how to fix it
Right now getTypeofMessageCheckChainKey() does:
if (node.type === AST_NODE_TYPES.LogicalExpression && node.operator === "&&") {
return getTypeofMessageCheckChainKey(node.right);
}So it only works when the .message typeof check is the rightmost operand. As soon as a developer writes another guard on the right, the rule silently stops matching even though the conditional still has the same semantics.
Please walk both sides of the && tree and return the first matching typeof <chain>.message === "string" predicate instead of assuming a specific operand order. Add a regression test for a shape like:
const m = typeof err.message === "string" && err.message.length > 0
? err.message
: String(err);That keeps the rule resilient to common guard rewrites instead of only one AST layout.
There was a problem hiding this comment.
Review: no-string-fallback-for-non-string-message ESLint rule
The implementation is clean, well-tested, and correctly targets the described anti-pattern.
Highlights:
memberChainKeycorrectly handlesChainExpressionwrappers (optional chaining) and bails out on computed properties — avoiding false positives.getTypeofMessageCheckChainKeycorrectly traverses&&-guarded conditions (e.g.typeof a.b === "object" && typeof a.b.message === "string") by recursing into the right-hand operand.- The final
alternateChainKey === messageChainKeyguard is the right gate to avoid false positives when the fallback already coerces the message itself. - Test cases cover the key valid/invalid scenarios including nested chains, unrelated checks, and the
&&-guard form.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 33.5 AIC · ⌖ 9.04 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule to detect unsafe error-message fallback patterns in setup JavaScript.
Changes:
- Implements and registers the new rule.
- Enables it at warning severity.
- Adds valid and invalid unit cases.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-string-fallback-for-non-string-message.ts |
Implements detection and diagnostics. |
eslint-factory/src/rules/no-string-fallback-for-non-string-message.test.ts |
Tests rule behavior. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| schema: [], | ||
| messages: { | ||
| stringifiesContainerInsteadOfMessage: | ||
| "This falls back to String({{containerExpr}}) instead of coercing {{messageChain}} itself when it exists but isn't a string — risks producing \"[object Object]\". Use String({{messageChain}}) instead.", |
| // Only report when String() is applied to a *different* expression | ||
| // than the `.message` chain (i.e. stringifying the container object). | ||
| if (alternateChainKey === messageChainKey) return; |
| docs: { | ||
| description: | ||
| "Disallow `typeof <x>.message === \"string\" ? <x>.message : String(<container>)` where the String() fallback stringifies a different (container) " + | ||
| "expression than the `.message` chain being tested. When `.message` exists but isn't a string, this pattern silently produces `[object Object]` " + | ||
| "instead of coercing the message value itself (e.g. `String(<x>.message)`). Mirrors the real bug found in error_helpers.cjs's getErrorMessage().", |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with improvement suggestions; no blocking correctness issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps on the
&&-guard path: the valid suite doesn't exercise the&&-unwrapping code path for accepted inputs, leaving a regression window. - Missing
hasSuggestions: the rule knows the exact fix but doesn't expose it; adding it would save every developer who hits this warning from manually deriving the same substitution. meta.type/ severity mismatch:type: "problem"paired with"warn"severity conflicts with ESLint convention;"suggestion"is more accurate if"warn"is the permanent severity.
Positive Highlights
- ✅ Well-scoped, low false-positive rule design — fires only when the
String()argument is structurally distinct from the tested.messagechain. - ✅ Good use of structural key comparison (
memberChainKey) to avoid source-text equality pitfalls. - ✅ Comprehensive invalid test cases mirroring real occurrences in the codebase.
- ✅ Clean integration into plugin exports and
eslint.config.cjs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 43.9 AIC · ⌖ 11.5 AIC · ⊞ 7.6K
Comment /matt to run again
| valid: [ | ||
| // Fallback coerces the message itself, not the container. | ||
| `const m = typeof err.message === "string" ? err.message : String(err.message);`, | ||
| // Nested chain, still coerces the same chain. |
There was a problem hiding this comment.
[/tdd] The &&-guarded path in getTypeofMessageCheckChainKey is exercised only by the third invalid case — the valid suite lacks a matching case with a correct fallback (e.g. typeof x === "object" && typeof x.message === "string" ? x.message : String(x.message)). Without it, the positive arm of the && branch is untested: a regression that accidentally dropped the && unwrapping would still pass the existing tests.
💡 Suggested test addition
Add to the valid array:
// Guarded by leading object-type check, fallback coerces the message itself.
`const m = typeof err === "object" && typeof err.message === "string" ? err.message : String(err.message);`,
// Deeply nested with leading guard, still correct fallback.
`const m = typeof err.response === "object" && typeof err.response.data.message === "string" ? err.response.data.message : String(err.response.data.message);`,This ensures the &&-unwrapping code path is exercised for accepted inputs too, not just flagged ones.
@copilot please address this.
| docs: { | ||
| description: | ||
| "Disallow `typeof <x>.message === \"string\" ? <x>.message : String(<container>)` where the String() fallback stringifies a different (container) " + | ||
| "expression than the `.message` chain being tested. When `.message` exists but isn't a string, this pattern silently produces `[object Object]` " + |
There was a problem hiding this comment.
[/codebase-design] The rule can provide a deterministic fix since the correct substitution is always String(<messageChainKey>), but meta declares neither fixable nor hasSuggestions. Developers hitting this warning must manually derive the fix at each site.
💡 Suggested improvement: add `hasSuggestions`
meta: {
type: "problem",
hasSuggestions: true,
// ...
messages: {
stringifiesContainerInsteadOfMessage: "...",
replaceWithMessageChain:
"Replace {{containerExpr}} with {{messageChain}} to coerce the message value instead.",
},
},And in context.report:
context.report({
node: node.alternate,
messageId: "stringifiesContainerInsteadOfMessage",
data: { containerExpr, messageChain: messageChainKey },
suggest: [
{
messageId: "replaceWithMessageChain",
data: { containerExpr, messageChain: messageChainKey },
fix(fixer) {
return fixer.replaceText(node.alternate, `String(${messageChainKey})`);
},
},
],
});Since the correct substitution is deterministic, a suggestion is low risk and avoids each developer having to re-derive the same fix.
@copilot please address this.
| export const noStringFallbackForNonStringMessageRule = createRule({ | ||
| name: "no-string-fallback-for-non-string-message", | ||
| meta: { | ||
| type: "problem", |
There was a problem hiding this comment.
[/tdd] The meta.type is set to "problem", which signals that violations are errors or likely bugs — yet the rule is wired at "warn" severity. This mismatch is low-risk but can confuse tooling and contributors: "problem" implies the rule should eventually be "error". If "warn" is the long-term intent (case-by-case decision), "suggestion" is the more accurate meta.type.
💡 Suggested fix
Change to one of:
// If warn is the permanent severity (per-site decision required):
type: "suggestion",
// If warn is temporary until all sites are fixed and rule moves to "error":
// keep type: "problem" but document the promotion plan in a comment or PROther rules in this factory that are registered at warn use "suggestion" as their meta.type, which is consistent.
@copilot please address this.
| `const m = typeof err.message === "string" ? err.message : err.toString();`, | ||
| ], | ||
| invalid: [], | ||
| }); |
There was a problem hiding this comment.
[/tdd] The valid suite has no test for the case where node.arguments.length !== 1 in getStringCallArgChainKey — e.g. String() or String(a, b). These are accepted today (returns null → no report), but there is no test confirming that behaviour. If someone changes the length guard, the test suite won't catch it.
💡 Suggested tests to add
// String() with no arguments — should not fire.
`const m = typeof err.message === "string" ? err.message : String();`,
// String() with multiple arguments (unusual, but syntactically valid).
`const m = typeof err.message === "string" ? err.message : String(err, "extra");`,@copilot please address this.
|
Please refresh the branch with the latest base changes if GitHub offers it, address any remaining review feedback or check noise, and then run the Run: https://github.com/github/gh-aw/actions/runs/32645772987
|
|
🎉 This pull request is included in a new release. Release: |
test minimal> Generated by PR Description Updater for #55052 · auto · 28 AIC · ⌖ 4.09 AIC · ⊞ 7.7K · ◷