eslint-factory: scope-aware Octokit client detection in no-github-request-interpolated-route#44706
Conversation
…ction Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Adds scope-aware Octokit client detection to eslint rule
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44706 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. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request improves the no-github-request-interpolated-route ESLint rule so it can detect Octokit .request() calls beyond a small hard-coded identifier allowlist, adding scope-aware alias resolution and explicit handling for context.github.
Changes:
- Add helpers to recognize Octokit “source” expressions and resolve simple
const/letaliases via ESLint scope analysis. - Expand rule matching to flag interpolated routes on
context.github.request(...)and on aliased Octokit clients. - Update tests and README to reflect the expanded detection behavior and add false-positive guards.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/no-github-request-interpolated-route.ts | Adds scope-aware client resolution helpers and updates the CallExpression matcher to use them. |
| eslint-factory/src/rules/no-github-request-interpolated-route.test.ts | Moves context.github.request(...) cases to invalid and adds new alias/false-positive-guard test coverage. |
| eslint-factory/README.md | Updates documentation for expanded Octokit client detection and out-of-scope patterns. |
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: 2
- Review effort level: Low
| description: | ||
| "Disallow template literals with interpolations or string concatenation as the route argument of Octokit " + | ||
| "github/octokit/githubClient/octokitClient .request() calls. " + | ||
| ".request() calls. Octokit clients are detected by well-known names (github, octokit, githubClient, octokitClient), " + | ||
| "getOctokit(...) call results, context.github, and simple const/let aliases of any of these. " + | ||
| 'Use the typed placeholder form instead: "GET /repos/{owner}/{repo}" with a separate params object.', |
| **Detected Octokit clients:** | ||
| - Well-known names: `github`, `octokit`, `githubClient`, `octokitClient`. | ||
| - `context.github` — the GitHub context object's client property. | ||
| - `getOctokit(...)` return values (bare call or module-member call). | ||
| - Simple `const`/`let` aliases of any of the above: | ||
| `const gh = github`, `const client = getOctokit(token)`, `const myClient = context.github`. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (5 tests)
Test BreakdownAll 5 new tests directly enforce the rule's scope-aware client detection contract:
All tests use ESLint RuleTester's built-in assertion framework (valid/invalid arrays with error expectations), providing strong contract verification. Each test covers both interpolation types (template literals and concatenation). Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review: scope-aware Octokit client detection
The PR correctly upgrades the linter from a hard-coded 4-name allowlist to a scope-aware resolver that handles aliases, getOctokit() results, and context.github. Implementation is clean and the test coverage is thorough.
One non-blocking suggestion: the MemberExpression callee branch in isOctokitSourceExpression (for someModule.getOctokit(token)) has no corresponding test case. All other new paths are covered. See inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 24.1 AIC · ⌖ 4.37 AIC · ⊞ 4.8K
| return ( | ||
| node.type === AST_NODE_TYPES.MemberExpression && !node.computed && node.object.type === AST_NODE_TYPES.Identifier && node.object.name === "context" && node.property.type === AST_NODE_TYPES.Identifier && node.property.name === "github" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The isOctokitSourceExpression function handles module-member getOctokit calls (e.g. actions.getOctokit(token)) via the MemberExpression callee branch here, but there is no test exercising that path. Consider adding an invalid test case such as:
const client = actions.getOctokit(token);
client.request(`GET /repos/${owner}/${repo}`);to confirm the branch works as intended.
@copilot please address this.
There was a problem hiding this comment.
Three correctness gaps need to be fixed before merge
The scope-aware detection approach is sound, but there are three issues that can produce incorrect linting results in real code.
📋 Issues summary
High — let reassignment not modeled (line 113): isIdentifierBoundToOctokitClient only inspects the VariableDeclarator init, not subsequent write-references. A let client = getOctokit(token); client = http; pattern still gets flagged (false positive). Restrict alias detection to const-only bindings to eliminate this class of bugs entirely.
High — anyObject.getOctokit() is treated as an Octokit source (line 63): The member-call path in isOctokitSourceExpression accepts any receiver. Code like db.getOctokit(token) will mark the result as an Octokit client. Either drop the member-call path or add a receiver constraint.
Medium — sourceCode.getScope() called with CallExpression, not the Identifier (line 106): Scope resolution is most precise when called with the reference node itself. Pass calleeObject (the Identifier) rather than callNode (the CallExpression) for reliable scope chain walking.
🔎 Code quality review by PR Code Quality Reviewer · 41.4 AIC · ⌖ 4.87 AIC · ⊞ 5.4K
Comment /review to run again
| for (const def of variable.defs) { | ||
| if (def.type !== "Variable") continue; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (declarator.init && isOctokitSourceExpression(declarator.init)) return true; |
There was a problem hiding this comment.
** reassignment makes the Octokit check stale**: only the declaration initializer is inspected, not subsequent write-references, so a reassigned binding produces a false positive (or false negative in the reverse direction).
💡 Details and suggested fix
The current logic only checks variable.defs (VariableDeclarator inits):
let client = getOctokit(token);
client = httpModule; // ← reassigned to non-Octokit
client.request(`GET /repos/${o}/${r}`); // ← still flagged (false positive)And the inverse:
let client = http;
client = getOctokit(token); // ← write reference, not a def
client.request(`GET /repos/${o}/${r}`); // ← NOT flagged (false negative)To handle mutable bindings correctly, also check write-references via variable.references.filter(r => r.isWrite()). Since full dataflow is expensive in ESLint, the safer option is to restrict detection to const-declared bindings only (check def.node.parent.kind === 'const'), which eliminates mutable aliasing entirely.
if (declarator.init && isOctokitSourceExpression(declarator.init)) {
// Only trust const bindings — let/var can be reassigned
const declKind = (def.node as TSESTree.VariableDeclarator).parent?.kind;
if (declKind === 'const') return true;
}This keeps the rule's stated contract ("simple const/let aliases") while avoiding the false-positive/negative on reassigned let bindings.
| if (node.type === AST_NODE_TYPES.CallExpression) { | ||
| const callee = node.callee; | ||
| if (callee.type === AST_NODE_TYPES.Identifier && callee.name === "getOctokit") return true; | ||
| if (callee.type === AST_NODE_TYPES.MemberExpression && !callee.computed && callee.property.type === AST_NODE_TYPES.Identifier && callee.property.name === "getOctokit") return true; |
There was a problem hiding this comment.
Any method named getOctokit on any object is accepted as an Octokit source — will produce false positives when unrelated APIs use the same method name.
💡 Details and suggested fix
The check callee.property.name === 'getOctokit' matches ANY member call:
const client = db.getOctokit(); // ← false positive
const c2 = myHelper.getOctokit(token); // ← false positiveOnly the bare getOctokit(...) identifier call (line 62) is reliably tied to @actions/github. The member-call form has no guard on the receiver object and broadens detection beyond what was intended.
Fix: remove the member-call path unless the receiver is also constrained, or document clearly that all .getOctokit() calls are in-scope:
// Safe: only bare getOctokit() calls
if (callee.type === AST_NODE_TYPES.Identifier && callee.name === 'getOctokit') return true;
// Remove the MemberExpression path below unless receiver is verifiedAt minimum, add a test case for myHelper.getOctokit(token) that asserts it is NOT flagged.
| function isIdentifierBoundToOctokitClient(name: string, scopeNode: TSESTree.Node): boolean { | ||
| if (OCTOKIT_CLIENT_NAMES.has(name)) return true; | ||
|
|
||
| let scope: SourceCodeScope | null = sourceCode.getScope(scopeNode); |
There was a problem hiding this comment.
sourceCode.getScope() is called with the CallExpression node, not the Identifier — scope resolution may be imprecise in some ESLint versions.
💡 Details
sourceCode.getScope(callNode) returns the scope at the call site (the CallExpression). The identifier being resolved (calleeObject.name) is a reference at or inside the call expression, so this will usually work, but ESLint's getScope semantics are node-type sensitive — for some node types the returned scope is the parent scope, not the node's own scope.
The safer and more precise call is to pass the Identifier node itself:
function isIdentifierBoundToOctokitClient(
name: string,
scopeNode: TSESTree.Node, // pass the Identifier node, not the CallExpression
): boolean {And in the caller:
// In resolveOctokitClientName:
return isIdentifierBoundToOctokitClient(calleeObject.name, calleeObject) ? calleeObject.name : null;
// ^^^^^^^^^^^^^^^^^^^
// Identifier node, not callNodeThis is a low-risk but subtle correctness gap that could surface in non-trivial scope nesting.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness/coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Untested code path: the
module.getOctokit()member-expression branch (line 63) has no test coverage. varreassignment false positive:isIdentifierBoundToOctokitClientiterates defs but does not guard againstvardeclarations with a later non-Octokit reassignment — contrary to the PR description promise ofconst/letonly.- Long lines: lines 46 and 63 are noticeably wider than the rest of the file.
Positive Highlights
- ✅ Excellent test expansion — 115 lines covering new patterns and false-positive guards.
- ✅ Scope-chain walk is clean and conservative (single level only).
- ✅
context.githubmoved correctly from valid to invalid. - ✅ README clearly documents expanded and still-out-of-scope forms.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 42.1 AIC · ⌖ 4.75 AIC · ⊞ 6.6K
Comment /matt to run again
| if (node.type === AST_NODE_TYPES.CallExpression) { | ||
| const callee = node.callee; | ||
| if (callee.type === AST_NODE_TYPES.Identifier && callee.name === "getOctokit") return true; | ||
| if (callee.type === AST_NODE_TYPES.MemberExpression && !callee.computed && callee.property.type === AST_NODE_TYPES.Identifier && callee.property.name === "getOctokit") return true; |
There was a problem hiding this comment.
[/tdd] The member-expression getOctokit branch matches any object's getOctokit method (e.g. myLib.getOctokit()), not only well-known toolkit shapes. There is no test for this code path.
💡 Suggested test + narrowing
Add a test to confirm the member-expression path works and consider narrowing to known caller objects:
it('invalid: module.getOctokit() alias is flagged', () => {
cjsRuleTester.run('...', rule, {
valid: [],
invalid: [{
code: 'const c = core.getOctokit(token); c.request(`GET /repos/${owner}/${repo}`);',
errors: [{ messageId: 'interpolatedRoute', data: { kind: 'template literal with interpolations', client: 'c' } }],
}],
});
});@copilot please address this.
| } | ||
| scope = scope.upper; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
[/diagnosing-bugs] When the variable is found in scope but has no Variable defs (e.g. a function parameter or a destructured import), the loop hits return false at line 115, which is correct. However, if a variable has multiple defs (e.g. a var that is declared then reassigned), only the first Variable def that passes isOctokitSourceExpression wins — later reassignments to a non-Octokit value are silently ignored, potentially causing false positives.
💡 Suggested test to surface the issue
it('valid: var re-assigned to non-Octokit value is not flagged', () => {
cjsRuleTester.run('...', rule, {
valid: [
// gh was initially octokit but is then overwritten
'var gh = github; gh = someOtherThing; gh.request(`GET /repos/${owner}/${repo}`);',
],
invalid: [],
});
});The PR description scopes this to const/let single-level assignments, so adding a guard if (def.parent?.kind !== 'const' && def.parent?.kind !== 'let') continue; would close this gap.
@copilot please address this.
| */ | ||
| function isContextGithubExpression(node: TSESTree.Node): boolean { | ||
| return ( | ||
| node.type === AST_NODE_TYPES.MemberExpression && !node.computed && node.object.type === AST_NODE_TYPES.Identifier && node.object.name === "context" && node.property.type === AST_NODE_TYPES.Identifier && node.property.name === "github" |
There was a problem hiding this comment.
[/codebase-design] Line 46 is very long (170+ characters). The existing helpers in this file each fit comfortably on one readable line. Breaking this into multiple lines would match the surrounding style and improve readability in code review and diffs.
💡 Suggested formatting
function isContextGithubExpression(node: TSESTree.Node): boolean {
return (
node.type === AST_NODE_TYPES.MemberExpression &&
!node.computed &&
node.object.type === AST_NODE_TYPES.Identifier &&
node.object.name === 'context' &&
node.property.type === AST_NODE_TYPES.Identifier &&
node.property.name === 'github'
);
}@copilot please address this.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 5336e5a. I fixed the unresolved rule/docs/test feedback in scope, ran targeted rule tests ( |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
The
no-github-request-interpolated-routerule matched.request()calls only on 4 hard-coded identifier names, silently missing off-list aliases (const fallbackClient = getOctokit(token)),const gh = githubrenames, andcontext.github.request(...)direct calls.Changes
New helpers in
no-github-request-interpolated-route.ts:isContextGithubExpression(node)— shared predicate forcontext.githubmember expressions.isOctokitSourceExpression(node)— syntactically recognizes known names (github,octokit,githubClient,octokitClient), anygetOctokit(...)call, andcontext.github.isIdentifierBoundToOctokitClient(name, scopeNode)— walks ESLint's scope chain to check if a variable is bound to an Octokit source via a simpleconst/letassignment (single level; does not broaden to all.request()callers).resolveOctokitClientName(calleeObject, callNode)— dispatcher returning the display name for the error message, ornullto skip.The
CallExpressionvisitor now usesresolveOctokitClientNameinstead of the hardIdentifier-only guard, so these are now flagged:http.request(...)/https.request(...)and aliases bound to unrelated sources remain unflagged — the fix is scope-aware, not name-broadening.Test & docs updates:
context.github.request(...)moved from valid → invalid test.http.request(...),https.request(...), and non-Octokit aliases.