feat(skills): pack assets, scripts, non-standard folders, and safe_mode into skill packages #169

Open
CoreRasurae wants to merge 2 commits from feature/m1-skill-directory-packing into develop
Member

Summary

  • Add SkillDirectoryPacker / pack_skill_directory to turn an on-disk Agent Skills directory into one validated, content-addressed pkg_skl_ mapping.
  • Add utils/pack_skill.py, a ready-to-use CLI: YAML to stdout by default, --output PATH for a publishable file, and --check for validate-only Package ID reporting.
  • Preserve references/, assets/, scripts/, non-standard folders, and root files; store valid UTF-8 verbatim and other bytes as base64; emit safe_mode only for an explicit false.
  • Add Behave coverage for round-trip, determinism, binary assets, safe_mode emission, invalid inputs, and CLI usage; add a Robot scenario that packs a real directory, publishes it as a local: package, and loads it end to end.
  • Document both the Python API and the end-user CLI workflow with examples.
  • Separate ci(benchmarks) commit: benchmark regression now resolves its baseline from the PR target branch (GITHUB_BASE_REF) or the active branch's stable base, instead of hard-coding origin/master.

Validation

  • nox -s lint, nox -s format, nox -s typecheck
  • nox -s unit_tests — 158 features / 3285 scenarios passed
  • nox -s integration_tests — 381 tests passed
  • nox -s coverage_report — 97.2% (>= 96.5% gate)
  • nox -s security_scan, nox -s dead_code, nox -s complexity
  • nox -s e2e_tests, nox -s docs, nox -s build
  • nox -s benchmark
  • nox -s benchmark_regression against origin/develop — no significant changes (the --quick informational CI mode is noisy; the full run is authoritative)
  • New Behave CLI scenarios pass via nox -s unit_tests -- features/skill_directory_packages.feature

Closes #165

## Summary - Add `SkillDirectoryPacker` / `pack_skill_directory` to turn an on-disk Agent Skills directory into one validated, content-addressed `pkg_skl_` mapping. - Add `utils/pack_skill.py`, a ready-to-use CLI: YAML to stdout by default, `--output PATH` for a publishable file, and `--check` for validate-only Package ID reporting. - Preserve `references/`, `assets/`, `scripts/`, non-standard folders, and root files; store valid UTF-8 verbatim and other bytes as base64; emit `safe_mode` only for an explicit `false`. - Add Behave coverage for round-trip, determinism, binary assets, safe_mode emission, invalid inputs, and CLI usage; add a Robot scenario that packs a real directory, publishes it as a `local:` package, and loads it end to end. - Document both the Python API and the end-user CLI workflow with examples. - Separate `ci(benchmarks)` commit: benchmark regression now resolves its baseline from the PR target branch (`GITHUB_BASE_REF`) or the active branch's stable base, instead of hard-coding `origin/master`. ## Validation - `nox -s lint`, `nox -s format`, `nox -s typecheck` - `nox -s unit_tests` — 158 features / 3285 scenarios passed - `nox -s integration_tests` — 381 tests passed - `nox -s coverage_report` — 97.2% (>= 96.5% gate) - `nox -s security_scan`, `nox -s dead_code`, `nox -s complexity` - `nox -s e2e_tests`, `nox -s docs`, `nox -s build` - `nox -s benchmark` - `nox -s benchmark_regression` against `origin/develop` — no significant changes (the `--quick` informational CI mode is noisy; the full run is authoritative) - New Behave CLI scenarios pass via `nox -s unit_tests -- features/skill_directory_packages.feature` Closes #165
Add a SkillDirectoryPacker facade and pack_skill_directory convenience function that turn an on-disk Agent Skills directory into a single validated pkg_skl_ package mapping. SKILL.md frontmatter is parsed into schema fields, the body becomes instructions, and every other file is preserved as a relative POSIX resource. Valid UTF-8 files stay verbatim while non-text files are base64-encoded. safe_mode is emitted only for an explicit false so the canonical safe default and existing Package IDs are preserved.

Refs: #165
ci(benchmarks): compare against the active branch baseline
Some checks failed
CI / lint (pull_request) Successful in 1m13s
CI / typecheck (pull_request) Successful in 1m29s
CI / security (pull_request) Successful in 1m42s
CI / build (pull_request) Successful in 2m1s
CI / quality (pull_request) Successful in 2m45s
CI / integration_tests (pull_request) Successful in 4m6s
CI / unit_tests (pull_request) Successful in 7m1s
CI / benchmark (pull_request) Failing after 30m46s
CI / coverage (pull_request) Successful in 5m10s
CI / status-check (pull_request) Successful in 2s
7e49f8819c
Resolve the benchmark regression baseline from the pull request's target branch (GITHUB_BASE_REF) or, locally, from the stable branch the current checkout is based on, instead of always comparing against origin/master. Fetch the develop, master, and pull request target refs in CI so the resolved baseline is available.

Refs: #165
feat(skills): add a CLI utility for packing skill directories
Some checks failed
CI / lint (pull_request) Successful in 1m20s
CI / typecheck (pull_request) Successful in 1m56s
CI / quality (pull_request) Successful in 1m37s
CI / security (pull_request) Successful in 1m59s
CI / build (pull_request) Successful in 1m42s
CI / integration_tests (pull_request) Successful in 3m30s
CI / unit_tests (pull_request) Successful in 5m29s
CI / coverage (pull_request) Failing after 14m3s
CI / benchmark (pull_request) Failing after 18m25s
CI / status-check (pull_request) Has been cancelled
eed7fb7a91
Add utils/pack_skill.py, a ready-to-use command-line front end for SkillDirectoryPacker. It writes the validated YAML mapping to stdout by default, supports --output for a publishable file, and --check for validation plus Package ID reporting. Diagnostics stay on stderr so stdout can be redirected or piped.

Cover the CLI with Behave scenarios and document both the Python API and the end-user CLI workflow. Include utils/ in the nox lint and format scopes.

Refs: #165
CoreRasurae force-pushed feature/m1-skill-directory-packing from eed7fb7a91
Some checks failed
CI / lint (pull_request) Successful in 1m20s
CI / typecheck (pull_request) Successful in 1m56s
CI / quality (pull_request) Successful in 1m37s
CI / security (pull_request) Successful in 1m59s
CI / build (pull_request) Successful in 1m42s
CI / integration_tests (pull_request) Successful in 3m30s
CI / unit_tests (pull_request) Successful in 5m29s
CI / coverage (pull_request) Failing after 14m3s
CI / benchmark (pull_request) Failing after 18m25s
CI / status-check (pull_request) Has been cancelled
to acc51465da
Some checks failed
CI / lint (pull_request) Successful in 45s
CI / typecheck (pull_request) Successful in 1m45s
CI / security (pull_request) Successful in 1m39s
CI / quality (pull_request) Successful in 1m37s
CI / build (pull_request) Successful in 2m27s
CI / integration_tests (pull_request) Successful in 4m21s
CI / unit_tests (pull_request) Successful in 5m5s
CI / benchmark (pull_request) Failing after 35m57s
CI / coverage (pull_request) Successful in 6m56s
CI / status-check (pull_request) Successful in 1s
2026-11-12 20:16:16 +00:00
Compare
hurui200320 left a comment

PR Review: !169 (Ticket #165)

Verdict: Approve

The PR fully implements ticket #165 and ADR-2034 D-3/D-6 authoring tooling. All acceptance criteria are met and covered by tests: the packed mapping validates through the existing SkillValidator, determinism and binary/text round-trips are asserted, the safe_mode emission rule (emit only explicit false, normalize explicit true to absence) matches D-9 and §16.1–16.2, and the name/directory match check mirrors the external agentskills.io requirement. The frontmatter allowlist exactly covers the external spec's field set (name, description, license, compatibility, metadata, allowed-tools) plus the CleverActors safe_mode extension, so conformant external directories are not rejected. The symlink-escape guard (_ensure_within_root) and path validation correctly enforce §16.1 resource-path rules. Behave coverage is broad (28 scenarios, real filesystem/validator/canonicalizer, no mocks in the packing path), the Robot scenario exercises the real pack → local: publish → load pipeline end to end, and the guide documentation is accurate against the implementation. No critical or major issues found; the items below are robustness/test-gap improvements.

Critical Issues

None

Major Issues

None

Minor Issues

  1. Unreadable resource files raise a raw OSError instead of SkillPackingError — src/cleveractors/agents/skill_packer.py:389-391. path.read_bytes() in _collect_resources can raise PermissionError/FileNotFoundError (also on TOCTOU deletion), which propagates unwrapped. The SKILL.md read path wraps this (skill_packer.py:305-308, and it is explicitly tested), so the resource path is inconsistent with the module's documented error contract; utils/pack_skill.py:74 only catches SkillPackingError, so the CLI prints a traceback and exits 1 instead of a clean error: ... with exit 2. Recommendation: wrap the resource read in the same except OSError → SkillPackingError("failed to read resource ...") handling and add a Behave scenario (e.g. chmod 000 resource) mirroring the unreadable-SKILL.md test.

  2. No exclusion (or documentation) for VCS/OS junk — packing a checked-out skill repo embeds the whole .git database — src/cleveractors/agents/skill_packer.py:378-396. Path.rglob("*") matches dot-files and recurses into dot-directories, so a directory that is a git checkout (the normal distribution form for agentskills.io skills) packs .git/HEAD, .git/objects/** (binary, base64), plus .DS_Store/__pycache__ etc. into resources. This bloats the single content-addressed blob and changes the Package ID with unrelated VCS state. Not a spec violation (the format permits any additional directory) and the ID stays deterministic, but it is a silent footgun for the tool's primary use case. Recommendation: at minimum document the "everything is packed, including hidden files" behavior in docs/guides/skill-packages.md; preferably skip well-known junk (e.g. .git, __pycache__, .DS_Store) or offer an ignore mechanism.

  3. Test gap: a directory containing only SKILL.md is never packed — features/skill_directory_packages.feature. No scenario covers the no-resources case, so the if resources: branch at src/cleveractors/agents/skill_packer.py:271-272 (omit the resources key entirely) is untested even though D-2/§16.1 make resources optional. Recommendation: add a scenario asserting a SKILL.md-only directory packs successfully and that the mapping has no resources key.

  4. New utils/pack_skill.py sits outside every static-analysis gate except ruff — pyproject.toml:109-114 (pyright include = ["src"]), bandit/vulture target src/cleveractors only, and coverage sources exclude it. The file is fully annotated, but nothing enforces it. Recommendation: add "utils" to the pyright include list (or document why the CLI is deliberately excluded from the strict gates).

Nits

  1. CLI: --check silently ignores --output — utils/pack_skill.py:78. Erroring or warning on the mutually confusing combination would be friendlier; args.output.write_text() (utils/pack_skill.py:94) can also raise an uncaught OSError (e.g. missing parent dir) producing a raw traceback instead of a clean message.
  2. UTF-8 BOM in SKILL.md is rejected as "must start with YAML frontmatter" — src/cleveractors/agents/skill_packer.py:107. \ufeff is not stripped by .strip(), and Windows-authored SKILL.md files commonly carry a BOM. Consider tolerating/stripping a leading BOM or documenting the restriction.
  3. Real-world ecosystem skills with client-specific frontmatter will be rejected with "unsupported field(s)" (src/cleveractors/agents/skill_packer.py:320-325). This is a deliberate, tested, documented design choice and the safe default (vs. silently dropping fields), but the guide could tell authors the workaround: move client-specific keys into metadata.
  4. SkillPackingError(ValueError) breaks the project's exception-hierarchy convention — src/cleveractors/agents/skill_packer.py:68. Every other domain error derives from CleverAgentsException; consider multiple inheritance (CleverAgentsException, ValueError) so generic handlers still catch it.
  5. ${{ github.base_ref }} is interpolated directly into the CI run: script — .gitea/workflows/ci.yml:376-377. Standard Actions hardening is env: indirection. Low risk here since base_ref is limited to existing branch names in the base repo.
  6. Mixed-concern PR — the ci(benchmarks) commit (1dda265) rides in the feat(skills) PR. It is disclosed in the PR description, is small and sensible (the dynamic baseline resolution is correct, including the GITHUB_BASE_REF → origin/<base> mapping and the develop-before-master ancestor fallback), so this is non-blocking — just slightly off the atomic-commit convention.

Summary

A high-quality, well-tested implementation of the directory-to-package packing tooling that ticket #165 and ADR-2034 D-3/D-6 call for. The core packing logic is correct and deterministic: frontmatter parsing (including both allowed-tools spellings and the canonical safe_mode rule), UTF-8/base64 classification, POSIX path normalization, symlink-escape protection, and Package ID computation via the existing validator/canonicalizer all check out against §16 and the ADR, and every ticket acceptance criterion has a matching scenario (28 Behave scenarios plus a real end-to-end Robot publish-and-load test). The minor items above are robustness polish — chiefly consistent OSError wrapping for resource reads, an explicit stance (and test) on the SKILL.md-only and hidden/.git-file cases — and none block merge.

## PR Review: !169 (Ticket #165) ### Verdict: Approve The PR fully implements ticket #165 and ADR-2034 D-3/D-6 authoring tooling. All acceptance criteria are met and covered by tests: the packed mapping validates through the existing `SkillValidator`, determinism and binary/text round-trips are asserted, the `safe_mode` emission rule (emit only explicit `false`, normalize explicit `true` to absence) matches D-9 and §16.1–16.2, and the name/directory match check mirrors the external agentskills.io requirement. The frontmatter allowlist exactly covers the external spec's field set (name, description, license, compatibility, metadata, allowed-tools) plus the CleverActors `safe_mode` extension, so conformant external directories are not rejected. The symlink-escape guard (`_ensure_within_root`) and path validation correctly enforce §16.1 resource-path rules. Behave coverage is broad (28 scenarios, real filesystem/validator/canonicalizer, no mocks in the packing path), the Robot scenario exercises the real pack → `local:` publish → load pipeline end to end, and the guide documentation is accurate against the implementation. No critical or major issues found; the items below are robustness/test-gap improvements. ### Critical Issues None ### Major Issues None ### Minor Issues 1. **Unreadable resource files raise a raw `OSError` instead of `SkillPackingError`** — `src/cleveractors/agents/skill_packer.py:389-391`. `path.read_bytes()` in `_collect_resources` can raise `PermissionError`/`FileNotFoundError` (also on TOCTOU deletion), which propagates unwrapped. The SKILL.md read path wraps this (`skill_packer.py:305-308`, and it is explicitly tested), so the resource path is inconsistent with the module's documented error contract; `utils/pack_skill.py:74` only catches `SkillPackingError`, so the CLI prints a traceback and exits 1 instead of a clean `error: ...` with exit 2. Recommendation: wrap the resource read in the same `except OSError → SkillPackingError("failed to read resource ...")` handling and add a Behave scenario (e.g. chmod 000 resource) mirroring the unreadable-SKILL.md test. 2. **No exclusion (or documentation) for VCS/OS junk — packing a checked-out skill repo embeds the whole `.git` database** — `src/cleveractors/agents/skill_packer.py:378-396`. `Path.rglob("*")` matches dot-files and recurses into dot-directories, so a directory that is a git checkout (the normal distribution form for agentskills.io skills) packs `.git/HEAD`, `.git/objects/**` (binary, base64), plus `.DS_Store`/`__pycache__` etc. into `resources`. This bloats the single content-addressed blob and changes the Package ID with unrelated VCS state. Not a spec violation (the format permits any additional directory) and the ID stays deterministic, but it is a silent footgun for the tool's primary use case. Recommendation: at minimum document the "everything is packed, including hidden files" behavior in `docs/guides/skill-packages.md`; preferably skip well-known junk (e.g. `.git`, `__pycache__`, `.DS_Store`) or offer an ignore mechanism. 3. **Test gap: a directory containing only `SKILL.md` is never packed** — `features/skill_directory_packages.feature`. No scenario covers the no-resources case, so the `if resources:` branch at `src/cleveractors/agents/skill_packer.py:271-272` (omit the `resources` key entirely) is untested even though D-2/§16.1 make `resources` optional. Recommendation: add a scenario asserting a SKILL.md-only directory packs successfully and that the mapping has no `resources` key. 4. **New `utils/pack_skill.py` sits outside every static-analysis gate except ruff** — `pyproject.toml:109-114` (pyright `include = ["src"]`), bandit/vulture target `src/cleveractors` only, and coverage sources exclude it. The file is fully annotated, but nothing enforces it. Recommendation: add `"utils"` to the pyright `include` list (or document why the CLI is deliberately excluded from the strict gates). ### Nits 1. **CLI: `--check` silently ignores `--output`** — `utils/pack_skill.py:78`. Erroring or warning on the mutually confusing combination would be friendlier; `args.output.write_text()` (`utils/pack_skill.py:94`) can also raise an uncaught `OSError` (e.g. missing parent dir) producing a raw traceback instead of a clean message. 2. **UTF-8 BOM in `SKILL.md` is rejected as "must start with YAML frontmatter"** — `src/cleveractors/agents/skill_packer.py:107`. `\ufeff` is not stripped by `.strip()`, and Windows-authored SKILL.md files commonly carry a BOM. Consider tolerating/stripping a leading BOM or documenting the restriction. 3. **Real-world ecosystem skills with client-specific frontmatter will be rejected** with "unsupported field(s)" (`src/cleveractors/agents/skill_packer.py:320-325`). This is a deliberate, tested, documented design choice and the safe default (vs. silently dropping fields), but the guide could tell authors the workaround: move client-specific keys into `metadata`. 4. **`SkillPackingError(ValueError)` breaks the project's exception-hierarchy convention** — `src/cleveractors/agents/skill_packer.py:68`. Every other domain error derives from `CleverAgentsException`; consider multiple inheritance (`CleverAgentsException, ValueError`) so generic handlers still catch it. 5. **`${{ github.base_ref }}` is interpolated directly into the CI `run:` script** — `.gitea/workflows/ci.yml:376-377`. Standard Actions hardening is `env:` indirection. Low risk here since `base_ref` is limited to existing branch names in the base repo. 6. **Mixed-concern PR** — the `ci(benchmarks)` commit (1dda265) rides in the `feat(skills)` PR. It is disclosed in the PR description, is small and sensible (the dynamic baseline resolution is correct, including the `GITHUB_BASE_REF` → `origin/<base>` mapping and the develop-before-master ancestor fallback), so this is non-blocking — just slightly off the atomic-commit convention. ### Summary A high-quality, well-tested implementation of the directory-to-package packing tooling that ticket #165 and ADR-2034 D-3/D-6 call for. The core packing logic is correct and deterministic: frontmatter parsing (including both `allowed-tools` spellings and the canonical `safe_mode` rule), UTF-8/base64 classification, POSIX path normalization, symlink-escape protection, and Package ID computation via the existing validator/canonicalizer all check out against §16 and the ADR, and every ticket acceptance criterion has a matching scenario (28 Behave scenarios plus a real end-to-end Robot publish-and-load test). The minor items above are robustness polish — chiefly consistent `OSError` wrapping for resource reads, an explicit stance (and test) on the SKILL.md-only and hidden/`.git`-file cases — and none block merge.
CoreRasurae force-pushed feature/m1-skill-directory-packing from acc51465da
Some checks failed
CI / lint (pull_request) Successful in 45s
CI / typecheck (pull_request) Successful in 1m45s
CI / security (pull_request) Successful in 1m39s
CI / quality (pull_request) Successful in 1m37s
CI / build (pull_request) Successful in 2m27s
CI / integration_tests (pull_request) Successful in 4m21s
CI / unit_tests (pull_request) Successful in 5m5s
CI / benchmark (pull_request) Failing after 35m57s
CI / coverage (pull_request) Successful in 6m56s
CI / status-check (pull_request) Successful in 1s
to 2c9d06e9fd
Some checks failed
CI / lint (pull_request) Successful in 42s
CI / typecheck (pull_request) Successful in 1m29s
CI / quality (pull_request) Successful in 1m29s
CI / security (pull_request) Successful in 1m33s
CI / build (pull_request) Successful in 2m26s
CI / integration_tests (pull_request) Successful in 3m25s
CI / unit_tests (pull_request) Successful in 6m13s
CI / coverage (pull_request) Successful in 5m20s
CI / status-check (pull_request) Successful in 7s
CI / benchmark (pull_request) Failing after 28m1s
2026-11-13 12:17:20 +00:00
Compare
Some checks failed
CI / lint (pull_request) Successful in 42s
CI / typecheck (pull_request) Successful in 1m29s
CI / quality (pull_request) Successful in 1m29s
CI / security (pull_request) Successful in 1m33s
CI / build (pull_request) Successful in 2m26s
CI / integration_tests (pull_request) Successful in 3m25s
CI / unit_tests (pull_request) Successful in 6m13s
CI / coverage (pull_request) Successful in 5m20s
CI / status-check (pull_request) Successful in 7s
CI / benchmark (pull_request) Failing after 28m1s
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/m1-skill-directory-packing:feature/m1-skill-directory-packing
git switch feature/m1-skill-directory-packing
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!169
No description provided.