feat(compartment-mapper): add MapNodeModulesHooks.canonicalNames

- This hook just dumps all known canonical names.
- The `unknownCanonicalName` hook has changed its parameters.

Author: boneskull

packages/compartment-mapper/src/hooks.js

@@ -337,9 +337,9 @@ export const makeDefaultHookConfiguration = (
     // these hooks for mapNodeModules are only applied if a policy was provided.
     mapNodeModules: policy
       ? {
-          unknownCanonicalName: ({ canonicalName, issue, log }) => {
+          unknownCanonicalName: ({ canonicalName, message, log }) => {
             log(
-              `WARN: Invalid resource ${q(canonicalName)} in policy: ${issue}`,
+              `WARN: Invalid resource ${q(canonicalName)} in policy: ${message}`,
             );
           },
 

packages/compartment-mapper/src/node-modules.js

@@ -1189,53 +1189,59 @@ const finalizeGraph = (
  *
  * @param {Set<CanonicalName>} canonicalNames Set of all known canonical names
  * @param {SomePolicy} policy Policy to validate
- * @returns {Array<{canonicalName: CanonicalName, issue: string, path:
- * string[]}>} Array of issue objects, or `undefined` if no issues were
+ * @returns {Array<{canonicalName: CanonicalName, message: string, path:
+ * string[], suggestion?: CanonicalName}>} Array of issue objects, or `undefined` if no issues were
  * found
  */
 const validatePolicyResources = (canonicalNames, policy) => {
   /**
-   * Appends a suggestion to `issueMessage` if `badName` is a suffix of any
+   * Finds a suggestion for `badName` if it is a suffix of any
    * canonical name in `canonicalNames`.
    *
    * @param {string} badName Unknown canonical name
-   * @param {string} issueMessage Existing issue message
-   * @returns {string}
+   * @returns {CanonicalName | undefined}
    */
-  const makeSuggestion = (badName, issueMessage) => {
+  const findSuggestion = badName => {
     for (const canonicalName of canonicalNames) {
       if (canonicalName.endsWith(`>${badName}`)) {
-        issueMessage += `; did you mean ${q(canonicalName)}?`;
-        break;
+        return canonicalName;
       }
     }
-    return issueMessage;
+    return undefined;
   };
 
-  /** @type {Array<{canonicalName: CanonicalName, issue: string, path: string[]}>} */
+  /** @type {Array<{canonicalName: CanonicalName, message: string, path: string[], suggestion?: CanonicalName}>} */
   const issues = [];
   for (const [resourceName, resourcePolicy] of entries(
     policy.resources ?? {},
   )) {
     if (!canonicalNames.has(resourceName)) {
-      let issueMessage = `Resource ${q(resourceName)} was not found`;
-      issueMessage = makeSuggestion(resourceName, issueMessage);
-      issues.push({
+      const issueMessage = `Resource ${q(resourceName)} was not found`;
+      const suggestion = findSuggestion(resourceName);
+      const issue = {
         canonicalName: resourceName,
-        issue: issueMessage,
+        message: issueMessage,
         path: ['resources', resourceName],
-      });
+      };
+      if (suggestion) {
+        issue.suggestion = suggestion;
+      }
+      issues.push(issue);
     }
     if (typeof resourcePolicy?.packages === 'object') {
       for (const packageName of keys(resourcePolicy.packages)) {
         if (!canonicalNames.has(packageName)) {
-          let issueMessage = `Resource ${q(packageName)} from resource ${q(resourceName)} was not found`;
-          issueMessage = makeSuggestion(packageName, issueMessage);
-          issues.push({
+          const issueMessage = `Resource ${q(packageName)} from resource ${q(resourceName)} was not found`;
+          const suggestion = findSuggestion(packageName);
+          const issue = {
             canonicalName: packageName,
-            issue: issueMessage,
+            message: issueMessage,
             path: ['resources', resourceName, 'packages', packageName],
-          });
+          };
+          if (suggestion) {
+            issue.suggestion = suggestion;
+          }
+          issues.push(issue);
         }
       }
     }
@@ -1323,16 +1329,27 @@ export const compartmentMapForNodeModules_ = async (
   if (policy) {
     const canonicalNames = new Set(canonicalNameMap.keys());
     const issues = validatePolicyResources(canonicalNames, policy) ?? [];
-    for (const { issue, canonicalName, path } of issues) {
-      executeHook('unknownCanonicalName', {
+    for (const { message, canonicalName, path, suggestion } of issues) {
+      const hookInput = {
         canonicalName,
-        issue,
+        message,
         path,
         log,
-      });
+      };
+      if (suggestion) {
+        hookInput.suggestion = suggestion;
+      }
+      executeHook('unknownCanonicalName', hookInput);
     }
   }
 
+  // Fire canonicalNames hook with all canonical names before translateGraph
+  const canonicalNames = new Set(canonicalNameMap.keys());
+  executeHook('canonicalNames', {
+    canonicalNames,
+    log,
+  });
+
   const compartmentMap = translateGraph(
     entryPackageLocation,
     entryModuleSpecifier,

packages/compartment-mapper/src/policy-format.js

@@ -18,7 +18,9 @@
  *    ImplicitAttenuationDefinition,
  *    FullAttenuationDefinition,
  *    UnionToIntersection,
- *    PackageNamingKit
+ *    PackageNamingKit,
+ *    WildcardPolicy,
+ *    SomePolicy
  *} from './types.js'
  */
 

packages/compartment-mapper/src/types/hooks.ts

@@ -190,7 +190,16 @@ export type MapNodeModulesHooks = {
   unknownCanonicalName: HookFn<{
     canonicalName: CanonicalName;
     path: string[];
-    issue: string;
+    message: string;
+    suggestion?: CanonicalName;
+  }>;
+
+  /**
+   * Executed with all canonical names found in the compartment map.
+   * Called once before translateGraph.
+   */
+  canonicalNames: HookFn<{
+    canonicalNames: Readonly<Set<CanonicalName>>;
   }>;
 
   /**

packages/compartment-mapper/src/types/internal.ts

@@ -34,7 +34,6 @@ import type {
   LinkHooks,
   LogOptions,
   MakeImportHookMakerHooks,
-  MakeModuleMapHookHooks,
   ModuleTransforms,
   ParseFn,
   ParserForLanguage,

packages/compartment-mapper/test/map-node-modules.hooks.test.js

@@ -537,13 +537,13 @@ test('mapNodeModules - packageDependencies hook no modification (snapshot)', asy
 test('mapNodeModules - unknownCanonicalName hook called for missing policy resources', async t => {
   t.plan(6);
 
-  /** @type {Array<{canonicalName: CanonicalName, path: string[], issue: string}>} */
+  /** @type {Array<{canonicalName: CanonicalName, path: string[], message: string}>} */
   const hookCalls = [];
 
   /** @type {HookConfiguration<MapNodeModulesHooks>} */
   const hooks = {
-    unknownCanonicalName: ({ canonicalName, path, issue }) => {
-      hookCalls.push({ canonicalName, path, issue });
+    unknownCanonicalName: ({ canonicalName, path, message }) => {
+      hookCalls.push({ canonicalName, path, message });
     },
   };
 
@@ -589,10 +589,10 @@ test('mapNodeModules - unknownCanonicalName hook called for missing policy resou
     'should provide correct path for unknown resource',
   );
   t.true(
-    unknownResourceCall?.issue.includes(
+    unknownResourceCall?.message.includes(
       'Resource "unknown-package" was not found',
     ),
-    'should provide descriptive issue message for unknown resource',
+    'should provide descriptive message for unknown resource',
   );
 
   // Check hook call for unknown nested package
@@ -625,3 +625,121 @@ test('mapNodeModules - unknownCanonicalName hook not called when all resources e
 
   t.false(hookCalled, 'should not call hook when all policy resources exist');
 });
+
+test('mapNodeModules - unknownCanonicalName hook includes suggestions when available', async t => {
+  /** @type {Array<{canonicalName: CanonicalName, path: string[], message: string, suggestion?: CanonicalName}>} */
+  const hookCalls = [];
+
+  /** @type {HookConfiguration<MapNodeModulesHooks>} */
+  const hooks = {
+    unknownCanonicalName: ({ canonicalName, path, message, suggestion }) => {
+      hookCalls.push({ canonicalName, path, message, suggestion });
+    },
+  };
+
+  // Policy with typos that should trigger suggestions
+  /** @type {SomePolicy} */
+  const policyWithTypo = {
+    entry: {
+      packages: WILDCARD_POLICY_VALUE,
+    },
+    resources: {
+      'dep-aa': {
+        // Not close enough to 'dep-a' to suggest, but contains 'dep-c'
+        // which should suggest 'dep-a>dep-c'
+        packages: {
+          'dep-c': true,
+        },
+      },
+    },
+  };
+
+  const readPowers = makeProjectFixtureReadPowers(testFixture);
+  await mapNodeModules(readPowers, testFixture.entrypoint, {
+    hooks,
+    policy: policyWithTypo,
+  });
+
+  t.is(
+    hookCalls.length,
+    2,
+    'should call hook twice for both unknown resources',
+  );
+
+  // Check the call for the unknown top-level resource (no close suggestion)
+  const unknownResourceCall = hookCalls.find(
+    call => call.canonicalName === 'dep-aa',
+  );
+  t.truthy(unknownResourceCall, 'should call hook for unknown resource');
+  t.deepEqual(
+    unknownResourceCall?.path,
+    ['resources', 'dep-aa'],
+    'should provide correct path for unknown resource',
+  );
+  t.true(
+    unknownResourceCall?.message.includes('Resource "dep-aa" was not found'),
+    'should provide descriptive message for unknown resource',
+  );
+  t.is(
+    unknownResourceCall?.suggestion,
+    undefined,
+    'should not suggest when no close match exists',
+  );
+
+  // Check the call for the nested package (should have suggestion)
+  const nestedPackageCall = hookCalls.find(
+    call => call.canonicalName === 'dep-c',
+  );
+  t.truthy(nestedPackageCall, 'should call hook for nested unknown package');
+  t.deepEqual(
+    nestedPackageCall?.path,
+    ['resources', 'dep-aa', 'packages', 'dep-c'],
+    'should provide correct path for nested unknown package',
+  );
+  t.true(
+    nestedPackageCall?.message.includes(
+      'Resource "dep-c" from resource "dep-aa" was not found',
+    ),
+    'should provide descriptive message for nested unknown package',
+  );
+  t.is(
+    nestedPackageCall?.suggestion,
+    'dep-a>dep-c',
+    'should suggest the closest matching canonical name',
+  );
+});
+
+test('mapNodeModules - canonicalNames hook called with all canonical names', async t => {
+  t.plan(1);
+
+  /** @type {Set<CanonicalName>} */
+  let receivedCanonicalNames = new Set();
+
+  /** @type {HookConfiguration<MapNodeModulesHooks>} */
+  const hooks = {
+    canonicalNames: ({ canonicalNames }) => {
+      receivedCanonicalNames = new Set(canonicalNames);
+    },
+  };
+
+  const readPowers = makeProjectFixtureReadPowers(testFixture);
+  await mapNodeModules(readPowers, testFixture.entrypoint, { hooks });
+
+  // Expected canonical names based on the test fixture:
+  // - $root$ (the entry package 'app' becomes '$root$')
+  // - dep-a (direct dependency)
+  // - dep-b (direct dependency)
+  // - dep-a>dep-c (transitive dependency through dep-a)
+  const expectedCanonicalNames = new Set([
+    '$root$',
+    'dep-a',
+    'dep-b',
+    'dep-a>dep-c',
+  ]);
+
+  t.deepEqual(
+    receivedCanonicalNames,
+    expectedCanonicalNames,
+    'should receive exactly the expected canonical names from the project fixture',
+  );
+});

packages/compartment-mapper/test/policy.test.js

@@ -107,6 +107,13 @@ const defaultExpectations = {
     builtins2: '{"c":3,"default":{"c":3}}',
   }),
 };
+const anyExpectations = {
+  namespace: moduleify({
+    ...defaultExpectations.namespace,
+    carol: { bluePill: 'number', redPill: 'number', purplePill: 'number' },
+  }),
+};
+
 
 const powerlessCarolExpectations = {
   namespace: moduleify({
@@ -173,15 +180,12 @@ scaffold(
   'policy - enforcement with "any" policy',
   test,
   fixture,
-  assertTestAlwaysThrows,
+  combineAssertions(
+    makeResultAssertions(anyExpectations),
+    assertExternalModuleNotFound,
+  ),
   2, // expected number of assertions
   {
-    shouldFailBeforeArchiveOperations: true,
-    onError: (t, { error }) => {
-      t.regex(error.message, /unknown resources found in policy/i);
-      // see the snapshot for the error hint in the message
-      t.snapshot(sanitizePaths(error.message));
-    },
     addGlobals: globals,
     policy: anyPolicy,
   },
@@ -195,10 +199,16 @@ scaffold(
   2, // expected number of assertions
   {
     shouldFailBeforeArchiveOperations: true,
-    onError: (t, { error }) => {
-      t.regex(error.message, /unknown resources found in policy/i);
+    onError: (t, { error, testCategoryHint }) => {
+      if (testCategoryHint === 'Archive') {
+        t.regex(error.message, /unknown resources found in policy/i);
+        t.snapshot(sanitizePaths(error.message), 'archive case error message');
+      } else {
+        t.regex(error.message, /cannot find external module/i);
+        t.snapshot(sanitizePaths(error.message), 'location case error message');
+      }
       // see the snapshot for the error hint in the message
-      t.snapshot(sanitizePaths(error.message));
+      
     },
     addGlobals: globals,
     policy: {
@@ -437,16 +447,19 @@ scaffold(
   'policy - nested export in attenuator',
   test,
   fixture,
-  assertTestAlwaysThrows,
-  1, // expected number of assertions
+  combineAssertions(
+    makeResultAssertions(defaultExpectations),
+    assertExternalModuleNotFound,
+  ),
+  2, // expected number of assertions
   {
-    shouldFailBeforeArchiveOperations: true,
-    onError: (t, { error }) => {
-      t.regex(
-        error.message,
-        /Resource "myattenuator\/attenuate" was not found/i,
-      );
-    },
+    // shouldFailBeforeArchiveOperations: true,
+    // onError: (t, { error }) => {
+    //   t.regex(
+    //     error.message,
+    //     /Resource "myattenuator\/attenuate" was not found/i,
+    //   );
+    // },
     addGlobals: globals,
     policy: nestedAttenuator(policy),
   },