feat(skills): add skill package schema and agent-side skill loading support #94

2026-07-31T21:24:58Z

CoreRasurae commented

2026-07-31 21:24:58 +00:00

Summary

Implements #88: a normative Skill package schema (Package Registry Standard §16) aligned with the open agentskills.io Agent Skills format, plus agent-side skill loading for type: llm agents via a new optional skills config field, per the approved ADR-2034.

SkillLoader, SkillValidator, SkillReferenceResolver (new): resolve skills references through the existing cleveractors.registry client (registry:/ID:/local: schemes), mirroring the established template package-reference resolution pattern rather than a parallel path.
Progressive disclosure in three stages: discovery (system prompt), activation (skill tool call returns instructions), execution (resource reads, reusing file_read's offset/max_chars pagination from ADR-2033).
AgentFactory invokes SkillLoader before LLMAgent construction; LLMAgent/ToolAgent gain no registry awareness — all resolution stays in SkillLoader.
docs/actor-registry-standard.md gains a new §16 (appended after the original body, version bumped 1.0.0 → 1.1.0) documenting the schema. The skills agent-config field itself is documented only in ADR-2034 — not mirrored into docs/index.md — matching how ADR-2031/ADR-2032 handled their own new LLM agent config fields.
docs/registry/ updated with pointers to the new schema.

Test plan

68 Behave scenarios (features/agent_skills.feature): schema validation (including all agentskills.io name/description constraints), all three reference schemes, loader orchestration, factory/agent integration, ToolAgent skill tool (activation, resource reads incl. base64, pagination, error paths).
Robot Framework suite (robot/agent_skills.robot, 6 cases): real LocalPackageStore/PackageContentResolver/SkillLoader resolution against a real on-disk skill file, end to end through a real ToolAgent dispatch — only the LLM API call is mocked.
nox -s coverage_report: 96.9% (threshold 96.5%).
Full nox suite green (lint, format, typecheck, security_scan, dead_code, complexity, unit_tests, integration_tests, e2e_tests, coverage_report, docs, build).
nox -s benchmark_regression: BENCHMARKS NOT SIGNIFICANTLY CHANGED.

Closes #88

## Summary Implements #88: a normative Skill package schema (Package Registry Standard §16) aligned with the open [agentskills.io](https://agentskills.io/specification) Agent Skills format, plus agent-side skill loading for `type: llm` agents via a new optional `skills` config field, per the approved ADR-2034. - `SkillLoader`, `SkillValidator`, `SkillReferenceResolver` (new): resolve `skills` references through the existing `cleveractors.registry` client (`registry:`/`ID:`/`local:` schemes), mirroring the established template package-reference resolution pattern rather than a parallel path. - Progressive disclosure in three stages: discovery (system prompt), activation (`skill` tool call returns instructions), execution (resource reads, reusing `file_read`'s `offset`/`max_chars` pagination from ADR-2033). - `AgentFactory` invokes `SkillLoader` before `LLMAgent` construction; `LLMAgent`/`ToolAgent` gain no registry awareness — all resolution stays in `SkillLoader`. - `docs/actor-registry-standard.md` gains a new §16 (appended after the original body, version bumped 1.0.0 → 1.1.0) documenting the schema. The `skills` agent-config field itself is documented only in ADR-2034 — not mirrored into `docs/index.md` — matching how ADR-2031/ADR-2032 handled their own new LLM agent config fields. - `docs/registry/` updated with pointers to the new schema. ## Test plan - [x] 68 Behave scenarios (`features/agent_skills.feature`): schema validation (including all agentskills.io name/description constraints), all three reference schemes, loader orchestration, factory/agent integration, ToolAgent skill tool (activation, resource reads incl. base64, pagination, error paths). - [x] Robot Framework suite (`robot/agent_skills.robot`, 6 cases): real `LocalPackageStore`/`PackageContentResolver`/`SkillLoader` resolution against a real on-disk skill file, end to end through a real `ToolAgent` dispatch — only the LLM API call is mocked. - [x] `nox -s coverage_report`: 96.9% (threshold 96.5%). - [x] Full `nox` suite green (lint, format, typecheck, security_scan, dead_code, complexity, unit_tests, integration_tests, e2e_tests, coverage_report, docs, build). - [x] `nox -s benchmark_regression`: BENCHMARKS NOT SIGNIFICANTLY CHANGED. Closes #88

CoreRasurae added this to the v2.1.0 milestone 2026-07-31 21:24:59 +00:00

CoreRasurae added a new dependency 2026-07-31 21:26:04 +00:00

#88 feat(skills): add Skill package schema and agent-side skill loading support

CoreRasurae added the

Type

Feature

label 2026-07-31 21:26:24 +00:00

CoreRasurae commented

2026-07-31 21:33:34 +00:00

Note: bundled `scripts/` resources are readable, not directly runnable

A question came up after this PR landed: if a skill's instructions say something like "run scripts/extract.py <file.pdf>", how does the type: llm agent actually execute that script? Documenting the answer here since it's not obvious from the code alone.

What the skill tool actually gives the model is text, not a runnable file. Calling skill(skill_name="pdf-processing", resource="scripts/extract.py") returns the script's source as a string (cleveractors.agents.tool.ToolAgent._skill_tool, commit bf1f138) — formatted as [SKILL_RESOURCE_READ]...[FILE_CONTENT_START]...[FILE_CONTENT_END]. Nothing is ever written to a real filesystem path; resources live only in the in-memory _loaded_skills mapping threaded through context["_skills"]. This is intentional, per ADR-2034 D-5: resources are exposed for execution-stage reads "without touching the host filesystem," not for by-path execution.

How a bundled Python script can actually run today — no code changes needed, but it requires the model to inline the source rather than reference it by path:

skill(skill_name="pdf-processing", resource="scripts/extract.py") → returns the source text
python_exec(code="<that source text>") → executes it via cleveractors.agents.tool.ToolAgent._execute_python_code

This only works when the agent config sets exec_python: true. Worth flagging: that sandbox isn't airtight — its restricted-builtins dict still includes an unrestricted __import__, so executed code can import os / import subprocess and go beyond the tool's documented builtin list. Pre-existing behavior, not something this PR changed, but relevant to anyone relying on it as a security boundary for skill-provided code.

What does not work today: a bundled .sh script, or any Python script that assumes it's a real file on disk (relative imports, sibling files, real argv, real cwd). There's no materialized file for shell or a real interpreter invocation to point at. A model could try to smuggle the whole script into bash -c '<script>' via the shell tool (needs allow_shell: true), but that's fragile and wasn't a designed path — it just happens to be technically possible given shell's existing arbitrary-command argument.

If genuine file-backed execution is wanted (skill resources materialized to a real temp directory so shell/python_exec/file_read can operate on real paths), that's a new capability this issue didn't build. It would need its own small design decision — materialization lifecycle, cleanup, interaction with safe_mode/unsafe_mode — similar in shape to the pack_skill_directory follow-up already flagged in ADR-2034's "Follow-up Required" section. Happy to scope that as a separate issue if it's wanted.

## Note: bundled `scripts/` resources are readable, not directly runnable A question came up after this PR landed: if a skill's `instructions` say something like *"run `scripts/extract.py <file.pdf>`"*, how does the `type: llm` agent actually execute that script? Documenting the answer here since it's not obvious from the code alone. **What the `skill` tool actually gives the model is text, not a runnable file.** Calling `skill(skill_name="pdf-processing", resource="scripts/extract.py")` returns the script's source as a string (`cleveractors.agents.tool.ToolAgent._skill_tool`, commit `bf1f138`) — formatted as `[SKILL_RESOURCE_READ]...[FILE_CONTENT_START]...[FILE_CONTENT_END]`. Nothing is ever written to a real filesystem path; resources live only in the in-memory `_loaded_skills` mapping threaded through `context["_skills"]`. This is intentional, per ADR-2034 D-5: resources are exposed for execution-stage reads "without touching the host filesystem," not for by-path execution. **How a bundled Python script can actually run today** — no code changes needed, but it requires the model to inline the source rather than reference it by path: 1. `skill(skill_name="pdf-processing", resource="scripts/extract.py")` → returns the source text 2. `python_exec(code="<that source text>")` → executes it via `cleveractors.agents.tool.ToolAgent._execute_python_code` This only works when the agent config sets `exec_python: true`. Worth flagging: that sandbox isn't airtight — its restricted-builtins dict still includes an unrestricted `__import__`, so executed code can `import os` / `import subprocess` and go beyond the tool's documented builtin list. Pre-existing behavior, not something this PR changed, but relevant to anyone relying on it as a security boundary for skill-provided code. **What does *not* work today:** a bundled `.sh` script, or any Python script that assumes it's a real file on disk (relative imports, sibling files, real argv, real cwd). There's no materialized file for `shell` or a real interpreter invocation to point at. A model could try to smuggle the whole script into `bash -c '<script>'` via the `shell` tool (needs `allow_shell: true`), but that's fragile and wasn't a designed path — it just happens to be technically possible given `shell`'s existing arbitrary-`command` argument. **If genuine file-backed execution is wanted** (skill resources materialized to a real temp directory so `shell`/`python_exec`/`file_read` can operate on real paths), that's a new capability this issue didn't build. It would need its own small design decision — materialization lifecycle, cleanup, interaction with `safe_mode`/`unsafe_mode` — similar in shape to the `pack_skill_directory` follow-up already flagged in ADR-2034's "Follow-up Required" section. Happy to scope that as a separate issue if it's wanted.

hurui200320 requested changes 2026-08-01 09:59:16 +00:00

Dismissed

hurui200320 left a comment

PR Review: !94 (Ticket #88)

Verdict: Request Changes

The implementation is solid overall and closely follows ADR-2034, reusing the existing registry client and preserving progressive disclosure. However, two functional issues require fixes before approval: skill tool context is lost in the stuck-model synthesis path, and duplicate skill names are silently overwritten. Additionally, there are a few minor documentation/coverage observations.

Critical Issues

None

Major Issues

src/cleveractors/agents/llm.py lines 1429-1431 — The stuck-model synthesis follow-up constructs tool_ctx_s as {"_unsafe_mode": True} if parent_unsafe else None instead of calling self._build_tool_context(parent_unsafe). Consequently, _loaded_skills is not forwarded to the ephemeral ToolAgent. If the model emits a skill tool call during this follow-up round, ToolAgent._skill_tool fails with "Skill '...' is not loaded for this agent." The main loop and the budget-exhaustion synthesis round both correctly use _build_tool_context; this path should too.
src/cleveractors/agents/skills.py lines 98-109 — SkillLoader.load stores resolved skills in loaded[skill.name] = skill, silently overwriting any earlier skill with the same name. Two skills references that resolve to identically-named skills (or two refs to the same skill) will yield a catalogue with one entry and the other's instructions will be lost. Validate uniqueness and raise AgentCreationError with a clear message.

Minor Issues

Coverage threshold discrepancy — The PR test plan reports 96.9% coverage and cites a 96.5% threshold, but CONTRIBUTING.md, .gitea/workflows/ci.yml, and the coverage_report session docstring all state the gate is 97%. The noxfile.py constant is 96.5, which creates an inconsistency. Before merge, confirm the project's enforced gate and ensure the reported coverage meets it. If the gate is 97%, additional tests are needed.
docs/actor-registry-standard.md §16.1 — The last paragraph references "Actor Configuration Standard §22" for the agent-side skills field, but docs/index.md has no §22. This appears to be a stale/future reference; it should point to the actual location (currently ADR-2034, or §4.4 once the deferred spec update lands).
src/cleveractors/agents/skill_schema.py Skill.to_context_dict — The context representation exposed to the skill tool and LLMAgent.get_metadata() drops metadata, allowed_tools, license, and compatibility. Since allowed_tools is documented as a pre-approved tool list, making it invisible to the model limits its usefulness. Consider surfacing at least allowed_tools and metadata in activation output or catalogue metadata.

Nits

Test gaps — Consider adding Behave/Robot scenarios for: duplicate skill names in the skills list; empty description/instructions strings; and the stuck-model synthesis path invoking the skill tool.
robot/SkillLoadingTestLib.py — tempfile.mkdtemp() directories are not cleaned up after tests.

Summary

The PR delivers the normative skill schema, reference resolution, loader orchestration, and tool-agent integration described in ADR-2034. The architecture is clean and the test suite is extensive. The two major issues above are localized but real regressions in functionality; once fixed and the coverage/doc observations addressed, this should be good to merge.

## PR Review: !94 (Ticket #88) ### Verdict: Request Changes The implementation is solid overall and closely follows ADR-2034, reusing the existing registry client and preserving progressive disclosure. However, two functional issues require fixes before approval: skill tool context is lost in the stuck-model synthesis path, and duplicate skill names are silently overwritten. Additionally, there are a few minor documentation/coverage observations. ### Critical Issues None ### Major Issues 1. **`src/cleveractors/agents/llm.py` lines 1429-1431** — The stuck-model synthesis follow-up constructs `tool_ctx_s` as `{"_unsafe_mode": True} if parent_unsafe else None` instead of calling `self._build_tool_context(parent_unsafe)`. Consequently, `_loaded_skills` is not forwarded to the ephemeral `ToolAgent`. If the model emits a `skill` tool call during this follow-up round, `ToolAgent._skill_tool` fails with "Skill '...' is not loaded for this agent." The main loop and the budget-exhaustion synthesis round both correctly use `_build_tool_context`; this path should too. 2. **`src/cleveractors/agents/skills.py` lines 98-109** — `SkillLoader.load` stores resolved skills in `loaded[skill.name] = skill`, silently overwriting any earlier skill with the same name. Two `skills` references that resolve to identically-named skills (or two refs to the same skill) will yield a catalogue with one entry and the other's instructions will be lost. Validate uniqueness and raise `AgentCreationError` with a clear message. ### Minor Issues 1. **Coverage threshold discrepancy** — The PR test plan reports 96.9% coverage and cites a 96.5% threshold, but `CONTRIBUTING.md`, `.gitea/workflows/ci.yml`, and the `coverage_report` session docstring all state the gate is 97%. The `noxfile.py` constant is 96.5, which creates an inconsistency. Before merge, confirm the project's enforced gate and ensure the reported coverage meets it. If the gate is 97%, additional tests are needed. 2. **`docs/actor-registry-standard.md` §16.1** — The last paragraph references "Actor Configuration Standard §22" for the agent-side `skills` field, but `docs/index.md` has no §22. This appears to be a stale/future reference; it should point to the actual location (currently ADR-2034, or §4.4 once the deferred spec update lands). 3. **`src/cleveractors/agents/skill_schema.py` `Skill.to_context_dict`** — The context representation exposed to the `skill` tool and `LLMAgent.get_metadata()` drops `metadata`, `allowed_tools`, `license`, and `compatibility`. Since `allowed_tools` is documented as a pre-approved tool list, making it invisible to the model limits its usefulness. Consider surfacing at least `allowed_tools` and `metadata` in activation output or catalogue metadata. ### Nits 1. **Test gaps** — Consider adding Behave/Robot scenarios for: duplicate skill names in the `skills` list; empty `description`/`instructions` strings; and the stuck-model synthesis path invoking the `skill` tool. 2. **`robot/SkillLoadingTestLib.py`** — `tempfile.mkdtemp()` directories are not cleaned up after tests. ### Summary The PR delivers the normative skill schema, reference resolution, loader orchestration, and tool-agent integration described in ADR-2034. The architecture is clean and the test suite is extensive. The two major issues above are localized but real regressions in functionality; once fixed and the coverage/doc observations addressed, this should be good to merge.

CoreRasurae force-pushed feature/skill-package-support from bf1f13846f to 05e5e52310

2026-08-01 15:05:03 +00:00

Compare

CoreRasurae added 1 commit 2026-08-01 15:41:02 +00:00

feat(skills): add skill package schema and agent-side skill loading support

CI / lint (pull_request) Successful in 51s

Details

CI / typecheck (pull_request) Successful in 1m36s

Details

CI / security (pull_request) Successful in 1m32s

Details

CI / quality (pull_request) Failing after 1m25s

Details

CI / unit_tests (pull_request) Failing after 1m28s

Details

CI / coverage (pull_request) Has been skipped

Details

CI / build (pull_request) Successful in 52s

Details

CI / integration_tests (pull_request) Failing after 1m53s

Details

CI / status-check (pull_request) Failing after 12s

Details

CI / benchmark (pull_request) Failing after 1m28s

Details

f9fea9e208

Extends the Package Registry Standard with a normative Skill package
schema (§16) aligned with the open agentskills.io Agent Skills format,
and adds an optional `skills` field to `type: llm` agent configs so an
Actor can attach one or more resolved Skill packages to an agent.

SkillLoader (new) resolves each `skills` reference through the existing
cleveractors.registry client (registry:/ID:/local: schemes), mirroring
the established template package-reference resolution pattern in
cleveractors.templates.base._resolve_package_ref rather than
introducing a parallel resolution path. SkillValidator (new) checks
resolved content against the D-2 schema (name/description constraints
lifted from agentskills.io, resource path/encoding rules) and computes
a content-addressed package_id via the existing Canonicalizer.

Resolved skills are disclosed to the model in three stages, per D-4:
discovery (name+description folded into system_prompt), activation (a
synthesized `skill` tool call returns full instructions), and execution
(a further call reads a bundled resource, reusing file_read's
offset/max_chars pagination convention from ADR-2033). All registry
awareness lives in SkillLoader at the factory layer; LLMAgent only
gains a `_skills` value forwarded through its existing tool-call
context, and ToolAgent gains one new built-in tool handler — neither
imports anything from cleveractors.registry.

The `skills` field itself is documented only in ADR-2034, not mirrored
into docs/index.md, matching how ADR-2031/ADR-2032 handled their own
new LLM agent config fields (graduation into the normative spec is
deferred to a future ADR). The Skill package schema, which has no other
home, is appended as a new §16 in docs/actor-registry-standard.md
(after the original §1-15 body, with a version bump to 1.1.0) rather
than spliced into the existing §3.2 package-type table.

68 Behave scenarios cover schema validation, all three reference
schemes, loader orchestration, factory/agent integration, and the
ToolAgent skill tool (activation, resource reads, pagination, error
paths). A Robot Framework suite exercises the same flow against a real
on-disk skill file and real LocalPackageStore/PackageContentResolver
resolution end to end, mocking only the LLM API call. Coverage: 96.9%
(threshold 96.5%); full nox suite green; ASV shows no significant
performance change.

ISSUES CLOSED: #88

CoreRasurae force-pushed feature/skill-package-support from c520a6d211 to f9fea9e208

2026-08-01 15:41:02 +00:00

Compare

hurui200320 requested changes 2026-08-02 13:29:46 +00:00

hurui200320 left a comment

PR Review: !94 (Ticket #88)

Verdict: Request Changes

The implementation still has the two functional regressions identified in the previous review: the stuck-model synthesis path drops the skill catalogue, and duplicate skill names are silently overwritten. In addition, the agent-side skills field is not documented in docs/index.md as required by the ticket acceptance criteria, and a new error-handling bug in SkillReferenceResolver can mask exceptions with UnboundLocalError. Once these are addressed, the PR should be in good shape.

Critical Issues

None

Major Issues

src/cleveractors/agents/llm.py lines 1429-1431 — The stuck-model synthesis follow-up constructs tool_ctx_s as {"_unsafe_mode": True} if parent_unsafe else None instead of calling self._build_tool_context(parent_unsafe). Consequently, _loaded_skills is not forwarded to the ephemeral ToolAgent. If the model emits a skill tool call during this follow-up round, ToolAgent._skill_tool fails with "Skill '...' is not loaded for this agent." The main loop and budget-exhaustion synthesis round both correctly use _build_tool_context; this path should too.
src/cleveractors/agents/skills.py lines 98-109 — SkillLoader.load stores resolved skills in loaded[skill.name] = skill, silently overwriting any earlier skill with the same name. Two skills references that resolve to identically-named skills (or two refs to the same skill) will yield a catalogue with one entry and the other's instructions will be lost. Validate uniqueness and raise AgentCreationError with a clear message.
docs/index.md §4.4 — The new optional skills configuration field for type: llm agents is not documented in the Actor Configuration Standard, even though ticket #88 acceptance criteria explicitly require it and ADR-2034 D-5 states the field is added to docs/index.md §4.4. Add the field documentation (optional list of package references using registry:, ID:, and local: schemes).
src/cleveractors/agents/skill_resolution.py lines 84-124 — resolved is only assigned inside the try block. If resolver.resolve() raises an exception other than FileNotFoundError/ValueError (e.g., RuntimeError for a registry reference called inside a running event loop), the finally runs and then if resolved is None: raises UnboundLocalError, masking the original failure. Initialize resolved = None before the try or add a broader except path that re-raises as RegistryError.

Minor Issues

Coverage threshold mismatch / below declared gate — CONTRIBUTING.md, .gitea/workflows/ci.yml, and the coverage_report session docstring state the gate is 97%, but noxfile.py sets COVERAGE_THRESHOLD = 96.5. The PR test plan reports 96.9% and cites 96.5%. Align noxfile.py with the documented 97% gate and add tests to reach it.
docs/actor-registry-standard.md §16.1 line 576 — The last paragraph references "Actor Configuration Standard §22" for the agent-side skills field, but docs/index.md has no §22. This appears to be a stale/future reference; it should point to the actual location (currently §4.4 or ADR-2034).
src/cleveractors/agents/skill_schema.py Skill.to_context_dict lines 72-88 — The context representation exposed to the skill tool and LLMAgent.get_metadata() drops metadata, allowed_tools, license, and compatibility. Since allowed_tools is documented as a pre-approved tool list, making it invisible to the model limits its usefulness. Consider surfacing at least allowed_tools and metadata in activation output or catalogue metadata.
src/cleveractors/agents/tool.py _decode_skill_resource line 1099 and src/cleveractors/agents/skill_schema.py SkillResource.decoded_text line 53 — Base64 resources are decoded with .decode("utf-8"), which will crash for genuine binary assets (images, PDFs, etc.) even though the spec requires supporting non-UTF-8 files via encoding: base64. Handle binary resources safely (e.g., return base64 text or skip UTF-8 decoding).
robot/SkillLoadingTestLib.py line 56 — tempfile.mkdtemp() directories are not cleaned up after tests. Add teardown cleanup to avoid leaking temp directories in CI.

Nits

Test gaps — Consider adding Behave/Robot scenarios for: duplicate skill names in the skills list; empty description/instructions strings; and the stuck-model synthesis path invoking the skill tool.
features/steps/agent_skills_steps.py — tempfile.mkdtemp() directories created for local-store tests are not cleaned up.
src/cleveractors/agents/skill_resolution.py lines 94-112 — When called inside a running event loop, resolver cleanup is scheduled with asyncio.ensure_future() and not awaited, so it may not complete before loop shutdown. Prefer the async resolution path where possible.

Summary

The PR delivers a clean, registry-agnostic skill-loading architecture that aligns well with ADR-2034 and reuses the existing package-resolution machinery. The test suite is extensive and the Robot integration test gives good end-to-end confidence. However, the two previously reported functional issues remain unfixed, the new SkillReferenceResolver error-handling path has a latent UnboundLocalError, and the skills field is missing from the Actor Configuration Standard. Addressing the major items above will make this ready to merge.

## PR Review: !94 (Ticket #88) ### Verdict: Request Changes The implementation still has the two functional regressions identified in the previous review: the stuck-model synthesis path drops the skill catalogue, and duplicate skill names are silently overwritten. In addition, the agent-side `skills` field is not documented in `docs/index.md` as required by the ticket acceptance criteria, and a new error-handling bug in `SkillReferenceResolver` can mask exceptions with `UnboundLocalError`. Once these are addressed, the PR should be in good shape. ### Critical Issues None ### Major Issues 1. **`src/cleveractors/agents/llm.py` lines 1429-1431** — The stuck-model synthesis follow-up constructs `tool_ctx_s` as `{"_unsafe_mode": True} if parent_unsafe else None` instead of calling `self._build_tool_context(parent_unsafe)`. Consequently, `_loaded_skills` is not forwarded to the ephemeral `ToolAgent`. If the model emits a `skill` tool call during this follow-up round, `ToolAgent._skill_tool` fails with "Skill '...' is not loaded for this agent." The main loop and budget-exhaustion synthesis round both correctly use `_build_tool_context`; this path should too. 2. **`src/cleveractors/agents/skills.py` lines 98-109** — `SkillLoader.load` stores resolved skills in `loaded[skill.name] = skill`, silently overwriting any earlier skill with the same name. Two `skills` references that resolve to identically-named skills (or two refs to the same skill) will yield a catalogue with one entry and the other's instructions will be lost. Validate uniqueness and raise `AgentCreationError` with a clear message. 3. **`docs/index.md` §4.4** — The new optional `skills` configuration field for `type: llm` agents is not documented in the Actor Configuration Standard, even though ticket #88 acceptance criteria explicitly require it and ADR-2034 D-5 states the field is added to `docs/index.md` §4.4. Add the field documentation (optional list of package references using `registry:`, `ID:`, and `local:` schemes). 4. **`src/cleveractors/agents/skill_resolution.py` lines 84-124** — `resolved` is only assigned inside the `try` block. If `resolver.resolve()` raises an exception other than `FileNotFoundError`/`ValueError` (e.g., `RuntimeError` for a registry reference called inside a running event loop), the `finally` runs and then `if resolved is None:` raises `UnboundLocalError`, masking the original failure. Initialize `resolved = None` before the `try` or add a broader `except` path that re-raises as `RegistryError`. ### Minor Issues 5. **Coverage threshold mismatch / below declared gate** — `CONTRIBUTING.md`, `.gitea/workflows/ci.yml`, and the `coverage_report` session docstring state the gate is **97%**, but `noxfile.py` sets `COVERAGE_THRESHOLD = 96.5`. The PR test plan reports **96.9%** and cites 96.5%. Align `noxfile.py` with the documented 97% gate and add tests to reach it. 6. **`docs/actor-registry-standard.md` §16.1 line 576** — The last paragraph references "Actor Configuration Standard §22" for the agent-side `skills` field, but `docs/index.md` has no §22. This appears to be a stale/future reference; it should point to the actual location (currently §4.4 or ADR-2034). 7. **`src/cleveractors/agents/skill_schema.py` `Skill.to_context_dict` lines 72-88** — The context representation exposed to the `skill` tool and `LLMAgent.get_metadata()` drops `metadata`, `allowed_tools`, `license`, and `compatibility`. Since `allowed_tools` is documented as a pre-approved tool list, making it invisible to the model limits its usefulness. Consider surfacing at least `allowed_tools` and `metadata` in activation output or catalogue metadata. 8. **`src/cleveractors/agents/tool.py` `_decode_skill_resource` line 1099 and `src/cleveractors/agents/skill_schema.py` `SkillResource.decoded_text` line 53** — Base64 resources are decoded with `.decode("utf-8")`, which will crash for genuine binary assets (images, PDFs, etc.) even though the spec requires supporting non-UTF-8 files via `encoding: base64`. Handle binary resources safely (e.g., return base64 text or skip UTF-8 decoding). 9. **`robot/SkillLoadingTestLib.py` line 56** — `tempfile.mkdtemp()` directories are not cleaned up after tests. Add teardown cleanup to avoid leaking temp directories in CI. ### Nits 10. **Test gaps** — Consider adding Behave/Robot scenarios for: duplicate skill names in the `skills` list; empty `description`/`instructions` strings; and the stuck-model synthesis path invoking the `skill` tool. 11. **`features/steps/agent_skills_steps.py`** — `tempfile.mkdtemp()` directories created for local-store tests are not cleaned up. 12. **`src/cleveractors/agents/skill_resolution.py` lines 94-112** — When called inside a running event loop, resolver cleanup is scheduled with `asyncio.ensure_future()` and not awaited, so it may not complete before loop shutdown. Prefer the async resolution path where possible. ### Summary The PR delivers a clean, registry-agnostic skill-loading architecture that aligns well with ADR-2034 and reuses the existing package-resolution machinery. The test suite is extensive and the Robot integration test gives good end-to-end confidence. However, the two previously reported functional issues remain unfixed, the new `SkillReferenceResolver` error-handling path has a latent `UnboundLocalError`, and the `skills` field is missing from the Actor Configuration Standard. Addressing the major items above will make this ready to merge.

CI / lint (pull_request) Successful in 51s

Details

CI / typecheck (pull_request) Successful in 1m36s

Details

CI / security (pull_request) Successful in 1m32s

Details

CI / quality (pull_request) Failing after 1m25s

Details

CI / unit_tests (pull_request) Failing after 1m28s

Details

CI / coverage (pull_request) Has been skipped

Details

CI / build (pull_request) Successful in 52s

Details

CI / integration_tests (pull_request) Failing after 1m53s

Details

CI / status-check (pull_request) Failing after 12s

Details

CI / benchmark (pull_request) Failing after 1m28s

Details

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/skill-package-support:feature/skill-package-support

git checkout feature/skill-package-support

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Blocks

#88 feat(skills): add Skill package schema and agent-side skill loading support

cleveragents/cleveractors-core

Reference: cleveragents/cleveractors-core#94

feat(skills): add skill package schema and agent-side skill loading support #94

Summary

Test plan

Note: bundled scripts/ resources are readable, not directly runnable

PR Review: !94 (Ticket #88)

Verdict: Request Changes

Critical Issues

Major Issues

Minor Issues

Nits

Summary

PR Review: !94 (Ticket #88)

Verdict: Request Changes

Critical Issues

Major Issues

Minor Issues

Nits

Summary

Checkout

Note: bundled `scripts/` resources are readable, not directly runnable