Add all manifest validation logic as separate rules (#2572)

packages/examples/packages/webpack-plugin/webpack.config.ts

@@ -7,7 +7,7 @@ const config: Configuration = {
   entry: './src/index.ts',
   mode: 'production',
   devtool: 'source-map',
-  stats: 'errors-only',
+  stats: 'errors-warnings',
 
   output: {
     filename: 'bundle.js',

packages/snaps-browserify-plugin/src/plugin.test.ts

@@ -7,10 +7,7 @@ import {
   logWarning,
   PostProcessWarning,
 } from '@metamask/snaps-utils/node';
-import {
-  DEFAULT_SNAP_BUNDLE,
-  getSnapManifest,
-} from '@metamask/snaps-utils/test-utils';
+import { DEFAULT_SNAP_BUNDLE } from '@metamask/snaps-utils/test-utils';
 import type { Options as BrowserifyOptions } from 'browserify';
 import browserify from 'browserify';
 import concat from 'concat-stream';
@@ -169,39 +166,48 @@ describe('plugin', () => {
   it('checks the manifest if configured', async () => {
     const mock = checkManifest as jest.MockedFunction<typeof checkManifest>;
     mock.mockResolvedValue({
-      manifest: getSnapManifest(),
-      warnings: [],
-      errors: [],
+      files: undefined,
+      updated: false,
+      reports: [],
     });
 
     await bundle({ options: { eval: false, manifestPath: '/foo' } });
 
     expect(mock).toHaveBeenCalledTimes(1);
-    expect(mock).toHaveBeenCalledWith('/', true, expect.any(String));
+    expect(mock).toHaveBeenCalledWith('/', {
+      updateAndWriteManifest: true,
+      sourceCode: expect.any(String),
+    });
   });
 
   it('does not fix the manifest if configured', async () => {
     const mock = checkManifest as jest.MockedFunction<typeof checkManifest>;
     mock.mockResolvedValue({
-      manifest: getSnapManifest(),
-      warnings: [],
-      errors: [],
+      files: undefined,
+      updated: false,
+      reports: [],
     });
 
     await bundle({
       options: { eval: false, manifestPath: '/foo', writeManifest: false },
     });
 
     expect(mock).toHaveBeenCalledTimes(1);
-    expect(mock).toHaveBeenCalledWith('/', false, expect.any(String));
+    expect(mock).toHaveBeenCalledWith('/', {
+      updateAndWriteManifest: false,
+      sourceCode: expect.any(String),
+    });
   });
 
   it('logs manifest errors if writeManifest is disabled and exits with error code 1', async () => {
     const mock = checkManifest as jest.MockedFunction<typeof checkManifest>;
     mock.mockResolvedValue({
-      manifest: getSnapManifest(),
-      warnings: [],
-      errors: ['foo', 'bar'],
+      files: undefined,
+      updated: false,
+      reports: [
+        { message: 'foo', severity: 'error' },
+        { message: 'bar', severity: 'error' },
+      ],
     });
 
     await expect(
@@ -214,9 +220,12 @@ describe('plugin', () => {
   it('logs manifest warnings', async () => {
     const mock = checkManifest as jest.MockedFunction<typeof checkManifest>;
     mock.mockResolvedValue({
-      manifest: getSnapManifest(),
-      warnings: ['foo', 'bar'],
-      errors: [],
+      files: undefined,
+      updated: false,
+      reports: [
+        { message: 'foo', severity: 'warning' },
+        { message: 'bar', severity: 'warning' },
+      ],
     });
 
     await bundle({
@@ -228,6 +237,26 @@ describe('plugin', () => {
     expect(logWarning).toHaveBeenCalledWith('Manifest Warning: bar');
   });
 
+  it('logs fixed problems', async () => {
+    const mock = checkManifest as jest.MockedFunction<typeof checkManifest>;
+    mock.mockResolvedValue({
+      files: undefined,
+      updated: true,
+      reports: [
+        { message: 'foo', severity: 'error', wasFixed: true },
+        { message: 'bar', severity: 'warning', wasFixed: true },
+      ],
+    });
+
+    await bundle({
+      options: { eval: false, manifestPath: 'foo' },
+    });
+
+    expect(logWarning).toHaveBeenCalledTimes(3);
+    expect(logWarning).toHaveBeenCalledWith('Manifest Problem Fixed: foo');
+    expect(logWarning).toHaveBeenCalledWith('Manifest Problem Fixed: bar');
+  });
+
   it('forwards errors', async () => {
     const mock = evalBundle as jest.MockedFunction<typeof evalBundle>;
     mock.mockRejectedValue(new Error('foo'));

packages/snaps-browserify-plugin/src/plugin.ts

@@ -33,15 +33,27 @@ async function postBundle(options: Partial<Options>, code: string) {
   }
 
   if (options.manifestPath) {
-    const { errors, warnings } = await checkManifest(
+    const { reports } = await checkManifest(
       pathUtils.dirname(options.manifestPath),
-      options.writeManifest,
-      code,
+      {
+        sourceCode: code,
+        updateAndWriteManifest: options.writeManifest,
+      },
     );
 
-    if (!options.writeManifest && errors.length > 0) {
+    const errorsUnfixed = reports
+      .filter((report) => report.severity === 'error' && !report.wasFixed)
+      .map((report) => report.message);
+    const warnings = reports
+      .filter((report) => report.severity === 'warning' && !report.wasFixed)
+      .map((report) => report.message);
+    const fixed = reports
+      .filter((report) => report.wasFixed)
+      .map((report) => report.message);
+
+    if (errorsUnfixed.length > 0) {
       throw new Error(
-        `Manifest Error: The manifest is invalid.\n${errors.join('\n')}`,
+        `Manifest Error: The manifest is invalid.\n${errorsUnfixed.join('\n')}`,
       );
     }
 
@@ -52,6 +64,14 @@ async function postBundle(options: Partial<Options>, code: string) {
 
       warnings.forEach((warning) => logWarning(`Manifest Warning: ${warning}`));
     }
+
+    if (fixed.length > 0) {
+      logWarning(
+        `Manifest Warning: Validation of snap.manifest.json fixed following problems.`,
+      );
+
+      fixed.forEach((error) => logWarning(`Manifest Problem Fixed: ${error}`));
+    }
   }
 }
 

packages/snaps-cli/src/commands/build/implementation.test.ts

@@ -93,7 +93,7 @@ describe('build', () => {
 
   it('builds the snap bundle using Webpack', async () => {
     jest.spyOn(process, 'cwd').mockReturnValue('/snap');
-    const log = jest.spyOn(console, 'log').mockImplementation();
+    const warn = jest.spyOn(console, 'warn').mockImplementation();
 
     const config = getMockConfig('webpack', {
       input: '/snap/input.js',
@@ -113,8 +113,9 @@ describe('build', () => {
 
     await build(config);
 
-    expect(log).toHaveBeenCalledWith(
-      expect.stringMatching(/Compiled 1 file in \d+ms\./u),
+    // Manifest checksum mismatch is the warning
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringMatching(/Compiled 1 file in \d+ms with 1 warning\./u),
     );
 
     const output = await fs.readFile('/snap/output.js', 'utf8');
@@ -125,7 +126,7 @@ describe('build', () => {
 
   it('builds an unminimized snap bundle using Webpack', async () => {
     jest.spyOn(process, 'cwd').mockReturnValue('/snap');
-    const log = jest.spyOn(console, 'log').mockImplementation();
+    const warn = jest.spyOn(console, 'warn').mockImplementation();
 
     const config = getMockConfig('webpack', {
       input: '/snap/input.js',
@@ -146,8 +147,9 @@ describe('build', () => {
 
     await build(config);
 
-    expect(log).toHaveBeenCalledWith(
-      expect.stringMatching(/Compiled 1 file in \d+ms\./u),
+    // Manifest checksum mismatch is the warning
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringMatching(/Compiled 1 file in \d+ms with 1 warning\./u),
     );
 
     const output = await fs.readFile('/snap/output.js', 'utf8');
@@ -191,7 +193,7 @@ describe('build', () => {
 
   it('builds the snap bundle using a legacy config', async () => {
     jest.spyOn(process, 'cwd').mockReturnValue('/snap');
-    const log = jest.spyOn(console, 'log').mockImplementation();
+    const warn = jest.spyOn(console, 'warn').mockImplementation();
 
     const config = getMockConfig('browserify', {
       cliOptions: {
@@ -204,8 +206,9 @@ describe('build', () => {
 
     await build(config);
 
-    expect(log).toHaveBeenCalledWith(
-      expect.stringMatching(/Compiled 1 file in \d+ms\./u),
+    // Manifest checksum mismatch is the warning
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringMatching(/Compiled 1 file in \d+ms with 1 warning\./u),
     );
 
     const output = await fs.readFile('/snap/output.js', 'utf8');
@@ -224,7 +227,7 @@ describe('build', () => {
 
   it('builds an unminimized snap bundle using a legacy config', async () => {
     jest.spyOn(process, 'cwd').mockReturnValue('/snap');
-    const log = jest.spyOn(console, 'log').mockImplementation();
+    const warn = jest.spyOn(console, 'warn').mockImplementation();
 
     const config = getMockConfig('browserify', {
       cliOptions: {
@@ -238,8 +241,9 @@ describe('build', () => {
 
     await build(config);
 
-    expect(log).toHaveBeenCalledWith(
-      expect.stringMatching(/Compiled 1 file in \d+ms\./u),
+    // Manifest checksum mismatch is the warning
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringMatching(/Compiled 1 file in \d+ms with 1 warning\./u),
     );
 
     const output = await fs.readFile('/snap/output.js', 'utf8');

packages/snaps-cli/src/commands/manifest/implementation.test.ts

@@ -78,7 +78,11 @@ describe('manifest', () => {
     const result = await manifest('/snap/snap.manifest.json', false, spinner);
     expect(result).toBe(false);
 
-    expect(warn).not.toHaveBeenCalled();
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringMatching(
+        '• Missing recommended package.json property: "repository"',
+      ),
+    );
     expect(spinner.stop).toHaveBeenCalled();
     expect(error).toHaveBeenCalledWith(
       expect.stringMatching('The snap manifest file is invalid.'),
@@ -88,11 +92,6 @@ describe('manifest', () => {
         '• "snap.manifest.json" "repository" field does not match the "package.json" "repository" field.',
       ),
     );
-    expect(error).toHaveBeenCalledWith(
-      expect.stringMatching(
-        '• "snap.manifest.json" "shasum" field does not match computed shasum.',
-      ),
-    );
   });
 
   it('validates a snap manifest with warnings', async () => {
@@ -112,7 +111,7 @@ describe('manifest', () => {
     expect(error).not.toHaveBeenCalled();
     expect(warn).toHaveBeenCalledWith(
       expect.stringMatching(
-        /Missing recommended package\.json properties:.*\n.*repository/u,
+        '• Missing recommended package.json property: "repository"',
       ),
     );
     expect(log).toHaveBeenCalledWith(

packages/snaps-cli/src/commands/manifest/implementation.ts

@@ -1,5 +1,6 @@
 import { checkManifest, indent } from '@metamask/snaps-utils/node';
-import { red, yellow } from 'chalk';
+import { assert } from '@metamask/utils';
+import { red, yellow, green } from 'chalk';
 import type { Ora } from 'ora';
 import { dirname } from 'path';
 
@@ -20,40 +21,58 @@ export async function manifest(
   write: boolean,
   spinner?: Ora,
 ): Promise<boolean> {
-  const { warnings, errors, updated } = await checkManifest(
-    dirname(path),
-    write,
-  );
+  const { reports, updated } = await checkManifest(dirname(path), {
+    updateAndWriteManifest: write,
+  });
 
-  if (write && updated) {
-    info('The snap manifest file has been updated.', spinner);
+  const errors = [];
+  const fixed = [];
+  const warnings = [];
+
+  for (const report of reports) {
+    if (report.severity === 'error' && !report.wasFixed) {
+      errors.push(indent(red(`• ${report.message}`)));
+    } else if (report.wasFixed) {
+      fixed.push(indent(yellow(`• ${report.message}`) + green(' (fixed)')));
+    } else {
+      assert(report.severity === 'warning');
+      warnings.push(indent(yellow(`• ${report.message}`)));
+    }
   }
 
-  if (!write && errors.length > 0) {
-    const formattedErrors = errors
-      .map((manifestError) => indent(red(`• ${manifestError}`)))
-      .join('\n');
+  if (errors.length > 0) {
+    const formattedErrors = errors.join('\n');
+    let message = `The snap manifest file is invalid.\n\n${formattedErrors}`;
+    if (!write) {
+      message +=
+        '\n\nRun the command with the `--fix` flag to attempt to fix the manifest.';
+    }
+
+    error(message, spinner);
+  }
 
-    error(
-      `The snap manifest file is invalid.\n\n${formattedErrors}\n\nRun the command with the \`--fix\` flag to attempt to fix the manifest.`,
+  if (write && updated) {
+    const formattedFixed = fixed.join('\n');
+    info(
+      `The snap manifest file has been updated.\n\n${formattedFixed}`,
       spinner,
     );
-
-    spinner?.stop();
-    process.exitCode = 1;
-    return false;
   }
 
   if (warnings.length > 0) {
-    const formattedWarnings = warnings.map((manifestWarning) =>
-      indent(yellow(`• ${manifestWarning}`)),
-    );
+    const formattedWarnings = warnings.join('\n');
 
     warn(
-      `The snap manifest file has warnings.\n\n${formattedWarnings.join('\n')}`,
+      `The snap manifest file has warnings.\n\n${formattedWarnings}`,
       spinner,
     );
   }
 
+  if (errors.length > 0) {
+    spinner?.stop();
+    process.exitCode = 1;
+    return false;
+  }
+
   return true;
 }

packages/snaps-cli/src/webpack/plugins.test.ts

@@ -126,10 +126,6 @@ describe('SnapsStatsPlugin', () => {
       ),
     );
 
-    expect(log).toHaveBeenCalledWith(
-      expect.stringMatching(/at .*Compilation\.js/u),
-    );
-
     expect(process.exitCode).toBe(1);
   });