feat(skills): pack assets, scripts, non-standard folders, and safe_mode into skill packages #169
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#165 feat(skills): pack assets, scripts, non-standard folders, and safe_mode into skill packages
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!169
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m1-skill-directory-packing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
SkillDirectoryPacker/pack_skill_directoryto turn an on-disk Agent Skills directory into one validated, content-addressedpkg_skl_mapping.utils/pack_skill.py, a ready-to-use CLI: YAML to stdout by default,--output PATHfor a publishable file, and--checkfor validate-only Package ID reporting.references/,assets/,scripts/, non-standard folders, and root files; store valid UTF-8 verbatim and other bytes as base64; emitsafe_modeonly for an explicitfalse.local:package, and loads it end to end.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-codingorigin/master.Validation
nox -s lint,nox -s format,nox -s typechecknox -s unit_tests— 158 features / 3285 scenarios passednox -s integration_tests— 381 tests passednox -s coverage_report— 97.2% (>= 96.5% gate)nox -s security_scan,nox -s dead_code,nox -s complexitynox -s e2e_tests,nox -s docs,nox -s buildnox -s benchmarknox -s benchmark_regressionagainstorigin/develop— no significant changes (the--quickinformational CI mode is noisy; the full run is authoritative)nox -s unit_tests -- features/skill_directory_packages.featureCloses #165
eed7fb7a91acc51465daPR 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, thesafe_modeemission rule (emit only explicitfalse, normalize explicittrueto 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 CleverActorssafe_modeextension, 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
Unreadable resource files raise a raw
OSErrorinstead ofSkillPackingError—src/cleveractors/agents/skill_packer.py:389-391.path.read_bytes()in_collect_resourcescan raisePermissionError/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:74only catchesSkillPackingError, so the CLI prints a traceback and exits 1 instead of a cleanerror: ...with exit 2. Recommendation: wrap the resource read in the sameexcept OSError → SkillPackingError("failed to read resource ...")handling and add a Behave scenario (e.g. chmod 000 resource) mirroring the unreadable-SKILL.md test.No exclusion (or documentation) for VCS/OS junk — packing a checked-out skill repo embeds the whole
.gitdatabase —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. intoresources. 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 indocs/guides/skill-packages.md; preferably skip well-known junk (e.g..git,__pycache__,.DS_Store) or offer an ignore mechanism.Test gap: a directory containing only
SKILL.mdis never packed —features/skill_directory_packages.feature. No scenario covers the no-resources case, so theif resources:branch atsrc/cleveractors/agents/skill_packer.py:271-272(omit theresourceskey entirely) is untested even though D-2/§16.1 makeresourcesoptional. Recommendation: add a scenario asserting a SKILL.md-only directory packs successfully and that the mapping has noresourceskey.New
utils/pack_skill.pysits outside every static-analysis gate except ruff —pyproject.toml:109-114(pyrightinclude = ["src"]), bandit/vulture targetsrc/cleveractorsonly, and coverage sources exclude it. The file is fully annotated, but nothing enforces it. Recommendation: add"utils"to the pyrightincludelist (or document why the CLI is deliberately excluded from the strict gates).Nits
--checksilently 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 uncaughtOSError(e.g. missing parent dir) producing a raw traceback instead of a clean message.SKILL.mdis rejected as "must start with YAML frontmatter" —src/cleveractors/agents/skill_packer.py:107.\ufeffis not stripped by.strip(), and Windows-authored SKILL.md files commonly carry a BOM. Consider tolerating/stripping a leading BOM or documenting the restriction.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 intometadata.SkillPackingError(ValueError)breaks the project's exception-hierarchy convention —src/cleveractors/agents/skill_packer.py:68. Every other domain error derives fromCleverAgentsException; consider multiple inheritance (CleverAgentsException, ValueError) so generic handlers still catch it.${{ github.base_ref }}is interpolated directly into the CIrun:script —.gitea/workflows/ci.yml:376-377. Standard Actions hardening isenv:indirection. Low risk here sincebase_refis limited to existing branch names in the base repo.ci(benchmarks)commit (1dda265) rides in thefeat(skills)PR. It is disclosed in the PR description, is small and sensible (the dynamic baseline resolution is correct, including theGITHUB_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-toolsspellings and the canonicalsafe_moderule), 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 consistentOSErrorwrapping for resource reads, an explicit stance (and test) on the SKILL.md-only and hidden/.git-file cases — and none block merge.acc51465da2c9d06e9fdView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.