cleanup yaml rules

Author: avivkeller

packages/remark-lint/src/api.mjs

@@ -18,11 +18,15 @@ export default {
   },
   plugins: [
     ...basePreset.plugins,
+
+    // Internal Rules
     duplicateStabilityNodes,
     yamlComments,
     hashedSelfReference,
     orderedReferences,
     requiredMetadata,
+
+    // External Rules
     remarkLintNoUnusedDefinitions,
     [remarkLintFencedCodeFlag, { allowEmpty: false }],
     [remarkLintMaximumLineLength, 120],

packages/remark-lint/src/rules/__tests__/utils.mjs

@@ -4,20 +4,43 @@ import { mock } from 'node:test';
 import remarkParse from 'remark-parse';
 import { unified } from 'unified';
 
-export const testRule = (rule, markdown, expected, vfileOptions) => {
-  const processor = unified().use(remarkParse);
-  const tree = processor.parse(markdown);
+/**
+ * Tests a markdown rule against a markdown string
+ */
+export const testRule = (rule, markdown, expected, vfileOptions = {}) => {
+  // Parse the markdown once
+  const tree = unified().use(remarkParse).parse(markdown);
 
+  // Create a mock vfile
   const vfile = {
     ...vfileOptions,
     message: mock.fn(),
     messages: [],
   };
 
+  // Execute the rule
   rule()(tree, vfile, () => {});
 
+  // Assert that the expected messages were reported
   assert.deepEqual(
     vfile.message.mock.calls.map(call => call.arguments[0]),
     expected
   );
 };
+
+/**
+ * Tests a YAML rule against a YAML string
+ */
+export function testYamlRule(rule, yaml, expected) {
+  // Create a mock reporter
+  const report = mock.fn();
+
+  // Execute the rule
+  rule(yaml, report);
+
+  // Assert that the expected messages were reported
+  assert.deepEqual(
+    report.mock.calls.flatMap(call => call.arguments),
+    expected
+  );
+}

packages/remark-lint/src/rules/yaml/__tests__/ordered-yaml-keys.test.mjs renamed to packages/remark-lint/src/rules/__tests__/yaml/ordered-yaml-keys.test.mjs

@@ -1,7 +1,7 @@
 import { describe, it } from 'node:test';
 
-import validateKeys from '../ordered-yaml-keys.mjs';
-import { testRule } from './utils.mjs';
+import validateKeys from '../../yaml/ordered-yaml-keys.mjs';
+import { testYamlRule } from '../utils.mjs';
 
 const testCases = [
   {
@@ -56,7 +56,7 @@ const testCases = [
 describe('ordered-yaml-keys', () => {
   for (const { name, input, expected } of testCases) {
     it(name, () => {
-      testRule(validateKeys, input, expected);
+      testYamlRule(validateKeys, input, expected);
     });
   }
 });

packages/remark-lint/src/rules/yaml/__tests__/validate-changes.test.mjs renamed to packages/remark-lint/src/rules/__tests__/yaml/validate-changes.test.mjs

@@ -1,7 +1,7 @@
 import { describe, it } from 'node:test';
 
-import validateChanges from '../validate-changes.mjs';
-import { testRule } from './utils.mjs';
+import validateChanges from '../../yaml/validate-changes.mjs';
+import { testYamlRule } from '../utils.mjs';
 
 const testCases = [
   {
@@ -138,7 +138,7 @@ const testCases = [
 describe('validate-changes', () => {
   for (const { name, input, expected } of testCases) {
     it(name, () => {
-      testRule(validateChanges, input, expected);
+      testYamlRule(validateChanges, input, expected);
     });
   }
 });

packages/remark-lint/src/rules/yaml/__tests__/validate-versions.test.mjs renamed to packages/remark-lint/src/rules/__tests__/yaml/validate-versions.test.mjs

@@ -1,7 +1,7 @@
 import { describe, it } from 'node:test';
 
-import validateVersionRule from '../validate-versions.mjs';
-import { testRule } from './utils.mjs';
+import validateVersionRule from '../../yaml/validate-versions.mjs';
+import { testYamlRule } from '../utils.mjs';
 
 const testCases = [
   {
@@ -59,7 +59,7 @@ const testCases = [
 describe('validate-version', () => {
   for (const { name, input, expected } of testCases) {
     it(name, () => {
-      testRule(validateVersionRule, input, expected);
+      testYamlRule(validateVersionRule, input, expected);
     });
   }
 });

packages/remark-lint/src/rules/yaml/__tests__/utils.mjs

@@ -6,24 +6,44 @@ import orderedYamlKeys from './ordered-yaml-keys.mjs';
 import validateChanges from './validate-changes.mjs';
 import validateVersions from './validate-versions.mjs';
 
-const isYaml = node =>
-  node.type === 'html' && node.value.match(/^<!--\s?YAML\n/);
+const YAML_HTML_COMMENT_RE = /^<!--\s?YAML\s*\n/;
+const HTML_COMMENT_OPEN = '<!--';
+const HTML_COMMENT_CLOSE = '-->';
+const RULES = [orderedYamlKeys, validateVersions, validateChanges];
 
-const rules = [orderedYamlKeys, validateVersions, validateChanges];
+/**
+ * Determine if a node is a YAML-bearing HTML comment.
+ * @param {import('unist').Node} node
+ */
+const isYamlHtmlComment = node =>
+  node.type === 'html' && YAML_HTML_COMMENT_RE.test(node.value);
 
-const yamlComments = (tree, vfile) => {
-  visit(tree, isYaml, node => {
-    if (!node.value.startsWith('<!-- ')) {
+/**
+ * Lints YAML embedded inside HTML comments in Markdown AST.
+ * @param {import('unist').Node} tree
+ * @param {import('vfile').VFile} vfile
+ */
+function yamlComments(tree, vfile) {
+  visit(tree, isYamlHtmlComment, node => {
+    const trimmed = node.value.trim();
+
+    // Consistency check for "<!-- " (space after open)
+    if (!trimmed[HTML_COMMENT_OPEN.length + 1] === ' ') {
       vfile.message("Expected ' ' after '<!--'", node);
     }
 
-    // "#" comments out the first line, and we remove the last
-    // three characters: "-->"
-    const yaml = parse(`#${node.value.slice(0, -3)}`);
+    if (!trimmed.endsWith(HTML_COMMENT_CLOSE)) {
+      vfile.message('YAML comment is not properly closed with "-->"', node);
+      return;
+    }
+
+    // "#" comments out the first line ("<!-- YAML"), and we remove the trailing "-->"
+    const parsed = parse(`#${trimmed.slice(0, -HTML_COMMENT_CLOSE.length)}`);
+
     const report = (...args) => vfile.message(...args, node);
 
-    rules.forEach(rule => rule(yaml, report));
+    RULES.forEach(rule => rule(parsed, report));
   });
-};
+}
 
 export default lintRule('node-core:yaml-comments', yamlComments);

packages/remark-lint/src/rules/yaml/index.mjs

@@ -1,6 +1,33 @@
-const VALID_KEYS = ['added', 'napiVersion', 'deprecated', 'removed', 'changes'];
+/**
+ * Default allowed keys and their required order at the top level.
+ * Order matters for validation.
+ */
+export const DEFAULT_VALID_KEYS = [
+  'added',
+  'napiVersion',
+  'deprecated',
+  'removed',
+  'changes',
+];
+
+/**
+ * Validate that:
+ * - Only valid keys are present
+ * - Keys appear in the expected order (relative order respected)
+ *
+ * @param {Record<string, unknown>} yaml - The YAML object to validate.
+ * @param {(message: string) => void} report - Reporting function
+ * @param {readonly string[]} [validKeys=DEFAULT_VALID_KEYS] - Allowed keys in the expected order.
+ * @param {string} [prefix=''] - Message prefix for context.
+ */
+export default function orderedYamlKeys(
+  yaml,
+  report,
+  validKeys = DEFAULT_VALID_KEYS,
+  prefix = ''
+) {
+  if (!yaml || typeof yaml !== 'object' || Array.isArray(yaml)) return;
 
-export default (yaml, report, validKeys = VALID_KEYS, prefix = '') => {
   const keys = Object.keys(yaml);
 
   // Check for invalid keys
@@ -15,6 +42,8 @@ export default (yaml, report, validKeys = VALID_KEYS, prefix = '') => {
     const index = validKeys.indexOf(key);
 
     if (index === -1) {
+      // Non-validated keys are ignored for ordering, since
+      // they were already reported as invalid above
       continue;
     }
 
@@ -26,4 +55,4 @@ export default (yaml, report, validKeys = VALID_KEYS, prefix = '') => {
     }
     lastIndex = index;
   }
-};
+}

packages/remark-lint/src/rules/yaml/ordered-yaml-keys.mjs

@@ -1,62 +1,90 @@
 import orderedYamlKeys from './ordered-yaml-keys.mjs';
 import { validateVersion } from './validate-versions.mjs';
 
-const VALID_KEYS = ['version', 'pr-url', 'description'];
-const VALID_PR_URL =
-  /^https:\/\/github.com\/nodejs(?:-private)?\/node(?:-private)?\/pull\/\d+$/;
+const CHANGE_VALID_KEYS = ['version', 'pr-url', 'description'];
+const VALID_PR_URL_RE =
+  /^https:\/\/github\.com\/nodejs(?:-private)?\/node(?:-private)?\/pull\/\d+$/;
+const PRIVATE_PR_STARTER =
+  'https://github.com/nodejs-private/node-private/pull/';
+const COMMIT_SHA_RE = /^[0-9a-f]{40}$/i;
 
+/**
+ * A change is security-related if it references a PR in the private Node.js repo,
+ * with a valid commit.
+ * @param {Record<string, unknown>} change
+ * @returns {boolean}
+ */
 const isSecurityRelated = change =>
-  typeof change['pr-url'] === 'string' &&
-  change['pr-url']?.startsWith(
-    'https://github.com/nodejs-private/node-private/pull/'
-  );
+  typeof change?.['pr-url'] === 'string' &&
+  change['pr-url'].startsWith(PRIVATE_PR_STARTER) &&
+  typeof change?.['commit'] === 'string';
 
+/**
+ * Anything below v1.0 is older than this format.
+ * @param {Record<string, unknown>} change
+ * @returns {boolean}
+ */
 const isAncient = change =>
-  typeof change.version === 'string' && change.version.startsWith('v0.');
+  typeof change?.version === 'string' && change.version.startsWith('v0.');
 
-export default ({ changes }, report) => {
+/**
+ * Validate the "changes" array within the YAML object.
+ *
+ * @param {{ changes?: unknown }} yaml
+ * @param {(message: string) => void} report
+ */
+export default function validateChanges({ changes }, report) {
   if (changes === undefined) {
+    // Nothing to validate
     return;
   }
 
   if (!Array.isArray(changes)) {
-    return report('"changes" must be an Array');
+    report('"changes" must be an Array');
+    return;
   }
 
   changes.forEach((change, index) => {
-    // Validate security information, if it exists
+    const prefix = `In "changes[${index}]": `;
+
+    if (!change || typeof change !== 'object' || Array.isArray(change)) {
+      report(`${prefix}Item must be an object`);
+    }
+
+    // Security-related validations
     if (isSecurityRelated(change)) {
-      if ('commit' in change) {
-        if (isNaN(`0x${change.commit}`)) {
-          report(`In "changes[${index}]": Invalid commit: "${change.commit}"`);
-        }
+      const commit = change.commit;
 
-        // Remove the "commit" key so we can validate keys like normal.
-        delete change.commit;
+      if (!COMMIT_SHA_RE.test(commit)) {
+        report(`${prefix}Invalid commit: "${commit}"`);
       }
+
+      // Remove the "commit" key so we can validate keys like normal.
+      delete change.commit;
     }
 
-    // Anything below v1.0 is older than this format
+    // For non-ancient entries, validate PR URL, keys, and description presence
     if (!isAncient(change)) {
-      // Validate the PR URL
-      if (!VALID_PR_URL.test(change['pr-url'])) {
-        report(
-          `In "changes[${index}]": "${change['pr-url']}" is not a valid PR URL.`
-        );
+      const prUrl = change['pr-url'];
+
+      if (!VALID_PR_URL_RE.test(prUrl)) {
+        report(`${prefix}"${prUrl}" is not a valid PR URL.`);
       }
 
-      // Validate the keys
-      orderedYamlKeys(change, report, VALID_KEYS, `In "changes[${index}]": `);
+      // Key validation
+      orderedYamlKeys(change, report, CHANGE_VALID_KEYS, prefix);
     }
 
-    // Validate the versions
+    // Version validation
     validateVersion(change.version, report, `changes[${index}].version`);
 
-    // Validate the description
-    if (change.description.trim().length === 0) {
-      report(`In "changes[${index}]": Description cannot be empty`);
+    // Description validation
+    if (typeof change.description !== 'string') {
+      report(`${prefix}Description must be a string`);
+    } else if (change.description.trim().length === 0) {
+      report(`${prefix}Description cannot be empty`);
     } else if (!change.description.endsWith('.')) {
-      report(`In "changes[${index}]": Description must end with a "."`);
+      report(`${prefix}Description must end with a "."`);
     }
   });
-};
+}