eslint-factory: narrow require-fetch-response-body-try-catch diagnostic to actual failure mode - #52981
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
require-fetch-response-body-try-catch diagnostic to actual failure mode
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52981 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel completed analysis: PR #52981 modifies only TypeScript ESLint rule tests, which fall outside the primary scoring categories (Go and JavaScript). The test change visible is a quality improvement (explicit message assertion vs. ID-based assertion). No violations detected. See comment for full analysis.
|
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.
Verdict
Non-blocking: this change just corrects the lint diagnostic wording to match the actual failure mode, and the focused test assertion covers the new text.
Reviewed themes
- checked the updated rule message for correctness against the rule behavior
- checked the test change for brittleness or false confidence
- found no changed-line issues that would justify blocking merge
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 2.76 AIC · ⌖ 6.09 AIC · ⊞ 6.9K
Comment /review to run again
There was a problem hiding this comment.
The change narrows the diagnostic message to explain the actual failure mode more clearly. The {{call}} placeholder is preserved correctly, and the test uses an exact message: match that aligns with the updated message text. No correctness or security issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15 AIC · ⌖ 6.99 AIC · ⊞ 5.6K
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.9 AIC · ⌖ 8.42 AIC · ⊞ 7.7K
Comment /matt to run again
| errors: [ | ||
| { | ||
| messageId: "requireTryCatch", | ||
| message: |
There was a problem hiding this comment.
[/tdd] The test now asserts the full literal error message rather than the messageId key — any future wording change to requireTryCatch will silently break this test (or tempt reverting the wording to avoid the failure).
💡 Suggestion
Keep the assertion on messageId: "requireTryCatch" for "is the right rule triggered" and add a separate, clearly-labelled snapshot or constant-driven test that pins the rendered text. Alternatively, extract the message string into a shared constant so the rule definition and test always reference the same source of truth.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Updates the ESLint rule diagnostic to describe lost parse context rather than action crashes.
Changes:
- Clarifies response-body parse failure semantics.
- Adds an exact diagnostic-message assertion.
Show a summary per file
| File | Description |
|---|---|
require-fetch-response-body-try-catch.ts |
Revises the rule diagnostic. |
require-fetch-response-body-try-catch.test.ts |
Verifies the revised message. |
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: 1
- Review effort level: Balanced
| requireTryCatch: | ||
| "Wrap {{call}} in try/catch — even after explicit HTTP-error handling (for example `if (!response.ok) throw ...`), " + | ||
| "reading a fetch() Response body can still reject (malformed JSON, truncated/errored stream). Without this call-site try/catch, " + | ||
| "you lose the original parse error context and get a generic, harder-to-diagnose stack instead of a specific message with `{ cause }`.", |
Test Quality Sentinel 🧪SummaryChanged Files: 2 (1 test file, 1 production file)
Test Framework Detected: TypeScript ESLint rule tests (using RuleTester) Scoring Status: The modified test file is TypeScript (ESLint rule test), which falls outside the primary scoring categories:
Test Changes AnalysisModified Test:
Lines Changed: +4, -1 (net +3 lines)
AssessmentThis PR makes a focused, high-confidence test improvement for an ESLint rule:
ConclusionThis PR cannot be scored with the standard Test Quality Sentinel rubric because it contains no Go or JavaScript tests. However, the test change visible in the TypeScript ESLint test is a quality improvement — it explicitly validates the user-facing diagnostic message text, which is a stronger behavioral contract than ID-based assertion. Note: Test Quality Sentinel analyzes Go (
|
|
🎉 This pull request is included in a new release. Release: |
The rule text for
require-fetch-response-body-try-catchoverstated impact by saying unhandled response-body parse failures “will crash the action.” Inactions/setup/js, entrypoint-level error handling already converts these failures into job failures; the missing value at this call site is preserved parse context and clearer diagnostics.Message semantics correction (no detection change)
meta.messages.requireTryCatchineslint-factory/src/rules/require-fetch-response-body-try-catch.ts.{ cause }.Scope clarification in user-facing guidance
if (!response.ok) throw ...), so the warning is clearly about the successful-response body parse call.Focused test update
eslint-factory/src/rules/require-fetch-response-body-try-catch.test.tsnow asserts the emitted diagnostic text for a representative flagged case.requireTryCatch: "Wrap {{call}} in try/catch — even after explicit HTTP-error handling (for example `if (!response.ok) throw ...`), reading a fetch() Response body can still reject ... Without this call-site try/catch, you lose the original parse error context ... with `{ cause }`."