From ec07b3c45e3bdade1968bcb431b5f901a8007839 Mon Sep 17 00:00:00 2001 From: Reaan Date: Thu, 17 Sep 2026 18:18:56 -0500 Subject: [PATCH 1/5] feat(package): make validation extensible --- odd/tasks/issue-9-package-validation.md | 84 ++++++++++++++++++++++++ package.json | 2 +- scripts/validate-package.mjs | 57 +++++++++++----- tests/validate-package.test.mjs | 86 +++++++++++++++++++++++++ 4 files changed, 213 insertions(+), 16 deletions(-) create mode 100644 odd/tasks/issue-9-package-validation.md create mode 100644 tests/validate-package.test.mjs diff --git a/odd/tasks/issue-9-package-validation.md b/odd/tasks/issue-9-package-validation.md new file mode 100644 index 0000000..286145f --- /dev/null +++ b/odd/tasks/issue-9-package-validation.md @@ -0,0 +1,84 @@ +# 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. +- [ ] Create one Conventional Commit containing implementation and tests. +- [ ] 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. +- Diff review: `git diff --check` passed; intended source, test, package, and task-document changes only. +- Commit: pending. + +## Next step + +Implementation and tests are complete; parent should inspect the diff and create the work-unit commit. diff --git a/package.json b/package.json index bdb3420..a2f216a 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 tests" }, "keywords": [ "pi-package", diff --git a/scripts/validate-package.mjs b/scripts/validate-package.mjs index 2b39dc9..e21df19 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,40 @@ 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.filter((path) => !path.endsWith("/")), ...skillFiles]; +} + +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 +354,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 +413,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..affa0be --- /dev/null +++ b/tests/validate-package.test.mjs @@ -0,0 +1,86 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, 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/); +}); From e1ffff42c020691f746cfc8308169248558b6830 Mon Sep 17 00:00:00 2001 From: Reaan Date: Thu, 17 Sep 2026 18:20:10 -0500 Subject: [PATCH 2/5] chore(odd): record issue 9 completion --- odd/tasks/issue-9-package-validation.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/odd/tasks/issue-9-package-validation.md b/odd/tasks/issue-9-package-validation.md index 286145f..8a41894 100644 --- a/odd/tasks/issue-9-package-validation.md +++ b/odd/tasks/issue-9-package-validation.md @@ -66,8 +66,8 @@ Do not implement installer behavior, npm publication, release workflow changes, ### T4 — Close the work unit - [x] Review the diff and authored line count. -- [ ] Create one Conventional Commit containing implementation and tests. -- [ ] Record the commit identity here. +- [x] Create one Conventional Commit containing implementation and tests. +- [x] Record the commit identity here. ## Progress and evidence @@ -77,8 +77,8 @@ Do not implement installer behavior, npm publication, release workflow changes, - 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. - Diff review: `git diff --check` passed; intended source, test, package, and task-document changes only. -- Commit: pending. +- Commit: `ec07b3c` (`feat(package): make validation extensible`). ## Next step -Implementation and tests are complete; parent should inspect the diff and create the work-unit commit. +Implementation, tests, verification, and the work-unit commit are complete. The branch is ready for the user-owned PR decision. From 3d70c062e637e77949e1725a9063938ae45d8f22 Mon Sep 17 00:00:00 2001 From: Reaan Date: Fri, 18 Sep 2026 11:09:42 -0500 Subject: [PATCH 3/5] fix(package): run Node test auto-discovery --- odd/tasks/issue-9-package-validation.md | 3 ++- package.json | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/odd/tasks/issue-9-package-validation.md b/odd/tasks/issue-9-package-validation.md index 8a41894..c0ea588 100644 --- a/odd/tasks/issue-9-package-validation.md +++ b/odd/tasks/issue-9-package-validation.md @@ -76,9 +76,10 @@ Do not implement installer behavior, npm publication, release workflow changes, - 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. CI rerun is pending. - 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, verification, and the work-unit commit are complete. The branch is ready for the user-owned PR decision. +Implementation and tests are complete. The corrective change is ready to push and re-run Package CI before the user-owned merge decision. diff --git a/package.json b/package.json index a2f216a..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 && node --test tests" + "test": "npm run validate && node --test" }, "keywords": [ "pi-package", From 8edb14bf009af1c935891d5a3ace1c2c42a9a054 Mon Sep 17 00:00:00 2001 From: Reaan Date: Fri, 18 Sep 2026 11:10:58 -0500 Subject: [PATCH 4/5] chore(odd): record PR 20 CI verification --- odd/tasks/issue-9-package-validation.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/odd/tasks/issue-9-package-validation.md b/odd/tasks/issue-9-package-validation.md index c0ea588..8ebe7a8 100644 --- a/odd/tasks/issue-9-package-validation.md +++ b/odd/tasks/issue-9-package-validation.md @@ -76,10 +76,10 @@ Do not implement installer behavior, npm publication, release workflow changes, - 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. CI rerun is pending. +- 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 and tests are complete. The corrective change is ready to push and re-run Package CI before the user-owned merge decision. +Implementation, tests, and the corrective CI verification are complete. The branch is ready for the user-owned merge decision. From 8dda2ac3834fde67c5aedfa7034076224622cb0b Mon Sep 17 00:00:00 2001 From: Reaan Date: Sun, 20 Sep 2026 23:54:03 -0500 Subject: [PATCH 5/5] fix(package): expand allowlisted inventory directories --- scripts/validate-package.mjs | 8 +++++++- tests/validate-package.test.mjs | 22 +++++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/scripts/validate-package.mjs b/scripts/validate-package.mjs index e21df19..63c7b07 100644 --- a/scripts/validate-package.mjs +++ b/scripts/validate-package.mjs @@ -242,7 +242,13 @@ export function discoverPackageInventory(packageRoot = root, readDirectory = rea `Package must contain exactly one canonical SKILL.md at ${canonicalSkillPath}; found ${skillManifests.length === 0 ? "none" : skillManifests.join(", ")}.`, ); } - return [...publishedTopLevelPaths.filter((path) => !path.endsWith("/")), ...skillFiles]; + 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) { diff --git a/tests/validate-package.test.mjs b/tests/validate-package.test.mjs index affa0be..1ceb6fd 100644 --- a/tests/validate-package.test.mjs +++ b/tests/validate-package.test.mjs @@ -1,6 +1,6 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -84,3 +84,23 @@ test("discovers recursive skill files and enforces the publication boundary", () 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 }); + } +});