From aeb8c3f1c3e7e8e1d097f7afa84b8985503d142c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 17 Sep 2026 05:15:21 +0000 Subject: [PATCH] fix(test): close remove-tree lint AST false positives and negatives Recognize shorthand maxRetries, shadowed local rm, named promises rebinds from fs/node:fs, and assertion/paren/satisfies option wrappers. Co-authored-by: Zack Jackson --- .../tests/check-test-remove-tree.test.ts | 81 ++++++++++++ scripts/check-test-remove-tree.d.mts | 2 +- scripts/check-test-remove-tree.mjs | 121 ++++++++++++++++-- 3 files changed, 192 insertions(+), 12 deletions(-) diff --git a/packages/agent-bundle/tests/check-test-remove-tree.test.ts b/packages/agent-bundle/tests/check-test-remove-tree.test.ts index 2b6458b8e..93654527e 100644 --- a/packages/agent-bundle/tests/check-test-remove-tree.test.ts +++ b/packages/agent-bundle/tests/check-test-remove-tree.test.ts @@ -240,3 +240,84 @@ it('still gates aliased and namespace Node fs.rm without maxRetries', () => { `await fs.rm(path, { ${recursiveTrue}, maxRetries: 5 });`, ]))).toEqual([]); }); + +it('accepts shorthand maxRetries and ignores a shadowed local rm', () => { + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + 'const maxRetries = 5;', + `await rm(root, { ${recursiveTrue}, maxRetries });`, + ]))).toEqual([expect.objectContaining({ hasRetries: true, line: 3 })]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { rm } from 'node:fs/promises';", + 'const maxRetries = 5;', + `await rm(root, { ${recursiveTrue}, maxRetries });`, + ]))).toEqual([]); + + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs';", + 'const rm = async () => undefined;', + `await rm(root, { ${recursiveTrue} });`, + ]))).toEqual([]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { rm } from 'node:fs';", + 'const rm = async () => undefined;', + `await rm(root, { ${recursiveTrue} });`, + ]))).toEqual([]); + + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs';", + `await rm(root, { ${recursiveTrue} });`, + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 2 })]); +}); + +it('flags promises-namespace and asserted options without maxRetries', () => { + expect(removalBindings(`import { promises as fs } from 'node:fs';`)).toEqual({ + bareNames: new Set(), + namespaceNames: new Set(['fs']), + }); + expect(removalBindings(`import { promises as fs } from 'fs';`)).toEqual({ + bareNames: new Set(), + namespaceNames: new Set(['fs']), + }); + + const promisesNs = recursiveRmCalls(sample([ + "import { promises as fs } from 'node:fs';", + `await fs.rm(path, { ${recursiveTrue} });`, + ])); + expect(promisesNs).toEqual([expect.objectContaining({ hasRetries: false, line: 2 })]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { promises as fs } from 'node:fs';", + `await fs.rm(path, { ${recursiveTrue} });`, + ]))).toEqual([ + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + ]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { promises as fs } from 'node:fs';", + `await fs.rm(path, { ${recursiveTrue}, maxRetries: 5 });`, + ]))).toEqual([]); + + const asserted = recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + `await rm(root, { ${recursiveTrue} } as const);`, + ])); + expect(asserted).toEqual([expect.objectContaining({ hasRetries: false, line: 2 })]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { rm } from 'node:fs/promises';", + `await rm(root, { ${recursiveTrue} } as const);`, + ]))).toEqual([ + 'packages/agent-bundle/tests/example.test.ts:2 bare recursive rm. Use removeTree.', + ]); + + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + `await rm(root, ({ ${recursiveTrue} }));`, + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 2 })]); + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + `await rm(root, { ${recursiveTrue} } satisfies Options);`, + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 2 })]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', sample([ + "import { rm } from 'node:fs/promises';", + `await rm(root, { ${recursiveTrue}, maxRetries: 5 } as const);`, + ]))).toEqual([]); +}); diff --git a/scripts/check-test-remove-tree.d.mts b/scripts/check-test-remove-tree.d.mts index d7056e395..883389100 100644 --- a/scripts/check-test-remove-tree.d.mts +++ b/scripts/check-test-remove-tree.d.mts @@ -8,7 +8,7 @@ export interface RecursiveRmCall { export interface RemovalBindings { /** Local names bound to `rm` from node:fs or node:fs/promises, including aliases. */ readonly bareNames: ReadonlySet; - /** Namespace and default import names whose `.rm` is node's. */ + /** Namespace, default, and `promises` rebind names whose `.rm` is node's. */ readonly namespaceNames: ReadonlySet; } diff --git a/scripts/check-test-remove-tree.mjs b/scripts/check-test-remove-tree.mjs index a63b1a9e3..42fdd6f4b 100644 --- a/scripts/check-test-remove-tree.mjs +++ b/scripts/check-test-remove-tree.mjs @@ -8,9 +8,12 @@ * Call, option, and import-binding detection is parser-backed (typescript-5): * only real node:fs(/promises) ImportDeclaration bindings count, only Node-bound * call expressions are considered, and `recursive` / `maxRetries` are read from - * the second argument's object-literal properties (including quoted keys). Nested + * the second argument's object-literal properties (including quoted keys, + * shorthand `maxRetries`, and Parenthesized / As / Satisfies wrappers). Nested * objects in the path argument, member calls, comments, strings, regexes, and * template substitutions are handled by the AST rather than text masking. + * Named `promises` rebinds from `fs` / `node:fs` count as `.rm` carriers. + * A later local binding that shadows an import is not treated as Node-bound. */ import { createRequire } from 'node:module'; import { readdir, readFile } from 'node:fs/promises'; @@ -49,7 +52,7 @@ const walk = async (directory, files) => { }; /** - * Named/aliased rm bindings and namespace/default bindings that expose .rm. + * Named/aliased rm bindings and namespace/default/`promises` bindings that expose .rm. * Import bindings are collected from the TypeScript AST so comments and local * identifiers cannot forge Node fs.rm bindings. */ @@ -87,15 +90,20 @@ export const removalBindings = (text, fileName = 'bindings.ts') => { } if (!ts.isNamedImports(bindings)) continue; + const isFsRoot = /^(?:node:)?fs$/u.test(statement.moduleSpecifier.text); for (const element of bindings.elements) { if (element.isTypeOnly) continue; - if (element.propertyName !== undefined) { - if (element.propertyName.text !== 'rm') continue; - bareNames.add(element.name.text); + const importedName = element.propertyName === undefined + ? element.name.text + : element.propertyName.text; + const localName = element.name.text; + if (importedName === 'rm') { + bareNames.add(localName); continue; } - if (element.name.text !== 'rm') continue; - bareNames.add('rm'); + if (importedName === 'promises' && isFsRoot) { + namespaceNames.add(localName); + } } } @@ -108,14 +116,102 @@ const propertyName = (name) => { return undefined; }; +const unwrapExpression = (node) => { + let current = node; + while (current !== undefined) { + if ( + ts.isParenthesizedExpression(current) + || ts.isAsExpression(current) + || ts.isSatisfiesExpression(current) + || ts.isTypeAssertionExpression(current) + ) { + current = current.expression; + continue; + } + break; + } + return current; +}; + +const declarationNameIs = (nameNode, name) => { + if (ts.isIdentifier(nameNode)) return nameNode.text === name; + if (ts.isObjectBindingPattern(nameNode) || ts.isArrayBindingPattern(nameNode)) { + return nameNode.elements.some((element) => { + if (ts.isOmittedExpression(element) || ts.isIdentifier(element)) { + return ts.isIdentifier(element) && element.text === name; + } + return ts.isBindingElement(element) && declarationNameIs(element.name, name); + }); + } + return false; +}; + +const statementDeclares = (statement, name) => { + if (ts.isVariableStatement(statement)) { + return statement.declarationList.declarations.some((decl) => declarationNameIs(decl.name, name)); + } + if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) { + return statement.name !== undefined && statement.name.text === name; + } + return false; +}; + +const functionLikeDeclares = (node, name) => { + if ( + !( + ts.isFunctionDeclaration(node) + || ts.isFunctionExpression(node) + || ts.isArrowFunction(node) + || ts.isMethodDeclaration(node) + || ts.isConstructorDeclaration(node) + ) + ) { + return false; + } + if (ts.isFunctionDeclaration(node) && node.name !== undefined && node.name.text === name) { + return true; + } + return node.parameters.some((parameter) => declarationNameIs(parameter.name, name)); +}; + +/** True when a later/inner local binding hides the Node fs import of `name`. */ +const identifierIsLocallyShadowed = (identifier) => { + const name = identifier.text; + let current = identifier.parent; + while (current !== undefined) { + if (ts.isSourceFile(current) || ts.isBlock(current) || ts.isModuleBlock(current)) { + const shadowed = current.statements.some((statement) => { + if (ts.isSourceFile(current) && ts.isImportDeclaration(statement)) return false; + return statementDeclares(statement, name); + }); + if (shadowed) return true; + } + if (functionLikeDeclares(current, name)) return true; + if ( + ts.isCatchClause(current) + && current.variableDeclaration !== undefined + && declarationNameIs(current.variableDeclaration.name, name) + ) { + return true; + } + current = current.parent; + } + return false; +}; + /** Options flags from a call's second-argument object literal only. */ const optionsFlags = (optionsArg) => { - if (optionsArg === undefined || !ts.isObjectLiteralExpression(optionsArg)) { + const unwrapped = unwrapExpression(optionsArg); + if (unwrapped === undefined || !ts.isObjectLiteralExpression(unwrapped)) { return { recursive: false, hasRetries: false }; } let recursive = false; let hasRetries = false; - for (const property of optionsArg.properties) { + for (const property of unwrapped.properties) { + if (ts.isShorthandPropertyAssignment(property)) { + if (property.name.text === 'maxRetries') hasRetries = true; + continue; + } if (!ts.isPropertyAssignment(property)) continue; const key = propertyName(property.name); if (key === 'recursive' && property.initializer.kind === ts.SyntaxKind.TrueKeyword) { @@ -127,14 +223,17 @@ const optionsFlags = (optionsArg) => { }; const isNodeBoundRmCall = (expression, bareNames, namespaceNames) => { - if (ts.isIdentifier(expression)) return bareNames.has(expression.text); + if (ts.isIdentifier(expression)) { + return bareNames.has(expression.text) && !identifierIsLocallyShadowed(expression); + } if ( ts.isPropertyAccessExpression(expression) && !expression.questionDotToken && expression.name.text === 'rm' && ts.isIdentifier(expression.expression) ) { - return namespaceNames.has(expression.expression.text); + return namespaceNames.has(expression.expression.text) + && !identifierIsLocallyShadowed(expression.expression); } return false; };