fix(from-assert): address PR review feedback

- Refuse to transform loose equality (equal/notEqual) in strict mode
  (behavior change is too risky; must use node:assert/strict)
- Strip trailing message arguments (bupkis doesn't support them)
- Fix CLI message to reference warnings instead of non-existent TODOs
- Fix exclude glob handling to properly match directory segments
- Include import modifications in the 'modified' check for file saving

Author: boneskull

packages/from-assert/src/cli.ts

@@ -116,7 +116,7 @@ const printResult = (result: TransformResult): void => {
 
   if (result.totalWarnings > 0) {
     console.log(
-      `${ansi.yellow}Note: Search for "TODO: Manual migration needed" in your code for items requiring manual review.${ansi.reset}`,
+      `${ansi.yellow}Note: Review the warnings above and the referenced files for assertions that may require manual migration.${ansi.reset}`,
     );
   }
 };

packages/from-assert/src/transform.ts

@@ -155,10 +155,20 @@ export const transform = async (
   for (const sourceFile of project.getSourceFiles()) {
     const filePath = sourceFile.getFilePath();
 
-    // Skip excluded files
-    if (
-      exclude.some((pattern) => filePath.includes(pattern.replace('**/', '')))
-    ) {
+    // Skip excluded files using simple substring matching for common patterns
+    // This handles patterns like '**/node_modules/**' by checking for '/node_modules/'
+    const shouldExclude = exclude.some((pattern) => {
+      // Extract the core directory/file name from glob patterns
+      const corePath = pattern
+        .replace(/^\*\*\//, '') // Remove leading **/
+        .replace(/\/\*\*$/, '') // Remove trailing /**
+        .replace(/\*/g, ''); // Remove remaining wildcards
+
+      // Check if the path contains the core pattern as a directory segment
+      return corePath && filePath.includes(`/${corePath}/`);
+    });
+
+    if (shouldExclude) {
       continue;
     }
 
@@ -182,10 +192,13 @@ export const transform = async (
     });
     warnings.push(...importResult.warnings);
 
+    // File is modified if either assertions or imports were changed
+    const fileModified = fileTransformCount > 0 || importResult.modified;
+
     const fileResult: FileTransformResult = {
       errors,
       filePath,
-      modified: fileTransformCount > 0,
+      modified: fileModified,
       transformCount: fileTransformCount,
       warnings,
     };
@@ -195,7 +208,7 @@ export const transform = async (
     totalWarnings += warnings.length;
     totalErrors += errors.length;
 
-    if (fileTransformCount > 0) {
+    if (fileModified) {
       modifiedFiles++;
       if (write) {
         await sourceFile.save();

packages/from-assert/src/transformers/assert-transformer.ts

@@ -184,24 +184,38 @@ const transformSingleAssert = (
   }
 
   // Determine subject and matcher args
+  // Note: node:assert methods often have an optional trailing 'message' argument
+  // that bupkis doesn't support, so we strip it for most assertions
   const subject = args[0] ?? '';
-  const matcherArgs = args.slice(1);
+  const allMatcherArgs = args.slice(1);
 
-  // Check for legacy loose assertions and add warning
+  // For most assertions, only take the expected value, not the message
+  // 'ok' assertions take only a value with no expected arg
+  const hasExpectedArg = baseMatcher !== 'ok';
+  const matcherArgs = hasExpectedArg ? allMatcherArgs.slice(0, 1) : [];
+
+  // Check for legacy loose assertions
   // Note: baseMatcher will be 'equal' for both 'equal' and 'notEqual' (via getBaseMethod)
   if (
     assertStyle === 'legacy' &&
     isLegacyMethod(baseMatcher) &&
     baseMatcher === 'equal'
   ) {
-    // In legacy mode with loose equality, transform but return a warning
+    // In strict mode, refuse to transform loose equality (behavior change is too risky)
+    if (mode === 'strict') {
+      throw new Error(
+        `Loose equality assertion '${method}' cannot be safely transformed - use node:assert/strict`,
+      );
+    }
+
+    // In best-effort mode, transform but return a warning
     // The warning alerts the user that behavior may differ (loose vs strict equality)
     const phrase = negated
       ? `not ${transform.bupkisPhrase}`
       : transform.bupkisPhrase;
     const newCode =
       matcherArgs.length > 0
-        ? `expect(${subject}, '${phrase}', ${matcherArgs.join(', ')})`
+        ? `expect(${subject}, '${phrase}', ${matcherArgs[0]})`
         : `expect(${subject}, '${phrase}')`;
     call.replaceWithText(newCode);
 
@@ -257,10 +271,10 @@ const transformSingleAssert = (
 
   if (matcherArgs.length > 0) {
     // Handle special case for rejects with Error type
-    if (baseMatcher === 'rejects' && matcherArgs.length > 0 && !negated) {
-      newCode = `${expectFn}(${subject}, 'to reject with', ${matcherArgs.join(', ')})`;
+    if (baseMatcher === 'rejects' && !negated) {
+      newCode = `${expectFn}(${subject}, 'to reject with', ${matcherArgs[0]})`;
     } else {
-      newCode = `${expectFn}(${subject}, '${phrase}', ${matcherArgs.join(', ')})`;
+      newCode = `${expectFn}(${subject}, '${phrase}', ${matcherArgs[0]})`;
     }
   } else {
     newCode = `${expectFn}(${subject}, '${phrase}')`;