Feature/skills - #16
Conversation
bgunnar5
left a comment
bgunnar5
left a comment
There was a problem hiding this comment.
We should add logging statements (debug and info for sure) to the code you've provided.
Please go through your code and flush out docstrings thoroughly.
We also need tests for the new skills files.
In the future we may want to port to Agent Framework's SkillsProvider class (see https://learn.microsoft.com/en-us/agent-framework/agents/skills?pivots=programming-language-python) to avoid having to manage all of the logic for resource/script discovery ourselves.
| Field Name Description Required? Default | ||
| ━━━━━━━━━━━━━━━ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━━━ ━━━━━━━━━ | ||
| skill_paths List of directories to search for skill folders No [] | ||
| containing SKILL.md. | ||
| ─────────────── ─────────────────────────────────────────────────────── ─────────── ───────── | ||
| skill_runtime Runtime settings for reading skill resources and No None | ||
| executing scripts. |
There was a problem hiding this comment.
this table here and below is not formatted properly. You can check how this will show up in the docs using mkdocs serve and then going to the URL that it provides.
| default_script_t Maximum runtime for a skill-owned No 30 | ||
| imeout_seconds script process. | ||
| ────────────────── ───────────────────────────────────── ─────────── ──────────────────────── | ||
| max_resource_byt Maximum size of a skill resource No implementation-defined |
| auto_approve_ski Whether all skill scripts may run No false | ||
| ll_scripts without approval. | ||
| ────────────────── ───────────────────────────────────── ─────────── ──────────────────────── | ||
| default_skill_sc Default approval mode for skill No prompt | ||
| ript_approval_mo scripts: prompt, approve, or deny. | ||
| de |
There was a problem hiding this comment.
help me understand the difference between these two settings. auto_approve_skill_script is only related to code execution while default_skill_script_approval_mode is related to prompts from the coding agent?
|
|
||
| ``` | ||
| ## (Optional) Skills Configuration |
There was a problem hiding this comment.
the blank line should go after the ```
|
|
||
| ``` | ||
| ## (Optional) Skills Configuration |
There was a problem hiding this comment.
everything in this section is indented when it shouldn't be
| def _resolve_skill_paths(skill_paths: List[str], config_file: str) -> List[Path]: | ||
| """ | ||
| Resolve configured skill discovery roots relative to the config file path. | ||
| """ | ||
| config_dir = Path(config_file).resolve().parent | ||
| resolved_paths = [] | ||
|
|
||
| for raw_path in skill_paths: | ||
| skill_path = Path(raw_path) | ||
| if not skill_path.is_absolute(): | ||
| skill_path = config_dir / skill_path | ||
| resolved_paths.append(skill_path.resolve()) | ||
|
|
||
| return resolved_paths |
There was a problem hiding this comment.
may want to just move this logic to the configuration class itself so any interface can use the Path versions of each skill file
| class CLISkillScriptApprover: | ||
| """Synchronous terminal approver for manifest skill script execution.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| input_func: Callable[[str], str] = input, | ||
| output_func: Callable[[str], None] = print, | ||
| ): | ||
| self.input_func = input_func | ||
| self.output_func = output_func | ||
|
|
||
| def approve_skill_script( | ||
| self, | ||
| request: SkillScriptApprovalRequest, | ||
| ) -> SkillScriptApprovalDecision: | ||
| args_display = " ".join(request.args) if request.args else "(none)" | ||
| self.output_func("") | ||
| self.output_func("Skill script approval required:") | ||
| self.output_func(f" Skill: {request.skill_name}") | ||
| self.output_func(f" Script: {request.script_name}") | ||
| self.output_func(f" Args: {args_display}") | ||
| self.output_func(f" Path: {Path(request.script_path)}") | ||
|
|
||
| response = self.input_func("Approve script execution? [y/N]: ").strip().lower() | ||
| if response in {"y", "yes"}: | ||
| return SkillScriptApprovalDecision( | ||
| approved=True, | ||
| reason="Skill script was approved by the CLI user.", | ||
| ) | ||
| return SkillScriptApprovalDecision( | ||
| approved=False, | ||
| reason="Skill script was denied by the CLI user.", | ||
| ) |
There was a problem hiding this comment.
I think this is logic that belongs in the skill approver class
| def _resolve_skill_paths(skill_paths: list[str], config_file: str) -> list[Path]: | ||
| """ | ||
| Resolve configured skill discovery roots relative to the config file path. | ||
| """ | ||
| config_dir = Path(config_file).resolve().parent | ||
| resolved_paths = [] | ||
|
|
||
| for raw_path in skill_paths: | ||
| skill_path = Path(raw_path) | ||
| if not skill_path.is_absolute(): | ||
| skill_path = config_dir / skill_path | ||
| resolved_paths.append(skill_path.resolve()) | ||
|
|
||
| return resolved_paths |
There was a problem hiding this comment.
related to my comment in the CLI interface, this should be moved to the config class
| def _initialize_skill_state(config: AppConfig, config_file: str): | ||
| """ | ||
| Discover manifest-based skills and build runtime tools for the UI. | ||
| """ | ||
| resolved_skill_paths = _resolve_skill_paths(config.skill_paths, config_file) | ||
| skill_registry = SkillRegistry.discover(resolved_skill_paths) | ||
| skill_runtime = SkillRuntime( | ||
| skill_registry, | ||
| config=config.skill_runtime_config, | ||
| script_approver=GradioPolicySkillScriptApprover(), | ||
| ) | ||
|
|
||
| skill_tools = [] | ||
| if skill_registry.has_skills_for_tool("load_skill"): | ||
| skill_tools.append(build_load_skill_tool(skill_runtime)) | ||
| if skill_registry.has_resources_for_tool("read_skill_resource"): | ||
| skill_tools.append(build_read_skill_resource_tool(skill_runtime)) | ||
| if skill_registry.has_scripts_for_tool("run_skill_script"): | ||
| skill_tools.append(build_run_skill_script_tool(skill_runtime)) | ||
|
|
||
| return skill_registry, skill_tools |
There was a problem hiding this comment.
identical logic in CLI interface. Condense to one location
| class GradioPolicySkillScriptApprover: | ||
| """Coarse app-level Gradio policy approver with explicit deny/approve modes.""" | ||
|
|
||
| def __init__(self, mode: str = "deny"): | ||
| normalized_mode = str(mode).strip().lower() | ||
| if normalized_mode not in {"deny", "approve"}: | ||
| raise ValueError("Gradio script approval mode must be 'deny' or 'approve'.") | ||
| self.mode = normalized_mode | ||
|
|
||
| def approve_skill_script( | ||
| self, | ||
| request: SkillScriptApprovalRequest, | ||
| ) -> SkillScriptApprovalDecision: | ||
| if self.mode == "approve": | ||
| return SkillScriptApprovalDecision( | ||
| approved=True, | ||
| reason=( | ||
| f"Skill script '{request.script_name}' for skill '{request.skill_name}' " | ||
| "was approved by the Gradio policy approver." | ||
| ), | ||
| ) | ||
|
|
||
| return SkillScriptApprovalDecision( | ||
| approved=False, | ||
| reason=( | ||
| f"Skill script '{request.script_name}' for skill '{request.skill_name}' " | ||
| "was denied by the Gradio policy approver." | ||
| ), | ||
| ) |
| } | ||
|
|
||
| ``` | ||
| ## (Optional) Skills Configuration |
There was a problem hiding this comment.
The rendered version of this file isn't formatted correctly. Maybe fixing the indents will fix it all.
| "skill_paths": [ | ||
| "./skills" | ||
| ], | ||
| "skill_runtime": { | ||
| "default_script_timeout_seconds": 30, | ||
| "max_resource_bytes": 65536, | ||
| "max_script_output_bytes": 32768, | ||
| "auto_approve_skill_scripts": false, | ||
| "default_skill_script_approval_mode": "prompt", | ||
| "skill_script_approval_modes": { | ||
| "documentation-helper": "approve", | ||
| "documentation-helper:scripts/publish.py": "deny" | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this at the top level of the configuration json or nested somewhere. Can you clarify?
| approval policy. | ||
|
|
||
|
|
||
| ## Full Example Configuration File |
There was a problem hiding this comment.
Do we have a full example config file with skills? Maybe we can add a generic one that tells the LLM to keep things simple, not write excess code, don't use one-off one-liner helper functions, etc....
| # Check skills configuration (optional) | ||
| if "skills" in config_dict: | ||
| raise ValueError( | ||
| "Top-level 'skills' is not supported. Use top-level 'skill_paths' for manifest-based skills." | ||
| ) | ||
|
|
There was a problem hiding this comment.
This goes with my comment on the configuration.md file. Can you clarify where the skills config goes in the configuration.json file?
| } | ||
|
|
||
|
|
||
| def _is_hidden_path(path: Path) -> bool: |
There was a problem hiding this comment.
Can we put this and other one-liner helper functions in-line if they are only used once?
| Your specialist agents (available as tools) can be delegated tasks. | ||
| """ |
There was a problem hiding this comment.
Revert formatting if nothing changed?
| Guidelines: | ||
| - Delegate to specialist agents when the request matches their expertise | ||
| - Answer directly only for questions about the system itself | ||
| - Avoid infinite loops between agents | ||
| - After receiving results, synthesize and respond to the user | ||
| """ |
There was a problem hiding this comment.
Revert formatting if nothing changed?
| cli = MADACLIInterface( | ||
| config, | ||
| skill_registry=skill_registry, | ||
| skill_tools=skill_tools, | ||
| ) |
There was a problem hiding this comment.
@bgunnar5 For some reason this PR isn't showing the latest? Can you try pulling develop again?
| skill_registry: SkillRegistry = None, | ||
| skill_tools: List[Any] = None, |
There was a problem hiding this comment.
@bgunnar5 For some reason this PR isn't showing the latest? Can you try pulling develop again?
| @@ -0,0 +1,39 @@ | |||
| """ | |||
There was a problem hiding this comment.
Can this and the CLI skill_approval.py files be combined somehow?
3 participants
Adds manifest-based skill support to MADA.
This PR introduces configuration and runtime support for discovering skills from skill_paths, advertising them to the planner as lightweight capabilities, and loading full skill content on demand. It also documents the configuration and user-facing skill behavior, including how load_skill, read_skill_resource, and run_skill_script fit into the flow.
Note: