feature/m3-agent-skills-loader #416

Closed
aditya wants to merge 4 commits from feature/m3-agent-skills-loader into master
Member

Description

Implements the Agent Skills Standard loader (M2.4a) for CleverAgents v3.
Adds AgentSkillLoader — a three-tier progressive disclosure engine that
parses SKILL.md files into structured tool definitions and registers them
in the Tool Registry with namespaced naming and read-only defaults.

Key additions:

  • SkillStep — structured step objects parsed from the frontmatter steps: list with stable 1-based indexing
  • AgentSkillSpec — full SKILL.md parser (frontmatter + Markdown body)
  • AgentSkillToolDescriptor — lightweight tool registry view with source="agent_skill", read_only=True, and read-only resource_slots
  • AgentSkillResourceSlot — binding slots for discovered scripts/, references/, assets/ directories
  • AgentSkillLoader — progressive disclosure lifecycle: discover()activate()list_resources()deactivate()
  • docs/reference/agent_skills.md — full reference covering folder layout, SKILL.md format, structured steps, resource binding slots, and tool mapping

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Documentation update
  • Test improvements

Quality Checklist

  • Code follows the project's coding standards (see CONTRIBUTING.md)
  • All public/protected methods have argument validation
  • Static typing is complete (no Any unless justified)
  • nox -s typecheck passes with no errors
  • nox -s lint passes with no errors
  • Unit tests written/updated (Behave scenarios in features/)
  • Integration tests written/updated (Robot suites in robot/) if applicable
  • Coverage remains above 85% (nox -s coverage_report)
  • No security issues introduced (nox -s security_scan)
  • No dead code introduced (nox -s dead_code)
  • Documentation updated if behavior changed

Testing

Unit tests follow the BDD-first approach defined in CONTRIBUTING.md.
50 Behave scenarios cover all public API paths with 100% branch and
statement coverage on agent_skills_loader.py. 9 Robot Framework
integration tests verify end-to-end behaviour against the sample skill
folder at examples/agent-skills/deploy-to-staging/.

Test Commands Run

nox -s lint             # Ruff lint + format — passed
nox -s typecheck        # Pyright static analysis — passed
nox -s security_scan    # Bandit — no issues
nox -s dead_code        # Vulture — no dead code
nox -s unit_tests       # 50 Behave scenarios — all passed
nox -s integration_tests # 9 Robot Framework tests — all passed
nox -s coverage_report  # 100% branch+statement on agent_skills_loader.py

Closes #160

Implementation Notes

Progressive disclosure follows the three-tier model from docs/specification.md § AgentSkillAdapter:

  • Tier 1 (discover()) — name + description only, ~50–100 tokens
  • Tier 2 (activate()) — full SKILL.md Markdown body loaded into active context
  • Tier 3 (list_resources()) — file paths from scripts/, references/, assets/ on demand

Structured steps are declared in the SKILL.md frontmatter as a YAML list under the steps: key. Each entry becomes a SkillStep(index, content) with a guaranteed stable 1-based index, enabling the agent to reference specific steps by number without relying on body text parsing.

Resource binding slots are emitted as AgentSkillResourceSlot entries in the tool descriptor — one per discovered sub-directory. All slots are unconditionally access="read_only" since Agent Skills may never write to their own resource directories.

Namespaced naming follows the namespace/short_name convention (ADR-002). Both segments must be alphanumeric with hyphens/underscores; violation raises a ValueError with an actionable message.

No existing behaviour was changed. All additions are net-new classes and functions inside src/cleveragents/skills/.

## Description Implements the Agent Skills Standard loader (`M2.4a`) for CleverAgents v3. Adds `AgentSkillLoader` — a three-tier progressive disclosure engine that parses `SKILL.md` files into structured tool definitions and registers them in the Tool Registry with namespaced naming and read-only defaults. Key additions: - `SkillStep` — structured step objects parsed from the frontmatter `steps:` list with stable 1-based indexing - `AgentSkillSpec` — full SKILL.md parser (frontmatter + Markdown body) - `AgentSkillToolDescriptor` — lightweight tool registry view with `source="agent_skill"`, `read_only=True`, and read-only `resource_slots` - `AgentSkillResourceSlot` — binding slots for discovered `scripts/`, `references/`, `assets/` directories - `AgentSkillLoader` — progressive disclosure lifecycle: `discover()` → `activate()` → `list_resources()` → `deactivate()` - `docs/reference/agent_skills.md` — full reference covering folder layout, SKILL.md format, structured steps, resource binding slots, and tool mapping ## Type of Change - [x] New feature (non-breaking change which adds functionality) - [x] Documentation update - [x] Test improvements ## Quality Checklist - [x] Code follows the project's coding standards (see CONTRIBUTING.md) - [x] All public/protected methods have argument validation - [x] Static typing is complete (no `Any` unless justified) - [x] `nox -s typecheck` passes with no errors - [x] `nox -s lint` passes with no errors - [x] Unit tests written/updated (Behave scenarios in `features/`) - [x] Integration tests written/updated (Robot suites in `robot/`) if applicable - [x] Coverage remains above 85% (`nox -s coverage_report`) - [x] No security issues introduced (`nox -s security_scan`) - [x] No dead code introduced (`nox -s dead_code`) - [x] Documentation updated if behavior changed ## Testing Unit tests follow the BDD-first approach defined in `CONTRIBUTING.md`. 50 Behave scenarios cover all public API paths with 100% branch and statement coverage on `agent_skills_loader.py`. 9 Robot Framework integration tests verify end-to-end behaviour against the sample skill folder at `examples/agent-skills/deploy-to-staging/`. ### Test Commands Run ```bash nox -s lint # Ruff lint + format — passed nox -s typecheck # Pyright static analysis — passed nox -s security_scan # Bandit — no issues nox -s dead_code # Vulture — no dead code nox -s unit_tests # 50 Behave scenarios — all passed nox -s integration_tests # 9 Robot Framework tests — all passed nox -s coverage_report # 100% branch+statement on agent_skills_loader.py ``` ## Related Issues Closes #160 ## Implementation Notes **Progressive disclosure** follows the three-tier model from `docs/specification.md § AgentSkillAdapter`: - Tier 1 (`discover()`) — name + description only, ~50–100 tokens - Tier 2 (`activate()`) — full SKILL.md Markdown body loaded into active context - Tier 3 (`list_resources()`) — file paths from `scripts/`, `references/`, `assets/` on demand **Structured steps** are declared in the SKILL.md frontmatter as a YAML list under the `steps:` key. Each entry becomes a `SkillStep(index, content)` with a guaranteed stable 1-based index, enabling the agent to reference specific steps by number without relying on body text parsing. **Resource binding slots** are emitted as `AgentSkillResourceSlot` entries in the tool descriptor — one per discovered sub-directory. All slots are unconditionally `access="read_only"` since Agent Skills may never write to their own resource directories. **Namespaced naming** follows the `namespace/short_name` convention (ADR-002). Both segments must be alphanumeric with hyphens/underscores; violation raises a `ValueError` with an actionable message. **No existing behaviour was changed.** All additions are net-new classes and functions inside `src/cleveragents/skills/`.
feat(skill): add MCP adapter for external tools
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 21s
CI / security (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 47s
CI / integration_tests (pull_request) Successful in 2m54s
CI / unit_tests (pull_request) Successful in 6m13s
CI / docker (pull_request) Successful in 39s
CI / benchmark-regression (pull_request) Successful in 19m58s
CI / coverage (pull_request) Failing after 22m0s
a6e56fe96e
Merge branch 'master' into develop-aditya-2
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
CI / build (pull_request) Successful in 17s
CI / quality (pull_request) Successful in 20s
CI / typecheck (pull_request) Successful in 47s
CI / security (pull_request) Successful in 50s
CI / integration_tests (pull_request) Successful in 3m41s
CI / unit_tests (pull_request) Successful in 7m9s
CI / docker (pull_request) Successful in 38s
CI / benchmark-regression (pull_request) Successful in 20m12s
CI / coverage (pull_request) Successful in 24m15s
80a56846fe
# Conflicts:
#	implementation_plan.md
feat(skill): add agent skills loader
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / quality (pull_request) Successful in 16s
CI / build (pull_request) Successful in 17s
CI / lint (pull_request) Successful in 21s
CI / typecheck (pull_request) Successful in 30s
CI / security (pull_request) Successful in 50s
CI / integration_tests (pull_request) Successful in 4m0s
CI / unit_tests (pull_request) Successful in 13m51s
CI / docker (pull_request) Successful in 1m23s
CI / benchmark-regression (pull_request) Successful in 21m50s
CI / coverage (pull_request) Successful in 48m58s
6267504006
Implement AgentSkillSpec loader that parses SKILL.md frontmatter and
progressive disclosure sections into structured SkillStep objects with
stable 1-based index ordering. Map Agent Skills to tool descriptors with
namespaced naming (namespace/short_name), read-only defaults
(read_only=True, writes=False), and read-only AgentSkillResourceSlot
entries for each discovered resource directory (scripts/, references/,
assets/). Add explicit validation for missing/invalid frontmatter fields
(name, description) and the optional steps list. Add
docs/reference/agent_skills.md describing folder layout, parsing rules,
step structure, resource binding slots, and tool mapping.

Tests: 50 Behave scenarios (100% branch+statement coverage on loader),
9 Robot Framework integration tests, ASV benchmarks for parsing
throughput and lifecycle performance.

ISSUES CLOSED: #160
brent.edwards left a comment

This passes all tests, though you will need to merge the file src/cleveragents/skills/__init__.py.

It has my approval.

This passes all tests, though you will need to merge the file `src/cleveragents/skills/__init__.py`. It has my approval.
freemo requested changes 2026-02-24 22:26:55 +00:00
Dismissed
freemo left a comment

Code Review: PR #416feature/m3-agent-skills-loader

The PR description is thorough and well-written, with clear quality checklist and implementation notes. The agent skills loader implementation itself looks solid. However, there are several structural and process issues that need to be addressed.

Critical Issues

1. No milestone assigned (CONTRIBUTING.md §11)
This PR is not assigned to any milestone. The linked issue #160 is on milestone v3.1.0 — this PR must be assigned to the same milestone. "A PR without a milestone will not be reviewed."

2. Multiple unrelated features bundled (CONTRIBUTING.md §2)
This PR contains 3 distinct features from 3 separate issues:

  • feat(skill): add agent skills loader — issue #160, branch should be feature/m3-agent-skills-loader
  • feat(skill): add MCP adapter for external tools — issue #159, branch should be feature/m3-mcp-adapter
  • feat(actor): extend hierarchical actor YAML schema and loader — no clear issue match

Per guidelines: "Each PR must be associated with a single Epic. Do not combine work from multiple unrelated Epics in one PR." These should be separate PRs, each scoped to a single feature/issue.

3. Not mergeable
The PR has merge conflicts with master that must be resolved before review can complete.

4. Missing commit issue references (CONTRIBUTING.md §4)
Only 1 of 3 feature commits (feat(skill): add agent skills loader) has an issue reference (ISSUES CLOSED: #160). The other two commits (feat(skill): add MCP adapter for external tools and feat(actor): extend hierarchical actor YAML schema and loader) have no issue reference in their footers.

5. No CHANGELOG update (CONTRIBUTING.md §6)
Three new features were added with no CHANGELOG entries.

6. No CONTRIBUTORS.md update (CONTRIBUTING.md §8)
Aditya Chhabra should be added to CONTRIBUTORS.md if not already listed.

Code Quality Issues

7. registry: Any in MCPToolAdapter.register_tools() (Type Safety)
The registry parameter is typed as Any instead of the concrete ToolRegistry type. Per CONTRIBUTING.md Type Safety guidelines: "never use inline comments or annotations to suppress individual type checking errors." If this is to avoid circular imports, use a TYPE_CHECKING forward reference:

from __future__ import annotations
from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from cleveragents.skills import ToolRegistry

8. Bare except Exception: in adapter.py:disconnect()
The disconnect method catches Exception without binding the variable. While exc_info=True logs the traceback, this is overly broad. Per guidelines: "Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic."

9. Broad exception handling in connect() and invoke()
adapter.py uses except Exception as exc catch-alls in both methods. Consider catching more specific exceptions.

What's Good

  • Excellent PR description with quality checklist
  • Proper Closes #160 issue linking
  • Comprehensive testing (50 Behave scenarios, 9 Robot tests, ASV benchmarks)
  • Good progressive disclosure architecture
  • Proper file placement in correct directories
  • Strong argument validation in public methods
  1. Split into 3 separate PRs (agent skills loader, MCP adapter, actor hierarchy) each with their own issue reference.
  2. Assign milestone v3.1.0.
  3. Add CHANGELOG and CONTRIBUTORS.md entries.
  4. Fix type annotation on registry parameter.
  5. Resolve merge conflicts.
## Code Review: PR #416 — `feature/m3-agent-skills-loader` The PR description is thorough and well-written, with clear quality checklist and implementation notes. The agent skills loader implementation itself looks solid. However, there are several structural and process issues that need to be addressed. ### Critical Issues **1. No milestone assigned (CONTRIBUTING.md §11)** This PR is not assigned to any milestone. The linked issue #160 is on milestone v3.1.0 — this PR must be assigned to the same milestone. "A PR without a milestone will not be reviewed." **2. Multiple unrelated features bundled (CONTRIBUTING.md §2)** This PR contains **3 distinct features** from 3 separate issues: - `feat(skill): add agent skills loader` — issue #160, branch should be `feature/m3-agent-skills-loader` - `feat(skill): add MCP adapter for external tools` — issue #159, branch should be `feature/m3-mcp-adapter` - `feat(actor): extend hierarchical actor YAML schema and loader` — no clear issue match Per guidelines: "Each PR must be associated with a single Epic. Do not combine work from multiple unrelated Epics in one PR." These should be separate PRs, each scoped to a single feature/issue. **3. Not mergeable** The PR has merge conflicts with `master` that must be resolved before review can complete. **4. Missing commit issue references (CONTRIBUTING.md §4)** Only 1 of 3 feature commits (`feat(skill): add agent skills loader`) has an issue reference (`ISSUES CLOSED: #160`). The other two commits (`feat(skill): add MCP adapter for external tools` and `feat(actor): extend hierarchical actor YAML schema and loader`) have no issue reference in their footers. **5. No CHANGELOG update (CONTRIBUTING.md §6)** Three new features were added with no CHANGELOG entries. **6. No CONTRIBUTORS.md update (CONTRIBUTING.md §8)** Aditya Chhabra should be added to CONTRIBUTORS.md if not already listed. ### Code Quality Issues **7. `registry: Any` in `MCPToolAdapter.register_tools()` (Type Safety)** The `registry` parameter is typed as `Any` instead of the concrete `ToolRegistry` type. Per CONTRIBUTING.md Type Safety guidelines: "never use inline comments or annotations to suppress individual type checking errors." If this is to avoid circular imports, use a `TYPE_CHECKING` forward reference: ```python from __future__ import annotations from typing import TYPE_CHECKING if TYPE_CHECKING: from cleveragents.skills import ToolRegistry ``` **8. Bare `except Exception:` in `adapter.py:disconnect()`** The disconnect method catches `Exception` without binding the variable. While `exc_info=True` logs the traceback, this is overly broad. Per guidelines: "Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic." **9. Broad exception handling in `connect()` and `invoke()`** `adapter.py` uses `except Exception as exc` catch-alls in both methods. Consider catching more specific exceptions. ### What's Good - Excellent PR description with quality checklist - Proper `Closes #160` issue linking - Comprehensive testing (50 Behave scenarios, 9 Robot tests, ASV benchmarks) - Good progressive disclosure architecture - Proper file placement in correct directories - Strong argument validation in public methods ### Recommended Path Forward 1. Split into 3 separate PRs (agent skills loader, MCP adapter, actor hierarchy) each with their own issue reference. 2. Assign milestone v3.1.0. 3. Add CHANGELOG and CONTRIBUTORS.md entries. 4. Fix type annotation on `registry` parameter. 5. Resolve merge conflicts.
freemo dismissed freemo's review 2026-02-24 22:45:34 +00:00
Reason:

dont want hard block

aditya closed this pull request 2026-02-25 14:24:32 +00:00
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / quality (pull_request) Successful in 16s
Required
Details
CI / build (pull_request) Successful in 17s
Required
Details
CI / lint (pull_request) Successful in 21s
Required
Details
CI / typecheck (pull_request) Successful in 30s
Required
Details
CI / security (pull_request) Successful in 50s
Required
Details
CI / integration_tests (pull_request) Successful in 4m0s
Required
Details
CI / unit_tests (pull_request) Successful in 13m51s
Required
Details
CI / docker (pull_request) Successful in 1m23s
Required
Details
CI / benchmark-regression (pull_request) Successful in 21m50s
CI / coverage (pull_request) Successful in 48m58s
Required
Details

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!416
No description provided.