diff --git a/CHANGELOG.md b/CHANGELOG.md index c8801bb..7506888 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm ### Added +- **Skill Package Schema and Agent-Side Skill Loading (issue #88)** (`agents/skills.py`, `agents/skill_schema.py`, `agents/skill_resolution.py`, `agents/factory.py`, `agents/llm.py`, `agents/tool.py`, `agents/llm_tools.py`): Adds a normative Skill package schema (Package Registry Standard §16) aligned with the open [agentskills.io](https://agentskills.io/specification) Agent Skills format, and a new optional `skills` field on `type: llm` agent configs that resolves, validates, and loads Skill packages through the existing `cleveractors.registry` client. + + A `skill`-type package is a single YAML mapping with required `name`, `description`, and `instructions`, plus optional `license`, `compatibility`, `metadata`, `allowed_tools`, and `resources` (bundled files, UTF-8 or base64-encoded) — see `docs/actor-registry-standard.md` §16 for the full schema. `SkillLoader` resolves each `skills` reference (via `registry:`, `ID:`, or `local:` schemes, mirroring the existing template package-reference resolution path in `cleveractors.templates.base._resolve_package_ref`), validates it with `SkillValidator`, and returns an augmented config with a discovery catalogue appended to `system_prompt`, a synthesized `skill` activation tool appended to `tools`, and the resolved catalogue under an internal `_loaded_skills` key. + + Progressive disclosure is implemented as three stages: discovery (name/description always in the system prompt), activation (a tool call retrieves a skill's full instructions), and execution (a tool call reads an individual bundled resource, with the same `offset`/`max_chars` pagination convention as `file_read`, ADR-2033). `LLMAgent.get_metadata()` and `get_capabilities()` surface `skills_loaded`, `skill_names`, and a `"skill-loading"` capability when skills are configured; `LLMAgent` itself has no registry awareness — all resolution lives in `SkillLoader`, invoked by `AgentFactory` before agent construction. + + See `docs/adr/ADR-2034-skill-package-support.md` for the full set of design decisions (schema alignment with agentskills.io, package identity and content-addressing, progressive disclosure, and the factory/loader placement rationale). + + **Module:** `src/cleveractors/agents/skills.py` (`SkillLoader`), `src/cleveractors/agents/skill_schema.py` (`Skill`, `SkillResource`, `SkillValidator`), `src/cleveractors/agents/skill_resolution.py` (`SkillReferenceResolver`), `src/cleveractors/agents/factory.py` (`AgentFactory`), `src/cleveractors/agents/llm.py` (`LLMAgent`), `src/cleveractors/agents/tool.py` (`ToolAgent._skill_tool`), `src/cleveractors/agents/llm_tools.py` (`_BUILTIN_TOOL_SCHEMAS["skill"]`). BDD: 68 scenarios in `features/agent_skills.feature`. Robot: 6 test cases in `robot/agent_skills.robot`. + - **Offset-Based Pagination for `file_read` (issue #83)** (`agents/tool.py`, `agents/llm_tools.py`): Adds an optional `offset` parameter so large files can be read across multiple bounded `file_read` calls instead of only ever seeing the first `max_chars` characters. `offset` is a character position (consistent with the existing `max_chars` unit) defaulting to `0`; omitting it reproduces the prior behavior unchanged. Combined with `max_chars`, it returns the window `[offset, offset + max_chars)`. Truncated responses now include a `MORE_CONTENT_AT_OFFSET: ` marker in the `[FILE_READ_SUCCESS]` header giving the exact offset for the next call, so the caller never has to compute `offset + max_chars` itself. Requesting an offset at or beyond the end of the file returns a `NO_MORE_CONTENT_AT_OFFSET` success response with empty content rather than an error; a negative or non-integer offset raises `ExecutionError`. diff --git a/docs/actor-registry-standard.md b/docs/actor-registry-standard.md index eac5994..e7cc300 100644 --- a/docs/actor-registry-standard.md +++ b/docs/actor-registry-standard.md @@ -1,6 +1,6 @@ # The Package Registry Standard -**Version:** 1.0.0 +**Version:** 1.1.0 **Status:** Normative --- @@ -514,10 +514,110 @@ A compliant implementation SHOULD be tested against (at minimum) the following c ## 15. Versioning of This Standard -This document is **Version 1.0.0** of the Package Registry Standard. Future revisions: +This document is **Version 1.1.0** of the Package Registry Standard. Future revisions: - A **patch** version (1.0.x) corrects errors and clarifies semantics without changing required behavior. - A **minor** version (1.x.0) adds new optional features without breaking conformance of existing implementations. - A **major** version (x.0.0) introduces breaking changes; non-backward-compatible. -Conformance MUST be declared against a specific version of this standard. Implementations MAY support multiple major versions concurrently. \ No newline at end of file +Conformance MUST be declared against a specific version of this standard. Implementations MAY support multiple major versions concurrently. + +### 15.1 Revision History + +| Version | Change | +|---------|--------| +| 1.1.0 | Added §16 (Skill Package Type) per [ADR-2034](adr/ADR-2034-skill-package-support.md) — a minor, additive extension under §1.3. | +| 1.0.0 | Initial normative release. | + +--- + +## 16. Specification Extension: Skill Package Type (ADR-2034) + +> **Status:** Normative addition, introduced at Version 1.1.0. Approved via +> [ADR-2034](adr/ADR-2034-skill-package-support.md) (2026-07-31, Issue #88). +> This section is appended after the original §1–§15 body rather than spliced +> into §3.2, so that the text approved by that ADR remains intact and +> traceable to its origin. It extends §3.2 under the extensibility clause in +> §1.3 ("A compliant implementation MAY provide additional package types +> beyond those defined here, provided they do not conflict with the names +> defined in this standard") and does not alter any `MUST`/`SHALL` +> requirement stated in §1–§15. + +### 16.1 Skill Package Schema + +A `skill`-type package (§3.2, prefix `pkg_skl_`) is a single YAML mapping +whose top-level fields mirror the frontmatter of a `SKILL.md` document from +the open [Agent Skills specification](https://agentskills.io/specification), +plus two fields that carry the instruction body and any bundled resource +files. Unlike the directory-based layout of that external format +(`SKILL.md` plus optional `scripts/`, `references/`, `assets/`), a `skill` +package remains a **single, canonicalized, content-addressed mapping**, +consistent with every other package type defined by §3.1. A compliant +implementation MUST support the following fields: + +| Field | Required | Type | Constraints | +|-------|----------|------|-------------| +| `skill` | Yes | boolean | Discriminator field; MUST be `true`. | +| `name` | Yes | string | 1–64 characters; lowercase ASCII letters, digits, and hyphens only (`[a-z0-9-]`); MUST NOT start or end with a hyphen and MUST NOT contain consecutive hyphens. | +| `description` | Yes | string | 1–1024 characters; non-empty; SHOULD describe what the skill does and when an agent should use it. | +| `instructions` | Yes | string | The procedural knowledge conveyed to an agent that activates this skill (the Markdown body of a `SKILL.md` document). | +| `license` | No | string | A license name, or a reference to a bundled license file under `resources`. | +| `compatibility` | No | string | 1–500 characters; describes environment requirements (product, system packages, network access). | +| `metadata` | No | mapping of string to string | Arbitrary, client-defined properties. | +| `allowed_tools` | No | string | A space-separated list of pre-approved tool names (mirrors the external format's `allowed-tools` field, rendered as `allowed_tools` for naming consistency with this repository's other snake_case configuration keys). | +| `resources` | No | mapping | Bundled files. Each key is a relative POSIX path (no leading slash, no `.`/`..` segments); each value is a mapping `{content: , encoding: <"utf-8"\|"base64">}`. Text files MUST use `encoding: utf-8` and store content verbatim; files that are not valid UTF-8 MUST use `encoding: base64`. | + +`name` and `description` are the two fields a compliant agent implementation +loads at **discovery** time; `instructions` is loaded at **activation** +time; `resources` entries are loaded individually at **execution** time. +This three-stage progressive-disclosure loading model lets an agent hold +many skills in its configuration at a small, fixed context cost, deferring +the larger `instructions` and `resources` payloads until they are actually +needed. See the Actor Configuration Standard §22 for the agent-side +`skills` configuration field that references packages of this type. + +### 16.2 Skill Package Identity + +A skill's Package ID is computed exactly as described in §5 and §6 for +every other package type: `pkg_skl_<40-hex-sha1>`, where the SHA-1 is taken +over the canonical form of the mapping. Per §6.1, the canonicalization +process strips lifecycle fields (`version`, `release_date`) at every depth +before hashing; a skill's registry version therefore lives outside its +content-addressed identity, and a `metadata.version` field carried over +from an external `SKILL.md` document is likewise stripped before hashing +and MUST NOT be relied upon as an identity discriminator. Two skills with +byte-identical `skill`/`name`/`description`/`instructions`/`license`/ +`compatibility`/`metadata`/`allowed_tools`/`resources` content resolve to +the same Package ID regardless of publication order or registry version tag. + +### 16.3 Example + +```yaml +skill: true +name: pdf-processing +description: > + Extract text and tables from PDFs, fill forms, and merge files. + Use when the user mentions PDFs, forms, or document extraction. +license: Apache-2.0 +compatibility: Requires Python 3.14+ and uv +metadata: + author: acme +instructions: | + # PDF processing + + ## Extracting text + Run `scripts/extract.py ` and read the emitted text. + See references/REFERENCE.md for the full CLI surface. +allowed_tools: "Read Bash(python:*)" +resources: + scripts/extract.py: + encoding: utf-8 + content: | + import sys + ... + references/REFERENCE.md: + encoding: utf-8 + content: | + # Reference + ... +``` \ No newline at end of file diff --git a/docs/adr/ADR-2034-skill-package-support.md b/docs/adr/ADR-2034-skill-package-support.md new file mode 100644 index 0000000..214be17 --- /dev/null +++ b/docs/adr/ADR-2034-skill-package-support.md @@ -0,0 +1,433 @@ +# ADR-2034: Skill Package Support — Registry Schema, Identity, and Agent-Side Loading + +**Status:** approved + +**Date:** 2026-07-31 + +**Author:** Luis Mendes (CoreRasurae) + +**Issue:** #88 — feat(skills): add Skill package schema and agent-side skill loading support + +--- + +## Context + +The Package Registry Standard (`docs/actor-registry-standard.md` §3.2) already +lists `skill` as a first-class package type — prefix `pkg_skl_`, description +"Skill packages defining capabilities for AI agents". The type is wired end to +end on the *client* side: `PackageType.SKILL = "skl"` +(`cleveractors.registry.types.PackageType`), +the local-store type detector (`cleveractors.registry.local_store.LocalPackageStore._detect_package_type`) +maps a document containing a `skill` key to `PackageType.SKILL`, the resolver +type map (`cleveractors.registry.reference_resolver.PackageContentResolver`) +maps `"skill" → "skl"`, and `ComponentReference.ref_type` +(`cleveractors.templates.base.ComponentReference`) already enumerates +`'skill'`. The registry docs use `package_type="skill"` throughout their +examples. + +Despite all of that plumbing, two things do not exist: + +1. **There is no normative Skill package schema.** Unlike an `actor` package + (which conforms to the Actor Configuration Standard, `docs/index.md`), a + `skill` package has no defined internal structure beyond the generic + package requirements (a YAML mapping with a name and a description). + Nothing says what fields a skill declares or how they are interpreted. + +2. **There is no agent-side consumption path.** The LLM agent + (`cleveractors.agents.llm.LLMAgent`) has no `skills` configuration field and + makes no reference to `cleveractors.registry`. A skill package can be + *fetched* by the generic client, but the resolved content has nowhere to + attach — no agent loads it, and no runtime behaviour reacts to it. + +Meanwhile, an open **Agent Skills** format has emerged at +[agentskills.io](https://agentskills.io/specification). It was originally +developed by Anthropic, released as an open standard, and is now consumed by a +large ecosystem of agentic clients (Claude Code, Claude, OpenAI Codex, Gemini +CLI, Cursor, GitHub Copilot, VS Code, Goose, OpenHands, and many more). In that +format a skill is a **directory** with a required `SKILL.md` (YAML frontmatter ++ Markdown body) plus optional `scripts/`, `references/`, and `assets/` +subdirectories, loaded by agents through **progressive disclosure** (discovery → +activation → execution). + +This creates a structural tension the ADR must resolve. The Package Registry +Standard models every package as a **single canonicalized YAML mapping**, +content-addressed as one SHA-1 blob (§6): `Canonicalizer.compute_package_id` +(`cleveractors.registry.canonical.Canonicalizer.compute_package_id`) requires +its `content` argument to be a `dict`, and `LocalPackageStore` only accepts a +single `.yaml`/`.yml` file. agentskills.io models a skill as a **file tree**. +A `pkg_skl_` package must faithfully round-trip an agentskills.io-conformant +skill *and* remain a single content-addressed mapping. + +Both standards permit this kind of extension. Package Registry Standard §1.3 +and Actor Configuration Standard §1.3 both allow compliant implementations to +"provide additional … types … beyond those defined here, provided they do not +conflict with the names defined in this standard." This ADR adds a Skill schema +and a `skills` LLM-agent config field without redefining any existing +`MUST`/`SHALL` requirement, mirroring the extension approach of ADR-2030 +(tool-calling schema extensions) and ADR-2031 (new §4.4 config fields). + +This ADR resolves the open design questions the issue identified: the Skill +package schema, how a skill's unique ID is computed, whether a skill is a +single registry package or a set of sub-component packages, how skills integrate +with the (external) registry server, and how an agent executes with a set of +loaded skill packages. + +--- + +## Decision + +### D-1: A Skill Is a Single Self-Contained Registry Package + +**What:** A skill is modelled as **one** `pkg_skl_` package — a single YAML +mapping that embeds the entire skill (its frontmatter metadata, its +instructions, and all bundled resource files). It is **not** decomposed into +sub-component packages (see D-2), and it is **not** stored as an opaque archive +blob (see Alternatives A-1). + +**Why:** Every other package type in the standard is a single content-addressed +mapping, and the whole registry pipeline — canonicalization, SHA-1 identity, +reference resolution, caching, `local:` detection — assumes exactly that shape. +Keeping a skill within one mapping means all of that machinery works unchanged. +It also gives a skill **one identity, one version, and one atomic fetch**: the +agent either has the complete, tamper-evident skill or it does not, with no +partially-assembled intermediate state. + +### D-2: Skill Package Schema + +**What:** A `skill`-type package is a YAML mapping whose top-level keys mirror +the agentskills.io `SKILL.md` frontmatter, plus two keys that carry the body and +the bundled files. The schema is added to `docs/actor-registry-standard.md` as a +new normative subsection for the `skill` package type. + +| Field | Required | Type | Constraints (aligned to agentskills.io) | +|-------|----------|------|------------------------------------------| +| `skill` | Yes | boolean | Discriminator; MUST be `true`. Its presence is already how `LocalPackageStore._detect_package_type` classifies the document as `PackageType.SKILL`. | +| `name` | Yes | string | 1–64 chars; lowercase `[a-z0-9-]` only; MUST NOT start/end with a hyphen or contain consecutive hyphens. | +| `description` | Yes | string | 1–1024 chars; non-empty; describes *what* the skill does and *when* to use it. | +| `instructions` | Yes | string | The Markdown body of `SKILL.md` (the procedural knowledge). | +| `license` | No | string | License name or reference to a bundled license file. | +| `compatibility` | No | string | 1–500 chars; environment requirements. | +| `metadata` | No | mapping | Arbitrary string→string map for client-defined properties. | +| `allowed_tools` | No | string | Space-separated pre-approved tool list (agentskills.io `allowed-tools`, rendered snake_case for YAML consistency with the rest of this repo's config keys). Experimental. | +| `resources` | No | mapping | Bundled files: maps a relative POSIX path (e.g. `scripts/extract.py`, `references/REFERENCE.md`) to a mapping `{content: , encoding: <"utf-8"|"base64">}`. Mirrors the agentskills.io `scripts/`, `references/`, `assets/` tree. | + +`name` and `description` are exactly the two fields agentskills.io loads at the +**discovery** stage; `instructions` is the SKILL.md body loaded at +**activation**; `resources` are the files loaded at **execution** (see D-4). + +**Why alignment, not invention:** Aligning field names and semantics with the +open format is what lets a CleverActors `pkg_skl_` package be produced from — and +consumed by — the broader agent-skills ecosystem. A bespoke schema would strand +us from that interoperability for no benefit (see Alternatives A-5). + +**Example (`skill`-type package document):** + +```yaml +skill: true +name: pdf-processing +description: > + Extract text and tables from PDFs, fill forms, and merge files. + Use when the user mentions PDFs, forms, or document extraction. +license: Apache-2.0 +compatibility: Requires Python 3.14+ and uv +metadata: + author: acme +instructions: | + # PDF processing + + ## Extracting text + Run `scripts/extract.py ` and read the emitted text. + See references/REFERENCE.md for the full CLI surface. +allowed_tools: "Read Bash(python:*)" +resources: + scripts/extract.py: + encoding: utf-8 + content: | + import sys + ... + references/REFERENCE.md: + encoding: utf-8 + content: | + # Reference + ... +``` + +### D-3: Unique ID Computation + +**What:** A skill's Package ID is computed exactly like every other package: +`Canonicalizer.compute_package_id(skill_mapping, PackageType.SKILL)`, yielding +`pkg_skl_<40-hex-sha1>` where the SHA-1 is taken over the RFC-8785 canonical +JSON of the mapping (`cleveractors.registry.canonical.Canonicalizer`). No new +hashing path is introduced. + +To keep the hash deterministic across the directory→mapping packing, the +`resources` mapping MUST obey these rules: + +- Keys are relative POSIX paths (forward slashes), normalized (no `.`/`..` + segments, no leading slash). +- Text files are stored verbatim as UTF-8 strings with `encoding: utf-8`. +- Files that are not valid UTF-8 (images, other binaries) are base64-encoded + with `encoding: base64`. +- Ordering is irrelevant to identity: the canonicalizer already sorts every + mapping's keys lexicographically and NFC-normalizes every string, so two + skills with the same files in a different YAML order hash identically. + +**Version is deliberately excluded from identity.** The canonicalizer strips the +lifecycle fields `version` and `release_date` **at every depth** +(`Canonicalizer._transform_dict` filters recursively), per Package Registry +Standard §6.1. Two consequences follow, both intended: + +1. A skill's registry **version** (`vX.Y.Z`, resolved by + `cleveractors.registry.resolver.resolve_version`) lives *outside* the + content-addressed identity — it is a mutable tag over an immutable blob, + consistent with how the standard treats all packages. +2. Because stripping is recursive, a `metadata.version` field (which + agentskills.io examples sometimes carry) is **also** removed before hashing. + Authors therefore MUST NOT rely on `metadata.version` as an identity + discriminator; re-publishing a byte-identical skill under a new version + yields the **same** `pkg_skl_` ID (natural deduplication), which is the + desired content-addressing behaviour. + +### D-4: Progressive Disclosure Is the Normative Loading Model + +**What:** A loaded skill is disclosed to the agent in the same three stages +agentskills.io defines, so that many skills can be attached to an agent at a +small context cost: + +1. **Discovery** (~always present): only `name` + `description` of each loaded + skill are placed in the agent's context — enough for the model to know a + skill exists and when it applies. +2. **Activation** (on demand): the full `instructions` body is pulled into + context only when a task matches a skill. +3. **Execution** (on demand): individual `resources` files are read only when + the instructions call for them. + +**Why:** Inlining every skill's full instructions up front would blow the +context budget and directly conflict with ADR-2031's `token_budget_percent` +accounting. Progressive disclosure is the whole point of the agentskills.io +format and must be preserved (see Alternatives A-3). + +### D-5: Agent-Side Loading via a `skills` Config Field and a `SkillLoader` + +**What:** A new optional `skills` field is added to the LLM agent config +(`docs/index.md` §4.4). It accepts a list of package references using the +existing reference schemes (`registry:`, `ID:`, `local:`): + +```yaml +agents: + support_agent: + type: llm + config: + provider: anthropic + model: claude-3-5-sonnet-20241022 + skills: + - "registry.example.com:acme/pdf-processing@v1.0.0" + - "ID:pkg_skl_a1b2c3d4e5f67890abcdef1234567890abcdef" + - "local:skills/house-style.yaml" +``` + +Resolution happens at **agent-creation time**, in a new component +`cleveractors.agents.skills.SkillLoader`, invoked from +`cleveractors.agents.factory.AgentFactory._create_agent_instance` on the `"llm"` +branch (the point that today builds the `LLMAgent(**kwargs)` call). The loader: + +1. Parses each reference and resolves it through the **existing** + `cleveractors.registry.reference_resolver.PackageContentResolver` + (`.aresolve(ref, package_type="skill")`), reusing its two-tier cache and the + `local:` `LocalPackageStore` — the same path already used for template + package references (`cleveractors.templates.base._resolve_package_ref`). +2. Validates each resolved mapping against the D-2 schema (see D-7). +3. **Augments the agent config the agent already understands**, so that + `LLMAgent` itself needs no registry awareness (respecting its read-only + `config` contract, `cleveractors.agents.llm.LLMAgent.__init__`): + - a compact **discovery catalogue** (`name` + `description` per skill) is + appended to the effective `system_prompt`, which `LLMAgent` already renders + and injects as a `SystemMessage` + (`cleveractors.agents.llm.LLMAgent.process_message` / + `.stream_message`); + - a synthesized **`skill` activation tool** is appended to `config["tools"]`, + normalized through the existing + `cleveractors.agents.llm_tools.normalize_tool_entry` machinery (ADR-2030), + so the model can call it to pull a named skill's full `instructions` + (activation) and enumerate its `resources`; + - `resources` are exposed for **execution**-stage reads via a + `ToolAgent`-backed handler (reusing the existing `file_read`-style access, + ADR-2033) rooted at a virtual skill directory, so bundled scripts and + reference files are readable without touching the host filesystem. + +The set of loaded skills becomes the agent's skill catalogue; `get_metadata` +(`cleveractors.agents.llm.LLMAgent.get_metadata`) is extended to surface the +loaded skill names/count, and `"skill-loading"` is added as an agent capability +identifier (Actor Configuration Standard §4.11). + +**Why the factory/loader layer and not `LLMAgent`:** package references are +*already* resolved upstream of agent construction in the template system, never +inside an agent. Placing skill resolution in a dedicated `SkillLoader` invoked +by the factory keeps `LLMAgent` free of registry coupling (SRP/DIP), reuses the +one resolution path, and matches the established pattern (see Alternatives A-4). + +### D-6: Registry & Server Integration Reuses the Standard, Unchanged + +**What:** Skills are published, discovered, and resolved through the **same** +Package Registry Standard §8 HTTP API as every other package type — no +skill-specific server endpoints or schema hooks: + +- **Publish:** `POST /publish` (§8.3) with a canonicalized `pkg_skl_` document. +- **Resolve:** `GET /{type}/{ns}/{name}?version=` (§8.2.2) with `type=skill`, + driving version-alias resolution (`resolve_version`). +- **Fetch:** `GET /packages/{pkg_skl_...}` (§8.2.1). +- **Browse/discover:** `GET /browse?type=skill` (§8.4.1); + `supported_types` already advertises `"skill"` (§8.4.2). + +The registry *server* is out of scope for this repository (cleveractors-core +ships the **client** only — `cleveractors.registry`). No server change is needed +beyond the generic package validation the standard already mandates: a skill is +just a well-formed mapping with a content-addressed SHA-1. Authoring/publishing +tooling (packing an on-disk skill **directory** into a `pkg_skl_` mapping) is a +follow-up: `local:` references today require a single `.yaml` file +(`LocalPackageStore`), so consuming an on-disk agentskills.io skill directory +needs a `pack_skill_directory` utility (or an extension to `LocalPackageStore`) +that applies the D-3 packing rules. This ADR defines the target format; the +packer is tracked as a follow-up subtask on issue #88. + +### D-7: Validation and Error Semantics + +**What:** `SkillLoader` validates each resolved skill against the D-2 schema and +the agentskills.io constraints (name charset/length, description length, +required `instructions`, `resources` path/encoding well-formedness). Failures +are surfaced fail-fast, consistent with the codebase's existing load-time +validation posture: + +- A malformed skill document raises the registry + `cleveractors.registry.exceptions.ValidationError`. +- A missing or inaccessible skill reference propagates the existing registry + errors (`PackageNotFoundError`, `VersionNotFoundError`, `AccessDeniedError`, + `RegistryNetworkError`). +- At agent-creation time these are wrapped as + `cleveractors.core.exceptions.AgentCreationError` (the error the factory + already raises when an agent cannot be built), so an actor with an unresolvable + or invalid skill fails to load rather than silently degrading. + +**Interplay with prior ADRs:** `allowed_tools` composes with the tool-schema +machinery of ADR-2030; injected `instructions` add to the conversation and are +therefore accounted for by ADR-2031's `token_budget_percent`; execution-stage +resource reads reuse the `file_read` surface governed by ADR-2033. None of these +ADRs' normative behaviours are altered. + +--- + +## Consequences + +### Positive + +- **Ecosystem interoperability:** a `pkg_skl_` package is a faithful, + round-trippable encoding of an agentskills.io skill, so skills authored for the + broader ecosystem can be consumed here and vice versa. +- **Zero new identity machinery:** skill IDs reuse `compute_package_id`; caching, + reference resolution, `local:`/`ID:`/`registry:` schemes, and version-alias + resolution all work with no registry-layer changes. +- **`LLMAgent` stays registry-agnostic:** all resolution lives in `SkillLoader` + at the factory layer, preserving the agent's read-only `config` contract and + the single existing resolution path. +- **Context-efficient:** progressive disclosure keeps many skills attachable at + ~discovery cost, cooperating with ADR-2031's budget accounting instead of + fighting it. +- **Atomic, tamper-evident skills:** one SHA-1 covers the whole skill (metadata + + instructions + resources); a skill is either wholly present or absent. +- **Backward compatible:** `skills` is optional; configs that omit it behave + exactly as today. No existing package, agent, or actor changes shape. + +### Negative / Risks + +- **Directory→mapping packing surface:** encoding a file tree as a `resources` + mapping (D-3) is new non-normative surface; a future change to the packing + rules would change skill IDs. Mitigated by pinning the rules in the spec update + and by content-addressing (any change is visible as a new ID). +- **Large/binary resources inflate the blob:** base64-encoded assets bloat the + single mapping and every fetch of it. An optional escape hatch is allowed — a + `resources` entry MAY instead be an `ID:`/`registry:` reference to a separate + package, resolved by the canonicalizer's existing reference-resolution pass — + but the default remains a single self-contained package. Guidance: keep skills + instruction-heavy and asset-light. +- **`metadata.version` stripping is a footgun:** authors migrating agentskills.io + skills that carry `metadata.version` must understand it is removed before + hashing (D-3). Documented explicitly in the spec update. +- **Activation via a synthesized tool couples skills to tool-calling:** models + without tool-calling get discovery + (optionally) static inlining only. Deemed + acceptable since the tool-call loop is already the agent's primary capability + surface (ADR-2030/2031). +- **Specification surface growth:** a new registry schema subsection, a new §4.4 + agent field, and a new runtime component enlarge the spec. Justified by §1.3 of + both standards and consistent with the ADR-2030/2031 precedent. + +### Follow-up Required + +- **`pack_skill_directory` tooling** (D-6): pack an on-disk agentskills.io skill + directory into a `pkg_skl_` mapping and validate it (candidate: a `skills-ref`- + style validator, per agentskills.io). +- **Specification updates (post-acceptance):** add the Skill schema subsection to + `docs/actor-registry-standard.md` and the `skills` field to `docs/index.md` + §4.4, per the Specification-First process. +- **BDD + Robot coverage:** single/multi-skill resolution, each reference scheme, + missing/invalid skill, name/description constraint violations, progressive- + disclosure activation, and `token_budget_percent` interaction. +- **Decision to graduate to normative status:** once battle-tested, fold the + `skills` field and Skill schema into the standards proper via a follow-up ADR, + as ADR-2030/2031/2032 anticipate for their own extensions. + +--- + +## Alternatives Considered + +### A-1: Store the skill as an opaque directory archive (tar/zip blob) + +**Rejected because:** an opaque blob is not a YAML mapping, so +`compute_package_id` cannot canonicalize it, the canonicalizer's internal +reference resolution cannot see into it, and validation (D-7) cannot inspect it. +It would require a parallel identity and storage path for one package type, +defeating the uniformity that makes the registry work. + +### A-2: Decompose a skill into sub-component packages + +**Rejected because:** modelling `SKILL.md` as one package and each +script/reference/asset as its own `pkg_*` package fragments the skill's identity +and versioning across many blobs, turns one fetch into N, and removes atomic +tamper-evidence (a skill could be half-updated). Progressive disclosure does not +require physical decomposition — it is achieved *within* one package by +disclosing `name`/`description`, then `instructions`, then individual +`resources` on demand (D-4). The single-package escape hatch in D-2/Negative +(a resource MAY reference another package) covers the genuine case for sharing a +large asset without making decomposition the default. + +### A-3: Fully inline every skill's instructions into the system prompt at load + +**Rejected as the default because:** it discards progressive disclosure, spends +the full instruction-token cost of every attached skill up front, and collides +with ADR-2031's `token_budget_percent`. It remains available as an optional +simple mode for a single small skill, but the normative model is discovery + +on-demand activation (D-4). + +### A-4: Give `LLMAgent` its own resolver and resolve `skills` internally + +**Rejected because:** it couples the agent to `cleveractors.registry`, +duplicates the resolution/caching path that already lives in the template system +(`_resolve_package_ref` → `PackageContentResolver`), and violates the agent's +read-only `config` contract. Resolving in a dedicated `SkillLoader` at the +factory layer (D-5) keeps a single resolution path and a clean separation of +concerns (SRP/DIP). + +### A-5: Invent a bespoke CleverActors skill schema, ignoring agentskills.io + +**Rejected because:** the agentskills.io format is an established open standard +with broad client adoption; aligning to it (D-2) yields cross-product skill +reuse at no cost, whereas a bespoke schema would strand cleveractors from that +ecosystem. §1.3 of both standards favours non-conflicting alignment over +reinvention. + +### A-6: Include the agentskills.io `version` in the content-addressed identity + +**Rejected because:** the canonicalizer strips lifecycle fields (`version`, +`release_date`) by design (§6.1), and content-addressing should not fold a +mutable version tag into an immutable blob's identity. Registry versioning +(`resolve_version`) already provides the mutable-version layer over the immutable +`pkg_skl_` ID (D-3). diff --git a/docs/registry/index.md b/docs/registry/index.md index 3b8492c..381927c 100644 --- a/docs/registry/index.md +++ b/docs/registry/index.md @@ -1,17 +1,24 @@ # Package Registry Client -**CleverActors Package Registry Subsystem — Client-Side Implementation of the Package Registry Standard v1.0.0** +**CleverActors Package Registry Subsystem — Client-Side Implementation of the Package Registry Standard v1.1.0** --- ## Overview -The Package Registry Client implements the [Package Registry Standard v1.0.0](../actor-registry-standard.md) +The Package Registry Client implements the [Package Registry Standard v1.1.0](../actor-registry-standard.md) on the client side. It provides a complete, async-first library for discovering, resolving, fetching, caching, and validating versioned packages of AI components — actors, graphs, agents, templates, skills, MCP servers, and LSP services — from any compliant registry server. +`skill` packages have a normative internal schema (name, description, +instructions, and bundled resources) — see +[§16 of the Package Registry Standard](../actor-registry-standard.md#16-specification-extension-skill-package-type-adr-2034) +and, for how a `type: llm` agent loads a resolved skill, ADR-2034. Every +other package type (`actor`, `graph`, `stream`, `agent`, `template`, `mcp`, +`lsp`) resolves through this exact same client unchanged. + ## Architecture ``` diff --git a/docs/registry/integration.md b/docs/registry/integration.md index fc09042..30705e2 100644 --- a/docs/registry/integration.md +++ b/docs/registry/integration.md @@ -109,6 +109,15 @@ Resolves categorization templates from a registry, fetches tool packages for domain-specific agents, caches for repeated use, and gracefully handles missing or inaccessible packages. +The `skill`-typed references below resolve to packages conforming to the +normative Skill Package Schema +([Package Registry Standard §16](../actor-registry-standard.md#16-specification-extension-skill-package-type-adr-2034)): +a `name`, `description`, `instructions` body, and optional bundled +`resources`. A `type: llm` Actor agent can load a resolved skill directly +via its `skills` configuration field — see ADR-2034 +(`docs/adr/ADR-2034-skill-package-support.md`) for the agent-side loading +model (discovery, activation, and execution stages). + ```python """ Email categorization system. diff --git a/features/agent_skills.feature b/features/agent_skills.feature new file mode 100644 index 0000000..37ed018 --- /dev/null +++ b/features/agent_skills.feature @@ -0,0 +1,438 @@ +Feature: Skill Package Loading for LLM Agents + + # ── SkillValidator: Skill Package Schema (§16, ADR-2034) ──────────────── + + Scenario: Valid skill content validates successfully + Given skill content with name "pdf-processing" and description "Extract text from PDFs" + When I validate the skill content + Then the skill should validate without error + And the validated skill name should be "pdf-processing" + + Scenario: Skill content missing the discriminator field fails validation + Given skill content without the "skill" discriminator field + When I validate the skill content + Then skill validation should fail containing "discriminator" + + Scenario Outline: Skill name constraint violations fail validation + Given skill content with name "" and description "A valid description" + When I validate the skill content + Then skill validation should fail containing "invalid name" + + Examples: + | name | + | UPPERCASE | + | -leading-hyphen | + | trailing-hyphen- | + | double--hyphen | + | invalid_underscore | + + Scenario: Skill content with an empty name fails validation + Given skill content with an empty name and description "A valid description" + When I validate the skill content + Then skill validation should fail containing "name" + + Scenario: Skill name exceeding max length fails validation + Given skill content with an over-long name and description "A valid description" + When I validate the skill content + Then skill validation should fail containing "invalid name" + + Scenario: Skill missing description fails validation + Given skill content with name "valid-name" and no description + When I validate the skill content + Then skill validation should fail containing "description" + + Scenario: Skill description exceeding max length fails validation + Given skill content with name "valid-name" and an over-long description + When I validate the skill content + Then skill validation should fail containing "description" + + Scenario: Skill missing instructions fails validation + Given skill content with name "valid-name" and description "Valid" and no instructions + When I validate the skill content + Then skill validation should fail containing "instructions" + + Scenario: Skill with non-string license fails validation + Given skill content with an invalid license type + When I validate the skill content + Then skill validation should fail containing "license" + + Scenario: Skill with over-long compatibility fails validation + Given skill content with an over-long compatibility string + When I validate the skill content + Then skill validation should fail containing "compatibility" + + Scenario: Skill with non-mapping metadata fails validation + Given skill content with metadata set to a list instead of a mapping + When I validate the skill content + Then skill validation should fail containing "metadata" + + Scenario: Skill with non-string metadata values fails validation + Given skill content with metadata containing a non-string value + When I validate the skill content + Then skill validation should fail containing "metadata" + + Scenario: Skill with non-mapping resources fails validation + Given skill content with resources set to a string instead of a mapping + When I validate the skill content + Then skill validation should fail containing "resources" + + Scenario Outline: Skill resource path constraint violations fail validation + Given skill content with a resource at path "" + When I validate the skill content + Then skill validation should fail containing "resource path" + + Examples: + | path | + | /absolute/path.md | + | ../escape.md | + | a/../b.md | + + Scenario: Skill content with an empty resource path fails validation + Given skill content with a resource at an empty path + When I validate the skill content + Then skill validation should fail containing "resource path" + + Scenario: Skill resource missing content field fails validation + Given skill content with a resource entry missing "content" + When I validate the skill content + Then skill validation should fail containing "content" + + Scenario: Skill resource with invalid encoding fails validation + Given skill content with a resource using encoding "rot13" + When I validate the skill content + Then skill validation should fail containing "encoding" + + Scenario: Skill resource entry that is not a mapping fails validation + Given skill content with a resource entry that is a string instead of a mapping + When I validate the skill content + Then skill validation should fail containing "must be a mapping" + + Scenario: Skill content with valid metadata validates successfully + Given skill content with name "pdf-processing" and description "Extract text from PDFs" + And that skill content has metadata author set to "acme" + When I validate the skill content + Then the skill should validate without error + And the validated skill metadata author should equal "acme" + + Scenario: A UTF-8 resource decodes to its own content unchanged + Given skill content with a UTF-8 resource and a base64-encoded resource + When I validate the skill content + Then the UTF-8 resource should decode to its own content unchanged + + Scenario: Validating with an empty source reference raises a value error + Given skill content with name "pdf-processing" and description "Extract text from PDFs" + When I validate the skill content with an empty source reference + Then a value error should be raised for skill validation + + Scenario: Validating non-mapping content raises a validation error + When I validate skill content that is a list instead of a mapping + Then skill validation should fail containing "must resolve to a mapping" + + Scenario: Skill content with excessively deep nested data fails validation with a clear error + Given skill content with an excessively deeply nested extra field + When I validate the skill content + Then skill validation should fail containing "canonicalized" + + Scenario: Valid skill with UTF-8 and base64 resources validates successfully + Given skill content with a UTF-8 resource and a base64-encoded resource + When I validate the skill content + Then the skill should validate without error + And the base64 resource should decode to its original text + + Scenario: Skill package ID is reused from an already-resolved local package + Given skill content that already carries a package id + When I validate the skill content + Then the validated skill package_id should equal the pre-existing package_id + + Scenario: Skill package ID is computed when not already present + Given skill content with name "pdf-processing" and description "Extract text from PDFs" + When I validate the skill content + Then the validated skill package_id should start with "pkg_skl_" + + Scenario: Two skills with identical content resolve to the same package ID + Given two separately-built skill contents with identical schema fields + When I validate both skill contents + Then both validated skills should have the same package_id + + # ── SkillReferenceResolver: reference schemes (§5.3) ──────────────────── + + Scenario: Resolving a local: skill reference reads real content from disk + Given a real skill YAML file on disk named "house-style.yaml" + When I resolve the "local:house-style.yaml" skill reference + Then the resolved skill content should have name "house-style" + + Scenario: Resolving a missing local: skill reference raises a registry error + Given no skill YAML file exists for "local:missing-skill.yaml" + When I attempt to resolve the "local:missing-skill.yaml" skill reference + Then a registry error should be raised for the skill reference + + Scenario: Resolving a registry: skill reference dispatches with package_type skill + Given a mocked package content resolver returning skill content for any reference + When I resolve the "registry.example.com:acme/pdf-processing@v1.0.0" skill reference + Then the mocked resolver should have been called with package_type "skill" + + Scenario: Resolving an ID: skill reference dispatches with package_type skill + Given a mocked package content resolver returning skill content for any reference + When I resolve the "ID:pkg_skl_a1b2c3d4e5a1b2c3d4e5a1b2c3d4e5a1b2c3d4e5" skill reference + Then the mocked resolver should have been called with package_type "skill" + + Scenario: Resolving an empty skill reference raises a registry error + When I attempt to resolve the "" skill reference + Then a registry error should be raised for the skill reference + + Scenario: Resolving a malformed skill reference raises a registry error + When I attempt to resolve the "not-a-valid-reference-scheme" skill reference + Then a registry error should be raised for the skill reference + + Scenario: Resolver returning None raises a registry error + Given a mocked package content resolver returning None for any reference + When I attempt to resolve the "registry.example.com:acme/missing@v1.0.0" skill reference + Then a registry error should be raised for the skill reference + + # ── SkillLoader: orchestration and config injection (D-5) ────────────── + + Scenario: Agent config without skills is returned unchanged + Given an LLM agent config with no skills field + When SkillLoader loads skills for agent "assistant" + Then the loaded config should be the exact same object + + Scenario: Loading skills with an empty agent name raises a value error + Given an LLM agent config with one resolvable skill "pdf-processing" + When SkillLoader loads skills for an empty agent name + Then a value error should be raised for skill loading + + Scenario: Loading skills with a non-dict agent config raises a value error + When SkillLoader loads skills for agent "assistant" with a non-dict config + Then a value error should be raised for skill loading + + Scenario: Loading a single skill injects a discovery catalogue into the system prompt + Given an LLM agent config with system prompt "You are helpful." and one resolvable skill "pdf-processing" + When SkillLoader loads skills for agent "assistant" + Then the loaded system prompt should contain "You are helpful." + And the loaded system prompt should contain "pdf-processing" + + Scenario: Loading skills appends the skill activation tool + Given an LLM agent config with one resolvable skill "pdf-processing" + When SkillLoader loads skills for agent "assistant" + Then the loaded tools should include the skill activation tool + + Scenario: Loading skills does not duplicate an already-declared skill tool + Given an LLM agent config with a "skill"-named tool already declared and one resolvable skill "pdf-processing" + When SkillLoader loads skills for agent "assistant" + Then the loaded tools should contain exactly one skill activation tool + + Scenario: Loading multiple skills includes all of them in the catalogue + Given an LLM agent config with two resolvable skills "pdf-processing" and "spreadsheet-analysis" + When SkillLoader loads skills for agent "assistant" + Then the loaded system prompt should contain "pdf-processing" + And the loaded system prompt should contain "spreadsheet-analysis" + And the loaded skills catalogue should contain 2 entries + + Scenario: Loading a skill with resources exposes them in the loaded catalogue + Given an LLM agent config with one resolvable skill "pdf-processing" that has a resource + When SkillLoader loads skills for agent "assistant" + Then the loaded skills catalogue skill "pdf-processing" should have resource "references/REFERENCE.md" + + Scenario: Loading a missing skill package raises AgentCreationError + Given an LLM agent config referencing a skill that cannot be resolved + When SkillLoader attempts to load skills for agent "assistant" + Then AgentCreationError should be raised containing "failed to load skill" + + Scenario: Loading an invalid skill package raises AgentCreationError + Given an LLM agent config referencing a skill that resolves to invalid content + When SkillLoader attempts to load skills for agent "assistant" + Then AgentCreationError should be raised containing "failed to load skill" + + Scenario: A non-list skills field raises AgentCreationError + Given an LLM agent config with skills set to a string instead of a list + When SkillLoader attempts to load skills for agent "assistant" + Then AgentCreationError should be raised containing "must be a list" + + # ── AgentFactory + LLMAgent integration (end-to-end config wiring) ────── + + Scenario: AgentFactory rejects a skill_loader that is not a SkillLoader + When I create an AgentFactory with an invalid skill_loader + Then AgentFactory creation should fail with ConfigurationError containing "skill_loader" + + Scenario: AgentFactory wires a resolved skill into a created LLMAgent + Given an AgentFactory with a skill loader that resolves skill "pdf-processing" + And an llm agent config with skills ["local:pdf-processing.yaml"] + When the factory creates the agent + Then the created agent metadata should have "skills_loaded" equal to 1 + And the created agent metadata skill_names should include "pdf-processing" + And the created agent capabilities should include "skill-loading" + + Scenario: AgentFactory without skills configured does not add skill-loading capability + Given an AgentFactory with a skill loader that resolves skill "pdf-processing" + And a basic llm agent config with no skills field + When the factory creates the agent + Then the created agent capabilities should NOT include "skill-loading" + And the created agent metadata should have "skills_loaded" equal to 0 + + Scenario: AgentFactory propagates skill loading failures as AgentCreationError + Given an AgentFactory with a skill loader that fails to resolve any skill + And an llm agent config with skills ["local:broken-skill.yaml"] + When the factory attempts to create the agent + Then factory agent creation should fail with AgentCreationError + + # ── ToolAgent "skill" tool: activation and execution (D-5, ADR-2033 style) ── + + Scenario: The skill tool is registered as a built-in tool + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + When I create the ToolAgent + Then ToolAgent creation should succeed + + Scenario: Activating a loaded skill returns its instructions + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions "Run scripts/extract.py" + When I create the ToolAgent + And I invoke the skill tool with skill_name "pdf-processing" + Then the skill tool result should contain "[SKILL_ACTIVATED]" + And the skill tool result should contain "Run scripts/extract.py" + + Scenario: Activating an unknown skill raises an execution error listing available skills + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions "Run scripts/extract.py" + When I create the ToolAgent + And I invoke the skill tool with skill_name "nonexistent-skill" + Then the skill tool should fail with message containing "is not loaded" + And the skill tool should fail with message containing "pdf-processing" + + Scenario: Invoking the skill tool without a loaded skill catalogue fails clearly + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + When I create the ToolAgent + And I invoke the skill tool with skill_name "anything" while no skills are loaded + Then the skill tool should fail with message containing "is not loaded" + + Scenario: The skill tool rejects a non-string resource argument + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions "Run scripts/extract.py" + When I create the ToolAgent + And I invoke the skill tool with skill_name "pdf-processing" and a non-string resource + Then the skill tool should fail with message containing "'resource' must be a string" + + Scenario: The skill tool rejects a non-integer max_chars argument + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions "Run scripts/extract.py" + When I create the ToolAgent + And I invoke the skill tool with skill_name "pdf-processing" and a non-integer max_chars + Then the skill tool should fail with message containing "max_chars must be an integer" + + Scenario: The skill tool rejects a non-integer offset argument + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions "Run scripts/extract.py" + When I create the ToolAgent + And I invoke the skill tool with skill_name "pdf-processing" and a non-integer offset + Then the skill tool should fail with message containing "offset must be an integer" + + Scenario: The skill tool rejects a negative offset argument + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions "Run scripts/extract.py" + When I create the ToolAgent + And I invoke the skill tool with skill_name "pdf-processing" and a negative offset of -5 + Then the skill tool should fail with message containing "offset must be non-negative" + + Scenario: Reading a UTF-8 resource from an activated skill + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" having a utf-8 resource "references/REFERENCE.md" with content "# Reference" + When I create the ToolAgent + And I read resource "references/REFERENCE.md" via the skill tool for skill "pdf-processing" + Then the skill tool result should contain "[SKILL_RESOURCE_READ]" + And the skill tool result should contain "# Reference" + + Scenario: Reading a base64-encoded resource from an activated skill + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" having a base64 resource "assets/logo.txt" encoding text "binary-ish content" + When I create the ToolAgent + And I read resource "assets/logo.txt" via the skill tool for skill "pdf-processing" + Then the skill tool result should contain "binary-ish content" + + Scenario: Reading an unknown resource raises an execution error listing available resources + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" having a utf-8 resource "references/REFERENCE.md" with content "# Reference" + When I create the ToolAgent + And I read resource "nonexistent/path.md" via the skill tool for skill "pdf-processing" + Then the skill tool should fail with message containing "has no resource" + + Scenario: The skill tool requires a non-empty skill_name + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + When I create the ToolAgent + And I invoke the skill tool with an empty skill_name + Then the skill tool should fail with message containing "requires a non-empty" + + Scenario: The skill tool paginates long instructions with max_chars and offset + Given a ToolAgent is configured with name "skill_tool_agent" and config + """ + { + "tools": [{"name": "skill"}] + } + """ + And a skills context with skill "pdf-processing" and instructions of 100 repeated "x" characters + When I create the ToolAgent + And I page through the skill tool for skill "pdf-processing" using max_chars 40 and offset 0 + Then the skill tool result should contain "TRUNCATED to 40 chars" + And the skill tool result should contain "MORE_CONTENT_AT_OFFSET: 40" + When I page through the skill tool for skill "pdf-processing" using max_chars 40 and offset 40 + Then the skill tool result should contain "MORE_CONTENT_AT_OFFSET: 80" + When I page through the skill tool for skill "pdf-processing" using max_chars 40 and offset 100 + Then the skill tool result should contain "NO_MORE_CONTENT_AT_OFFSET: 100" diff --git a/features/steps/agent_skills_steps.py b/features/steps/agent_skills_steps.py new file mode 100644 index 0000000..378bb94 --- /dev/null +++ b/features/steps/agent_skills_steps.py @@ -0,0 +1,957 @@ +"""Step definitions for Skill Package Loading BDD tests (ADR-2034).""" + +from __future__ import annotations + +import base64 +import json +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import Mock + +from behave import given, then, when +from behave.api.async_step import async_run_until_complete + +from cleveractors.agents.factory import AgentFactory +from cleveractors.agents.skill_resolution import SkillReferenceResolver +from cleveractors.agents.skill_schema import SkillValidator +from cleveractors.agents.skills import SkillLoader +from cleveractors.core.exceptions import ( + AgentCreationError, + ConfigurationError, + ExecutionError, +) +from cleveractors.registry import ( + LocalPackageStore, + PackageContentResolver, + PackageNotFoundError, + RegistryError, + ValidationError, +) +from cleveractors.templates.renderer import TemplateRenderer + +# ─── Shared helpers ───────────────────────────────────────────────────────── + + +def _base_skill_content(**overrides: Any) -> dict[str, Any]: + content: dict[str, Any] = { + "skill": True, + "name": "valid-name", + "description": "A valid description", + "instructions": "Do the thing.", + } + content.update(overrides) + return content + + +def _resolvable_skill_content(name: str) -> dict[str, Any]: + return { + "skill": True, + "name": name, + "description": f"Description for {name}.", + "instructions": f"Instructions for {name}.", + } + + +def _make_mock_skill_resolver(content_by_ref: dict[str, dict[str, Any]]) -> Mock: + mock_resolver = Mock(spec=SkillReferenceResolver) + + def _resolve(reference: str) -> dict[str, Any]: + if reference not in content_by_ref: + raise PackageNotFoundError( + f"no content for {reference}", original_reference=reference + ) + return content_by_ref[reference] + + mock_resolver.resolve = Mock(side_effect=_resolve) + return mock_resolver + + +def _make_template_renderer() -> Mock: + renderer = Mock(spec=TemplateRenderer) + renderer.render_string.return_value = "rendered system prompt" + return renderer + + +def _skill_context_entry( + name: str, + description: str = "A skill.", + instructions: str = "Do the thing.", +) -> dict[str, Any]: + return { + "name": name, + "description": description, + "instructions": instructions, + "resources": {}, + } + + +# ─── SkillValidator scenarios ─────────────────────────────────────────────── + + +@given('skill content with name "{name}" and description "{description}"') +def step_skill_content_name_desc(context: Any, name: str, description: str) -> None: + context.skill_content = _base_skill_content(name=name, description=description) + + +@given('skill content without the "skill" discriminator field') +def step_skill_content_no_discriminator(context: Any) -> None: + content = _base_skill_content() + del content["skill"] + context.skill_content = content + + +@given('skill content with an empty name and description "{description}"') +def step_skill_content_empty_name(context: Any, description: str) -> None: + context.skill_content = _base_skill_content(name="", description=description) + + +@given('skill content with an over-long name and description "{description}"') +def step_skill_content_overlong_name(context: Any, description: str) -> None: + context.skill_content = _base_skill_content(name="a" * 65, description=description) + + +@given('skill content with name "{name}" and no description') +def step_skill_content_no_description(context: Any, name: str) -> None: + content = _base_skill_content(name=name) + del content["description"] + context.skill_content = content + + +@given('skill content with name "{name}" and an over-long description') +def step_skill_content_overlong_description(context: Any, name: str) -> None: + context.skill_content = _base_skill_content(name=name, description="d" * 1025) + + +@given( + 'skill content with name "{name}" and description "{description}" ' + "and no instructions" +) +def step_skill_content_no_instructions( + context: Any, name: str, description: str +) -> None: + content = _base_skill_content(name=name, description=description) + del content["instructions"] + context.skill_content = content + + +@given("skill content with an invalid license type") +def step_skill_content_invalid_license(context: Any) -> None: + context.skill_content = _base_skill_content(license=123) + + +@given("skill content with an over-long compatibility string") +def step_skill_content_overlong_compatibility(context: Any) -> None: + context.skill_content = _base_skill_content(compatibility="c" * 501) + + +@given("skill content with metadata set to a list instead of a mapping") +def step_skill_content_metadata_list(context: Any) -> None: + context.skill_content = _base_skill_content(metadata=["a", "b"]) + + +@given("skill content with metadata containing a non-string value") +def step_skill_content_metadata_bad_value(context: Any) -> None: + context.skill_content = _base_skill_content(metadata={"author": 123}) + + +@given("skill content with resources set to a string instead of a mapping") +def step_skill_content_resources_string(context: Any) -> None: + context.skill_content = _base_skill_content(resources="not-a-mapping") + + +@given('skill content with a resource at path "{path}"') +def step_skill_content_resource_path(context: Any, path: str) -> None: + context.skill_content = _base_skill_content( + resources={path: {"content": "text", "encoding": "utf-8"}} + ) + + +@given("skill content with a resource at an empty path") +def step_skill_content_resource_empty_path(context: Any) -> None: + context.skill_content = _base_skill_content( + resources={"": {"content": "text", "encoding": "utf-8"}} + ) + + +@given('skill content with a resource entry missing "content"') +def step_skill_content_resource_missing_content(context: Any) -> None: + context.skill_content = _base_skill_content( + resources={"scripts/x.py": {"encoding": "utf-8"}} + ) + + +@given('skill content with a resource using encoding "{encoding}"') +def step_skill_content_resource_bad_encoding(context: Any, encoding: str) -> None: + context.skill_content = _base_skill_content( + resources={"scripts/x.py": {"content": "text", "encoding": encoding}} + ) + + +@given("skill content with a resource entry that is a string instead of a mapping") +def step_skill_content_resource_entry_string(context: Any) -> None: + context.skill_content = _base_skill_content( + resources={"scripts/x.py": "not-a-mapping"} + ) + + +@given("skill content with an excessively deeply nested extra field") +def step_skill_content_excessively_deep(context: Any) -> None: + nested: Any = "leaf" + for _ in range(110): + nested = {"x": nested} + content = _base_skill_content() + content["_extra_deep_field"] = nested + context.skill_content = content + + +@given('that skill content has metadata author set to "{value}"') +def step_skill_content_add_metadata_author(context: Any, value: str) -> None: + context.skill_content["metadata"] = {"author": value} + + +@given("skill content with a UTF-8 resource and a base64-encoded resource") +def step_skill_content_utf8_and_base64(context: Any) -> None: + original_text = "original text" + encoded = base64.b64encode(original_text.encode("utf-8")).decode("ascii") + context._base64_original_text = original_text + context.skill_content = _base_skill_content( + resources={ + "references/REFERENCE.md": { + "content": "# Reference", + "encoding": "utf-8", + }, + "assets/data.bin": {"content": encoded, "encoding": "base64"}, + } + ) + + +@given("skill content that already carries a package id") +def step_skill_content_existing_package_id(context: Any) -> None: + content = _base_skill_content() + content["_package_id"] = "pkg_skl_" + "a" * 40 + context.skill_content = content + + +@given("two separately-built skill contents with identical schema fields") +def step_two_identical_skill_contents(context: Any) -> None: + content_a = _base_skill_content(name="identical-skill") + content_b = dict(content_a) + content_a["_original_reference"] = "local:a.yaml" + content_b["_original_reference"] = ( + "registry.example.com:acme/identical-skill@v1.0.0" + ) + context.skill_content_a = content_a + context.skill_content_b = content_b + + +@when("I validate the skill content") +def step_validate_skill_content(context: Any) -> None: + validator = SkillValidator() + try: + context.validated_skill = validator.validate( + context.skill_content, source_reference="test-ref" + ) + context.validation_error = None + except ValidationError as exc: + context.validated_skill = None + context.validation_error = exc + + +@when("I validate both skill contents") +def step_validate_both_skill_contents(context: Any) -> None: + validator = SkillValidator() + context.validated_skill_a = validator.validate( + context.skill_content_a, source_reference="ref-a" + ) + context.validated_skill_b = validator.validate( + context.skill_content_b, source_reference="ref-b" + ) + + +@when("I validate the skill content with an empty source reference") +def step_validate_skill_content_empty_ref(context: Any) -> None: + validator = SkillValidator() + try: + validator.validate(context.skill_content, source_reference="") + context.value_error = None + except ValueError as exc: + context.value_error = exc + + +@when("I validate skill content that is a list instead of a mapping") +def step_validate_content_not_mapping(context: Any) -> None: + validator = SkillValidator() + not_a_mapping: Any = ["not", "a", "mapping"] + try: + validator.validate(not_a_mapping, source_reference="test-ref") + context.validation_error = None + except ValidationError as exc: + context.validation_error = exc + + +@then("the skill should validate without error") +def step_skill_validates_ok(context: Any) -> None: + assert context.validation_error is None, ( + f"Expected no validation error, got: {context.validation_error}" + ) + assert context.validated_skill is not None + + +@then('skill validation should fail containing "{text}"') +def step_skill_validation_fails(context: Any, text: str) -> None: + assert context.validation_error is not None, ( + "Expected a ValidationError but none was raised" + ) + assert text.lower() in str(context.validation_error).lower(), ( + f"Expected {text!r} in error message: {context.validation_error}" + ) + + +@then('the validated skill name should be "{name}"') +def step_validated_skill_name(context: Any, name: str) -> None: + assert context.validated_skill.name == name + + +@then("the base64 resource should decode to its original text") +def step_base64_resource_decodes(context: Any) -> None: + resource = context.validated_skill.resources["assets/data.bin"] + assert resource.decoded_text() == context._base64_original_text + + +@then("the validated skill package_id should equal the pre-existing package_id") +def step_package_id_reused(context: Any) -> None: + assert context.validated_skill.package_id == context.skill_content["_package_id"] + + +@then('the validated skill package_id should start with "{prefix}"') +def step_package_id_prefix(context: Any, prefix: str) -> None: + assert context.validated_skill.package_id.startswith(prefix), ( + f"package_id {context.validated_skill.package_id!r} does not start " + f"with {prefix!r}" + ) + + +@then("both validated skills should have the same package_id") +def step_same_package_id(context: Any) -> None: + assert context.validated_skill_a.package_id == context.validated_skill_b.package_id + + +@then('the validated skill metadata author should equal "{value}"') +def step_validated_metadata_author(context: Any, value: str) -> None: + assert context.validated_skill.metadata.get("author") == value + + +@then("the UTF-8 resource should decode to its own content unchanged") +def step_utf8_resource_decodes_unchanged(context: Any) -> None: + resource = context.validated_skill.resources["references/REFERENCE.md"] + assert resource.decoded_text() == "# Reference" + + +@then("a value error should be raised for skill validation") +def step_value_error_raised_validation(context: Any) -> None: + assert context.value_error is not None, "Expected a ValueError but none was raised" + assert isinstance(context.value_error, ValueError) + + +# ─── SkillReferenceResolver scenarios ─────────────────────────────────────── + + +def _build_resolver(context: Any) -> SkillReferenceResolver: + return SkillReferenceResolver( + resolver=getattr(context, "_mock_resolver", None), + local_store=getattr(context, "_local_store", None), + ) + + +@given('a real skill YAML file on disk named "{filename}"') +def step_real_skill_file(context: Any, filename: str) -> None: + tmp_dir = tempfile.mkdtemp() + file_path = Path(tmp_dir) / filename + file_path.write_text( + "skill: true\n" + "name: house-style\n" + "description: Apply the house writing style.\n" + "instructions: Use active voice.\n" + ) + context._local_store = LocalPackageStore(base_dir=tmp_dir) + + +@given('no skill YAML file exists for "local:{filename}"') +def step_no_skill_file(context: Any, filename: str) -> None: + context._local_store = LocalPackageStore(base_dir=tempfile.mkdtemp()) + + +@given("a mocked package content resolver returning skill content for any reference") +def step_mocked_resolver_returns_content(context: Any) -> None: + mock_resolver = Mock(spec=PackageContentResolver) + mock_resolver.resolve = Mock( + return_value={ + "skill": True, + "name": "mocked-skill", + "description": "A mocked skill.", + "instructions": "Do the mocked thing.", + } + ) + context._mock_resolver = mock_resolver + + +@given("a mocked package content resolver returning None for any reference") +def step_mocked_resolver_returns_none(context: Any) -> None: + mock_resolver = Mock(spec=PackageContentResolver) + mock_resolver.resolve = Mock(return_value=None) + context._mock_resolver = mock_resolver + + +@when('I resolve the "{reference}" skill reference') +def step_resolve_skill_reference(context: Any, reference: str) -> None: + resolver = _build_resolver(context) + context._resolved_content = resolver.resolve(reference) + context._resolve_error = None + + +@when('I attempt to resolve the "{reference}" skill reference') +def step_attempt_resolve_skill_reference(context: Any, reference: str) -> None: + resolver = _build_resolver(context) + try: + context._resolved_content = resolver.resolve(reference) + context._resolve_error = None + except RegistryError as exc: + context._resolved_content = None + context._resolve_error = exc + + +@when('I attempt to resolve the "" skill reference') +def step_attempt_resolve_empty_skill_reference(context: Any) -> None: + step_attempt_resolve_skill_reference(context, "") + + +@then('the resolved skill content should have name "{name}"') +def step_resolved_content_name(context: Any, name: str) -> None: + assert context._resolved_content is not None + assert context._resolved_content.get("name") == name + + +@then("a registry error should be raised for the skill reference") +def step_registry_error_raised(context: Any) -> None: + assert context._resolve_error is not None, ( + "Expected a RegistryError but none was raised" + ) + assert isinstance(context._resolve_error, RegistryError) + + +@then('the mocked resolver should have been called with package_type "{package_type}"') +def step_mock_called_with_package_type(context: Any, package_type: str) -> None: + call = context._mock_resolver.resolve.call_args + assert call is not None, "mocked resolver.resolve was never called" + assert call.kwargs.get("package_type") == package_type, ( + f"resolve() called with package_type={call.kwargs.get('package_type')!r}, " + f"expected {package_type!r}" + ) + + +# ─── SkillLoader scenarios ────────────────────────────────────────────────── + + +@given("an LLM agent config with no skills field") +def step_config_no_skills(context: Any) -> None: + context._skill_loader_config = {"name": "assistant", "provider": "openai"} + context._skill_loader = SkillLoader() + + +@given( + 'an LLM agent config with system prompt "{prompt}" and one resolvable ' + 'skill "{skill_name}"' +) +def step_config_with_system_prompt_and_skill( + context: Any, prompt: str, skill_name: str +) -> None: + ref = f"local:{skill_name}.yaml" + context._skill_loader_config = { + "name": "assistant", + "system_prompt": prompt, + "skills": [ref], + } + context._skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver({ref: _resolvable_skill_content(skill_name)}) + ) + + +@given('an LLM agent config with one resolvable skill "{skill_name}"') +def step_config_with_one_skill(context: Any, skill_name: str) -> None: + ref = f"local:{skill_name}.yaml" + context._skill_loader_config = {"name": "assistant", "skills": [ref]} + context._skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver({ref: _resolvable_skill_content(skill_name)}) + ) + + +@given( + 'an LLM agent config with one resolvable skill "{skill_name}" that has a resource' +) +def step_config_with_one_skill_with_resource(context: Any, skill_name: str) -> None: + ref = f"local:{skill_name}.yaml" + content = _resolvable_skill_content(skill_name) + content["resources"] = { + "references/REFERENCE.md": {"content": "# Reference", "encoding": "utf-8"} + } + context._skill_loader_config = {"name": "assistant", "skills": [ref]} + context._skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver({ref: content}) + ) + + +@given( + 'an LLM agent config with a "skill"-named tool already declared and one ' + 'resolvable skill "{skill_name}"' +) +def step_config_with_skill_tool_and_one_skill(context: Any, skill_name: str) -> None: + ref = f"local:{skill_name}.yaml" + context._skill_loader_config = { + "name": "assistant", + "tools": [{"name": "skill"}], + "skills": [ref], + } + context._skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver({ref: _resolvable_skill_content(skill_name)}) + ) + + +@given('an LLM agent config with two resolvable skills "{skill_a}" and "{skill_b}"') +def step_config_with_two_skills(context: Any, skill_a: str, skill_b: str) -> None: + ref_a = f"local:{skill_a}.yaml" + ref_b = f"local:{skill_b}.yaml" + context._skill_loader_config = {"name": "assistant", "skills": [ref_a, ref_b]} + context._skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver( + { + ref_a: _resolvable_skill_content(skill_a), + ref_b: _resolvable_skill_content(skill_b), + } + ) + ) + + +@given("an LLM agent config referencing a skill that cannot be resolved") +def step_config_unresolvable_skill(context: Any) -> None: + context._skill_loader_config = { + "name": "assistant", + "skills": ["local:missing.yaml"], + } + context._skill_loader = SkillLoader(resolver=_make_mock_skill_resolver({})) + + +@given("an LLM agent config referencing a skill that resolves to invalid content") +def step_config_invalid_skill_content(context: Any) -> None: + ref = "local:invalid.yaml" + context._skill_loader_config = {"name": "assistant", "skills": [ref]} + context._skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver({ref: {"skill": True, "name": "x"}}) + ) + + +@given("an LLM agent config with skills set to a string instead of a list") +def step_config_skills_not_list(context: Any) -> None: + context._skill_loader_config = {"name": "assistant", "skills": "not-a-list"} + context._skill_loader = SkillLoader() + + +@when('SkillLoader loads skills for agent "{agent_name}"') +def step_skill_loader_load(context: Any, agent_name: str) -> None: + context._loaded_config = context._skill_loader.load( + agent_name, context._skill_loader_config + ) + context._skill_loader_error = None + + +@when('SkillLoader attempts to load skills for agent "{agent_name}"') +def step_skill_loader_attempt_load(context: Any, agent_name: str) -> None: + try: + context._loaded_config = context._skill_loader.load( + agent_name, context._skill_loader_config + ) + context._skill_loader_error = None + except AgentCreationError as exc: + context._loaded_config = None + context._skill_loader_error = exc + + +@when("SkillLoader loads skills for an empty agent name") +def step_skill_loader_load_empty_name(context: Any) -> None: + try: + context._skill_loader.load("", context._skill_loader_config) + context._value_error = None + except ValueError as exc: + context._value_error = exc + + +@when('SkillLoader loads skills for agent "{agent_name}" with a non-dict config') +def step_skill_loader_load_bad_config(context: Any, agent_name: str) -> None: + loader = SkillLoader() + bad_config: Any = "not-a-dict" + try: + loader.load(agent_name, bad_config) + context._value_error = None + except ValueError as exc: + context._value_error = exc + + +@then("a value error should be raised for skill loading") +def step_value_error_raised_loading(context: Any) -> None: + assert context._value_error is not None, "Expected a ValueError but none was raised" + assert isinstance(context._value_error, ValueError) + + +@then("the loaded config should be the exact same object") +def step_loaded_config_same_object(context: Any) -> None: + assert context._loaded_config is context._skill_loader_config + + +@then('the loaded system prompt should contain "{text}"') +def step_loaded_prompt_contains(context: Any, text: str) -> None: + prompt = context._loaded_config["system_prompt"] + assert text in prompt, f"Expected {text!r} in system_prompt: {prompt!r}" + + +@then("the loaded tools should include the skill activation tool") +def step_loaded_tools_include_skill(context: Any) -> None: + tools = context._loaded_config["tools"] + assert any(isinstance(t, dict) and t.get("name") == "skill" for t in tools), ( + f"skill tool not found in {tools!r}" + ) + + +@then("the loaded tools should contain exactly one skill activation tool") +def step_loaded_tools_exactly_one_skill(context: Any) -> None: + tools = context._loaded_config["tools"] + matches = [t for t in tools if isinstance(t, dict) and t.get("name") == "skill"] + assert len(matches) == 1, ( + f"Expected exactly one skill tool, found {len(matches)}: {tools!r}" + ) + + +@then("the loaded skills catalogue should contain {n:d} entries") +def step_loaded_catalogue_count(context: Any, n: int) -> None: + catalogue = context._loaded_config["_loaded_skills"] + assert len(catalogue) == n, ( + f"Expected {n} skills, got {len(catalogue)}: {list(catalogue)}" + ) + + +@then('the loaded skills catalogue skill "{skill_name}" should have resource "{path}"') +def step_loaded_catalogue_skill_has_resource( + context: Any, skill_name: str, path: str +) -> None: + catalogue = context._loaded_config["_loaded_skills"] + resources = catalogue[skill_name]["resources"] + assert path in resources, ( + f"{path!r} not found in resources for {skill_name!r}: {list(resources)}" + ) + + +@then('AgentCreationError should be raised containing "{text}"') +def step_agent_creation_error_raised(context: Any, text: str) -> None: + err = getattr(context, "_skill_loader_error", None) or getattr( + context, "_factory_create_error", None + ) + assert err is not None, "Expected AgentCreationError but none was raised" + assert isinstance(err, AgentCreationError), ( + f"Expected AgentCreationError, got {type(err).__name__}: {err}" + ) + assert text in str(err), f"Expected {text!r} in error message: {err}" + + +# ─── AgentFactory + LLMAgent integration scenarios ────────────────────────── + + +@when("I create an AgentFactory with an invalid skill_loader") +def step_create_factory_bad_skill_loader(context: Any) -> None: + bad_skill_loader: Any = "not-a-skill-loader" + try: + AgentFactory( + config={}, + template_renderer=_make_template_renderer(), + skill_loader=bad_skill_loader, + ) + context._factory_init_error = None + except ConfigurationError as exc: + context._factory_init_error = exc + + +@then('AgentFactory creation should fail with ConfigurationError containing "{text}"') +def step_factory_init_fails(context: Any, text: str) -> None: + assert context._factory_init_error is not None, ( + "Expected a ConfigurationError but none was raised" + ) + assert isinstance(context._factory_init_error, ConfigurationError) + assert text in str(context._factory_init_error) + + +@given('an AgentFactory with a skill loader that resolves skill "{skill_name}"') +def step_factory_with_resolving_skill_loader(context: Any, skill_name: str) -> None: + ref = f"local:{skill_name}.yaml" + skill_loader = SkillLoader( + resolver=_make_mock_skill_resolver({ref: _resolvable_skill_content(skill_name)}) + ) + context._factory = AgentFactory( + config={}, + template_renderer=_make_template_renderer(), + skill_loader=skill_loader, + ) + + +@given("an AgentFactory with a skill loader that fails to resolve any skill") +def step_factory_with_failing_skill_loader(context: Any) -> None: + skill_loader = SkillLoader(resolver=_make_mock_skill_resolver({})) + context._factory = AgentFactory( + config={}, + template_renderer=_make_template_renderer(), + skill_loader=skill_loader, + ) + + +@given("an llm agent config with skills {skills_json}") +def step_llm_config_with_skills(context: Any, skills_json: str) -> None: + refs = json.loads(skills_json) + context._factory_agent_config = { + "provider": "openai", + "api_key": "test-key", + "skills": refs, + } + + +@given("a basic llm agent config with no skills field") +def step_basic_llm_config_no_skills(context: Any) -> None: + context._factory_agent_config = { + "provider": "openai", + "api_key": "test-key", + } + + +@when("the factory creates the agent") +def step_factory_creates_agent(context: Any) -> None: + context._factory_agent = context._factory._create_agent_instance( + "assistant", "llm", context._factory_agent_config + ) + context._factory_create_error = None + + +@when("the factory attempts to create the agent") +def step_factory_attempts_create_agent(context: Any) -> None: + try: + context._factory_agent = context._factory._create_agent_instance( + "assistant", "llm", context._factory_agent_config + ) + context._factory_create_error = None + except AgentCreationError as exc: + context._factory_agent = None + context._factory_create_error = exc + + +@then('the created agent metadata should have "{field}" equal to {n:d}') +def step_created_agent_metadata_int(context: Any, field: str, n: int) -> None: + metadata = context._factory_agent.get_metadata() + assert metadata.get(field) == n, ( + f"metadata.{field} = {metadata.get(field)!r} != {n}" + ) + + +@then('the created agent metadata skill_names should include "{name}"') +def step_created_agent_metadata_skill_names(context: Any, name: str) -> None: + metadata = context._factory_agent.get_metadata() + assert name in metadata.get("skill_names", []), ( + f"{name!r} not in skill_names: {metadata.get('skill_names')!r}" + ) + + +@then('the created agent capabilities should include "{capability}"') +def step_created_agent_capabilities_include(context: Any, capability: str) -> None: + assert capability in context._factory_agent.get_capabilities() + + +@then('the created agent capabilities should NOT include "{capability}"') +def step_created_agent_capabilities_exclude(context: Any, capability: str) -> None: + assert capability not in context._factory_agent.get_capabilities() + + +@then("factory agent creation should fail with AgentCreationError") +def step_factory_create_fails(context: Any) -> None: + assert context._factory_create_error is not None + assert isinstance(context._factory_create_error, AgentCreationError) + + +# ─── ToolAgent "skill" tool scenarios ─────────────────────────────────────── + + +@then("ToolAgent creation should succeed") +def step_toolagent_creation_succeeds(context: Any) -> None: + assert context.error is None, f"Expected no error, got: {context.error}" + assert context.agent is not None + + +def _skills_ctx(context: Any) -> dict[str, Any]: + if not hasattr(context, "_skills_ctx"): + context._skills_ctx = {} + return context._skills_ctx + + +@given('a skills context with skill "{skill_name}" and instructions "{instructions}"') +def step_skills_context_basic(context: Any, skill_name: str, instructions: str) -> None: + _skills_ctx(context)[skill_name] = _skill_context_entry( + skill_name, instructions=instructions + ) + + +@given( + 'a skills context with skill "{skill_name}" and instructions of {n:d} ' + 'repeated "{char}" characters' +) +def step_skills_context_long_instructions( + context: Any, skill_name: str, n: int, char: str +) -> None: + _skills_ctx(context)[skill_name] = _skill_context_entry( + skill_name, instructions=char * n + ) + + +@given( + 'a skills context with skill "{skill_name}" having a utf-8 resource ' + '"{path}" with content "{content}"' +) +def step_skills_context_utf8_resource( + context: Any, skill_name: str, path: str, content: str +) -> None: + ctx = _skills_ctx(context) + entry = ctx.setdefault(skill_name, _skill_context_entry(skill_name)) + entry["resources"][path] = {"content": content, "encoding": "utf-8"} + + +@given( + 'a skills context with skill "{skill_name}" having a base64 resource ' + '"{path}" encoding text "{text}"' +) +def step_skills_context_base64_resource( + context: Any, skill_name: str, path: str, text: str +) -> None: + ctx = _skills_ctx(context) + entry = ctx.setdefault(skill_name, _skill_context_entry(skill_name)) + encoded = base64.b64encode(text.encode("utf-8")).decode("ascii") + entry["resources"][path] = {"content": encoded, "encoding": "base64"} + + +async def _invoke_skill_tool( + context: Any, args: dict[str, Any], use_skills_context: bool = True +) -> None: + tool_context: dict[str, Any] = {} + if use_skills_context and hasattr(context, "_skills_ctx"): + tool_context["_skills"] = context._skills_ctx + context.skill_tool_error = None + context.skill_tool_result = None + try: + context.skill_tool_result = await context.agent.invoke( + "skill", args, context=tool_context + ) + except ExecutionError as exc: + context.skill_tool_error = exc + + +@when('I invoke the skill tool with skill_name "{skill_name}"') +@async_run_until_complete +async def step_invoke_skill_tool(context: Any, skill_name: str) -> None: + await _invoke_skill_tool(context, {"skill_name": skill_name}) + + +@when( + 'I invoke the skill tool with skill_name "{skill_name}" while no skills are loaded' +) +@async_run_until_complete +async def step_invoke_skill_tool_no_context(context: Any, skill_name: str) -> None: + await _invoke_skill_tool( + context, {"skill_name": skill_name}, use_skills_context=False + ) + + +@when('I read resource "{resource}" via the skill tool for skill "{skill_name}"') +@async_run_until_complete +async def step_invoke_skill_tool_with_resource( + context: Any, resource: str, skill_name: str +) -> None: + await _invoke_skill_tool(context, {"skill_name": skill_name, "resource": resource}) + + +@when("I invoke the skill tool with an empty skill_name") +@async_run_until_complete +async def step_invoke_skill_tool_empty_name(context: Any) -> None: + await _invoke_skill_tool(context, {"skill_name": ""}) + + +@when( + 'I invoke the skill tool with skill_name "{skill_name}" and a non-string resource' +) +@async_run_until_complete +async def step_invoke_skill_tool_bad_resource(context: Any, skill_name: str) -> None: + await _invoke_skill_tool(context, {"skill_name": skill_name, "resource": 123}) + + +@when( + 'I invoke the skill tool with skill_name "{skill_name}" and a non-integer max_chars' +) +@async_run_until_complete +async def step_invoke_skill_tool_bad_max_chars(context: Any, skill_name: str) -> None: + await _invoke_skill_tool( + context, {"skill_name": skill_name, "max_chars": "not-an-int"} + ) + + +@when('I invoke the skill tool with skill_name "{skill_name}" and a non-integer offset') +@async_run_until_complete +async def step_invoke_skill_tool_bad_offset(context: Any, skill_name: str) -> None: + await _invoke_skill_tool( + context, {"skill_name": skill_name, "offset": "not-an-int"} + ) + + +@when( + 'I invoke the skill tool with skill_name "{skill_name}" and a negative offset ' + "of {offset:d}" +) +@async_run_until_complete +async def step_invoke_skill_tool_negative_offset( + context: Any, skill_name: str, offset: int +) -> None: + await _invoke_skill_tool(context, {"skill_name": skill_name, "offset": offset}) + + +@when( + 'I page through the skill tool for skill "{skill_name}" using max_chars ' + "{max_chars:d} and offset {offset:d}" +) +@async_run_until_complete +async def step_invoke_skill_tool_paginated( + context: Any, skill_name: str, max_chars: int, offset: int +) -> None: + await _invoke_skill_tool( + context, + {"skill_name": skill_name, "max_chars": max_chars, "offset": offset}, + ) + + +@then('the skill tool result should contain "{text}"') +def step_skill_tool_result_contains(context: Any, text: str) -> None: + assert context.skill_tool_result is not None, ( + "skill tool raised an error instead of returning a result: " + f"{context.skill_tool_error}" + ) + assert text in context.skill_tool_result, ( + f"Expected {text!r} in result: {context.skill_tool_result!r}" + ) + + +@then('the skill tool should fail with message containing "{text}"') +def step_skill_tool_fails(context: Any, text: str) -> None: + assert context.skill_tool_error is not None, ( + "Expected an ExecutionError but none was raised" + ) + assert text in str(context.skill_tool_error), ( + f"Expected {text!r} in error message: {context.skill_tool_error}" + ) diff --git a/robot/SkillLoadingTestLib.py b/robot/SkillLoadingTestLib.py new file mode 100644 index 0000000..304b9aa --- /dev/null +++ b/robot/SkillLoadingTestLib.py @@ -0,0 +1,241 @@ +"""Library for Skill package loading integration tests (ADR-2034). + +Writes a real Skill package YAML file to a real temp directory and +resolves it through the real registry/package-resolution machinery +(``LocalPackageStore``, ``PackageContentResolver``, ``SkillLoader``), wires +it into a real ``LLMAgent`` via ``AgentFactory``, and drives a conversation +through a mocked chat model — only the LLM API call itself is mocked, so no +real network LLM calls happen here, matching every other ``*.robot`` suite +in this project (``ToolCallingTestLib.py``, ``TokenBudgetTestLib.py``). +Skill resolution, validation, config injection, and tool dispatch are all +exercised for real, with no mocks anywhere in that path. +""" + +from __future__ import annotations + +import asyncio +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from langchain_core.messages import AIMessage, SystemMessage, ToolMessage + +from cleveractors.agents.base import Agent +from cleveractors.agents.factory import AgentFactory +from cleveractors.agents.skill_resolution import SkillReferenceResolver +from cleveractors.agents.skills import SkillLoader +from cleveractors.core.exceptions import AgentCreationError +from cleveractors.registry import LocalPackageStore +from cleveractors.templates.renderer import TemplateRenderer + + +class SkillLoadingTestLib: + """Keywords for integration-style tests of Skill package loading.""" + + def __init__(self) -> None: + self._tmp_dir: str | None = None + self._factory: AgentFactory | None = None + self._agent: Agent | None = None + self._create_error: Exception | None = None + self._mock_ainvoke_calls: list[dict[str, Any]] = [] + self._patches: list[Any] = [] + self._result: str | None = None + + def _teardown_patches(self) -> None: + for p in self._patches: + p.stop() + self._patches.clear() + + def write_real_skill_package( + self, + filename: str = "pdf-processing.yaml", + name: str = "pdf-processing", + ) -> None: + """Write a real, valid Skill package YAML file to a real temp directory.""" + self._tmp_dir = tempfile.mkdtemp() + file_path = Path(self._tmp_dir) / filename + file_path.write_text( + "skill: true\n" + f"name: {name}\n" + "description: Extract text and tables from PDFs.\n" + "instructions: |\n" + " Run scripts/extract.py to extract text.\n" + "resources:\n" + " references/REFERENCE.md:\n" + " encoding: utf-8\n" + " content: |\n" + " # Reference\n" + " Full CLI surface documented here.\n" + ) + + def create_agent_factory_with_local_skill_store(self) -> None: + """Create a real AgentFactory wired to resolve local: skill refs for real.""" + assert self._tmp_dir is not None, "write_real_skill_package must run first" + local_store = LocalPackageStore(base_dir=self._tmp_dir) + skill_loader = SkillLoader( + resolver=SkillReferenceResolver(local_store=local_store) + ) + self._factory = AgentFactory( + config={}, + template_renderer=TemplateRenderer(), + skill_loader=skill_loader, + ) + + def _install_mock_chat_model(self) -> None: + self._teardown_patches() + self._mock_ainvoke_calls = [] + + def _build_mock_model(*args: Any, **kwargs: Any) -> Any: + mock_model = MagicMock() + mock_model.temperature = 0.7 + + async def _mock_ainvoke(messages: Any, **invoke_kwargs: Any) -> AIMessage: + self._mock_ainvoke_calls.append( + {"kwargs": invoke_kwargs, "messages": list(messages)} + ) + call_count = len(self._mock_ainvoke_calls) + if call_count == 1: + return AIMessage( + content="", + usage_metadata={ + "input_tokens": 40, + "output_tokens": 10, + "total_tokens": 50, + }, + tool_calls=[ + { + "id": "call_skill_001", + "name": "skill", + "args": {"skill_name": "pdf-processing"}, + } + ], + ) + return AIMessage( + content="Final answer using skill instructions", + usage_metadata={ + "input_tokens": 20, + "output_tokens": 8, + "total_tokens": 28, + }, + ) + + mock_model.ainvoke = _mock_ainvoke + return mock_model + + patcher = patch( + "cleveractors.agents.llm.build_chat_model", side_effect=_build_mock_model + ) + patcher.start() + self._patches.append(patcher) + + def create_llm_agent_with_skill( + self, filename: str = "pdf-processing.yaml" + ) -> None: + """Create a real LLMAgent (via AgentFactory) configured with skills:.""" + self._install_mock_chat_model() + assert self._factory is not None, ( + "create_agent_factory_with_local_skill_store must run first" + ) + try: + self._agent = self._factory._create_agent_instance( # noqa: SLF001 + "pdf_assistant", + "llm", + { + "provider": "openai", + "api_key": "mock-key", + "model": "gpt-3.5-turbo", + "system_prompt": "You help users work with PDF documents.", + "skills": [f"local:{filename}"], + }, + ) + self._create_error = None + except AgentCreationError as exc: + self._agent = None + self._create_error = exc + + def create_llm_agent_with_missing_skill(self) -> None: + """Attempt to create an LLMAgent referencing a skill file that does not exist.""" + self._install_mock_chat_model() + assert self._factory is not None, ( + "create_agent_factory_with_local_skill_store must run first" + ) + try: + self._agent = self._factory._create_agent_instance( # noqa: SLF001 + "broken_assistant", + "llm", + { + "provider": "openai", + "api_key": "mock-key", + "skills": ["local:does-not-exist.yaml"], + }, + ) + self._create_error = None + except AgentCreationError as exc: + self._agent = None + self._create_error = exc + + def process_message_with_agent(self, message: str) -> None: + assert self._agent is not None, "No agent was created" + try: + self._result = asyncio.run(self._agent.process_message(message)) + finally: + self._teardown_patches() + + def agent_should_have_been_created(self) -> None: + assert self._create_error is None, ( + f"Agent creation failed: {self._create_error}" + ) + assert self._agent is not None + + def agent_creation_should_have_failed_with_agent_creation_error(self) -> None: + assert self._create_error is not None, ( + "Expected AgentCreationError but agent was created" + ) + assert isinstance(self._create_error, AgentCreationError), ( + f"Expected AgentCreationError, got {type(self._create_error).__name__}" + ) + + def agent_metadata_skills_loaded_should_equal(self, expected: int) -> None: + assert self._agent is not None + metadata = self._agent.get_metadata() + actual = metadata.get("skills_loaded") + assert actual == int(expected), f"skills_loaded = {actual}, expected {expected}" + + def agent_capabilities_should_include(self, capability: str) -> None: + assert self._agent is not None + capabilities = self._agent.get_capabilities() + assert capability in capabilities, f"{capability!r} not in {capabilities!r}" + + def system_prompt_sent_to_model_should_contain(self, text: str) -> None: + assert self._mock_ainvoke_calls, "chat_model.ainvoke was never called" + first_call_messages = self._mock_ainvoke_calls[0]["messages"] + system_texts = [ + str(m.content) for m in first_call_messages if isinstance(m, SystemMessage) + ] + assert any(text in t for t in system_texts), ( + f"{text!r} not found in system messages: {system_texts}" + ) + + def tool_message_sent_to_model_should_contain(self, text: str) -> None: + assert len(self._mock_ainvoke_calls) >= 2, ( + "Expected at least 2 ainvoke calls (tool round + final answer)" + ) + second_call_messages = self._mock_ainvoke_calls[1]["messages"] + tool_texts = [ + str(m.content) for m in second_call_messages if isinstance(m, ToolMessage) + ] + assert any(text in t for t in tool_texts), ( + f"{text!r} not found in tool messages sent back to the model: {tool_texts}" + ) + + def model_should_have_been_offered_the_skill_tool(self) -> None: + for call in self._mock_ainvoke_calls: + for tool in call.get("kwargs", {}).get("tools") or []: + if tool.get("function", {}).get("name") == "skill": + return + raise AssertionError("model was never invoked with the skill tool declared") + + def result_should_contain(self, text: str) -> None: + assert self._result is not None + assert text in self._result, f"Expected {text!r} in result: {self._result!r}" diff --git a/robot/agent_skills.robot b/robot/agent_skills.robot new file mode 100644 index 0000000..27a95a8 --- /dev/null +++ b/robot/agent_skills.robot @@ -0,0 +1,69 @@ +*** Settings *** +Documentation Skill package loading integration tests (issue #88, ADR-2034). +... Exercises the real registry/package-resolution pipeline +... (LocalPackageStore, PackageContentResolver, SkillLoader) end +... to end with a real on-disk Skill package, wired into a real +... LLMAgent via AgentFactory. Only the LLM API call itself is +... mocked (no real network LLM calls), matching every other +... suite in this project — no mocks are used anywhere in the +... skill resolution, validation, or tool-dispatch path. +Library SkillLoadingTestLib.py + +*** Test Cases *** +LLM Agent Resolves A Real Local Skill Package + [Documentation] skills: [local:...] resolves a real on-disk YAML file + ... through the real LocalPackageStore/PackageContentResolver/SkillLoader + ... pipeline and successfully creates the agent. + Write Real Skill Package + Create Agent Factory With Local Skill Store + Create Llm Agent With Skill + Agent Should Have Been Created + +Resolved Skill Is Surfaced In Agent Metadata And Capabilities + [Documentation] A resolved skill is observable via get_metadata() + ... and get_capabilities(), per the progressive-disclosure model. + Write Real Skill Package + Create Agent Factory With Local Skill Store + Create Llm Agent With Skill + Agent Metadata Skills Loaded Should Equal 1 + Agent Capabilities Should Include skill-loading + +Discovery Catalogue Is Injected Into The Effective System Prompt + [Documentation] The skill's name/description are appended to the + ... system prompt sent to the model (discovery stage). + Write Real Skill Package + Create Agent Factory With Local Skill Store + Create Llm Agent With Skill + Process Message With Agent Can you help me extract text from a PDF? + System Prompt Sent To Model Should Contain pdf-processing + System Prompt Sent To Model Should Contain Extract text and tables from PDFs. + +Model Is Offered The Skill Activation Tool + [Documentation] The synthesized "skill" tool is included in the + ... tools schema sent to the model. + Write Real Skill Package + Create Agent Factory With Local Skill Store + Create Llm Agent With Skill + Process Message With Agent Can you help me extract text from a PDF? + Model Should Have Been Offered The Skill Tool + +Activating The Skill End To End Returns Its Real Instructions + [Documentation] When the model calls the skill tool, the REAL + ... resolved instructions (read from the real YAML file on disk) + ... flow back into the conversation via a real ToolAgent dispatch — + ... proving genuine end-to-end resolution, not just metadata. + Write Real Skill Package + Create Agent Factory With Local Skill Store + Create Llm Agent With Skill + Process Message With Agent Can you help me extract text from a PDF? + Tool Message Sent To Model Should Contain [SKILL_ACTIVATED] + Tool Message Sent To Model Should Contain Run scripts/extract.py to extract text. + Result Should Contain Final answer using skill instructions + +Missing Local Skill Package Fails Agent Creation + [Documentation] A skills: reference to a nonexistent local file fails + ... agent creation with AgentCreationError (fail-fast, D-7). + Write Real Skill Package + Create Agent Factory With Local Skill Store + Create Llm Agent With Missing Skill + Agent Creation Should Have Failed With Agent Creation Error diff --git a/src/cleveractors/agents/factory.py b/src/cleveractors/agents/factory.py index ea3aea8..c31b96c 100644 --- a/src/cleveractors/agents/factory.py +++ b/src/cleveractors/agents/factory.py @@ -17,6 +17,7 @@ from cleveractors.agents.llm import ( LLMAgent, ) from cleveractors.agents.llm_providers import validate_credentials_structure +from cleveractors.agents.skills import SkillLoader from cleveractors.agents.tool import ToolAgent from cleveractors.core.exceptions import AgentCreationError, ConfigurationError from cleveractors.templates.renderer import TemplateRenderer @@ -70,6 +71,7 @@ class AgentFactory: credentials: dict[str, dict[str, str]] | None = None, *, tool_agent_class: type[ToolAgent] = ToolAgent, + skill_loader: SkillLoader | None = None, ) -> None: """ Initialize the AgentFactory. @@ -95,11 +97,17 @@ class AgentFactory: internal factory code path that constructs or registers a ``ToolAgent`` uses this class instead of the module-level default, removing the need for monkey-patching (issue #73). + skill_loader: Optional :class:`~cleveractors.agents.skills.SkillLoader` + to use for resolving ``skills`` references on ``"llm"``-typed + agents. Defaults to a fresh ``SkillLoader``. Callers inject a + custom instance mainly for testing (e.g. one wired to a fake + resolver). Raises: ConfigurationError: If ``config`` is not a dict, ``template_renderer`` is not a :class:`~cleveractors.templates.renderer.TemplateRenderer`, - or ``tool_agent_class`` is not a subclass of ``ToolAgent``. + ``tool_agent_class`` is not a subclass of ``ToolAgent``, or + ``skill_loader`` is not a ``SkillLoader`` instance. """ # Argument validation (CONTRIBUTING.md §Argument Validation) if not isinstance(config, dict): @@ -119,7 +127,12 @@ class AgentFactory: f"tool_agent_class must be a subclass of ToolAgent, " f"got {tool_agent_class!r}" ) + if skill_loader is not None and not isinstance(skill_loader, SkillLoader): + raise ConfigurationError( + f"skill_loader must be a SkillLoader, got {type(skill_loader).__name__}" + ) self._tool_agent_class: type[ToolAgent] = tool_agent_class + self._skill_loader: SkillLoader = skill_loader or SkillLoader() self.credentials = validate_credentials_structure(credentials) self.agent_types: dict[str, type[Agent]] = { @@ -291,6 +304,12 @@ class AgentFactory: ): final_config["context"] = self.config["context"]["global"].copy() + # Resolve `skills` references (ADR-2034) before LLMAgent is + # constructed, so the discovery catalogue / activation tool are + # already part of the config LLMAgent reads at __init__ time. + if agent_type == "llm" and final_config.get("skills"): + final_config = self._skill_loader.load(agent_name, final_config) + # Build constructor kwargs. For LLMAgent, thread the credentials # dict through so per-request credential injection (ADR-2026) is # available without modifying the stored config. diff --git a/src/cleveractors/agents/llm.py b/src/cleveractors/agents/llm.py index 05467e6..62e1c90 100644 --- a/src/cleveractors/agents/llm.py +++ b/src/cleveractors/agents/llm.py @@ -879,6 +879,36 @@ class LLMAgent(AgentWithMemory): ) return (parsed, _pp, _pc) + # ------------------------------------------------------------------ + # _build_tool_context — shared ephemeral-ToolAgent context builder + # ------------------------------------------------------------------ + + def _build_tool_context(self, parent_unsafe: bool) -> dict[str, Any] | None: + """Build the context dict passed to an ephemeral tool-call ToolAgent. + + Always includes ``_unsafe_mode`` when the parent agent runs + unsafe, and additionally forwards the resolved skill catalogue + (``_loaded_skills``, injected by ``SkillLoader`` at agent-creation + time — see :class:`cleveractors.agents.skills.SkillLoader`) under + ``_skills`` so the synthesized ``skill`` tool can activate a skill + or read its resources. Returns ``None`` when there is nothing to + forward, preserving the context contract tools relied on before + skill loading existed. + + Args: + parent_unsafe: Whether the parent LLM agent's ``unsafe_mode`` + config is enabled. + + Returns: + A context dict, or ``None`` when neither unsafe mode nor a + loaded skill catalogue applies. + """ + context: dict[str, Any] = {"_unsafe_mode": True} if parent_unsafe else {} + loaded_skills = self.config.get("_loaded_skills") + if loaded_skills: + context["_skills"] = loaded_skills + return context or None + # ------------------------------------------------------------------ # _execute_tool_loop — shared tool-call orchestration helper # ------------------------------------------------------------------ @@ -1065,11 +1095,7 @@ class LLMAgent(AgentWithMemory): config=tool_cfg, template_renderer=self.template_renderer, ) - t_ctx: dict[str, Any] | None = ( - {"_unsafe_mode": True} - if parent_unsafe - else None - ) + t_ctx = self._build_tool_context(parent_unsafe) if tool_name not in _declared_names: messages.append( ToolMessage( @@ -1238,9 +1264,7 @@ class LLMAgent(AgentWithMemory): config=tool_config, template_renderer=self.template_renderer, ) - tool_ctx: dict[str, Any] | None = ( - {"_unsafe_mode": True} if parent_unsafe else None - ) + tool_ctx = self._build_tool_context(parent_unsafe) raw_tool_output = await agent.process_message( json.dumps({"tool": tool_name, "args": args_m}) if isinstance(args_m, dict) @@ -2318,6 +2342,10 @@ class LLMAgent(AgentWithMemory): if self._lc_tools: capabilities.append("tool-calling") + # Add skill-loading capability when skills were resolved (§4.11). + if self.config.get("_loaded_skills"): + capabilities.append("skill-loading") + return capabilities def get_metadata(self) -> dict[str, Any]: @@ -2336,6 +2364,8 @@ class LLMAgent(AgentWithMemory): "tools_configured": self._lc_tools is not None and len(self._lc_tools) > 0, "tool_count": len(self._lc_tools) if self._lc_tools else 0, + "skills_loaded": len(self.config.get("_loaded_skills") or {}), + "skill_names": sorted(self.config.get("_loaded_skills") or {}), } ) return metadata diff --git a/src/cleveractors/agents/llm_tools.py b/src/cleveractors/agents/llm_tools.py index cfbab3b..e73f14e 100644 --- a/src/cleveractors/agents/llm_tools.py +++ b/src/cleveractors/agents/llm_tools.py @@ -14,6 +14,10 @@ from typing import Any from cleveractors.core.exceptions import ConfigurationError +SKILL_TOOL_NAME = "skill" +"""Name of the synthesized tool SkillLoader appends to an LLM agent's +``tools`` config when ``skills`` is configured (ADR-2034 D-5).""" + _BUILTIN_TOOL_SCHEMAS: dict[str, dict[str, Any]] = { "echo": { "description": "Echo back the provided text.", @@ -248,6 +252,32 @@ _BUILTIN_TOOL_SCHEMAS: dict[str, dict[str, Any]] = { "additionalProperties": False, }, }, + SKILL_TOOL_NAME: { + "description": "Activate a loaded skill to retrieve its full instructions, or read one of its bundled resource files. Call with only 'skill_name' to activate a skill and load its instructions. Call again with 'resource' set to a path from the instructions (e.g. 'scripts/extract.py') to read that bundled file. Use 'max_chars' and 'offset' to page through long instructions or resources: the response's 'MORE_CONTENT_AT_OFFSET: N' marker gives the exact offset to pass on the next call.", + "parameters": { + "type": "object", + "properties": { + "skill_name": { + "type": "string", + "description": "Name of the loaded skill to activate, exactly as given in the discovery catalogue.", + }, + "resource": { + "type": "string", + "description": "Optional relative path of a bundled resource file to read (e.g. 'references/REFERENCE.md'). When omitted, the skill's instructions are returned instead.", + }, + "max_chars": { + "type": "integer", + "description": "Maximum characters to return. Content exceeding this will show 'TRUNCATED'. Recommended for long instructions or resources.", + }, + "offset": { + "type": "integer", + "description": "Character position to start reading from (default 0). Combine with max_chars to page through long content using the response's MORE_CONTENT_AT_OFFSET marker.", + }, + }, + "required": ["skill_name"], + "additionalProperties": False, + }, + }, } diff --git a/src/cleveractors/agents/skill_resolution.py b/src/cleveractors/agents/skill_resolution.py new file mode 100644 index 0000000..6355c23 --- /dev/null +++ b/src/cleveractors/agents/skill_resolution.py @@ -0,0 +1,125 @@ +"""Skill package reference resolution. + +Mirrors the established resolution pattern used for template package +references (``cleveractors.templates.base._resolve_package_ref``): reference +scheme parsing is delegated entirely to +:class:`~cleveractors.registry.PackageReference`, and the same +:class:`~cleveractors.registry.PackageContentResolver` machinery (two-tier +cache, ``local:`` support via +:class:`~cleveractors.registry.LocalPackageStore`) is reused rather than +reimplemented. +""" + +from __future__ import annotations + +import asyncio +import logging +from typing import Any + +from cleveractors.registry import ( + InvalidPackageReferenceError, + LocalPackageStore, + PackageContentResolver, + PackageNotFoundError, + PackageReference, +) + +logger = logging.getLogger(__name__) + + +class SkillReferenceResolver: + """Resolves a Skill package reference string to raw content.""" + + def __init__( + self, + resolver: PackageContentResolver | None = None, + local_store: LocalPackageStore | None = None, + ) -> None: + """Initialize the resolver. + + Args: + resolver: A pre-configured resolver to reuse (and whose cache + is shared across resolutions). When ``None``, an ad-hoc + resolver is created and torn down per call to + :meth:`resolve`. + local_store: Store used to resolve ``local:`` references when + *resolver* is ``None``. Ignored when *resolver* is given — + configure the store on the injected resolver instead. + """ + self._resolver = resolver + self._local_store = local_store + + def resolve(self, reference: str) -> dict[str, Any]: + """Resolve *reference* to raw Skill package content. + + Args: + reference: A package reference string using the ``registry:``, + ``ID:``, or ``local:`` scheme. + + Returns: + The raw resolved content dict (not yet schema-validated). + + Raises: + RegistryError: If the reference is malformed or resolution + fails. + """ + if not isinstance(reference, str) or not reference: + raise InvalidPackageReferenceError( + f"Skill reference must be a non-empty string, got {reference!r}" + ) + + try: + package_ref = PackageReference.from_string(reference) + except ValueError as exc: + raise InvalidPackageReferenceError( + f"Invalid skill reference '{reference}': {exc}", + original_reference=reference, + ) from exc + + resolver = self._resolver + owns_resolver = resolver is None + if resolver is None: + resolver = PackageContentResolver(local_store=self._local_store) + + try: + resolved = resolver.resolve(package_ref, package_type="skill") + except (FileNotFoundError, ValueError) as exc: + raise PackageNotFoundError( + f"Failed to resolve skill reference '{reference}': {exc}", + original_reference=reference, + ) from exc + finally: + if owns_resolver: + try: + try: + asyncio.get_running_loop() + except RuntimeError: + # No loop running (e.g. called from plain sync code) -- + # asyncio.run() is the correct, safe way to drive the + # cleanup coroutine to completion. + asyncio.run(resolver.close_all()) + else: + # Called synchronously from within an already-running + # loop (e.g. AgentFactory.create_agent(), invoked + # mid-await from _execute_graph/_execute_llm). + # asyncio.run() cannot nest inside a running loop -- + # calling it here would raise immediately, before the + # close_all() coroutine object it was given ever ran, + # leaving that coroutine dangling and triggering + # "coroutine 'close_all' was never awaited" once + # garbage-collected. Schedule it as a real background + # task on the running loop instead. + asyncio.ensure_future(resolver.close_all()) + except Exception: + logger.debug( + "Error closing ad-hoc resolver for %s", + reference, + exc_info=True, + ) + + if resolved is None: + raise PackageNotFoundError( + f"Failed to resolve skill reference '{reference}'", + original_reference=reference, + ) + return resolved diff --git a/src/cleveractors/agents/skill_schema.py b/src/cleveractors/agents/skill_schema.py new file mode 100644 index 0000000..d11b33a --- /dev/null +++ b/src/cleveractors/agents/skill_schema.py @@ -0,0 +1,331 @@ +"""Skill Package Schema value objects and validation. + +Validates resolved package content against the Skill Package Schema +(Package Registry Standard §16, added by ADR-2034) and produces the +immutable :class:`Skill` / :class:`SkillResource` value objects consumed by +:class:`~cleveractors.agents.skills.SkillLoader`. +""" + +from __future__ import annotations + +import base64 +import re +from dataclasses import dataclass, field +from typing import Any + +from cleveractors.registry import Canonicalizer, PackageType, ValidationError + +_NAME_PATTERN = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") +_VALID_ENCODINGS = frozenset({"utf-8", "base64"}) +_RESOLVER_METADATA_KEYS = frozenset( + { + "_original_reference", + "_package_id", + "_server", + "_namespace", + "_name", + "_version", + "id", + "type", + } +) +_MAX_NAME_LENGTH = 64 +_MAX_DESCRIPTION_LENGTH = 1024 +_MAX_COMPATIBILITY_LENGTH = 500 + + +@dataclass(frozen=True) +class SkillResource: + """One bundled file within a resolved Skill package (§16.1).""" + + path: str + content: str + encoding: str + + def decoded_text(self) -> str: + """Return the resource's content decoded to text. + + Returns: + The UTF-8 text of the resource, base64-decoding first when + ``encoding`` is ``"base64"``. + """ + if self.encoding == "base64": + return base64.b64decode(self.content).decode("utf-8") + return self.content + + +@dataclass(frozen=True) +class Skill: + """A validated, resolved Skill package (Package Registry Standard §16).""" + + name: str + description: str + instructions: str + package_id: str + source_reference: str + license: str | None = None + compatibility: str | None = None + metadata: dict[str, str] = field(default_factory=dict) + allowed_tools: str | None = None + resources: dict[str, SkillResource] = field(default_factory=dict) + + def to_context_dict(self) -> dict[str, Any]: + """Return a plain-data representation for tool-call context. + + Returns: + A JSON-serialisable mapping consumed by + ``ToolAgent._skill_tool`` and ``LLMAgent.get_metadata`` — plain + data only, so neither needs any registry-type awareness. + """ + return { + "name": self.name, + "description": self.description, + "instructions": self.instructions, + "resources": { + path: {"content": resource.content, "encoding": resource.encoding} + for path, resource in self.resources.items() + }, + } + + +class SkillValidator: + """Validates resolved package content against the Skill Package Schema. + + See Package Registry Standard §16.1 (added by ADR-2034). + """ + + def __init__(self, canonicalizer: Canonicalizer | None = None) -> None: + """Initialize the validator. + + Args: + canonicalizer: Canonicalizer used to compute a skill's Package + ID when the resolved content did not already carry one. + Defaults to a fresh :class:`Canonicalizer`. + """ + self._canonicalizer = canonicalizer or Canonicalizer() + + def validate(self, content: dict[str, Any], *, source_reference: str) -> Skill: + """Validate *content* and return a :class:`Skill` value object. + + Args: + content: The raw resolved package content, as returned by + :class:`~cleveractors.registry.PackageContentResolver`. + source_reference: The original reference string used to + resolve *content* — included in error messages. + + Returns: + A validated, immutable :class:`Skill`. + + Raises: + ValidationError: If *content* does not conform to the Skill + Package Schema. + """ + if not isinstance(source_reference, str) or not source_reference: + raise ValueError("source_reference must be a non-empty string") + if not isinstance(content, dict): + raise ValidationError( + f"Skill reference '{source_reference}' must resolve to a " + f"mapping, got {type(content).__name__}", + original_reference=source_reference, + ) + + if content.get("skill") is not True: + raise ValidationError( + f"Skill reference '{source_reference}' is missing the " + "required discriminator field 'skill: true'", + original_reference=source_reference, + ) + + name = self._require_string(content, "name", source_reference) + self._validate_name(name, source_reference) + description = self._require_string(content, "description", source_reference) + self._validate_length( + description, "description", 1, _MAX_DESCRIPTION_LENGTH, source_reference + ) + instructions = self._require_string(content, "instructions", source_reference) + + license_ = self._optional_string(content, "license", source_reference) + compatibility = self._optional_string( + content, "compatibility", source_reference + ) + if compatibility is not None: + self._validate_length( + compatibility, + "compatibility", + 1, + _MAX_COMPATIBILITY_LENGTH, + source_reference, + ) + allowed_tools = self._optional_string( + content, "allowed_tools", source_reference + ) + metadata = self._validate_metadata(content, source_reference) + resources = self._validate_resources(content, source_reference) + package_id = self._resolve_package_id(content, source_reference) + + return Skill( + name=name, + description=description, + instructions=instructions, + package_id=package_id, + source_reference=source_reference, + license=license_, + compatibility=compatibility, + metadata=metadata, + allowed_tools=allowed_tools, + resources=resources, + ) + + @staticmethod + def _require_string( + content: dict[str, Any], field_name: str, source_reference: str + ) -> str: + value = content.get(field_name) + if not isinstance(value, str) or not value: + raise ValidationError( + f"Skill reference '{source_reference}' is missing required " + f"non-empty string field '{field_name}'", + original_reference=source_reference, + ) + return value + + @staticmethod + def _optional_string( + content: dict[str, Any], field_name: str, source_reference: str + ) -> str | None: + value = content.get(field_name) + if value is None: + return None + if not isinstance(value, str): + raise ValidationError( + f"Skill reference '{source_reference}' field '{field_name}' " + f"must be a string, got {type(value).__name__}", + original_reference=source_reference, + ) + return value + + @staticmethod + def _validate_length( + value: str, + field_name: str, + min_len: int, + max_len: int, + source_reference: str, + ) -> None: + if not min_len <= len(value) <= max_len: + raise ValidationError( + f"Skill reference '{source_reference}' field '{field_name}' " + f"must be {min_len}-{max_len} characters, got {len(value)}", + original_reference=source_reference, + ) + + @staticmethod + def _validate_name(name: str, source_reference: str) -> None: + if not 1 <= len(name) <= _MAX_NAME_LENGTH or not _NAME_PATTERN.match(name): + raise ValidationError( + f"Skill reference '{source_reference}' has invalid name " + f"{name!r}: must be 1-{_MAX_NAME_LENGTH} lowercase " + "alphanumeric characters and hyphens, with no leading, " + "trailing, or consecutive hyphens", + original_reference=source_reference, + ) + + @staticmethod + def _validate_metadata( + content: dict[str, Any], source_reference: str + ) -> dict[str, str]: + metadata = content.get("metadata") + if metadata is None: + return {} + if not isinstance(metadata, dict): + raise ValidationError( + f"Skill reference '{source_reference}' field 'metadata' " + f"must be a mapping, got {type(metadata).__name__}", + original_reference=source_reference, + ) + for key, value in metadata.items(): + if not isinstance(key, str) or not isinstance(value, str): + raise ValidationError( + f"Skill reference '{source_reference}' field 'metadata' " + "must map strings to strings", + original_reference=source_reference, + ) + return dict(metadata) + + @staticmethod + def _validate_resources( + content: dict[str, Any], source_reference: str + ) -> dict[str, SkillResource]: + resources = content.get("resources") + if resources is None: + return {} + if not isinstance(resources, dict): + raise ValidationError( + f"Skill reference '{source_reference}' field 'resources' " + f"must be a mapping, got {type(resources).__name__}", + original_reference=source_reference, + ) + validated: dict[str, SkillResource] = {} + for path, entry in resources.items(): + segments = path.split("/") if isinstance(path, str) else [] + if ( + not isinstance(path, str) + or not path + or path.startswith("/") + or any(segment in ("", ".", "..") for segment in segments) + ): + raise ValidationError( + f"Skill reference '{source_reference}' has invalid " + f"resource path {path!r}: must be a non-empty relative " + "POSIX path with no leading slash and no '.'/'..' " + "segments", + original_reference=source_reference, + ) + if not isinstance(entry, dict): + raise ValidationError( + f"Skill reference '{source_reference}' resource " + f"'{path}' must be a mapping, got {type(entry).__name__}", + original_reference=source_reference, + ) + resource_content = entry.get("content") + encoding = entry.get("encoding", "utf-8") + if not isinstance(resource_content, str): + raise ValidationError( + f"Skill reference '{source_reference}' resource " + f"'{path}' must have a string 'content' field", + original_reference=source_reference, + ) + if encoding not in _VALID_ENCODINGS: + raise ValidationError( + f"Skill reference '{source_reference}' resource " + f"'{path}' has invalid encoding {encoding!r}: must be " + f"one of {sorted(_VALID_ENCODINGS)}", + original_reference=source_reference, + ) + validated[path] = SkillResource( + path=path, content=resource_content, encoding=encoding + ) + return validated + + def _resolve_package_id( + self, content: dict[str, Any], source_reference: str + ) -> str: + existing = content.get("_package_id") + if isinstance(existing, str) and existing: + return existing + schema_content = { + key: value + for key, value in content.items() + if key not in _RESOLVER_METADATA_KEYS + } + try: + package_id = self._canonicalizer.compute_package_id( + schema_content, PackageType.SKILL + ) + except (TypeError, ValueError) as exc: + raise ValidationError( + f"Skill reference '{source_reference}' could not be " + f"canonicalized: {exc}", + original_reference=source_reference, + ) from exc + return package_id.id_string diff --git a/src/cleveractors/agents/skills.py b/src/cleveractors/agents/skills.py new file mode 100644 index 0000000..62a7525 --- /dev/null +++ b/src/cleveractors/agents/skills.py @@ -0,0 +1,146 @@ +"""Skill loading facade for LLM agents. + +Orchestrates :class:`~cleveractors.agents.skill_resolution.SkillReferenceResolver` +and :class:`~cleveractors.agents.skill_schema.SkillValidator` to resolve a +``type: llm`` agent's ``skills`` config field and inject the result back +into the agent config (discovery catalogue, activation tool, resolved +catalogue) per the Skill Package Schema (Package Registry Standard §16, +added by ADR-2034). +""" + +from __future__ import annotations + +from typing import Any + +from cleveractors.agents.llm_tools import SKILL_TOOL_NAME +from cleveractors.agents.skill_resolution import SkillReferenceResolver +from cleveractors.agents.skill_schema import Skill, SkillResource, SkillValidator +from cleveractors.core.exceptions import AgentCreationError +from cleveractors.registry import RegistryError, ValidationError + +__all__ = [ + "Skill", + "SkillLoader", + "SkillReferenceResolver", + "SkillResource", + "SkillValidator", +] + + +class SkillLoader: + """Resolves and injects ``skills`` package references into an LLM + agent's configuration. + + Invoked by :class:`~cleveractors.agents.factory.AgentFactory` on the + ``"llm"`` branch, before :class:`~cleveractors.agents.llm.LLMAgent` is + constructed. Produces a new config dict — the input is never mutated — + with the discovery catalogue appended to ``system_prompt``, the + ``skill`` activation tool appended to ``tools``, and the resolved + catalogue stored under the internal ``_loaded_skills`` key so + ``LLMAgent`` can surface it in metadata/capabilities and forward it to + tool-call context without any registry awareness of its own. + """ + + def __init__( + self, + resolver: SkillReferenceResolver | None = None, + validator: SkillValidator | None = None, + ) -> None: + """Initialize the loader. + + Args: + resolver: Resolver used to fetch raw skill content. Defaults + to a fresh :class:`SkillReferenceResolver`. + validator: Validator used to check resolved content against + the Skill Package Schema. Defaults to a fresh + :class:`SkillValidator`. + """ + self._resolver = resolver or SkillReferenceResolver() + self._validator = validator or SkillValidator() + + def load(self, agent_name: str, agent_config: dict[str, Any]) -> dict[str, Any]: + """Resolve ``agent_config["skills"]`` and return an augmented config. + + Args: + agent_name: Name of the agent being created (used for error + context only). + agent_config: The agent's configuration dict, as it will be + passed to ``LLMAgent``. + + Returns: + *agent_config* unchanged (same object) when no ``skills`` are + configured; otherwise a new dict with ``system_prompt``, + ``tools``, and ``_loaded_skills`` augmented. + + Raises: + AgentCreationError: If ``skills`` is malformed, or a skill + reference cannot be resolved or fails schema validation. + """ + if not isinstance(agent_name, str) or not agent_name: + raise ValueError("agent_name must be a non-empty string") + if not isinstance(agent_config, dict): + raise ValueError( + f"agent_config must be a dict, got {type(agent_config).__name__}" + ) + + skill_refs = agent_config.get("skills") + if not skill_refs: + return agent_config + + if not isinstance(skill_refs, list) or not all( + isinstance(ref, str) and ref for ref in skill_refs + ): + raise AgentCreationError( + f"Agent '{agent_name}': 'skills' must be a list of " + "non-empty reference strings" + ) + + loaded: dict[str, Skill] = {} + for reference in skill_refs: + try: + raw_content = self._resolver.resolve(reference) + skill = self._validator.validate( + raw_content, source_reference=reference + ) + except (RegistryError, ValidationError) as exc: + raise AgentCreationError( + f"Agent '{agent_name}': failed to load skill '{reference}': {exc}" + ) from exc + loaded[skill.name] = skill + + new_config = dict(agent_config) + new_config["system_prompt"] = self._build_system_prompt(agent_config, loaded) + new_config["tools"] = self._build_tools(agent_config) + new_config["_loaded_skills"] = { + name: skill.to_context_dict() for name, skill in loaded.items() + } + return new_config + + @staticmethod + def _build_system_prompt( + agent_config: dict[str, Any], loaded: dict[str, Skill] + ) -> str: + base_prompt = agent_config.get("system_prompt") or "" + catalogue_lines = "\n".join( + f"- **{skill.name}**: {skill.description}" + for skill in sorted(loaded.values(), key=lambda s: s.name) + ) + catalogue = ( + "## Available Skills\n" + f"Call the `{SKILL_TOOL_NAME}` tool with `skill_name` set to " + "activate one of the skills below and load its full " + f"instructions.\n{catalogue_lines}" + ) + return f"{base_prompt}\n\n{catalogue}" if base_prompt else catalogue + + @staticmethod + def _build_tools(agent_config: dict[str, Any]) -> list[Any]: + tools = list(agent_config.get("tools") or []) + already_present = any( + (isinstance(t, str) and t == SKILL_TOOL_NAME) + or (isinstance(t, dict) and t.get("name") == SKILL_TOOL_NAME) + for t in tools + ) + if not already_present: + tools.append({"name": SKILL_TOOL_NAME}) + return tools diff --git a/src/cleveractors/agents/tool.py b/src/cleveractors/agents/tool.py index 556f4af..f2bef89 100644 --- a/src/cleveractors/agents/tool.py +++ b/src/cleveractors/agents/tool.py @@ -6,6 +6,7 @@ capabilities within the RxPy-based reactive architecture. """ import asyncio +import base64 import json import logging import os @@ -16,6 +17,7 @@ from typing import Any, List, Literal, Optional, Union import aiohttp from cleveractors.agents.base import Agent +from cleveractors.agents.llm_tools import SKILL_TOOL_NAME from cleveractors.core.exceptions import AgentCreationError, ExecutionError from cleveractors.templates.renderer import TemplateRenderer @@ -76,6 +78,7 @@ class ToolAgent(Agent): "file_read": self._file_read_tool, "file_write": self._file_write_tool, "progress_bar": self._progress_bar_tool, + SKILL_TOOL_NAME: self._skill_tool, } if self.allow_shell: self.builtin_tools["shell"] = self._shell_tool @@ -1000,6 +1003,103 @@ class ToolAgent(Agent): return f"Progress updated: {rendered}" + async def _skill_tool( + self, args: dict[str, Any], context: Optional[dict[str, Any]] + ) -> str: + """Activate a loaded skill, or read one of its bundled resources. + + Reads the resolved skill catalogue from ``context["_skills"]``, + injected by :class:`~cleveractors.agents.llm.LLMAgent` when the + parent agent has ``skills`` configured (see + :class:`~cleveractors.agents.skills.SkillLoader`). Supports the + same `offset`/`max_chars` pagination convention as + :meth:`_file_read_tool` (ADR-2033). + """ + skill_name = args.get("skill_name", "") + if not isinstance(skill_name, str) or not skill_name: + raise ExecutionError("Skill tool requires a non-empty 'skill_name'") + + resource = args.get("resource") + if resource is not None and not isinstance(resource, str): + raise ExecutionError("'resource' must be a string when provided") + + max_chars = args.get("max_chars") + if max_chars is not None: + try: + max_chars = int(max_chars) + except (TypeError, ValueError) as exc: + raise ExecutionError( + f"max_chars must be an integer, got {max_chars}" + ) from exc + + offset = args.get("offset", 0) + if offset is None: + offset = 0 + try: + offset = int(offset) + except (TypeError, ValueError) as exc: + raise ExecutionError(f"offset must be an integer, got {offset}") from exc + if offset < 0: + raise ExecutionError(f"offset must be non-negative, got {offset}") + + skills = (context or {}).get("_skills") or {} + skill = skills.get(skill_name) + if skill is None: + available = ", ".join(sorted(skills)) or "(none loaded)" + raise ExecutionError( + f"Skill '{skill_name}' is not loaded for this agent. " + f"Available skills: {available}" + ) + + if resource is None: + full_text = skill.get("instructions", "") + header_prefix = f"[SKILL_ACTIVATED]📘 Skill: {skill_name}" + start_marker, end_marker = "[SKILL_CONTENT_START]", "[SKILL_CONTENT_END]" + else: + resources = skill.get("resources") or {} + resource_entry = resources.get(resource) + if resource_entry is None: + available_resources = ", ".join(sorted(resources)) or "(none)" + raise ExecutionError( + f"Skill '{skill_name}' has no resource '{resource}'. " + f"Available resources: {available_resources}" + ) + full_text = self._decode_skill_resource(resource_entry) + header_prefix = ( + f"[SKILL_RESOURCE_READ]📄 Skill: {skill_name} | Resource: {resource}" + ) + start_marker, end_marker = "[FILE_CONTENT_START]", "[FILE_CONTENT_END]" + + char_count = len(full_text) + if offset > 0 and offset >= char_count: + header = ( + f"{header_prefix} | Size: {char_count} chars | " + f"NO_MORE_CONTENT_AT_OFFSET: {offset}" + ) + return f"{header}\n{start_marker}\n{end_marker}" + + window_end = ( + char_count if max_chars is None else min(char_count, offset + max_chars) + ) + truncated = max_chars is not None and window_end < char_count + windowed = full_text[offset:window_end] + + header = f"{header_prefix} | Size: {char_count} chars" + if truncated: + header += ( + f" | TRUNCATED to {max_chars} chars" + f" | MORE_CONTENT_AT_OFFSET: {window_end}" + ) + return f"{header}\n{start_marker}\n{windowed}\n{end_marker}" + + @staticmethod + def _decode_skill_resource(entry: dict[str, Any]) -> str: + """Decode a skill resource entry to text, base64-decoding if needed.""" + content = entry.get("content", "") + if entry.get("encoding") == "base64": + return base64.b64decode(content).decode("utf-8") + return str(content) + def get_capabilities(self) -> List[str]: """Get the capabilities of the tool agent.""" capabilities = ["tool-execution", "command-execution"] diff --git a/src/cleveractors/registry/local_store.py b/src/cleveractors/registry/local_store.py index 3d12b9b..5bf05c9 100644 --- a/src/cleveractors/registry/local_store.py +++ b/src/cleveractors/registry/local_store.py @@ -88,6 +88,14 @@ class LocalPackageStore: self._canonicalizer = canonicalizer or Canonicalizer() self._fail_on_unresolvable_refs = fail_on_unresolvable_refs self._ref_canonicalizer = Canonicalizer(reference_resolver=self._resolve_ref) + # Identity Map: every package this store resolves -- directly or + # indirectly while canonicalizing a nested `local:` reference in + # _resolve_ref -- becomes findable by its content-addressed ID with + # no re-read/re-hash. This is what lets an `ID:pkg_...` reference + # that a nested resolution already produced be answered locally + # (see PackageContentResolver's _IdReferenceStrategy) instead of + # only being resolvable by its original `local:` spelling. + self._identity_map: dict[str, LocalPackage] = {} # ------------------------------------------------------------------ # Public API @@ -144,12 +152,32 @@ class LocalPackageStore: relative_path, package_id.id_string, ) - return LocalPackage( + local_package = LocalPackage( package_id=package_id, content=resolved_content, file_path=file_path, original_reference=f"local:{relative_path}", ) + self._identity_map[package_id.id_string] = local_package + return local_package + + def find_by_id(self, package_id: str) -> LocalPackage | None: + """Identity Map lookup: a package this store has already resolved. + + Returns the ``LocalPackage`` previously produced by + :meth:`resolve_package` for *package_id*, or ``None`` if this store + has not resolved a package with that ID (yet). Populated as a side + effect of every ``resolve_package()`` call, including ones made + indirectly while canonicalizing a nested ``local:`` reference + (:meth:`_resolve_ref`) -- so a skill/agent/graph reference that was + rewritten from ``local:`` to ``ID:pkg_...`` during an outer + package's canonicalization is still answerable here, without + re-reading or re-hashing anything. + + Does not scan the filesystem and never raises; a miss simply means + this store hasn't resolved that ID in this process yet. + """ + return self._identity_map.get(package_id) # ------------------------------------------------------------------ # Internal helpers diff --git a/src/cleveractors/registry/reference_resolver.py b/src/cleveractors/registry/reference_resolver.py index 2d16492..e06d9a6 100644 --- a/src/cleveractors/registry/reference_resolver.py +++ b/src/cleveractors/registry/reference_resolver.py @@ -12,6 +12,7 @@ import asyncio import copy import logging import threading +from abc import ABC, abstractmethod from collections import OrderedDict from typing import TYPE_CHECKING, Any, Optional @@ -21,7 +22,7 @@ from cleveractors.registry.exceptions import InvalidPackageReferenceError from cleveractors.registry.types import PackageReference, ReferenceType if TYPE_CHECKING: - from cleveractors.registry.local_store import LocalPackageStore + from cleveractors.registry.local_store import LocalPackage, LocalPackageStore logger = logging.getLogger(__name__) @@ -40,6 +41,194 @@ _TEMPLATE_TYPE_TO_PACKAGE_TYPE: dict[str, str] = { } +def _local_package_result( + local_pkg: "LocalPackage", original_reference: str +) -> dict[str, Any]: + """Shape a resolved ``LocalPackage`` into the enriched content dict + every resolution strategy that finds a package locally returns.""" + return { + **local_pkg.content, + "_original_reference": original_reference, + "_package_id": local_pkg.package_id.id_string, + } + + +class ReferenceResolutionStrategy(ABC): + """Resolves one :class:`~cleveractors.registry.types.ReferenceType` + variant of a :class:`~cleveractors.registry.types.PackageReference`. + + One subclass per variant, selected by :class:`PackageContentResolver` + via a ``{ReferenceType: ReferenceResolutionStrategy}`` mapping -- + replacing a growing ``if ref.reference_type == ...`` chain with + per-variant classes (adding a reference type means adding a class, not + another branch). Each strategy owns both fetching *and* caching the + result, since caching for a REGISTRY reference is nested inside its own + async client/cache-tier plumbing and can't be factored out uniformly. + """ + + @abstractmethod + def resolve( + self, + resolver: "PackageContentResolver", + ref: PackageReference, + package_type: Optional[str], + cache_key: str, + ) -> Optional[dict[str, Any]]: + """Synchronous resolution.""" + + @abstractmethod + async def aresolve( + self, + resolver: "PackageContentResolver", + ref: PackageReference, + package_type: Optional[str], + cache_key: str, + ) -> Optional[dict[str, Any]]: + """Async resolution counterpart.""" + + +class _LocalReferenceStrategy(ReferenceResolutionStrategy): + """``local:`` -- delegates to the attached ``LocalPackageStore``.""" + + def resolve(self, resolver, ref, package_type, cache_key): + if resolver.local_store is None: + raise InvalidPackageReferenceError( + "Cannot resolve LOCAL reference: " + "no LocalPackageStore configured on PackageContentResolver" + ) + local_pkg = resolver.local_store.resolve_package(ref.name) + result = _local_package_result(local_pkg, ref.original_reference) + with resolver._lock: + resolver._put_cache(cache_key, result) + return copy.deepcopy(result) + + async def aresolve(self, resolver, ref, package_type, cache_key): + if resolver.local_store is None: + raise InvalidPackageReferenceError( + "Cannot resolve LOCAL reference: " + "no LocalPackageStore configured on PackageContentResolver" + ) + local_pkg = await asyncio.to_thread( + resolver.local_store.resolve_package, ref.name + ) + result = _local_package_result(local_pkg, ref.original_reference) + async with resolver._get_async_lock(): + resolver._put_cache(cache_key, result) + return copy.deepcopy(result) + + +class _IdReferenceStrategy(ReferenceResolutionStrategy): + """``ID:pkg__`` -- a content-addressed identifier, not a + location. Resolvable only against something that remembers what that + hash maps to: the attached ``LocalPackageStore``'s Identity Map, which + already holds every package it has resolved so far -- including ones + resolved indirectly while canonicalizing a nested ``local:`` reference + (e.g. a `skills:` entry rewritten from `local:foo.yaml` to its `ID:` + form during an outer package's own canonicalization). Falls back to an + unresolved placeholder when the ID is unknown to it (e.g. a genuinely + remote/registry-only ID with no local store attached). + """ + + def resolve(self, resolver, ref, package_type, cache_key): + result = self._from_identity_map(resolver, ref) or self._placeholder(ref) + with resolver._lock: + resolver._put_cache(cache_key, result) + return copy.deepcopy(result) + + async def aresolve(self, resolver, ref, package_type, cache_key): + result = self._from_identity_map(resolver, ref) or self._placeholder(ref) + async with resolver._get_async_lock(): + resolver._put_cache(cache_key, result) + return copy.deepcopy(result) + + @staticmethod + def _from_identity_map( + resolver: "PackageContentResolver", ref: PackageReference + ) -> Optional[dict[str, Any]]: + if resolver.local_store is None: + return None + local_pkg = resolver.local_store.find_by_id(ref.id_string) + if local_pkg is None: + return None + return _local_package_result(local_pkg, ref.original_reference) + + @staticmethod + def _placeholder(ref: PackageReference) -> dict[str, Any]: + return { + "id": ref.id_string, + "type": "id", + "_original_reference": ref.original_reference, + } + + +class _RegistryReferenceStrategy(ReferenceResolutionStrategy): + """``server:namespace/name@version`` -- fetched from a live registry. + + Delegates back to the resolver's own connection-pool/content-cache + machinery (``_get_client``, ``_get_cache_for_resolved``, + ``_resolve_registry_async``), which already performs its own caching; + unlike the other two strategies, this one does not additionally cache + at the call site. + """ + + def resolve(self, resolver, ref, package_type, cache_key): + if not ref.server or not ref.namespace or not ref.name: + logger.warning("Incomplete registry reference: %s", ref) + return None + pkg_type = package_type or ref.package_type + if pkg_type is None: + logger.error( + "Cannot resolve REGISTRY reference %s without package_type; " + "provide package_type=... or set ref.package_type", + ref, + ) + return None + resolved_type = _TEMPLATE_TYPE_TO_PACKAGE_TYPE.get(pkg_type, pkg_type) + + try: + asyncio.get_running_loop() + except RuntimeError: + return asyncio.run( + resolver._resolve_registry_async( + ref.server, + ref.namespace, + ref.name, + ref.version or "latest", + cache_key, + resolved_type, + ref.original_reference, + ) + ) + raise RuntimeError( + f"asyncio.run() cannot be called from a running event loop. " + f"Use await resolver.aresolve({ref!r}, package_type={pkg_type!r}) instead." + ) + + async def aresolve(self, resolver, ref, package_type, cache_key): + if not ref.server or not ref.namespace or not ref.name: + logger.warning("Incomplete registry reference: %s", ref) + return None + pkg_type = package_type or ref.package_type + if pkg_type is None: + logger.error( + "Cannot resolve REGISTRY reference %s without package_type; " + "provide package_type=... or set ref.package_type", + ref, + ) + return None + resolved_type = _TEMPLATE_TYPE_TO_PACKAGE_TYPE.get(pkg_type, pkg_type) + + return await resolver._resolve_registry_async( + ref.server, + ref.namespace, + ref.name, + ref.version or "latest", + cache_key, + resolved_type, + ref.original_reference, + ) + + class PackageContentResolver: """Resolves package references using a pool of RegistryClient instances. @@ -84,6 +273,16 @@ class PackageContentResolver: total_stats: Aggregated CacheStats across all content caches. """ + # One ReferenceResolutionStrategy per ReferenceType (Strategy pattern): + # resolve()/aresolve() dispatch through this mapping instead of an + # if/elif chain on ref.reference_type. Strategies are stateless, so a + # single shared instance per type is safe across all resolvers. + _STRATEGIES: dict[ReferenceType, ReferenceResolutionStrategy] = { + ReferenceType.LOCAL: _LocalReferenceStrategy(), + ReferenceType.ID: _IdReferenceStrategy(), + ReferenceType.REGISTRY: _RegistryReferenceStrategy(), + } + def __init__( self, api_key: Optional[str] = None, @@ -204,67 +403,10 @@ class PackageContentResolver: self.cache.move_to_end(cache_key) return copy.deepcopy(self.cache[cache_key]) - if ref.reference_type == ReferenceType.LOCAL: - if self.local_store is None: - raise InvalidPackageReferenceError( - "Cannot resolve LOCAL reference: " - "no LocalPackageStore configured on PackageContentResolver" - ) - local_pkg = self.local_store.resolve_package(ref.name) - result: dict[str, Any] = { - **local_pkg.content, - "_original_reference": ref.original_reference, - "_package_id": local_pkg.package_id.id_string, - } - with self._lock: - self._put_cache(cache_key, result) - return copy.deepcopy(result) - - if ref.reference_type == ReferenceType.ID: - result = { - "id": ref.id_string, - "type": "id", - "_original_reference": ref.original_reference, - } - with self._lock: - self._put_cache(cache_key, result) - return copy.deepcopy(result) - - if ref.reference_type == ReferenceType.REGISTRY: - if not ref.server or not ref.namespace or not ref.name: - logger.warning("Incomplete registry reference: %s", ref) - return None - pkg_type = package_type or ref.package_type - if pkg_type is None: - logger.error( - "Cannot resolve REGISTRY reference %s without package_type; " - "provide package_type=... or set ref.package_type", - ref, - ) - return None - - resolved_type = _TEMPLATE_TYPE_TO_PACKAGE_TYPE.get(pkg_type, pkg_type) - - try: - asyncio.get_running_loop() - except RuntimeError: - return asyncio.run( - self._resolve_registry_async( - ref.server, - ref.namespace, - ref.name, - ref.version or "latest", - cache_key, - resolved_type, - ref.original_reference, - ) - ) - raise RuntimeError( - f"asyncio.run() cannot be called from a running event loop. " - f"Use await resolver.aresolve({ref!r}, package_type={pkg_type!r}) instead." - ) - - return None + strategy = self._STRATEGIES.get(ref.reference_type) + if strategy is None: + return None + return strategy.resolve(self, ref, package_type, cache_key) async def aresolve( self, @@ -288,60 +430,10 @@ class PackageContentResolver: self.cache.move_to_end(cache_key) return copy.deepcopy(self.cache[cache_key]) - if ref.reference_type == ReferenceType.LOCAL: - if self.local_store is None: - raise InvalidPackageReferenceError( - "Cannot resolve LOCAL reference: " - "no LocalPackageStore configured on PackageContentResolver" - ) - local_pkg = await asyncio.to_thread( - self.local_store.resolve_package, ref.name - ) - result: dict[str, Any] = { - **local_pkg.content, - "_original_reference": ref.original_reference, - "_package_id": local_pkg.package_id.id_string, - } - async with self._get_async_lock(): - self._put_cache(cache_key, result) - return copy.deepcopy(result) - - if ref.reference_type == ReferenceType.ID: - result = { - "id": ref.id_string, - "type": "id", - "_original_reference": ref.original_reference, - } - async with self._get_async_lock(): - self._put_cache(cache_key, result) - return copy.deepcopy(result) - - if ref.reference_type == ReferenceType.REGISTRY: - if not ref.server or not ref.namespace or not ref.name: - logger.warning("Incomplete registry reference: %s", ref) - return None - pkg_type = package_type or ref.package_type - if pkg_type is None: - logger.error( - "Cannot resolve REGISTRY reference %s without package_type; " - "provide package_type=... or set ref.package_type", - ref, - ) - return None - - resolved_type = _TEMPLATE_TYPE_TO_PACKAGE_TYPE.get(pkg_type, pkg_type) - - return await self._resolve_registry_async( - ref.server, - ref.namespace, - ref.name, - ref.version or "latest", - cache_key, - resolved_type, - ref.original_reference, - ) - - return None + strategy = self._STRATEGIES.get(ref.reference_type) + if strategy is None: + return None + return await strategy.aresolve(self, ref, package_type, cache_key) def _put_cache(self, key: str, value: dict[str, Any]) -> None: """Insert into cache, evicting oldest if at max size. diff --git a/src/cleveractors/runtime.py b/src/cleveractors/runtime.py index 50ae235..d914f33 100644 --- a/src/cleveractors/runtime.py +++ b/src/cleveractors/runtime.py @@ -14,7 +14,7 @@ into a clean, stable interface. from __future__ import annotations from collections.abc import AsyncGenerator -from typing import Any +from typing import TYPE_CHECKING, Any from cleveractors.agents.tool import ToolAgent from cleveractors.core.exceptions import ConfigurationError @@ -28,6 +28,9 @@ from cleveractors.runtime_dispatch import ( _execute_tool, ) +if TYPE_CHECKING: + from cleveractors.registry.local_store import LocalPackageStore + # Re-export so existing ``from cleveractors.runtime import ActorResult`` # imports continue to work without modification. __all__ = [ @@ -56,6 +59,12 @@ class Executor: Defaults to :class:`~cleveractors.agents.tool.ToolAgent`. Supply a subclass to override tool agent behaviour at runtime without monkey-patching (issue #73). + local_store (LocalPackageStore | None): Optional store used to + resolve ``local:`` / (already-canonicalized) ``ID:`` package + references on ``type: llm`` agents' ``skills`` field. When + ``None`` (the default), agents with a ``skills`` entry that + can only be resolved locally will fail agent creation with + ``AgentCreationError`` -- unchanged from prior behavior. """ def __init__( @@ -66,6 +75,7 @@ class Executor: pricing: dict[str, Any], *, tool_agent_class: type[ToolAgent] = ToolAgent, + local_store: LocalPackageStore | None = None, ): if not isinstance(config_dict, dict): raise ConfigurationError("config_dict must be a dict") @@ -83,6 +93,7 @@ class Executor: f"tool_agent_class must be a subclass of ToolAgent, " f"got {tool_agent_class!r}" ) + self.local_store = local_store self.config = config_dict self.credentials = credentials self.limits = limits @@ -253,6 +264,7 @@ def create_executor( pricing: dict[str, Any] | None = None, *, tool_agent_class: type[ToolAgent] = ToolAgent, + local_store: LocalPackageStore | None = None, ) -> Executor: """Construct an :class:`Executor` for the supplied actor configuration. @@ -269,6 +281,16 @@ def create_executor( path that constructs or registers a ``ToolAgent`` uses this class instead of the module-level default, removing the need for monkey-patching (issue #73). + local_store: Optional ``LocalPackageStore`` used to resolve any + ``skills:`` references on ``type: llm`` agents. Pass the same + store used to load *config_dict* itself (e.g. via + ``LocalPackageStore.resolve_package``) so that a ``skills:`` + entry originally written as ``local:`` -- which + canonicalization may have already rewritten to its ``ID:`` + form -- resolves against that store's Identity Map instead of + failing. When omitted, agents with unresolvable ``skills`` + entries fail agent creation with ``AgentCreationError``, + matching prior behavior. Returns: An :class:`Executor` instance. Call ``await executor.execute(message)`` @@ -280,4 +302,5 @@ def create_executor( limits=limits or {}, pricing=pricing or {}, tool_agent_class=tool_agent_class, + local_store=local_store, ) diff --git a/src/cleveractors/runtime_dispatch.py b/src/cleveractors/runtime_dispatch.py index 7063a6b..a99a6c7 100644 --- a/src/cleveractors/runtime_dispatch.py +++ b/src/cleveractors/runtime_dispatch.py @@ -33,6 +33,8 @@ from cleveractors.agents.llm import ( LLMAgent, last_token_usage_var, ) +from cleveractors.agents.skill_resolution import SkillReferenceResolver +from cleveractors.agents.skills import SkillLoader from cleveractors.core.exceptions import ( AgentCreationError, ConfigurationError, @@ -49,6 +51,27 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) + +def _build_skill_loader(executor: Executor) -> SkillLoader | None: + """Build a SkillLoader wired to ``executor.local_store``, when set. + + Every ``AgentFactory`` constructed by this module's dispatch functions + passes the result here as ``skill_loader=``, so a `type: llm` agent's + ``skills:`` references resolve against the same store the caller used + to load the Actor config itself (sharing its Identity Map -- see + ``LocalPackageStore.find_by_id`` / ``PackageContentResolver``'s + ``_IdReferenceStrategy``). Returns ``None`` when no local_store is + configured, so ``AgentFactory`` falls back to its own default + (storeless) ``SkillLoader`` -- unchanged behavior for every existing + caller of ``create_executor`` that doesn't pass ``local_store``. + """ + if executor.local_store is None: + return None + return SkillLoader( + resolver=SkillReferenceResolver(local_store=executor.local_store) + ) + + # Regex matching the graph-actor placeholder node_id format "<{name}:no_llm>". # Used in _execute_multi_actor to normalise sub-graph placeholders to the # canonical "" form before prefixing with the actor name (m7 of @@ -141,6 +164,7 @@ async def _execute_llm( credentials=executor.credentials, template_renderer=renderer, tool_agent_class=executor.tool_agent_class, + skill_loader=_build_skill_loader(executor), ) # Build context dict forwarding conversation history for multi-turn support. @@ -366,6 +390,7 @@ async def _execute_graph( credentials=executor.credentials, template_renderer=renderer, tool_agent_class=executor.tool_agent_class, + skill_loader=_build_skill_loader(executor), ) agents: dict[str, Any] = {} @@ -607,6 +632,7 @@ async def _execute_multi_actor( limits=executor.limits, pricing=executor.pricing, tool_agent_class=executor.tool_agent_class, + local_store=executor.local_store, ) result = await sub_executor.execute(message, messages=messages, state=state) @@ -824,6 +850,7 @@ async def _execute_llm_stream( credentials=executor.credentials, template_renderer=renderer, tool_agent_class=executor.tool_agent_class, + skill_loader=_build_skill_loader(executor), ) llm_context: dict[str, Any] | None = None @@ -1225,6 +1252,7 @@ async def _execute_graph_stream( credentials=executor.credentials, template_renderer=renderer, tool_agent_class=executor.tool_agent_class, + skill_loader=_build_skill_loader(executor), ) agents: dict[str, Any] = {}