Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/daily-byok-ollama-test.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

92 changes: 76 additions & 16 deletions eslint-factory/src/rules/core-method-resolve.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,79 @@
import { AST_NODE_TYPES, TSESLint, TSESTree } from "@typescript-eslint/utils";
import { CORE_ALIASES } from "./core-aliases";

/**
* Matches `@param {typeof import('@actions/core')} <name>` (single or double quotes)
* in a JSDoc block-comment value. Used as an unambiguous signal that a parameter
* is a dependency-injected `@actions/core`-like object.
*/
const JSDOC_CORE_PARAM_RE = /@param\s*\{typeof\s+import\(['"]@actions\/core['"]\)\}\s+([$\w]+)/g;

/**
* Returns true when the enclosing function carries a JSDoc block comment with
* `@param {typeof import('@actions/core')} <paramName>`.
* For function expressions / arrow functions assigned to a variable the JSDoc is
* typically before the VariableDeclaration, not the function node itself, so both
* positions are checked.
*/
function hasJSDocCoreParamAnnotation(functionNode: TSESTree.Node, paramName: string, sourceCode: TSESLint.SourceCode): boolean {
const nodesToCheck: TSESTree.Node[] = [functionNode];
const parent = functionNode.parent;
if (parent?.type === AST_NODE_TYPES.VariableDeclarator && parent.parent) {

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.

JSDoc lookup misses a real, used pattern in this codebase: export function/export async function (e.g. main).

💡 Details

hasJSDocCoreParamAnnotation only checks comments before functionNode itself, and, when the parent is a VariableDeclarator, before the enclosing VariableDeclaration. It never checks a preceding ExportNamedDeclaration/ExportDefaultDeclaration wrapper. For:

/**
 * `@param` {typeof import('`@actions/core`')} coreArg
 */
export async function main(coreArg) { coreArg.setFailed("x"); }

sourceCode.getCommentsBefore(functionNode) returns an empty array because the JSDoc precedes the ExportNamedDeclaration, not the FunctionDeclaration node — so the annotation is silently ignored and the rule falls through to false-negative behavior. This pattern is already used elsewhere in this codebase (e.g. export async function main() fixtures in no-core-error-then-process-exitcode.test.ts), so this is a concrete gap, not a hypothetical one. Fix: also check functionNode.parent when it is an ExportNamedDeclaration/ExportDefaultDeclaration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 60e26fahasJSDocCoreParamAnnotation now also pushes functionNode.parent onto nodesToCheck when the parent is an ExportNamedDeclaration or ExportDefaultDeclaration, so JSDoc before export async function main(coreArg) is correctly recognized. ESM valid and invalid test cases were added to both test files.

nodesToCheck.push(parent.parent);
}
// JSDoc before `export function` / `export async function` / `export default function`
// is attached to the ExportNamedDeclaration/ExportDefaultDeclaration, not the inner
// FunctionDeclaration/FunctionExpression, so check the export wrapper as well.
if (parent?.type === AST_NODE_TYPES.ExportNamedDeclaration || parent?.type === AST_NODE_TYPES.ExportDefaultDeclaration) {
nodesToCheck.push(parent);
}
for (const node of nodesToCheck) {
for (const comment of sourceCode.getCommentsBefore(node)) {
if (comment.type !== "Block" || !comment.value.startsWith("*")) continue;
JSDOC_CORE_PARAM_RE.lastIndex = 0;
let m: RegExpExecArray | null;
while ((m = JSDOC_CORE_PARAM_RE.exec(comment.value)) !== null) {
if (m[1] === paramName) return true;
}
}
}
return false;
}

/**
* Returns true when `identifier` resolves (via scope chain) to a function parameter
* that carries a JSDoc `@param {typeof import('@actions/core')}` annotation.
* Used as the slow-path for DI parameters not covered by CORE_ALIASES.
*/
function isJSDocCoreParamInScope(identifier: TSESTree.Identifier, sourceCode: TSESLint.SourceCode): boolean {
let currentScope: TSESLint.Scope.Scope | null = sourceCode.getScope(identifier);
while (currentScope !== null) {
const variable = currentScope.set.get(identifier.name);
if (variable !== undefined) {
if (variable.defs.length !== 1) return false;
const def = variable.defs[0];
if (def.type !== "Parameter" || def.name.type !== AST_NODE_TYPES.Identifier) return false;
return hasJSDocCoreParamAnnotation(def.node, identifier.name, sourceCode);
}
currentScope = currentScope.upper;
}
return false;
}

/**
* Checks whether an Identifier is a single-assignment alias for a core-like
* object (e.g., `const c = core`). Re-assigned let bindings are rejected.
* Local shadows (e.g., a parameter also named `c`) are excluded because they
* are found first in the scope chain and their definition type will not match.
*
* Additionally, a plain function parameter whose name is in CORE_ALIASES is
* accepted to support the dependency-injection pattern:
* Additionally, a plain function parameter is accepted to support the
* dependency-injection pattern:
* `async function f(core) { core.setFailed(msg); }`
* Gating: only parameters whose name exactly matches a known alias (i.e. is in
* CORE_ALIASES) are accepted, preventing arbitrary parameters from being treated
* as core. Destructured parameters (e.g. `{ core }`) are excluded.
* Two gating strategies prevent arbitrary parameters from being treated as core:
* 1. Fast path — parameter name is in CORE_ALIASES (exact known aliases).
* 2. Slow path — enclosing function has a JSDoc `@param {typeof import('@actions/core')}`
* annotation for this parameter (strong, unambiguous signal).
* Destructured parameters (e.g. `{ core }`) are excluded in both paths.
*/
export function isCoreAliasIdentifier(identifier: TSESTree.Identifier, sourceCode: TSESLint.SourceCode): boolean {
let currentScope: TSESLint.Scope.Scope | null = sourceCode.getScope(identifier);
Expand All @@ -22,21 +83,20 @@ export function isCoreAliasIdentifier(identifier: TSESTree.Identifier, sourceCod
if (variable.defs.length !== 1) return false;
const def = variable.defs[0];
if (def.type === "Parameter") {
// Accept a plain function-parameter whose name is a known core alias.
// This covers the DI pattern: `function f(core) { core.setFailed(...) }`.
// For Parameter defs, `def.node` is the enclosing function node; `def.name`
// is the binding pattern (Identifier for simple params, ObjectPattern for
// destructured params). We gate on `def.name.type === Identifier` to exclude
// destructured parameters, and on `CORE_ALIASES` to avoid false positives.
// `identifier.name` equals the parameter name because the scope look-up
// above resolved the variable by that name.
return def.name.type === AST_NODE_TYPES.Identifier && CORE_ALIASES.has(identifier.name);
// Only plain (non-destructured) parameters are accepted.
if (def.name.type !== AST_NODE_TYPES.Identifier) return false;
// Fast path: known exact alias names (e.g. `core`, `coreObj`).
if (CORE_ALIASES.has(identifier.name)) return true;
// Slow path: JSDoc @param {typeof import('@actions/core')} annotation on
// the enclosing function is an unambiguous signal for DI-style parameters
// with non-canonical names (e.g. `coreArg`, `coreLib`).
return hasJSDocCoreParamAnnotation(def.node, identifier.name, sourceCode);
Comment on lines +90 to +93

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9cfa1fa — the variable branch in isCoreAliasIdentifier now checks || isJSDocCoreParamInScope(declarator.init as TSESTree.Identifier, sourceCode) so const c = coreArg is recognized when coreArg is JSDoc-annotated.

}
if (def.type !== "Variable") return false;
if (variable.references.some(ref => ref.isWrite() && !ref.init)) return false;
const declarator = def.node as TSESTree.VariableDeclarator;
if (!declarator.init) return false;
return declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init.type === AST_NODE_TYPES.Identifier && CORE_ALIASES.has(declarator.init.name);
return declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init.type === AST_NODE_TYPES.Identifier && (CORE_ALIASES.has(declarator.init.name) || isJSDocCoreParamInScope(declarator.init as TSESTree.Identifier, sourceCode));
}
currentScope = currentScope.upper;
}
Expand All @@ -61,7 +121,7 @@ export function isDestructuredCoreMethodIdentifier(identifier: TSESTree.Identifi
if (variable.references.some(ref => ref.isWrite() && !ref.init)) return false;
const declarator = def.node as TSESTree.VariableDeclarator;
if (!declarator.init) return false;
if (declarator.id.type === AST_NODE_TYPES.ObjectPattern && declarator.init.type === AST_NODE_TYPES.Identifier && CORE_ALIASES.has(declarator.init.name)) {
if (declarator.id.type === AST_NODE_TYPES.ObjectPattern && declarator.init.type === AST_NODE_TYPES.Identifier && (CORE_ALIASES.has(declarator.init.name) || isJSDocCoreParamInScope(declarator.init, sourceCode))) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 9cfa1fa and 60e26fa — both test files now include valid/invalid cases for const { setFailed } = coreArg (CJS) and const { setOutput } = coreArg (CJS), exercising the destructuring branch directly.

return declarator.id.properties.some(prop => {
if (prop.type !== AST_NODE_TYPES.Property || prop.computed) return false;
const keyIsMethod = prop.key.type === AST_NODE_TYPES.Identifier && prop.key.name === methodName;
Expand Down
Loading
Loading