Refine interpolated-route diagnostics for opaque Octokit route helpers#45364
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45364 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Refines ESLint diagnostics to distinguish opaque dynamic routes from known-path interpolation.
Changes:
- Adds
opaqueWholeRoutedetection and tailored guidance. - Tests template-literal and concatenation variants.
- Documents the new diagnostic behavior.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-github-request-interpolated-route.ts |
Classifies and reports opaque whole-route interpolation. |
eslint-factory/src/rules/no-github-request-interpolated-route.test.ts |
Verifies distinct diagnostic IDs. |
eslint-factory/README.md |
Documents opaque-route guidance. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refine interpolated-route diagnostics for opaque Octokit route helpers
The implementation correctly splits the existing rule into two distinct diagnostic paths.
isOpaqueWholeRouteInterpolationhandles both the template-literal and string-concatenation forms with proper null guards oncookedvalues.- Ordering in the reporter is correct: opaque-route check runs first, fallthrough to
interpolatedRoutehandled via the refactoredgetInterpolatedRouteKindhelper. - Nested concatenation (
"POST " + owner + "/" + repo) correctly falls through to the placeholder-remedy diagnostic. - Tests assert distinct
messageIds for both new cases with correctdatafields. - README accurately documents the new opaque-route case.
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 24.1 AIC · ⌖ 5.19 AIC · ⊞ 4.8K
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 41.9 AIC · ⌖ 4.54 AIC · ⊞ 6.6K
Comment /matt to run again
| }); | ||
| }); | ||
|
|
||
| it("invalid: opaque whole-route helpers are flagged with tailored guidance", () => { |
There was a problem hiding this comment.
[/tdd] New tests only exercise the github client — the existing interpolatedRoute tests cover all four known client names (github, octokit, githubClient, octokitClient), but the new opaqueWholeRoute block tests only github. If a client name is added or removed in future, this gap won't surface.
💡 Suggested additions
Add at least one entry per additional client name, mirroring the pattern in the interpolatedRoute test block:
{
code: "function addReaction(endpoint) { octokit.request(`POST ${endpoint}`, {}); }",
errors: [{ messageId: "opaqueWholeRoute", data: { kind: "template literal with interpolations", client: "octokit" } }],
},
{
code: "function addReaction(endpoint) { githubClient.request(`POST ${endpoint}`, {}); }",
errors: [{ messageId: "opaqueWholeRoute", data: { kind: "template literal with interpolations", client: "githubClient" } }],
},This gives the test suite the same breadth as the existing interpolatedRoute coverage.
@copilot please address this.
| const hasTwoQuasis = node.quasis.length === 2; | ||
| const hasSingleExpression = node.expressions.length === 1; | ||
| const hasMethodPrefix = hasTwoQuasis && isValidHttpMethodPrefix(node.quasis[0].value.cooked); | ||
| const hasEmptyTrailingQuasi = hasTwoQuasis && node.quasis[1].value.cooked === ""; |
There was a problem hiding this comment.
[/codebase-design] hasEmptyTrailingQuasi checks === "" but doesn't guard against null — quasis[1].value.cooked is null when the template contains an invalid escape sequence (e.g. POST ${endpoint}\u{FFFFFFFF}). A null cooked value would make this check pass, potentially mis-classifying the route as opaque.
💡 Suggested fix
const hasEmptyTrailingQuasi =
hasTwoQuasis && node.quasis[1].value.cooked === "";→
const hasEmptyTrailingQuasi =
hasTwoQuasis && (node.quasis[1].value.cooked ?? null) === "";Or more defensively:
const trailingCooked = node.quasis[1]?.value.cooked;
const hasEmptyTrailingQuasi = trailingCooked === "";Also apply the same null-guard to the hasMethodPrefix check on quasis[0].value.cooked — isValidHttpMethodPrefix already accepts null, so that path is safe, but documenting the intent is clearer.
@copilot please address this.
There was a problem hiding this comment.
Two correctness issues in isOpaqueWholeRouteInterpolation that can produce wrong diagnostic guidance, plus a missing regression test at the classification boundary.
Blocking issues
cooked can be null, breaking the trailing-quasi check (line 76)
TemplateElement.value.cooked is typed as string | null. The current === "" check evaluates to false for null, silently falling through to interpolatedRoute instead of opaqueWholeRoute.
BinaryExpression branch fires on multi-value path concatenations (line 85)
"POST " + (owner + "/" + repo) has a dynamic BinaryExpression as right. isRightDynamic is true, so isOpaqueWholeRouteInterpolation fires and the user gets the wrong guidance. The RHS check should require a single Identifier or MemberExpression.
Missing boundary regression test
No test verifies that multi-expression templates (2+ interpolations) stay on the interpolatedRoute path rather than opaqueWholeRoute.
🔎 Code quality review by PR Code Quality Reviewer · 61.4 AIC · ⌖ 4.82 AIC · ⊞ 5.4K
Comment /review to run again
| const hasTwoQuasis = node.quasis.length === 2; | ||
| const hasSingleExpression = node.expressions.length === 1; | ||
| const hasMethodPrefix = hasTwoQuasis && isValidHttpMethodPrefix(node.quasis[0].value.cooked); | ||
| const hasEmptyTrailingQuasi = hasTwoQuasis && node.quasis[1].value.cooked === ""; |
There was a problem hiding this comment.
Unguarded null check may silently misclassify opaque templates: node.quasis[1].value.cooked can be null for template literals with invalid Unicode escape sequences, and null === "" evaluates to false, causing the node to fall through to the generic interpolatedRoute message instead of the intended opaqueWholeRoute message.
💡 Suggested fix
const hasEmptyTrailingQuasi = hasTwoQuasis && (node.quasis[1].value.cooked === "" || node.quasis[1].value.cooked == null);Or extract a helper:
function isCookedEmpty(quasi: TSESTree.TemplateElement): boolean {
return quasi.value.cooked === "" || quasi.value.cooked == null;
}Template literals with invalid escapes (e.g., POST \u{notvalid}${endpoint} ) will set cooked to null on the quasi containing the invalid escape. If that quasi is the trailing one and happens to be otherwise empty, the classification silently degrades to a less specific diagnostic.
| if (node.left.type !== "Literal" || typeof node.left.value !== "string") return false; | ||
| const hasMethodPrefix = isValidHttpMethodPrefix(node.left.value); | ||
| const isRightDynamic = !isStaticRouteExpression(node.right); | ||
| return hasMethodPrefix && isRightDynamic; |
There was a problem hiding this comment.
BinaryExpression opaque-route check misclassifies right-associative concatenations: "POST " + (owner + "/" + repo) has left = "POST " (Literal) and right = (owner + "/" + repo) (dynamic BinaryExpression). This hits the hasMethodPrefix && isRightDynamic guard and fires opaqueWholeRoute, but the right-hand side is a multi-value path interpolation that should get the typed-placeholder interpolatedRoute guidance instead.
💡 Details and suggested fix
The distinction that makes isOpaqueWholeRouteInterpolation meaningful is whether the RHS is a single identifier (the whole route), not just any dynamic expression. Tighten the check:
if (node.type === "BinaryExpression" && node.operator === "+") {
if (node.left.type !== "Literal" || typeof node.left.value !== "string") return false;
const hasMethodPrefix = isValidHttpMethodPrefix(node.left.value);
// Require the right side to be a single identifier (or member expression), not another concatenation
const isRightSingleDynamic = node.right.type === "Identifier" || node.right.type === "MemberExpression";
return hasMethodPrefix && isRightSingleDynamic;
}Add a regression test:
{
code: 'github.request("POST " + owner + "/" + repo, { });',
errors: [{ messageId: "interpolatedRoute" }],
}| }, | ||
| ], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing regression test for the critical boundary between opaqueWholeRoute and interpolatedRoute: No test verifies that a multi-expression template like POST /repos/${owner}/${repo} (3 quasis, 2 expressions) falls through to interpolatedRoute instead of opaqueWholeRoute. The hasSingleExpression guard is the only protection and is untested.
💡 Suggested test
it("boundary: multi-expression template stays interpolatedRoute, not opaqueWholeRoute", () => {
cjsRuleTester.run("no-github-request-interpolated-route", noGithubRequestInterpolatedRouteRule, {
valid: [],
invalid: [
{
code: "github.request(`POST /repos/${owner}/${repo}/reactions`, { content: '+1' });",
errors: [{ messageId: "interpolatedRoute" }],
},
],
});
});Without this, a future change to isOpaqueWholeRouteInterpolation could silently regress the most common path-interpolation case to a less actionable diagnostic.
|
Run: https://github.com/github/gh-aw/actions/runs/29313771262
|
1 similar comment
|
Run: https://github.com/github/gh-aw/actions/runs/29313771262
|
pelikhan
deleted the
copilot/eslint-factory-fix-no-github-request-interpolated
branch
The
no-github-request-interpolated-routerule treated two different cases as one: value interpolation inside a known route path, and helpers that interpolate an entire opaque route such as`POST ${endpoint}`. The placeholder-based remedy is correct for the first case, but misleading for the second.Rule behavior
github.request(`POST ${endpoint}`, ...)github.request("POST " + endpoint, ...)`POST /repos/${owner}/${repo}/...`Diagnostic guidance
{owner}/{repo}rewriteCoverage
messageIds for:interpolatedRouteopaqueWholeRouteroute_slash_command.cjsplaceholder-remedy casesDocs
Example: