chore(package): make validation extensible

odd/tasks/issue-9-package-validation.md

@@ -0,0 +1,85 @@
+# Issue #9: Extensible Package Validation
+
+## Objective
+
+Make package validation extensible and independently testable so the published package can grow beyond the current exact seven-file tarball without weakening its publication boundary.
+
+## Problem and rationale
+
+`scripts/validate-package.mjs` currently embeds an exact tarball file list and runs as one package-level validation command. The multi-CLI installer will add references, runtime files, and tests, so the validator needs a named top-level allowlist plus recursive discovery of the canonical skill tree. Existing validation behavior, especially `pi.skills`, must remain enforced.
+
+## Authorized scope
+
+- `scripts/validate-package.mjs`
+- `tests/validate-package.test.mjs`
+- `package.json`
+- `odd/tasks/issue-9-package-validation.md`
+
+Do not implement installer behavior, npm publication, release workflow changes, or unrelated documentation.
+
+## Constraints and decisions
+
+- Generated technical artifacts remain in English.
+- Preserve the existing Node built-in runtime and package validation entry point.
+- Use package-relative POSIX paths for npm tarball comparisons.
+- Keep an explicit allowlist for published top-level files and directories.
+- Derive files below `skills/mobile-agent-orchestrator/` recursively instead of maintaining a reference-file list.
+- Reject missing, duplicate, or misplaced canonical `SKILL.md` files.
+- Preserve `pi.skills` destination validation.
+- Do not silently broaden publication through npm defaults.
+- Delivery strategy: `ask-on-risk`; this change is expected to remain below the 400-line review heuristic.
+- TDD mode: no explicit project TDD configuration was found during exploration; use ordinary checks with Node's built-in test runner and record observed results.
+
+## Acceptance criteria
+
+- [x] Legitimate files added below `skills/mobile-agent-orchestrator/` are accepted without editing an exact tarball file list.
+- [x] Unexpected published top-level paths fail validation.
+- [x] Exactly one canonical `skills/mobile-agent-orchestrator/SKILL.md` exists and is validated.
+- [x] Existing `pi.skills` path validation remains enforced.
+- [x] Unit tests cover SemVer parsing/comparison, release transitions, frontmatter parsing, required sections, local destination decoding, tarball comparison, and the new inventory/allowlist behavior.
+- [x] `npm test` runs package validation and the unit-test suite, and fails when either fails.
+- [x] `npm run pack:check` succeeds.
+
+## Task checklist
+
+### T1 — Establish the package inventory contract
+
+- [x] Add named canonical paths and the explicit published top-level allowlist.
+- [x] Recursively discover canonical skill-tree files using package-relative POSIX paths.
+- [x] Validate the canonical `SKILL.md` count and preserve existing skill content checks.
+- [x] Replace the hardcoded tarball reference list with the derived inventory.
+
+### T2 — Add unit-test coverage
+
+- [x] Add Node's built-in test-runner entry point.
+- [x] Add focused tests for all exported helpers and the inventory/allowlist contract.
+- [x] Cover malformed and boundary inputs named by the issue acceptance criteria.
+
+### T3 — Verify and record evidence
+
+- [x] Run `npm test`.
+- [x] Run `npm run pack:check`.
+- [x] Confirm a nested canonical reference is accepted without validator edits.
+- [x] Confirm an unexpected published top-level path is rejected by the validator tests.
+- [x] Update this document with command results and the next step.
+
+### T4 — Close the work unit
+
+- [x] Review the diff and authored line count.
+- [x] Create one Conventional Commit containing implementation and tests.
+- [x] Record the commit identity here.
+
+## Progress and evidence
+
+- Branch: `feat/issue-9-package-validation`
+- Base: `main` at `9561127`
+- Maintainer update: issue #9 comment published before implementation.
+- Exploration: current validator exports pure helper seams but has no test directory and hardcodes seven tarball files.
+- Verification: `npm test` and `npm run pack:check` passed after implementation; unit tests cover the recursive inventory and publication boundary.
+- Follow-up fix: `npm test` now uses Node's test auto-discovery (`node --test`) instead of passing `tests` as a module path; local verification passes on Node v26.9.0 and Package CI run `35366849908` passes on Node 22.
+- Diff review: `git diff --check` passed; intended source, test, package, and task-document changes only.
+- Commit: `ec07b3c` (`feat(package): make validation extensible`).
+
+## Next step
+
+Implementation, tests, and the corrective CI verification are complete. The branch is ready for the user-owned merge decision.

package.json

@@ -19,7 +19,7 @@
   "scripts": {
     "validate": "node scripts/validate-package.mjs",
     "pack:check": "npm pack --dry-run --json",
-    "test": "npm run validate"
+    "test": "npm run validate && node --test"
   },
   "keywords": [
     "pi-package",

scripts/validate-package.mjs

@@ -6,6 +6,9 @@ import { spawnSync } from "node:child_process";
 import { fileURLToPath } from "node:url";
 
 const root = resolve(dirname(fileURLToPath(import.meta.url)), "..");
+export const canonicalSkillRoot = "skills/mobile-agent-orchestrator/";
+export const canonicalSkillPath = "skills/mobile-agent-orchestrator/SKILL.md";
+export const publishedTopLevelPaths = ["LICENSE", "README.md", "package.json", "skills/"];
 const semverPattern = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[A-Za-z-][0-9A-Za-z-]*)(?:\.(?:0|[1-9]\d*|\d*[A-Za-z-][0-9A-Za-z-]*))*))?(?:\+([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?$/;
 const requiredSections = [
   "Activation Contract",
@@ -149,7 +152,7 @@ export function parseSkillFrontmatter(skill) {
   return { frontmatter, body: lines.slice(closingIndex + 1) };
 }
 
-function parseYamlScalar(value, lineNumber) {
+export function parseYamlScalar(value, lineNumber) {
   if (value.startsWith('"')) {
     try {
       const parsed = JSON.parse(value);
@@ -212,17 +215,46 @@ export function decodeLocalDestination(destination) {
   }
 }
 
-function walk(directory, predicate) {
+function walk(directory, predicate, readDirectory = readdirSync) {
   const results = [];
-  for (const entry of readdirSync(directory, { withFileTypes: true })) {
+  for (const entry of readDirectory(directory, { withFileTypes: true })) {
     if ([".git", "node_modules", ".codegraph"].includes(entry.name)) continue;
     const path = resolve(directory, entry.name);
-    if (entry.isDirectory()) results.push(...walk(path, predicate));
+    if (entry.isDirectory()) results.push(...walk(path, predicate, readDirectory));
     else if (predicate(path)) results.push(path);
   }
   return results;
 }
 
+function toPackagePath(path) {
+  return path.replaceAll(sep, "/");
+}
+
+export function discoverPackageInventory(packageRoot = root, readDirectory = readdirSync) {
+  const skillFiles = walk(
+    resolve(packageRoot, canonicalSkillRoot),
+    () => true,
+    readDirectory,
+  ).map((path) => toPackagePath(relative(packageRoot, path))).sort();
+  const skillManifests = skillFiles.filter((path) => path.endsWith("/SKILL.md") || path === "SKILL.md");
+  if (skillManifests.length !== 1 || skillManifests[0] !== canonicalSkillPath) {
+    throw new Error(
+      `Package must contain exactly one canonical SKILL.md at ${canonicalSkillPath}; found ${skillManifests.length === 0 ? "none" : skillManifests.join(", ")}.`,
+    );
+  }
+  return publishedTopLevelPaths.flatMap((path) => {
+    if (!path.endsWith("/")) return [path];
+    if (path === "skills/") return skillFiles;
+    return walk(resolve(packageRoot, path), () => true, readDirectory)
+      .map((filePath) => toPackagePath(relative(packageRoot, filePath)))
+      .sort();
+  });
+}
+
+export function validatePublishedInventory(actualFiles, expectedFiles) {
+  return compareTarballFiles(actualFiles, expectedFiles);
+}
+
 function validateLocalTarget(markdownPath, destination, fail) {
   if (!destination || destination.startsWith("#") || /^(?:[a-z][a-z\d+.-]*:|\/\/)/i.test(destination)) {
     return;
@@ -328,23 +360,18 @@ function validateTarball(packageName, fail) {
   const packedFiles = runNpmPack(packageName, fail);
   if (packedFiles.length === 0) return;
 
-  const expectedFiles = [
-    "LICENSE",
-    "README.md",
-    "package.json",
-    "skills/mobile-agent-orchestrator/SKILL.md",
-    "skills/mobile-agent-orchestrator/references/guided-install.md",
-    "skills/mobile-agent-orchestrator/references/platform-matrix.md",
-    "skills/mobile-agent-orchestrator/references/verification-and-recovery.md",
-  ];
-  for (const error of compareTarballFiles(packedFiles, expectedFiles)) fail(error);
+  try {
+    for (const error of validatePublishedInventory(packedFiles, discoverPackageInventory())) fail(error);
+  } catch (error) {
+    fail(error.message);
+  }
 }
 
 function validatePackage() {
   const errors = [];
   const fail = (message) => errors.push(message);
   const packagePath = resolve(root, "package.json");
-  const skillPath = resolve(root, "skills/mobile-agent-orchestrator/SKILL.md");
+  const skillPath = resolve(root, canonicalSkillPath);
   const changelogPath = resolve(root, "CHANGELOG.md");
   const readmePath = resolve(root, "README.md");
   let packageJson = null;
@@ -392,6 +419,12 @@ function validatePackage() {
     }
   }
 
+  try {
+    discoverPackageInventory();
+  } catch (error) {
+    fail(error.message);
+  }
+
   if (existsSync(skillPath)) {
     try {
       const parsedSkill = parseSkillFrontmatter(readFileSync(skillPath, "utf8"));

tests/validate-package.test.mjs

@@ -0,0 +1,106 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+import {
+  canonicalSkillPath,
+  canonicalSkillRoot,
+  compareSemVer,
+  compareTarballFiles,
+  assertReleaseTransition,
+  decodeLocalDestination,
+  discoverPackageInventory,
+  parseSemVer,
+  parseSkillFrontmatter,
+  validatePublishedInventory,
+  validateRequiredSections,
+  publishedTopLevelPaths,
+} from "../scripts/validate-package.mjs";
+
+test("parses and compares SemVer, including prerelease precedence", () => {
+  assert.deepEqual(parseSemVer("1.2.3-alpha.1+build.7"), {
+    major: "1", minor: "2", patch: "3", prerelease: ["alpha", "1"], build: ["build", "7"],
+  });
+  assert.equal(parseSemVer("01.2.3"), null);
+  assert.equal(compareSemVer("1.0.0-alpha", "1.0.0"), -1);
+  assert.equal(compareSemVer("1.0.0-2", "1.0.0-10"), -1);
+  assert.equal(compareSemVer("1.0.0+one", "1.0.0+two"), 0);
+  assert.throws(() => compareSemVer("bad", "1.0.0"), /valid SemVer/);
+});
+
+test("validates stable, increasing release transitions", () => {
+  assert.doesNotThrow(() => assertReleaseTransition("0.1.0", "0.1.1"));
+  assert.throws(() => assertReleaseTransition("0.1.0", "0.1.0"), /strictly increase/);
+  assert.throws(() => assertReleaseTransition("0.1.0", "0.1.1-rc.1"), /stable/);
+  assert.throws(() => assertReleaseTransition("0.1.0", "0.1.1+build"), /build metadata/);
+});
+
+test("parses valid frontmatter and rejects malformed structure", () => {
+  const skill = `---\nname: example-skill\ndescription: "A useful skill"\nlicense: MIT\nmetadata:\n  author: 'Example''s team'\n  version: 1.0.0\n---\n# Body`;
+  const parsed = parseSkillFrontmatter(skill);
+  assert.equal(parsed.frontmatter.metadata.author, "Example's team");
+  assert.equal(parsed.frontmatter.version, undefined);
+  assert.throws(() => parseSkillFrontmatter(skill.replace("  version: 1.0.0", " version: 1.0.0")), /unsupported YAML/);
+  assert.throws(() => parseSkillFrontmatter(skill.replace('description: "A useful skill"', "description: [bad]")), /unsupported plain scalar/);
+});
+
+test("requires each section once, in order, outside code fences", () => {
+  const sections = ["Activation Contract", "Hard Rules", "Decision Gates", "Execution Steps", "Output Contract", "References"];
+  const body = sections.map((section) => `## ${section}`).join("\n").split("\n");
+  assert.doesNotThrow(() => validateRequiredSections(body));
+  assert.throws(() => validateRequiredSections(["## Activation Contract", ...body]), /exactly one/);
+  assert.throws(() => validateRequiredSections([...body].reverse()), /out of order/);
+  assert.doesNotThrow(() => validateRequiredSections(["```", "## Activation Contract", "```", ...body]));
+});
+
+test("decodes local Markdown destinations and rejects malformed encodings", () => {
+  assert.equal(decodeLocalDestination("references/a%20b.md?x=1"), "references/a b.md");
+  assert.equal(decodeLocalDestination("#section"), "");
+  assert.throws(() => decodeLocalDestination("bad%2"), /Malformed percent encoding/);
+});
+
+test("compares tarball files as an exact allowlist", () => {
+  const expected = ["LICENSE", canonicalSkillPath];
+  assert.deepEqual(compareTarballFiles(expected, expected), []);
+  assert.match(compareTarballFiles(["LICENSE", "unexpected"], expected).join("\n"), /non-allowlisted/);
+  assert.match(compareTarballFiles(["LICENSE", "LICENSE"], expected).join("\n"), /duplicate/);
+  assert.match(validatePublishedInventory(["LICENSE"], expected).join("\n"), /missing/);
+});
+
+test("discovers recursive skill files and enforces the publication boundary", () => {
+  const packageRoot = mkdtempSync(join(tmpdir(), "package-validation-"));
+  const skillRoot = join(packageRoot, canonicalSkillRoot);
+  mkdirSync(join(skillRoot, "references", "nested"), { recursive: true });
+  writeFileSync(join(skillRoot, "SKILL.md"), "");
+  writeFileSync(join(skillRoot, "references", "nested", "extra.md"), "");
+  assert.deepEqual(discoverPackageInventory(packageRoot), [
+    "LICENSE", "README.md", "package.json", canonicalSkillPath, "skills/mobile-agent-orchestrator/references/nested/extra.md",
+  ]);
+  assert.deepEqual(publishedTopLevelPaths, ["LICENSE", "README.md", "package.json", "skills/"]);
+  assert.throws(() => {
+    writeFileSync(join(skillRoot, "references", "nested", "SKILL.md"), "");
+    discoverPackageInventory(packageRoot);
+  }, /exactly one canonical SKILL.md/);
+});
+
+test("expands recursively allowlisted non-skill directories", () => {
+  const packageRoot = mkdtempSync(join(tmpdir(), "package-validation-"));
+  const originalPaths = [...publishedTopLevelPaths];
+  try {
+    const skillRoot = join(packageRoot, canonicalSkillRoot);
+    mkdirSync(skillRoot, { recursive: true });
+    writeFileSync(join(skillRoot, "SKILL.md"), "");
+    mkdirSync(join(packageRoot, "bin"), { recursive: true });
+    writeFileSync(join(packageRoot, "bin", "tool.js"), "");
+    publishedTopLevelPaths.push("bin/");
+
+    assert.deepEqual(discoverPackageInventory(packageRoot), [
+      "LICENSE", "README.md", "package.json", canonicalSkillPath, "bin/tool.js",
+    ]);
+  } finally {
+    publishedTopLevelPaths.splice(0, publishedTopLevelPaths.length, ...originalPaths);
+    rmSync(packageRoot, { recursive: true, force: true });
+  }
+});