diff --git a/odd/tasks/issue-9-package-validation.md b/odd/tasks/issue-9-package-validation.md new file mode 100644 index 0000000..8ebe7a8 --- /dev/null +++ b/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. diff --git a/package.json b/package.json index bdb3420..47d6cd5 100644 --- a/package.json +++ b/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", diff --git a/scripts/validate-package.mjs b/scripts/validate-package.mjs index 2b39dc9..63c7b07 100644 --- a/scripts/validate-package.mjs +++ b/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")); diff --git a/tests/validate-package.test.mjs b/tests/validate-package.test.mjs new file mode 100644 index 0000000..1ceb6fd --- /dev/null +++ b/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 }); + } +});