[eslint-miner] Add ESLint rule: require-error-code-in-thrown-error - #51042
Conversation
Adds a new custom ESLint rule that flags thrown Error messages lacking a standardized error code (ERR_*, from error_codes.cjs) in files that already import error_codes.cjs. This keeps error-code coverage consistent so logs/dashboards can filter reliably. Motivated by issue #51018 (add_labels.cjs / dismiss_pull_request_review.cjs missing standardized error codes) — scanning actions/setup/js found 111 throw sites across 27 files that import error_codes.cjs but still throw uncoded errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work, Summary: This adds a focused, well-tested ESLint rule (
Why this matters: Issue #51018 flagged 111 uncoded Validation: The PR body documents:
The code is focused, well-scoped to Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100). |
|
|
|
|
|
|
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule intended to enforce standardized error codes in setup-handler errors.
Changes:
- Implements and tests error-code detection.
- Registers the rule in the custom plugin.
- Enables it at warning severity.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-error-code-in-thrown-error.ts |
Implements error-code validation. |
eslint-factory/src/rules/require-error-code-in-thrown-error.test.ts |
Adds rule tests. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule. |
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: 3
- Review effort level: Balanced
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | ||
|
|
||
| const ERROR_CODE_PATTERN = /\bERR_[A-Z_]+\b|\bE[0-9]{3}\b/; |
| const importsErrorCodes = /require\(\s*["']\.\/error_codes\.cjs["']\s*\)/.test(fullText); | ||
|
|
||
| if (!importsErrorCodes) { | ||
| return {}; |
| const arg = node.argument; | ||
| if (!arg || arg.type !== AST_NODE_TYPES.NewExpression) return; |
🧪 Test Quality Sentinel ReportOverviewPR adds a new ESLint rule ( Test Coverage Summary
Test Cases Analyzed4 Test Cases — All Design-Focused
Structural Quality✅ Pattern Coverage
✅ Framework Idiomatic
✅ Valid + Invalid Paths
✅ Test/Production Ratio: 0.72:1
SummaryAll 4 tests are design-focused (verify user-visible rule behavior, not implementation details). No mock-heavy patterns, no implementation inflation. The test suite covers:
No violations detected. Ready to merge. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100 Excellent. All 4 tests are design-focused with 0% implementation-test ratio (well below 30% threshold). Test/production ratio is 0.72:1 (excellent balance). No hard violations detected. Design invariants properly covered: import context, valid error code formats, missing code detection, and type boundaries.
There was a problem hiding this comment.
Review: require-error-code-in-thrown-error
Good addition — the motivation is clear and the opt-in approach (only activates when error_codes.cjs is already imported) avoids false positives in unrelated files. Two correctness bugs need fixing before merge, plus a minor test naming issue.
Blocking issues (2)
-
Full-text regex for import detection (line 58) — matches
require('./error_codes.cjs')inside comments or strings, activating the rule incorrectly. Use AST walking (CallExpression) instead. See inline comment. -
.rawcast on non-Literal nodes (line 15) — the leading(node as unknown as {raw?:string}).rawcast is dead code forIdentifier/BinaryExpressionand reads the wrong property forLiteralnodes (rawvsvalue). See inline comment.
Non-blocking (1)
- Misleading test title (test file line 50) —
"invalid: non-Error throws..."block has onlyvalidentries. Rename to"valid: ...". See inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25 AIC · ⊞ 5.4K
| } | ||
|
|
||
| return { | ||
| ThrowStatement(node: TSESTree.ThrowStatement) { |
There was a problem hiding this comment.
Bug: import detection via full-text regex can false-positive on commented-out require calls.
Using sourceCode.getText() regex to detect the error_codes.cjs import will match occurrences inside comments or string literals, e.g.:
// const { ERR_API } = require('./error_codes.cjs') // commented out
throw new Error('plain message');This activates the rule on a file that doesn't actually use the convention, causing spurious warnings.
Prefer walking the AST instead:
let importsErrorCodes = false;
return {
CallExpression(node) {
if (
node.callee.type === AST_NODE_TYPES.Identifier &&
node.callee.name === 'require' &&
node.arguments[0]?.type === AST_NODE_TYPES.Literal &&
(node.arguments[0] as TSESTree.Literal).value === './error_codes.cjs'
) {
importsErrorCodes = true;
}
},
ThrowStatement(node) {
if (!importsErrorCodes) return;
// ... existing logic
},
};@copilot please address this.
| function messageReferencesErrorCode(node: TSESTree.Node): boolean { | ||
| const text = (node as unknown as { raw?: string }).raw ?? ""; | ||
| if (ERROR_CODE_PATTERN.test(text)) return true; | ||
| if (node.type === AST_NODE_TYPES.TemplateLiteral) { |
There was a problem hiding this comment.
Bug: .raw cast on non-Literal/TemplateLiteral nodes returns "" and silently skips the code check.
The function begins with:
const text = (node as unknown as { raw?: string }).raw ?? "";
if (ERROR_CODE_PATTERN.test(text)) return true;For Identifier or BinaryExpression nodes, .raw is undefined, so text is always "". This first check never contributes for those node types — it's dead code that adds confusion.
For Literal nodes the correct property is node.value (a string), not node.raw (which includes surrounding quotes, e.g. "\"ERR_API: ...\""). This means a plain string literal like "ERR_API: failed" would match only because node.raw includes the quotes around the string that happen to contain ERR_API. While the pattern still matches, the logic is incidental.
Recommend replacing the raw-cast with an explicit Literal branch:
if (node.type === AST_NODE_TYPES.Literal && typeof node.value === 'string') {
return ERROR_CODE_PATTERN.test(node.value);
}and removing the leading .raw cast entirely.
@copilot please address this.
| it("invalid: non-Error throws and other constructs are ignored", () => { | ||
| cjsRuleTester.run("require-error-code-in-thrown-error", requireErrorCodeInThrownErrorRule, { | ||
| valid: [ | ||
| `const { ERR_API } = require("./error_codes.cjs"); function f() { throw someError; }`, |
There was a problem hiding this comment.
Misleading test description: "invalid" test block contains only valid cases.
The it block at line ~49 is titled "invalid: non-Error throws and other constructs are ignored" but its RuleTester call has only valid entries and an empty invalid array. The title implies rule violations are tested, but none are. Rename to "valid: non-Error throws and other constructs are not flagged" to avoid confusion for future contributors.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two correctness issues in the rule implementation and two test-quality gaps.
📋 Key Themes & Highlights
Key Themes
- Fragile activation guard (line 57, rule file): the
importsErrorCodescheck scans raw source text, making it vulnerable to comments or string literals that happen to containrequire("./error_codes.cjs"). AST traversal is the correct approach here. - Unsafe cast + redundant code path (line 13, rule file): reading
.rawviaas unknown as { raw?: string }before type-narrowing creates an unclear code path. Literal nodes should be handled explicitly first. - Missing boundary test (test file): no coverage for paths like
../error_codes.cjsthat intentionally should not activate the rule, leaving the scope contract undocumented. - Under-constrained error assertions (test file):
errorsarrays only checkmessageId, not node type — a future report-location change would pass silently.
Positive Highlights
- ✅ Excellent scope discipline: the
importsErrorCodesguard keeps false-positive rate at zero for non-convention files. - ✅ Good coverage of the detection patterns: template literals, identifiers, binary concatenation, and member expressions.
- ✅ Correct severity (
warn) consistent with the rest of the factory rules. - ✅ Clean integration: registration in
index.tsandeslint.config.cjsfollows established patterns exactly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.3 AIC · ⊞ 7.1K
Comment /matt to run again
| return {}; | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
[/tdd] The require() detection uses a full-text regex scan (sourceCode.getText()), which means a commented-out require("./error_codes.cjs") or a string literal containing that path will falsely activate the rule. Prefer AST traversal over source-text matching.
💡 Suggested fix
Add a CallExpression visitor that sets a flag when the actual AST require() call is detected:
let importsErrorCodes = false;
return {
CallExpression(node) {
if (
node.callee.type === AST_NODE_TYPES.Identifier &&
node.callee.name === "require" &&
node.arguments[0]?.type === AST_NODE_TYPES.Literal &&
(node.arguments[0] as TSESTree.StringLiteral).value === "./error_codes.cjs"
) importsErrorCodes = true;
},
ThrowStatement(node) {
if (!importsErrorCodes) return;
// ... existing logic
},
};This avoids false positives from comments or string literals containing the path.
@copilot please address this.
| * or a SAFE_OUTPUT_E001-style numeric code). | ||
| */ | ||
| function messageReferencesErrorCode(node: TSESTree.Node): boolean { | ||
| const text = (node as unknown as { raw?: string }).raw ?? ""; |
There was a problem hiding this comment.
[/tdd] messageReferencesErrorCode reads .raw via an unsafe cast to { raw?: string } before checking the node type. If the node is a Literal (not a template), .raw may exist but the code falls through to specific type checks anyway — the early regex test on .raw is redundant and could match non-Literal nodes unexpectedly. The type narrowing should come first.
💡 Suggested refactor
Move the Literal check ahead of the generic .raw access:
function messageReferencesErrorCode(node: TSESTree.Node): boolean {
if (node.type === AST_NODE_TYPES.Literal) {
return ERROR_CODE_PATTERN.test(String(node.value));
}
if (node.type === AST_NODE_TYPES.TemplateLiteral) { ... }
if (node.type === AST_NODE_TYPES.Identifier) { ... }
if (node.type === AST_NODE_TYPES.BinaryExpression && node.operator === "+") { ... }
return false;
}This removes the as unknown cast, improves type safety, and makes the logic easier to follow.
@copilot please address this.
| }, | ||
| }); | ||
|
|
||
| describe("require-error-code-in-thrown-error", () => { |
There was a problem hiding this comment.
[/tdd] There is no test for a file that imports error_codes.cjs via a path that does not match the exact literal ./error_codes.cjs (e.g. '../error_codes.cjs' or a variable). Documenting this as an intentional scope boundary (or adding a test for it) would prevent confusion.
💡 Suggestion
Add an explicit valid case to the "files that do not import error_codes.cjs" group:
// Files importing from a different relative path are not flagged
`const { ERR_API } = require('../error_codes.cjs'); throw new Error('no code');`This makes the scope boundary visible in the test suite and guards against accidentally widening the regex in future.
@copilot please address this.
| code: `const { ERR_API } = require("./error_codes.cjs"); function f(id) { throw new Error("Cannot mark issue as duplicate of " + id); }`, | ||
| errors: [{ messageId: "missingErrorCode" }], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[/tdd] The invalid test cases only verify messageId but not the reported node location. Without a loc assertion, a future refactor that moves the report from the NewExpression to the ThrowStatement (or the message argument) would silently pass these tests despite a different UX.
💡 Suggestion
Add type: 'NewExpression' (or column/line) to pin the reported node:
errors: [{ messageId: "missingErrorCode", type: "NewExpression" }],This matches the pattern used in other rule tests across the repo and documents intent.
@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes
The rule's design has a correctness gap that undermines its stated purpose, plus a fragile detection mechanism and an inherent heuristic weakness.
Themes
- Coverage gap (high): only
throw new Error(...)is checked;throw Error(...)(nonew) is completely ignored, so a meaningful subset of the 111 uncoded throws this PR aims to surface could slip through undetected. - Fragile import detection (medium/high): whole-file textual regex for
require("./error_codes.cjs")can false-positive on comments/strings and false-negative on ESM imports or path variants. - Heuristic limitation (medium): name-based identifier matching (
ERR_*-named variable) doesn't verify the actual referenced value contains a code — acceptable as a heuristic, but worth documenting as a known limitation.
No test coverage exists for the throw Error() (no new) case, +-concatenated identifier-only messages, or ESM import detection — these gaps in tests mirror the gaps in the implementation.
🔎 Code quality review by PR Code Quality Reviewer · auto · 67.2 AIC · ⊞ 7.8K
Comment /review to run again
| return { | ||
| ThrowStatement(node: TSESTree.ThrowStatement) { | ||
| const arg = node.argument; | ||
| if (!arg || arg.type !== AST_NODE_TYPES.NewExpression) return; |
There was a problem hiding this comment.
This only detects throw new Error(...) — throw Error(...) (no new, valid and common JS) silently bypasses the rule entirely.
💡 Missing throw-without-new coverage
arg.type !== AST_NODE_TYPES.NewExpression rejects any CallExpression callee, so throw Error("failed to fetch") is never inspected. Some of the 27 offending files referenced in the PR description may well use bare Error(...) calls, and this rule silently gives them a pass — undermining the stated goal of catching all uncoded throws.
Fix: also accept CallExpression with callee Error.
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
| const fullText = sourceCode.getText(); | ||
| const importsErrorCodes = /require\(\s*["']\.\/error_codes\.cjs["']\s*\)/.test(fullText); |
There was a problem hiding this comment.
The whole-file textual detection of error_codes.cjs usage is a brittle regex that can both false-positive and false-negative.
💡 Fragile presence check via source-text regex
/require\(\s*["']\.\/error_codes\.cjs["']\s*\)/ matches the literal string anywhere in the file's text — including inside comments or unrelated string literals — causing false activation. It also fails to detect ESM import { ERR_API } from "./error_codes.cjs", different relative paths (../error_codes.cjs), or destructured require("./error_codes") without the .cjs suffix, causing false negatives that silently disable the rule for files that do use the convention.
Better: walk the AST for CallExpression/ImportDeclaration nodes with a source matching /error_codes(\.cjs)?$/ rather than a raw text regex.
| if (ERROR_CODE_PATTERN.test(quasi.value.raw)) return true; | ||
| } | ||
| for (const expr of node.expressions) { | ||
| if (expr.type === AST_NODE_TYPES.Identifier && ERROR_CODE_PATTERN.test(expr.name)) return true; |
There was a problem hiding this comment.
Matching against an identifier's name (not its value) is a weak heuristic that can be satisfied without the message actually containing a code at runtime.
💡 Name-based check doesn't verify actual code presence
expr.type === AST_NODE_TYPES.Identifier && ERROR_CODE_PATTERN.test(expr.name) passes as soon as a variable is merely named like ERR_something, regardless of what string it actually holds. E.g. const ERR_LABEL = "whoops"; throw new Error(\${ERR_LABEL}: bad`)` would pass the lint even though the rendered message never contains a real error code — defeating the stated goal ("logs/dashboards can filter reliably"). Given this file only checks identifier names heuristically (no cross-file/data-flow resolution), that's an inherent limitation worth calling out in the rule's doc comment so contributors don't over-trust the warning.
|
🎉 This pull request is included in a new release. Release: |
Overview
Adds a new custom ESLint rule,
require-error-code-in-thrown-error, to theeslint-factorypackage (part of the "eslint-miner" family of custom lint rules). The rule enforces that thrownErrormessages reference a standardized error code (ERR_*constant fromerror_codes.cjs) in files that already import that module, keeping error-code coverage consistent for log/dashboard filtering. Registered at"warn"severity.Files changed
eslint-factory/eslint.config.cjsgh-aw-custom/require-error-code-in-thrown-erroras"warn"in the shared config.eslint-factory/src/index.tsrequireErrorCodeInThrownErrorRuleand adds it to the plugin'srulesmap.eslint-factory/src/rules/require-error-code-in-thrown-error.ts(new, 78 lines)eslint-factory/src/rules/require-error-code-in-thrown-error.test.ts(new, 56 lines)RuleTestercoverage.Rule logic
@typescript-eslint/utils(ESLintUtils.RuleCreator).require\(\s*["']\.\/error_codes\.cjs["']\s*\)— i.e. files that already importerror_codes.cjs.ThrowStatement, checks if the argument is anew Error(...)call (callee identifier exactly"Error"; other constructors likeTypeErroror plain thrown identifiers are ignored).TemplateLiteral,Literal,Identifier, orBinaryExpression) viamessageReferencesErrorCode(), which recursively checks forERROR_CODE_PATTERN = /ERR_[A-Z_]+|E[0-9]{3}/against:IdentifierorMemberExpression.propertyname),Identifiername,+BinaryExpression, recursively.missingErrorCodeon thenew Error(...)node when no code reference is found.Test coverage
require-error-code-in-thrown-error.test.tsuses vitest + ESLintRuleTester(CommonJS, ecmaVersion 2022) across 4 groups:error_codes.cjsare never flagged, even without an error code.ERR_*codes (via template interpolation, string concatenation, or embedded literal) are valid.error_codes.cjswiththrow new Error(...)lacking anyERR_reference are flagged (missingErrorCode).Errorthrows (throw someError) and other constructors (throw new TypeError(...)) are not flagged.> Generated by PR Description Updater for [eslint-miner] Add ESLint rule: require-error-code-in-thrown-error #51042 · auto · 28.9 AIC · ⊞ 6.8K · ◷