feat(skills): skill import pipeline — archive, well-known registry, and ClawHub sources#294
feat(skills): skill import pipeline — archive, well-known registry, and ClawHub sources#294anconina wants to merge 168 commits into
Conversation
- isSpecPureFrontmatter: true only when every top-level key is one of the six spec fields (name/description/license/compatibility/metadata/allowed-tools) - SPEC_PURE_TOP_LEVEL_FIELDS: frozen, alphabetical, single source of the rule - SkillManifestSchema must accept an optional compatibility string
- isSpecPureFrontmatter + frozen SPEC_PURE_TOP_LEVEL_FIELDS in spec-purity.ts: one shared source of the six-field rule, own-enumerable key check only - SkillManifestSchema gains compatibility: z.string().optional() beside license (validated, unread; the length warning lives in the lift) - public re-export from @comis/skills for the corpus gate + future callers
… cases - lift.test.ts: round-trip A (spare key) + B (only version+comis -> metadata undefined), allowed-tools split, metadata.version/comis extraction, malformed and non-object metadata.comis naming the key, compatibility >500 advisory, per-key deprecation warning, prototype-key rejection - parser.test.ts: same equivalences end-to-end through parseSkillManifest, spec minimal + optional-fields examples parse verbatim, prototype-pollution refused
…lidation liftAuthoredFrontmatter converges both authored forms onto the internal shape: - spec-pure form: split allowed-tools, pull metadata.version, parse the metadata.comis JSON string (no reviver) and merge only its known extension keys; an emptied residual metadata map normalizes to undefined so the only-version+comis form matches the pre-migration form - pre-migration top-level form: passed through, with a per-key deprecation warning naming each moved field's authored home - malformed metadata.comis fails naming the key; a prototype-polluting key at any depth is refused (the strict schema does not flag __proto__ own-keys) Wired into parseSkillManifest (lift runs before SkillManifestSchema.safeParse) with an optional logger/skillName; publicly re-exported from @comis/skills.
…igration form - spec-pure SKILL.md (allowed-tools string + metadata.comis bag) must load through the registry with its allowed-tools lifted to allowedTools[] - pre-migration top-level form must load read-compat and draw exactly one deprecation warning naming each moved key - RED: the inline strict safeParse rejects the spec-pure carrier and no lift runs at the load path yet
…lidation - loadPromptSkillImpl normalizes the authored carrier (spec-pure metadata.comis form or pre-migration top-level form) via liftAuthoredFrontmatter immediately after parseFrontmatter, before the capability strip and SkillManifestSchema - order is lift -> capability-strip -> strict safeParse, so a spec-pure skill no longer rejects on allowed-tools/metadata.comis - passes the real logger, so the pre-migration form draws one deprecation warning here (the only parse site with a logger); a malformed metadata.comis returns err
…ichment - a spec-pure skill (userInvocable + comis.requires under metadata.comis) must keep those extensions through extractMetadataFromSkillMd (discovery) and initFromSdkSkills (cache enrichment) - a pre-migration top-level skill discovers unchanged and draws NO deprecation warning from discovery (the load path is the single warning carrier) - RED: sites #3/#4 read only top-level fields, so the extensions are dropped
…ichment paths - extractMetadataFromSkillMd (discovery) and initFromSdkSkills (cache enrichment) now normalize the authored carrier via liftAuthoredFrontmatter right after parseFrontmatter, so a spec-pure skill's userInvocable/comis.* resolve from the reconstructed top level exactly as parseSkillManifest sees them - both lift normalize-only (no logger): the load path is the single deprecation warning carrier, so no duplicate warnings - a malformed metadata.comis is treated as the existing parse-failure path (discovery -> null; enrichment -> SDK-provided fields stand)
- Walk every packages/daemon/bundled-skills/*/SKILL.md and assert each frontmatter carries only the community-spec six top-level fields. - Import the allowed-set rule (SPEC_PURE_TOP_LEVEL_FIELDS) from the shipped package rather than hardcoding the field list. - Non-vacuity guard: the walk must find the full bundled corpus. - Parse the spec's minimal and optional-fields examples verbatim. - RED on the current corpus (stray top-level type/version).
- Drop the top-level type: line and move version: under a metadata: block as metadata.version across all five bundled skills. - Preserve each skill's name, description, version value, and body verbatim. - Reword the skill-creator authoring prose to describe advanced fields as carried under one metadata.comis key. - Turns the bundled-corpus spec-conformance gate green.
- Expose extractVersion as a test seam and drive it with an injected reader. - Ground-truth fixture: a spec-pure manifest whose long description pushes metadata.version past byte 512 (self-asserted), plus a short-frontmatter metadata.version case — both fail on the current fixed-window extractor. - Keep top-level-version fallback, no-version, and malformed guards green.
… frontmatter - Parse the entire leading frontmatter block with the yaml parser and read metadata.version (with a top-level version fallback), instead of scanning a fixed head window for a top-level version line. - A long-description skill whose metadata.version sits past the old window is now versioned correctly, so it re-seeds only on a real version change rather than overwriting the installed copy on every boot. - Correct the ground-truth fixture to valid YAML (em-dash, not colon) so it exercises the past-512 path the parser actually reads.
…dicate - Use isSpecPureFrontmatter from the package for each file's pass/fail verdict, with SPEC_PURE_TOP_LEVEL_FIELDS driving the offending-key citation — both the rule predicate and its field set come from shipped code, never hardcoded.
The cross-language drift guard asserted the Python validator's top-level field set equals the internal SkillManifestSchema shape. That premise no longer holds: the authored on-disk frontmatter is exactly the six spec fields while the internal manifest keeps its extension keys (the carrier moves under metadata.comis, the internal semantics do not). Point the guard at the shipped six-field allowed-set rule instead; the name bounds, comis namespace keys, and content-scan categories still track the Zod schemas. Fails against the pre-rewrite 14-field Python set.
…ntmatter The bundled skill-creator validator hardcoded the pre-migration 14-field top-level set, flagging the very skills the platform now emits as unknown top-level fields and rejecting an allowed-tools string as a non-array. Move its allowed set to the six authored spec fields (name/description/license/compatibility/metadata/allowed-tools), accept allowed-tools as a space-separated string and compatibility as prose (advisory warning past the length cap), and parse the metadata.comis JSON carrier with an honest failure that names the key (no eval, no reviver; a prototype-polluting payload is refused). A top-level extension field now loads read-compat with a per-key deprecation warning naming its authored home, mirroring the platform lift; a genuinely unknown key is still flagged. The permissions/comis-namespace shape checks are preserved, now applied wherever the fields are carried.
Rewrite the in-product manifest reference to the six authored top-level fields (name, description, license, compatibility, allowed-tools, metadata), document the metadata.comis JSON-string carrier for every platform extension (userInvocable/disableModelInvocation/argumentHint/ permissions/inputSchema/comis/mcpServers) with version under metadata.version, and add a read-compatibility section naming each pre-migration top-level key and its new home. The permissions and comis namespace blocks now show their metadata.comis home. Body-constraints, content-scanning, discovery, hot-reload, and eligibility sections are unchanged; the host-ecosystem prose is stated neutrally.
- Reduce the top-level fields to the six spec fields: name, description, license, compatibility, allowed-tools (space-separated string), metadata - Replace the top-level Permissions / Comis namespace / Input Schema sections with a single metadata.comis carrier section; version rides metadata.version - Rewrite the Complete Example spec-pure with a valid metadata.comis JSON string - Add a read-compatibility note: the pre-migration top-level form still loads with a deprecation warning naming each moved key and its new home
…atter - examples.mdx: convert all five worked-example frontmatter blocks to the metadata.comis carrier (userInvocable/disableModelInvocation/argumentHint/ permissions/comis namespace) and allowed-tools space strings; align prose - prompt-skills.mdx: allowed-tools rename, argumentHint/inputSchema/ disableModelInvocation prose now name the metadata.comis home; drop the incidental top-level argumentHint from the two template-variable examples - tool-policy.mdx: per-skill restriction example uses allowed-tools string - custom-skills.mdx: on-disk frontmatter field table + comis-namespace note rewritten spec-pure (internal SkillManifest type is unchanged)
liftAuthoredFrontmatter + LiftContext are consumed only via relative imports (the manifest parser + registry discovery/cache), never through the @comis/skills barrel — so re-exporting them left dead public exports. Remove them from the barrel; the relative importers are unaffected. isSpecPureFrontmatter + SPEC_PURE_TOP_LEVEL_FIELDS stay public — they are the single documented source of the manifest's authored top-level field set, consumed through the barrel by the frontmatter-conformance and schema-drift gates (both test files, which the public-export-consumers walker excludes). Track them in public-api-policy so the dead-export gate recognizes their documented-API status.
…apshot The bundled SKILL.md version moved to metadata.version (indented), so the terminal platform-profile parity guard's column-0 `^version:` regex read undefined. Parse the whole frontmatter block with the YAML parser and read metadata.version (falling back to a top-level version) — the same field the boot seeder reads, keeping the guard authoritative. The non-vacuity example now uses the spec-pure metadata.version shape so it still proves the parser returns a real value and a mismatch is detected. Refresh the SkillManifestSchema JSON-Schema snapshot for the additive optional `compatibility` field (the only diff: no field reorder/removal).
A YAML-list (or any non-string) allowed-tools is currently dropped silently, leaving allowedTools at the unrestricted default -- a fail-open on the tool-restriction boundary. These tests pin the required fail-closed reject; they fail on the pre-patch lift.
isSpecPureFrontmatter classifies by key name, so a list-valued allowed-tools reached liftSpecPure and was dropped by the typeof===string guard, leaving allowedTools at the empty no-restriction default. Reject it here (matching validate-skill.py) so the skill can never run with a weakened tool policy. Threat: fail-open on the tool-restriction security boundary.
The prototype-pollution guard over-broadly refuses any inputSchema property literally named constructor or prototype -- both valid JSON-Schema names. These are data keys in an opaque stored value, not assignment key-paths, so only __proto__ is the real vector. Pins the required accept for constructor/prototype and the retained __proto__ reject; the accept test fails on the pre-patch guard.
The deep-scan refused any constructor/prototype key anywhere in the parsed bag, falsely rejecting a valid inputSchema property so named. Only __proto__ is a real own-key pollution vector (a later bracket copy invokes its setter); constructor/prototype as data keys merely shadow. Narrow the shared set (deep scan + the metadata-map shallow skip) to __proto__ and align the mirrored validate-skill.py guard.
The bundled SKILL.md files carry the spec-pure frontmatter but kept their prior version numbers, so the version-gated seeder skips them on upgrade: existing installs keep the old on-disk copy and the daemon warns about its own bundled skills. Patch-bump each metadata.version so shouldSeed fires. claude-code and codex have a paired terminal profile whose platformVersion must equal the SKILL.md version (build- time parity guard) -- bumped in lockstep.
When the lift fails on a malformed metadata.comis JSON string, discovery drops the file with a generic "valid YAML frontmatter" hint that blames the (valid) surrounding YAML. Pins the requirement that the specific lift error naming the carrier reaches the operator; fails on the current generic hint.
extractMetadataFromSkillMd collapsed every failure to null, so a malformed metadata.comis was dropped with a generic "valid YAML frontmatter" hint that misdirects to the surrounding YAML. Return a discriminated outcome carrying the specific hint and thread the lift's own error (which names the carrier) into the skip WARN.
… PyYAML The no-PyYAML fallback line-parser cannot represent the nested metadata carrier: it flattened metadata to an empty string and hoisted version to the top level, so a correctly authored spec-pure skill was reported INVALID (metadata must be an object) with a spurious version deprecation warning. Drop the broken degradation: require a real parser and, when absent, skip the structural checks with a clear "install pyyaml" note while still running the content-security scan.
The WARN framed the top-level extension form as a historical migration
("uses pre-migration top-level fields; read-compatibility applied").
Runtime strings must state the current constraint, not a build-history
origin. Reword to name what to do; keep the movedKeys/errorKind payload
and the present-tense deprecation-warning wording.
…uilders - assert makeZip STORE/DEFLATE recover byte-for-byte (inflateRawSync oracle) - assert EOCD points at the central directory and exec-bit rides external attrs - assert makeTar ustar walk + gunzip round-trip + pax header for long names
officialPublisher is derived solely from the registry's self-claimed isOfficial / channel, so requireOfficialPublisher is a publisher-provenance signal, not a cryptographic guarantee — a compromised registry could claim official. Document the trust boundary at the derivation site, on the config-schema field, and in the import docs, pointing at the real integrity floor (content scan + verify verdict + TLS + self-computed content-hash pin). No behavior change.
The skills_manage confirm param description names only the pin-divergence class; the wire contract documents two warnable classes a confirm acknowledges (pin divergence AND a non-official registry publisher). Pin that the agent-facing description enumerates both while still stating confirm never overrides a flat collision refusal. RED: the description omits the non-official-publisher class on the pre-patch code.
… + CLI) The skills_manage confirm param description and the CLI --confirm help both named only the pin-divergence class, while the wire contract documents two warnable classes a single confirm acknowledges: a pin divergence AND a non-official registry publisher. Update both surfaces to enumerate both classes and to state plainly that confirm never overrides a flat collision refusal or a blocking scan verdict. GREEN: the agent-facing tool description now names both classes.
On a confirmed import that tripped a warnable class the daemon returns warnings[], but the default human-readable success path prints only name/path/files/agent — an operator who confirms via CLI is not told they just installed a non-official or hash-divergent skill. Pin that the acknowledged warnings appear on the default success output. RED: the warnings are absent from the default output on the pre-patch code.
After the name/path/files/agent lines, print each response warning as an 'Acknowledged: <warning>' line (yellow) when present, so an operator who confirms an import sees the non-official / hash-divergent classes the --confirm covered — not only in --format json. The refusal path already surfaced them. GREEN: the default success output now lists the acknowledged warnings.
…escription The file-backed tool-registry parity snapshot captures the skills_manage param descriptions; widening the confirm description to name both warnable classes changed it. Regenerate the snapshot (a skills_manage-confirm-only diff) so the parity gate tracks the runtime-facing description, per the same-change fixture rule.
…level form to spec-pure The mapper is a writer, so SPEC-04 requires it to emit ONLY spec-pure frontmatter and SPEC-03/D8 forbids any writer from emitting the legacy top-level form. Today the legacy-Comis branch passes the top-level extension keys through unchanged, so the installed SKILL.md is not spec-pure and re-emits a per-key deprecation WARN on every load. Assert the mapped carrier is fully spec-pure (isSpecPureFrontmatter true, version under metadata.version, extensions under the metadata.comis JSON string, allowedTools canonicalized to allowed-tools, no legacy top-level keys), that loading the installed file emits no deprecation WARN, and that it lifts to the SAME internal manifest as the legacy form (no semantic loss). Fails on the pre-patch passthrough.
…o spec-pure The mapper is a writer, so SPEC-04 requires it to emit ONLY spec-pure frontmatter and SPEC-03/D8 forbids any writer from emitting the legacy top-level form. The legacy-Comis branch instead passed the top-level extension keys through unchanged, so the installed SKILL.md was not spec-pure and re-emitted a per-key deprecation WARN on every subsequent load. Relocate the legacy extension keys (userInvocable, disableModelInvocation, argumentHint, permissions, inputSchema, the comis namespace, mcpServers) under the metadata.comis JSON-string carrier and normalize the camelCase allowedTools onto the spec allowed-tools string (version already went to metadata.version, type already dropped). The emitted carrier is now fully spec-pure and lifts back to the identical internal manifest, so the installed file loads with no deprecation warning. Extract the shared allowed-tools normalization into one helper. Fold the foreign-mapped comis namespace under the extension bag's comis key, preserving the foreign branch's exact output. Update the manifest mapping-table docs to match.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
PR description incompletePlease fill in all required sections before this PR can be reviewed. Required sections: Description, Related Issue, Type of Change, Checklist, RED Test Proof. For code changes in See CONTRIBUTING.md for the full contribution bar. |
Code reviewFound 2 issues:
Preserve site: comis/packages/skills/src/skills/import/frontmatter-map.ts Lines 331 to 337 in 5094263 Clobber site: comis/packages/skills/src/skills/import/frontmatter-map.ts Lines 600 to 606 in 5094263
Docs: Lines 1430 to 1438 in 5094263 CLI implementation: comis/packages/cli/src/commands/skills.ts Lines 52 to 64 in 5094263 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code review (addendum)Follow-up from a deeper comment-vs-code pass over the import pipeline. Found 3 additional issues (the
Unguarded read loop: comis/packages/skills/src/skills/import/acquire.ts Lines 160 to 174 in 5094263 "Never throws" contract + unguarded await: comis/packages/skills/src/skills/import/acquire.ts Lines 218 to 221 in 5094263 comis/packages/skills/src/skills/import/acquire.ts Lines 241 to 247 in 5094263 Guarded sibling for comparison: comis/packages/skills/src/skills/import/sources/clawhub-source.ts Lines 221 to 256 in 5094263
comis/packages/daemon/src/skills/bundle-install-helper.ts Lines 6 to 12 in 5094263 comis/packages/daemon/src/skills/bundle-install-helper.ts Lines 426 to 429 in 5094263 comis/packages/daemon/src/skills/bundle-install-helper.ts Lines 492 to 497 in 5094263 Actual call sites: comis/packages/daemon/src/api/skill-handlers.ts Lines 257 to 259 in 5094263 comis/packages/daemon/src/api/skill-handlers.ts Lines 650 to 652 in 5094263
comis/packages/core/src/api-contracts/workspace/skill-handlers.ts Lines 18 to 25 in 5094263 Live dispatch: comis/packages/daemon/src/api/skill-handlers.ts Lines 339 to 341 in 5094263 comis/packages/daemon/src/api/skill-handlers.ts Lines 359 to 361 in 5094263 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…th relocated fields (never clobbered)
…ted fields instead of clobbering it
…ejects (never throws)
…jects (never throws)
…their own timeout (cold-run flake)
…e run - token-store: stat-poll seam (watchUsePolling) so the watcher assertion no longer depends on macOS FSEvents throughput under a workspace fs storm - embedding-cache TTL: expiry decided by 2min-stale rows vs a 60s TTL and a poll, never by sleep timing at the TTL boundary - setup-memory: 30s describe timeout — the 5s default fired mid-setup cold and its abandoned continuation double-called mocks in the next test - cli offline-obs/support-bundle: 120s budgets for the daemon-graph lazy-load tests that exceeded 30s cold
…/update are the trusted path; import/upload go through the staged pipeline
… with the staged import pipeline
…ering against the real provenance store
…line cites and internal traceability IDs, carve the assembly-merge exception into the no-spread wording
…-import Conflict resolutions (union of both branches' additions): - config-posture: keep imported-skill drift (this branch) alongside main's unresolved-model, media-credential-gap, browser/terminal-sandbox fields — extractors, fleet findings, record builder, severity docstring, and tests - daemon.ts: both posture blocks merged, then the whole snapshot derivation extracted to recordBootConfigPosture (wiring/main-helpers.ts) to stay under the 3000-line architecture cap - cli.test.ts: command count 30 -> 31 (skills import here + main's messages) - test-naming verb set: keep refuse* (here) and neutralize* (main) - web contracts artifacts regenerated via pnpm contracts:generate (218 contracts, size budget green) after pnpm install picked up main's pi-ai 0.80.6 catalog pnpm validate green: build:clean, cycles, lint:security, docs:check, test:coverage (39489 passed).
Summary
Skills can now be imported from packaged archives and remote registries, not just GitHub directory URLs — through one staged, fail-closed pipeline shared by the
skills_manageagent tool and thecomis skills importCLI command.Four import sources
.skill,.zip,.tar/.tar.gz) via--source archive— unpacked by an in-house, bounded extractor (no archive lib dependency)--source wellknown --registry <url>— registries must be explicitly allowlisted (default-empty allowlist)@owner/slugvia--source clawhub— honors the top-level verify verdict fail-closed, refuses before download on a failing verdict, pins the download by sha256 when the registry provides onePipeline hardening
skills.import.requireOfficialPublisher(defaulttrue) gates ClawHub installs on server-asserted publisher provenance;officialPublisheris recorded in the skill's provenanceimportedtrust tier; a boot sweep retrofits/repairs prior imports and resumes interrupted commits (two-phase import-commit)Frontmatter mapping
SKILL.mdfrontmatter is rewritten to the spec-pure form with Comis-specific fields undermetadata.comis; the legacy Comis top-level form is still accepted with a WARN during a migration windowObservability & docs
config_posturerecord andcomis fleetfindingsdocs/skills/importing.mdx; updates to the manifest, data-directory, CLI, JSON-RPC, and config referencesTesting
pnpm validate(docs:check, clean build, cycle checks, security lint, coverage) runs via the pre-push hook;pnpm validate:fullon Linux is still pending before merge🤖 Generated with Claude Code