Merge manifest validation reports to include latest validation errors and warnings

Author: Mrtenz

packages/snaps-utils/src/manifest/manifest.test.ts

@@ -30,6 +30,13 @@ import {
 import { NpmSnapFileNames } from '../types';
 
 jest.mock('fs');
+jest.mock('../fs', () => ({
+  ...jest.requireActual('../fs'),
+  useFileSystemCache:
+    <Type>(_key: string, _ttl: number, fn: () => Promise<Type>) =>
+    async () =>
+      fn(),
+}));
 
 const BASE_PATH = '/snap';
 const MANIFEST_PATH = join(BASE_PATH, NpmSnapFileNames.Manifest);
@@ -157,6 +164,39 @@ describe('checkManifest', () => {
     expect(version).toBe('1.0.0');
   });
 
+  it('includes new validation warnings', async () => {
+    fetchMock.mockResponseOnce(MOCK_GITHUB_RESPONSE).mockResponseOnce(
+      JSON.stringify({
+        dependencies: {
+          '@metamask/snaps-sdk': '1.0.0',
+        },
+      }),
+    );
+
+    const manifest = getSnapManifest({
+      shasum: '29MYwcRiruhy9BEJpN/TBIhxoD3t0P4OdXztV9rW8tc=',
+    });
+    delete manifest.platformVersion;
+
+    await fs.writeFile(MANIFEST_PATH, JSON.stringify(manifest));
+
+    const { files, updated, reports } = await checkManifest(BASE_PATH);
+    const unfixed = reports.filter((report) => !report.wasFixed);
+    const fixed = reports.filter((report) => report.wasFixed);
+
+    const defaultManifest = await getDefaultManifest();
+
+    expect(files?.manifest.result).toStrictEqual(defaultManifest);
+    expect(updated).toBe(true);
+    expect(unfixed).toHaveLength(1);
+    expect(fixed).toHaveLength(2);
+
+    const file = await readJsonFile<SnapManifest>(MANIFEST_PATH);
+    const { source, version } = file.result;
+    expect(source.shasum).toBe(defaultManifest.source.shasum);
+    expect(version).toBe('1.0.0');
+  });
+
   it('returns a warning if package.json is missing recommended fields', async () => {
     const { repository, ...packageJson } = getPackageJson();
 
@@ -348,7 +388,7 @@ describe('runFixes', () => {
     const rule: ValidatorMeta = {
       severity: 'error',
       semanticCheck(_, context) {
-        context.report('Always fail', (files) => files);
+        context.report('always-fail', 'Always fail', (files) => files);
       },
     };
 
@@ -360,7 +400,9 @@ describe('runFixes', () => {
     expect(fixesResults).toStrictEqual({
       files,
       updated: false,
-      reports: [{ severity: 'error', message: 'Always fail' }],
+      reports: [
+        { id: 'always-fail', severity: 'error', message: 'Always fail' },
+      ],
     });
   });
 });

packages/snaps-utils/src/manifest/manifest.ts

@@ -232,6 +232,8 @@ export async function runFixes(
   assert(fixResults.files);
   fixResults.files.manifest = fixResults.files.manifest.clone();
 
+  const mergedReports: ValidatorReport[] = deepClone(fixResults.reports);
+
   for (
     let attempts = 1;
     shouldRunFixes && attempts <= MAX_ATTEMPTS;
@@ -259,15 +261,21 @@ export async function runFixes(
 
     fixResults = await runValidators(fixResults.files, rules);
     shouldRunFixes = hasFixes(fixResults, errorsOnly);
+
+    mergedReports.push(
+      ...fixResults.reports.filter(
+        (report) =>
+          !mergedReports.find((mergedReport) => mergedReport.id === report.id),
+      ),
+    );
   }
 
-  const initialReports: (CheckManifestReport & ValidatorReport)[] = deepClone(
-    results.reports,
-  );
+  const allReports: (CheckManifestReport & ValidatorReport)[] =
+    deepClone(mergedReports);
 
   // Was fixed
   if (!shouldRunFixes) {
-    for (const report of initialReports) {
+    for (const report of allReports) {
       if (report.fix) {
         report.wasFixed = true;
         delete report.fix;
@@ -277,18 +285,18 @@ export async function runFixes(
     return {
       files: fixResults.files,
       updated: true,
-      reports: initialReports,
+      reports: allReports,
     };
   }
 
-  for (const report of initialReports) {
+  for (const report of allReports) {
     delete report.fix;
   }
 
   return {
     files: results.files,
     updated: false,
-    reports: initialReports,
+    reports: allReports,
   };
 }
 

packages/snaps-utils/src/manifest/validator-types.ts

@@ -27,11 +27,12 @@ export type ValidatorContextOptions = {
 export type ValidatorSeverity = 'error' | 'warning';
 
 export type ValidatorContext = {
-  readonly report: (message: string, fix?: ValidatorFix) => void;
+  readonly report: (id: string, message: string, fix?: ValidatorFix) => void;
   readonly options?: ValidatorContextOptions;
 };
 
 export type ValidatorReport = {
+  id: string;
   severity: ValidatorSeverity;
   message: string;
   fix?: ValidatorFix;
@@ -41,7 +42,8 @@ export type ValidatorMeta = {
   severity: ValidatorSeverity;
 
   /**
-   * 1. Run the validator on unverified files to ensure that the files are structurally sound.
+   * 1. Run the validator on unverified files to ensure that the files are
+   * structurally sound.
    *
    * @param files - Files to be verified
    * @param context - Validator context to report errors

packages/snaps-utils/src/manifest/validator.ts

@@ -33,12 +33,22 @@ class Context implements ValidatorContext {
     this.#options = options;
   }
 
-  report(message: string, fix?: ValidatorFix): void {
+  /**
+   * Report a validation error or warning.
+   *
+   * @param id - The unique identifier for the report.
+   * @param message - The message describing the validation issue.
+   * @param fix - An optional fix function that can be used to automatically
+   * resolve the issue.
+   */
+  report(id: string, message: string, fix?: ValidatorFix): void {
     assert(this.#nextSeverity !== undefined);
+
     this.reports.push({
-      severity: this.#nextSeverity,
+      id,
       message,
       fix,
+      severity: this.#nextSeverity,
     });
   }
 
@@ -80,6 +90,7 @@ export async function runValidators(
     context.prepareForValidator({
       severity: rule.severity,
     });
+
     await rule.structureCheck?.(files, context);
   }
 
@@ -93,6 +104,7 @@ export async function runValidators(
     context.prepareForValidator({
       severity: rule.severity,
     });
+
     await rule.semanticCheck?.(files as SnapFiles, context);
   }
 

packages/snaps-utils/src/manifest/validators/checksum.test.ts

@@ -27,6 +27,7 @@ describe('checksum', () => {
 
     expect(report).toHaveBeenCalledTimes(1);
     expect(report).toHaveBeenCalledWith(
+      'checksum',
       expect.stringContaining(
         '"snap.manifest.json" "shasum" field does not match computed shasum.',
       ),
@@ -38,7 +39,7 @@ describe('checksum', () => {
     const manifest = getSnapManifest({ shasum: 'foobar' });
 
     let fix: ValidatorFix | undefined;
-    const report = (_: any, fixer?: ValidatorFix) => {
+    const report = (_id: string, _message: string, fixer?: ValidatorFix) => {
       assert(fixer !== undefined);
       fix = fixer;
     };

packages/snaps-utils/src/manifest/validators/checksum.ts

@@ -14,6 +14,7 @@ export const checksum: ValidatorMeta = {
     const expectedChecksum = await getSnapChecksum(fetchedFiles);
     if (gotChecksum !== expectedChecksum) {
       context.report(
+        'checksum',
         `"${NpmSnapFileNames.Manifest}" "shasum" field does not match computed shasum. Got "${gotChecksum}", expected "${expectedChecksum}".`,
         async ({ manifest }) => {
           manifest.source.shasum = expectedChecksum;

packages/snaps-utils/src/manifest/validators/expected-files.test.ts

@@ -25,6 +25,7 @@ describe('expectedFiles', () => {
 
       expect(report).toHaveBeenCalledTimes(1);
       expect(report).toHaveBeenCalledWith(
+        'expected-files',
         expect.stringContaining('Missing file'),
       );
     },

packages/snaps-utils/src/manifest/validators/expected-files.ts

@@ -17,7 +17,10 @@ export const expectedFiles: ValidatorMeta = {
   structureCheck(files, context) {
     for (const expectedFile of EXPECTED_SNAP_FILES) {
       if (!files[expectedFile]) {
-        context.report(`Missing file "${SnapFileNameFromKey[expectedFile]}".`);
+        context.report(
+          'expected-files',
+          `Missing file "${SnapFileNameFromKey[expectedFile]}".`,
+        );
       }
     }
   },

packages/snaps-utils/src/manifest/validators/icon-declared.test.ts

@@ -23,6 +23,7 @@ describe('iconDeclared', () => {
     });
 
     expect(report).toHaveBeenCalledWith(
+      'icon-declared',
       'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.',
     );
   });

packages/snaps-utils/src/manifest/validators/icon-declared.ts

@@ -8,6 +8,7 @@ export const iconDeclared: ValidatorMeta = {
   semanticCheck(files, context) {
     if (!files.manifest.result.source.location.npm.iconPath) {
       context.report(
+        'icon-declared',
         'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.',
       );
     }