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..4f0d02119 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,151 @@ 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([]); + + const shadowed = sample([ + "import { rm } from 'node:fs';", + 'const cleanup = async () => {', + ' const rm = async () => undefined;', + ` await rm(root, { ${recursiveTrue} });`, + '};', + ]); + expect(recursiveRmCalls(shadowed)).toEqual([]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/example.test.ts', shadowed)).toEqual([]); + + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs';", + `await rm(root, { ${recursiveTrue} });`, + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 2 })]); +}); + +it('ignores loop, switch-case, and named function expression shadows only in their scope', () => { + expect(recursiveRmCalls(sample([ + "import { promises as fs } from 'node:fs';", + `for (const fs of mockFilesystems) await fs.rm(root, { ${recursiveTrue} });`, + `for (const [, fs] of entries) { await fs.rm(root, { ${recursiveTrue} }); }`, + `for (let fs = mock; fs; fs = undefined) await fs.rm(root, { ${recursiveTrue} });`, + `for (const fs in mocks) await fs.rm(root, { ${recursiveTrue} });`, + `await fs.rm(root, { ${recursiveTrue} });`, + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 6 })]); + + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + 'switch (mode) {', + ' case 1:', + ' const rm = mockRm;', + ` await rm(root, { ${recursiveTrue} });`, + ' break;', + ' default:', + ' const [rm2, rm] = mocks;', + ` await rm(root, { ${recursiveTrue} });`, + '}', + `const again = async function rm() { await rm(root, { ${recursiveTrue} }); };`, + `const inner = () => { const rm = mockRm; return rm(root, { ${recursiveTrue} }); };`, + `await rm(root, { ${recursiveTrue} });`, + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 13 })]); +}); + +it('does not let a parameter shadow a computed method key or decorator', () => { + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + 'const hooks = {', + ` [rm(root, { ${recursiveTrue} })](rm) {},`, + ` async cleanup(rm) { await rm(root, { ${recursiveTrue} }); },`, + '};', + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 3 })]); + + expect(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + 'class Suite {', + ` @hook(rm(root, { ${recursiveTrue} })) run(rm) {}`, + '}', + ]))).toEqual([expect.objectContaining({ hasRetries: false, line: 3 })]); +}); + +it('parses TSX and JS test files with their own script kind', () => { + const tsx = sample([ + "import { rm } from 'node:fs/promises';", + 'const view =
;', + `await rm(root, { ${recursiveTrue} });`, + ]); + expect(bareRecursiveRmFailures('packages/workbench/tests/view.test.tsx', tsx)).toEqual([ + 'packages/workbench/tests/view.test.tsx:3 bare recursive rm. Use removeTree.', + ]); + expect(bareRecursiveRmFailures('packages/agent-bundle/tests/fixture.mjs', sample([ + "import * as fs from 'node:fs/promises';", + `await fs.rm(root, { ${recursiveTrue} });`, + ]))).toEqual([ + 'packages/agent-bundle/tests/fixture.mjs:2 bare recursive rm. Use removeTree.', + ]); +}); + +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(recursiveRmCalls(sample([ + "import { rm } from 'node:fs/promises';", + `await rm(root, { ${recursiveTrue} });`, + ]))).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);`, + `await rm(root, ({ ${recursiveTrue}, maxRetries: 5 }));`, + `await rm(root, { ${recursiveTrue}, maxRetries: 5 } satisfies Options);`, + `await rm(root, { ${recursiveTrue}, maxRetries: 5 });`, + ]))).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..32bebb86c 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,127 @@ const propertyName = (name) => { return undefined; }; +const unwrapExpression = (node) => { + let current = node; + while ( + current !== undefined + && ( + ts.isParenthesizedExpression(current) + || ts.isAsExpression(current) + || ts.isSatisfiesExpression(current) + || ts.isTypeAssertionExpression(current) + ) + ) { + current = current.expression; + } + 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) => + ts.isBindingElement(element) && declarationNameIs(element.name, name)); + } + return false; +}; + +const declarationListDeclares = (list, name) => + list.declarations.some((decl) => declarationNameIs(decl.name, name)); + +const statementDeclares = (statement, name) => { + if (ts.isVariableStatement(statement)) { + return declarationListDeclares(statement.declarationList, name); + } + if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) { + return statement.name !== undefined && statement.name.text === name; + } + return false; +}; + +/** `from` is the child the walk came up through; names, computed keys, and decorators sit outside the function scope. */ +const functionLikeDeclares = (node, from, name) => { + if ( + !( + ts.isFunctionDeclaration(node) + || ts.isFunctionExpression(node) + || ts.isArrowFunction(node) + || ts.isMethodDeclaration(node) + || ts.isConstructorDeclaration(node) + ) + ) { + return false; + } + if (from !== node.body && !node.parameters.includes(from)) return false; + if ( + (ts.isFunctionDeclaration(node) || ts.isFunctionExpression(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`. + * ponytail: `var` is treated as block-scoped, so a `var` hoisted out of a nested + * block is missed and its call still fails the gate (false positive, never a + * false negative). Track function-scoped `var` if that ever bites. + */ +const identifierIsLocallyShadowed = (identifier) => { + const name = identifier.text; + let from = identifier; + let current = identifier.parent; + while (current !== undefined) { + if ( + ts.isSourceFile(current) + || ts.isBlock(current) + || ts.isModuleBlock(current) + || ts.isCaseClause(current) + || ts.isDefaultClause(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 ( + (ts.isForStatement(current) || ts.isForOfStatement(current) || ts.isForInStatement(current)) + && current.initializer !== undefined + && ts.isVariableDeclarationList(current.initializer) + && declarationListDeclares(current.initializer, name) + ) { + return true; + } + if (functionLikeDeclares(current, from, name)) return true; + if ( + ts.isCatchClause(current) + && current.variableDeclaration !== undefined + && declarationNameIs(current.variableDeclaration.name, name) + ) { + return true; + } + from = current; + 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 +248,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; }; @@ -147,7 +271,7 @@ const scriptKindFor = (fileName) => { }; export const recursiveRmCalls = (text, fileName = 'check.ts') => { - const { bareNames, namespaceNames } = removalBindings(text); + const { bareNames, namespaceNames } = removalBindings(text, fileName); const sourceFile = ts.createSourceFile( fileName, text,