Simplify skill content return and revise usage instructions for resources laoading - #83
Simplify skill content return and revise usage instructions for resources laoading#83bm23btech11013-hash wants to merge 2 commits into
Conversation
Removed resource listing from skill content return. Signed-off-by: Atharva Kulkarni <bm23btech11013@iith.ac.in>
Updated skill usage instructions and formatting in registry.py. Signed-off-by: Atharva Kulkarni <bm23btech11013@iith.ac.in>
There was a problem hiding this comment.
Pull request overview
This PR updates the skills UX surfaced to the LLM by (1) revising the “available skills” prompt content to include clearer activation instructions and a new skills listing format, and (2) simplifying set_skill() output by no longer appending a resources list.
Changes:
- Reworked
SkillsRegistry.build_trigger_table()output into step-by-step usage guidance plus a trigger-based skills listing. - Simplified
set_skill()to return only the skill header + content (no “Available Resources” appendix).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
agentflow/skills/registry.py |
Changes the injected skills prompt text and the per-skill listing format (now trigger-focused). |
agentflow/skills/activation.py |
Changes the set_skill() return contract by removing the appended resources listing/instructions. |
| lines.append(f"| `{meta.name}` | {safe_desc} |") | ||
| # Skill header with triggers | ||
| max_triggers_display = 4 | ||
| triggers_str = ", ".join(f'"{t}"' for t in meta.triggers[:max_triggers_display]) |
There was a problem hiding this comment.
meta.triggers values can include | or newlines (see existing tests), but the new rendering no longer escapes/sanitizes them. This can break the markdown formatting of the injected system prompt and will fail the current sanitization expectations. Consider applying the same escaping as before (replace | with \\|, \n with spaces) per-trigger before joining.
| triggers_str = ", ".join(f'"{t}"' for t in meta.triggers[:max_triggers_display]) | |
| display_triggers = [] | |
| for t in meta.triggers[:max_triggers_display]: | |
| # Escape characters that can break markdown formatting. | |
| sanitized = t.replace("\n", " ").replace("|", "\\|") | |
| display_triggers.append(f'"{sanitized}"') | |
| triggers_str = ", ".join(display_triggers) |
| triggers_str = ", ".join(f'"{t}"' for t in meta.triggers[:max_triggers_display]) | ||
| if len(meta.triggers) > max_triggers_display: | ||
| triggers_str += f" (+{len(meta.triggers) - max_triggers_display} more)" |
There was a problem hiding this comment.
If a skill has no triggers (meta.triggers is empty), the output becomes triggers: with nothing after it. Previously the table fell back to using meta.description in that case, which provides actionable guidance and is covered by existing tests. Please add a fallback (e.g., show the description or a clear "no triggers" marker).
| triggers_str = ", ".join(f'"{t}"' for t in meta.triggers[:max_triggers_display]) | |
| if len(meta.triggers) > max_triggers_display: | |
| triggers_str += f" (+{len(meta.triggers) - max_triggers_display} more)" | |
| if meta.triggers: | |
| triggers_str = ", ".join( | |
| f'"{t}"' for t in meta.triggers[:max_triggers_display] | |
| ) | |
| if len(meta.triggers) > max_triggers_display: | |
| triggers_str += f" (+{len(meta.triggers) - max_triggers_display} more)" | |
| else: | |
| # Fallback: when no triggers are defined, use the skill description | |
| # (previous behavior, and covered by existing tests). | |
| triggers_str = meta.description or "no triggers" |
| "3. If you need a specific resource mentioned in the skill, " | ||
| "call `set_skill(skill_name, resource_name)`\n", | ||
| "4. Then provide your answer using the loaded content\n", | ||
| "### Skills & Resources\n", |
There was a problem hiding this comment.
The section header says "### Skills & Resources", but the list that follows only includes triggers and doesn't mention resources (names or counts). Either include resource info here or rename the header to avoid misleading prompt content.
| "### Skills & Resources\n", | |
| "### Skills\n", |
| content = registry.load_content(skill_name, hot_reload=hot_reload) | ||
| if not content: | ||
| return f"ERROR: Skill '{skill_name}' found but content could not be loaded." | ||
|
|
||
| header = skill_name.upper().replace("-", " ") | ||
| result = f"## SKILL: {header}\n\n{content}" | ||
|
|
||
| # List available resources (if any) | ||
| if meta.resources: | ||
| resource_list = "\n".join(f" - {r}" for r in meta.resources) | ||
| result += ( | ||
| f"\n\n---\n### Available Resources\n" | ||
| f"This skill has reference documents available. " | ||
| f'Call `set_skill("{skill_name}", "<resource_name>")` to load any you need:\n' | ||
| f"{resource_list}" | ||
| ) | ||
|
|
||
| return result | ||
| return f"## SKILL: {header}\n\n{content}" | ||
|
|
There was a problem hiding this comment.
This change removes the "Available Resources" section from the skill output, but the existing test suite asserts that set_skill(name) includes a resource list (e.g., tests/test_skills.py::test_skill_content_lists_resources). Update the tests (and any dependent docs) to match the new contract, or provide an alternative way for callers/LLMs to discover resource names.
|
This pr is replaced by #86 |
This pull request updates how skill information and instructions are presented to users. The changes focus on improving the clarity and usability of skill activation and documentation, especially regarding how to use skills and their resources.
Improvements to skill usage instructions and documentation:
build_trigger_tablemethod ofregistry.pyto provide a clearer, step-by-step guide on how to activate skills and access additional resources, and restructured the skills listing for better readability.set_skillinactivation.pyby removing the appended list of available resources and related instructions, making the returned content more concise.