Fix embedded error-code detection in thrown-error rule - #52972
Conversation
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.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52972 does not have the 'implementation' label and has 0 new lines of code in business logic directories (2 files changed, none in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, or api/).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Diff is a minimal regex fix (one-line change) plus targeted regression tests; no over-engineering, dead code, or speculative abstraction to cut.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
|
There was a problem hiding this comment.
Verdict
Non-blocking: the regex boundary change and accompanying regression cases look internally consistent, and I did not find a changed-line correctness or maintainability issue worth blocking this PR over.
Highlights
- The new negative-lookbehind still rejects partial suffix matches like
SAFE_OUTPUT_E099XandERR_CONFIGmore. - The added tests cover both embedded numeric codes and embedded
ERR_segments, plus several nearby false-positive/false-negative cases. - I did not find a missed changed-line edge case in the updated matcher logic.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 2.9 AIC · ⌖ 6.45 AIC · ⊞ 6.9K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Updates the ESLint rule to recognize error-code segments embedded after underscores without accepting partial matches.
Changes:
- Refines error-code boundary matching.
- Adds regression tests for valid embedded codes and invalid lookalikes.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-error-code-in-thrown-error.ts |
Updates error-code detection boundaries. |
eslint-factory/src/rules/require-error-code-in-thrown-error.test.ts |
Adds embedded-code regression coverage. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
The regex fix is correct and well-tested.
Root cause: The old pattern \bERR_[A-Z_]+\b used \b word boundaries. Since _ is a word character, the boundary before ERR_ is not triggered when preceded by _ (e.g. MY_PREFIX_ERR_CONFIG). This made the rule miss valid error codes embedded after underscored prefixes.
Fix: The new pattern replaces \b with a negative lookbehind (?<![A-Za-z0-9]), anchoring only on alphanumeric characters. This correctly matches MY_PREFIX_ERR_CONFIG and SAFE_OUTPUT_E099 while still rejecting FOOERR_CONFIG, MY_PREFIX_ERROR_CONFIG, and other non-error-code names.
All new valid and invalid test cases are well-chosen and cover the key boundary conditions.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.2 AIC · ⌖ 7.98 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — changes look correct, one minor comment suggestion.
📋 Key Themes & Highlights
Key Themes
- Correct root cause fix: Replacing
\bwith(?<![A-Za-z0-9])so underscore serves as a valid separator is the right call —\btreats_as a word character, which is whySAFE_OUTPUT_E099failed to match. - Good regression tests: Valid cases for
SAFE_OUTPUT_E099andMY_PREFIX_ERR_CONFIGplus invalid boundary cases (E099X,FOOERR_CONFIG,ERR_CONFIGmore) cover the interesting edges well.
One Minor Point
- The non-obvious choice of
[A-Za-z0-9]over\bdeserves a short inline comment to prevent a future simplification regressing the fix.
Positive Highlights
- ✅ Minimal, surgical change — 1 line of production code
- ✅ Regression tests added alongside the fix, matching
/tddred-green-refactor - ✅ Both
ERR_*andE###patterns updated symmetrically in the regex
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.7 AIC · ⌖ 8.51 AIC · ⊞ 7.7K
Comment /matt to run again
| 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 ERROR_CODE_PATTERN = /(?<![A-Za-z0-9])ERR_[A-Z_]+\b|(?<![A-Za-z0-9])E[0-9]{3}\b/; |
There was a problem hiding this comment.
[/diagnosing-bugs] The lookbehind uses [A-Za-z0-9] (not \b) to intentionally treat _ as a separator, enabling SAFE_OUTPUT_ERR_CONFIG to match while blocking FOOERR_CONFIG. Without a comment, a future maintainer may simplify this back to \b and re-introduce the bug.
💡 Suggested comment
// Use [A-Za-z0-9] — not \b — so underscore acts as a word separator.
// This lets SAFE_OUTPUT_ERR_CONFIG match while rejecting FOOERR_CONFIG.
const ERROR_CODE_PATTERN = /(?<![A-Za-z0-9])ERR_[A-Z_]+\b|(?<![A-Za-z0-9])E[0-9]{3}\b/;@copilot please address this.
🧪 Test Quality Sentinel ReportSummaryTest Quality Score: 92/100 ✅ Excellent This PR adds 6 comprehensive test cases to validate the regex fix in the ESLint error-code detection rule. All tests directly validate the behavioral contract (pattern matching) with strong edge case coverage. Test AnalysisChanged Files
Test Cases AddedValid Pattern Cases (2 tests)
Classification: Invalid Pattern Cases (4 tests) — Edge Cases for Negative Lookbehind
Classification: Quality Metrics
Scoring DetailsExcellent threshold: ≥80 ✅ Recommendations✅ All checks passed. The tests comprehensively validate the regex pattern fix with clear edge case scenarios. No changes needed. Why this is high quality:
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
|
🎉 This pull request is included in a new release. Release: |
require-error-code-in-thrown-errormissed valid error-code constants when embedded after underscores, such asSAFE_OUTPUT_E099. This caused false positives in setup action scripts using SAFE_OUTPUT-style codes.ERR_*andE###segments after identifier separators.SAFE_OUTPUT_E099X.