fix(skills): vet skill bundles before writing them by anconina · Pull Request #359 · comisai/comis

anconina · 2026-07-26T16:56:02Z

Description

The gap. Comis's skill format is prompt-only and its content scanner is real — but the scanner does not sit where the security story assumes.

skills.import and skills.upload ran no content scan at all. They walked the GitHub Contents API (or took an uploaded file map) and wrote every file to disk. Only create and update scanned, and only a single string.
No scan has ever read a support file. The load-time scanner sees the sanitized SKILL.md body — never references/, scripts/, or templates/. A skill's reference files are injected into agent context by every "read the reference" instruction in its body, and scripts/*.sh is exactly what an agent will exec.
A malformed manifest installed silently. Discovery logged a WARN and skipped it, so the operator saw a successful install and an absent skill.

So a remotely-imported skill landed in an agent workspace uninspected, and the layer meant to catch that ran later, over a fraction of the content, and could be turned off.

What this does. One pure gate, inserted into the five-step sequence all four install paths already share, between scope resolution and the first writeRegularFile. A block writes zero files, mirroring applyBundleInstall's Phase-A-reject-⇒-zero-side-effects. The content rules are unchanged — the same CONTENT_SCAN_RULES the load-time scanner uses, so the two sites cannot drift. Only the surface widens.

The gate is not operator-disableable. skills.contentScanning still relaxes load-time scanning; no config opens the install door. Only the bounds are tunable, via a new skills.installVetting block.

Then two things the gate made possible:

Durable provenance — ~/.comis/installed-skills.json (0o600, keyed <scope>:<name>) recording origin, content hash, trust tier, and verdict. Written after the write succeeds, never at vet time: the gate runs before the first write, so a record written there would claim an install a later disk error could still have prevented.

A trust × verdict policy — the same finding set resolves differently by origin:

	safe	caution	dangerous
`first-party`	allow	allow	allow
`operator`	allow	allow	confirm
`community`	allow	confirm	block
`agent-authored`	allow	confirm	block

force upgrades a confirm and nothing else — no flag makes a community CRITICAL bundle installable. The verdict stays a property of the content and never varies; only the decision does, which is what lets one scan serve every origin.

Behavior changes (all six in the CHANGELOG):

import/upload now refuse content they previously wrote.
All four paths scan every text member, not just SKILL.md.
Malformed frontmatter is refused instead of installing invisibly.
Kebab-case frontmatter (allowed-tools, argument-hint) now installs — it previously installed and then failed validation at load. This ships with the gate deliberately: the gate makes a parse failure a block, so without the mapper a large class of benign skills would be newly refused.
A WARN-only community skill needs force to install.
An operator-authored CRITICAL asks for confirmation rather than hard-rejecting.

Deliberately unchanged: bundled-MCP persist/connect behavior, the load-time scanner and its config, the local/shared scope model and its authz, and the approval gate on skills_manage.

Reviewing this. The branch mixes one fix: with two feat:s. They could have been three PRs; the features build directly on the fix and were kept together deliberately. Reviewable in commit order: the failing tests alone → the gate → provenance → the policy matrix → the CHANGELOG. Most of the logic is pure and lives in packages/skills/src/skills/import/ (no fs, no net, no clock), with only the fs/audit edge in packages/daemon/src/skills/.

Related Issue

No issue linked yet — one is needed before merge. This is a behavior + security change, so the template's N/A escape does not apply. Flagging rather than inventing a number; happy to file one describing the unvetted-install gap if that is the preferred order.

Type of Change

Checklist

Targeted checks for the changed area pass
Full repository validation passes (pnpm validate)
New or changed behavior has a test that demonstrated the RED state before the production change
Documentation updated (if applicable)
No secrets or credentials committed
Security implications considered
The change is focused on one concern and links the related issue, or states the allowed N/A reason

On the last box: deliberately unchecked. This is one fix: plus two feat:s on one branch, and no issue is linked yet. Stated up front rather than left for a reviewer to discover in the diff.

RED Test Proof

Committed as ae09cb59 (tests only), red against shipped code before any production change. 7 failed / 4 passed:

 ❯ |@comis/daemon| src/api/skill-handlers.vetting.test.ts (11 tests | 7 failed) 30ms
     × rejects a CRITICAL SKILL.md body and writes ZERO files 8ms
     × rejects when SKILL.md is clean but a reference file carries a CRITICAL pattern 3ms
     × rejects when a script member pipes a download into a shell 3ms
     × blocks an unparseable-frontmatter bundle instead of installing an invisible skill 2ms
     × INV-V2: contentScanning.enabled=false does NOT disable the install gate 2ms
     × rejects a CRITICAL SKILL.md body and writes ZERO files 2ms          (skills.upload)
     × rejects when SKILL.md is clean but an uploaded support file is poisoned 2ms

 FAIL  src/api/skill-handlers.vetting.test.ts > skills.import — pre-write vetting gate >
       rejects a CRITICAL SKILL.md body and writes ZERO files
AssertionError: promise resolved "{ ok: true, …(1) }" instead of rejecting

- Expected
+ Received

- Error {
-   "message": "rejected promise",
+ {
+   "ok": true,
+   "path": "/tmp/skill-vet-.../ws/skills/my-skill",
  }

 Test Files  1 failed (1)
      Tests  7 failed | 4 passed (11)

The 4 that passed are regression guards (a clean bundle still installs; create/update already rejected a CRITICAL body). The 5 pure-module specs in packages/skills/src/skills/import/ failed at module resolution in the same commit — the correct tests-first state for new modules.

Evidence and Residual Risk

Contribution path: public attack path — an untrusted skill bundle reaching an agent workspace unexamined.
Evidence level: deterministic test. 40+ unit tests over pure modules, end-to-end tests against a real temp data dir asserting files on disk and the provenance record's contents and mode. No live registry was exercised (no remote source mode ships in this PR).
Tested profile: macOS (darwin 25.3.0), Node 22, pnpm validate exit 0; full unit suite 41,251 passed; architecture gates 875 passed; integration tier 3,315 passed (run separately — validate does not cover it, and the install-path tests live there). .linux tests do not touch this path. The Docker image build was not run; no package or dependency was added, so the hand-maintained COPY list is unaffected and dockerfile-workspace-packages.test.ts passes.
Residual risk:
- The scanner is a regex net over six categories, not a proof. A novel injection passing all rules installs as untrusted prose. Mitigation stays layered: prompt-only format, community tier ceiling, and the existing response-path injection detection.
- Bundled MCP servers from community skills still persist and connect immediately. That gate is not in this PR — it needs a decision on what tier pre-existing skills get on upgrade, since treating them as community would drop working connections. This is the largest remaining gap in the story.
- No re-import tamper refusal yet. The hash is recorded but not compared.
- Already-installed skills are not re-vetted. This gates new installs only; retroactive rejection on upgrade would break running deployments for no security gain, since those bytes are already on disk.

Additional Notes

Follow-ups deliberately not in this PR:

Tamper refusal on re-import — small, once name-collision handling is settled.
The bundled-MCP connect gate — see residual risk above.
An architecture gate for config reads. During this work the gate read config.skills, which does not exist (skills config is per-agent), so the bounds would have silently never applied. Two tests now cover the resolved path and the fallback, but the general class deserves a gate asserting daemon config reads resolve against AppConfigSchema.

🤖 Generated with Claude Code

mintlify · 2026-07-26T16:57:08Z

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
comis	🟢 Ready	View Preview	Jul 26, 2026, 4:57 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Adds the failing tests for a gate that does not exist yet, so the gap is reproducible from this commit alone. The gap: `skills.import` and `skills.upload` write skill files with no content scan at all (skill-handlers.ts:323-476, :160-321). Only `skills.create` / `skills.update` scan, and only a single string. The load-time scanner (skill-registry-discovery.ts:192) sees just the sanitized SKILL.md body, and only blocks when `contentScanning. blockOnCritical` is set. So a bundle's `references/`, `scripts/`, and `templates/` members are never inspected on any path, at install or at load, and a malformed-frontmatter skill installs successfully and then goes invisible at discovery (discovery.ts:374-380). skill-handlers.vetting.test.ts runs against real code — 7 red, 4 green: red import: CRITICAL SKILL.md body writes files red import: poisoned references/setup.md (clean manifest) red import: poisoned scripts/install.sh red import: unparseable frontmatter installs red import: contentScanning.enabled=false still writes red upload: CRITICAL body writes files red upload: poisoned support file writes files green import: clean multi-file bundle installs (regression guard) green import: kebab-case frontmatter installs (see note in file — passes pre-patch for the wrong reason; the mapper's proof is frontmatter-map.test.ts) green create: CRITICAL body rejected green update: CRITICAL body rejected without overwrite Five pure specs under packages/skills/src/skills/import/ fail at module resolution, pinning the contracts prose cannot: byte-vs-char size measurement, magic-byte binary detection under a .md name, the NUL separator defeating a path/content hash collision, hash stability across string-vs-bytes sources, and short-circuit proof that structural checks reject before any regex pass. The tree is intentionally red until the gate lands; pushing this commit alone needs --no-verify. The existing 68 skill-handlers.test.ts tests still pass.

`skills.import` and `skills.upload` wrote skill files with no content scan at all, and the load-time scanner has only ever read the sanitized SKILL.md body — never `references/`, `scripts/`, or `templates/`. So a remotely-imported skill landed in an agent workspace uninspected, its support files were never scanned on any path, and a malformed-frontmatter skill installed successfully and then silently failed to appear. One pure gate, inserted into the five-step sequence all four install paths already share, between scope resolution and the first writeRegularFile. A block writes zero files, mirroring applyBundleInstall's Phase-A-reject-⇒-zero-side-effects. vet-bundle.ts structure → manifest → content scan → verdict bundle-structure.ts 11 rules: paths, links, binary bytes, bounds frontmatter-map.ts kebab-case mapping; drop with a named warning trust-tier.ts tier from the call, never from the content bundle-hash.ts canonical sha256 over sorted `path\0bytes` vet-install-gate.ts daemon edge: derive, gate, reject, audit, log Content rules are unchanged — the same CONTENT_SCAN_RULES the load-time scanner uses, so the two sites cannot drift. Only the surface widens. The gate is not operator-disableable: `contentScanning.enabled` still relaxes load-time scanning, but no config opens the install door. Only the bounds are tunable, via the new `skills.installVetting` block. The frontmatter mapper ships WITH the gate, not after it. A parse failure is now a block, and Comis's manifest is a strictObject — so without the mapper every skill spelling `allowed-tools` in the community convention would newly be refused rather than merely failing at load. Behavior changes (CHANGELOG-worthy): - import/upload now refuse content they previously wrote - all four paths now scan every text member, not just SKILL.md - malformed frontmatter is refused instead of installing invisibly - kebab-case frontmatter now installs and loads Four corrections the implementation made to the design: - limit breaches are CRITICAL, not WARN — a cap that is exceeded and then allowed is not a cap - SkillBundleFile.type added: a link's content is byte-identical to a text file's, so the symlink rule needs source metadata - the scope guard runs BEFORE the gate on create; authorization is the cheaper, more specific failure than a content verdict - `skills` config is per-agent (agents.<id>.skills), so reading config.skills would have pinned the bounds to their defaults forever Updates the 47 lines of placeholder fixtures in skill-handlers.test.ts that the stricter manifest check now refuses, per tests-first. Docs: security-scanning.mdx substantially rewritten (two scan points, structure checks, frontmatter normalization, the not-disableable note), plus config-yaml.mdx and defense-in-depth.mdx. 193 tests green across the three suites; full suite 41251; validate exit 0.

Answers "which skills are installed, from where, at what content hash, at what trust" — durably, so the answer survives a daemon restart. The vetting gate already computed all of it and threw it away after the log line. ${dataDir}/installed-skills.json, mode 0o600, keyed <scope>:<name> Mirrors bundle-install-state.ts: daemon-private, and deliberately NOT a lockfile inside the skills tree — discovery walks that tree and chokidar watches it (skills.watchEnabled defaults true), so a state file there would either be discovered as content or trigger reload churn on every write. A missing or malformed file reads as empty; provenance is evidence, not a lock, so one corrupt file must not block boot. Individually-malformed entries drop while valid siblings survive, and a hand-edited file cannot invent a trust tier the policy layer does not understand. Recorded AFTER the write succeeds, via a new runPostInstallHooks seam that wraps the existing runBundleInstallHook. Not at vet time: that gate runs BEFORE the first write, so a record written there would claim an install that a subsequent disk error could still have prevented. The store only ever describes skills that exist on disk. skills.delete now forgets the record. Not tidiness — a stale record outlives the skill it describes, so a later re-import of that name gets compared against a hash and tier corresponding to nothing on disk, and a higher-tier stale record would make a legitimate re-import look like tampering. Handlers pass facts (scope, source, ref, the vet result); the hook assembles the record. skill-handlers.ts sits at 795/800 lines, so each of the four call sites had to stay one line — and the record's shape belongs in one place rather than duplicated four ways. Deliberately NOT added: the skill.import / skill.import.reject audit actions. The shipped skill.vet record already carries source, trust, verdict, contentHash, and findingCounts — the whole provenance tuple, at the same lifecycle point — so a second action type would duplicate rather than add. Tests: 18 store unit tests + 7 end-to-end against a real data dir (github import records community; default-agent create records operator; a non-default agent records agent-authored; a BLOCKED install writes no record; update refreshes the hash; delete drops it). Unblocks the two pieces that are not: the install-policy matrix (needs a decision on how a `confirm` verdict is realized) and tamper refusal on re-import (that, plus collision handling). validate exit 0.

The vetting gate shipped with a fixed fail-closed policy: any CRITICAL blocks, WARN informs, regardless of where the skill came from. That treats a typo in the operator's own skill exactly like a payload from a stranger. safe caution dangerous first-party allow allow allow operator allow allow confirm community allow confirm block agent-authored allow confirm block The verdict stays a property of the content and never varies — only the decision does. That split is what lets one scan serve every origin. Two properties are load-bearing and tested directly: - Monotonicity. A worse verdict is never a softer decision at the same tier, and a lower-trust tier is never softer for the same verdict. A future edit breaking either has created a privilege inversion. - `force` cannot override a `block`. It upgrades a `confirm` and nothing else, so no flag makes a community CRITICAL bundle installable. Realizes `confirm` as a structured refusal naming the findings plus the exact re-run, NOT as a second approval prompt. The tool layer's ApprovalGate already fires BEFORE the RPC (admin-manage-factory.ts:150), so it has not seen the bundle and cannot carry a verdict; raising another prompt there would ask the operator twice for one action. One extra round trip buys them the findings before they decide, and reuses the `force` field already on three of the four contracts. `skills.update` gains `force` (it lacked it) — an additive optional field; generated contract artifacts regenerated. The un-forced decision is kept alongside the effective one on the audit record, so "installed despite a confirm" stays reconstructable after the fact. Docs: the tier table and the decision matrix in security-scanning.mdx; the two failure shapes (`skill_vet_confirm` vs `skill_vet_rejected`) and what `force` does in json-rpc.mdx. Tests: 23 for the matrix (every cell, both monotonicity invariants, and that force cannot rescue a community CRITICAL) + 6 end-to-end (a WARN-only community import needs confirming and writes zero files; the message names the findings and the re-run; force installs it; the same bundle authored by the operator installs with no prompt; force never installs a community CRITICAL; an operator CRITICAL asks rather than refuses). Unblocks nothing further on its own — tamper refusal on re-import still needs a decision on name-collision handling. validate exit 0.

Six user-visible changes across the three commits on this branch: the pre-write gate, the bundle-wide scan surface, the trust x verdict policy, provenance recording, the frontmatter mapping that makes kebab-case skills install, and the malformed-manifest refusal that replaces a silent invisible install.

mintlify · 2026-07-26T20:04:27Z

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
comis	🟡 Building	–	Jul 26, 2026, 4:56 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

mintlify Bot deployed to staging - docs July 26, 2026 16:57 View deployment

comis-agent added 5 commits July 26, 2026 20:35

anconina force-pushed the fix/skill-import-vetting-gate branch from 86fc950 to 3a54f07 Compare July 26, 2026 17:45

mintlify Bot deployed to staging - docs July 26, 2026 17:46 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(skills): vet skill bundles before writing them - #359

fix(skills): vet skill bundles before writing them#359
anconina wants to merge 5 commits into
mainfrom
fix/skill-import-vetting-gate

anconina commented Jul 26, 2026 •

edited

Loading

Uh oh!

mintlify Bot commented Jul 26, 2026

Uh oh!

mintlify Bot commented Jul 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

anconina commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Type of Change

Checklist

RED Test Proof

Evidence and Residual Risk

Additional Notes

Uh oh!

mintlify Bot commented Jul 26, 2026

Uh oh!

mintlify Bot commented Jul 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

anconina commented Jul 26, 2026 •

edited

Loading