From db01d7c1c873f59b656618bb23890bb518d4c0a3 Mon Sep 17 00:00:00 2001 From: Alessandro <155005371+3clyp50@users.noreply.github.com> Date: Tue, 16 Jun 2026 14:24:03 +0200 Subject: [PATCH] Persist loaded skill instructions in history Move explicit skill loads out of per-turn prompt extras and into normal tool-result history, with revision metadata for duplicate detection. Duplicate load calls now omit the full body only when the same revision remains model-visible after history output assembly. --- .../message_loop_prompts_after/AGENTS.md | 1 + .../_65_include_loaded_skills.py | 21 +--- prompts/agent.system.tool.skills.md | 2 +- tests/test_tool_action_contracts.py | 100 ++++++++++++++++++ tools/skills_tool.py | 81 ++++++++++++-- tools/skills_tool.py.dox.md | 9 +- 6 files changed, 187 insertions(+), 27 deletions(-) diff --git a/extensions/python/message_loop_prompts_after/AGENTS.md b/extensions/python/message_loop_prompts_after/AGENTS.md index 441540972..99b799f6c 100644 --- a/extensions/python/message_loop_prompts_after/AGENTS.md +++ b/extensions/python/message_loop_prompts_after/AGENTS.md @@ -13,6 +13,7 @@ - Keep injected content bounded and clearly attributed. - Preserve ordering where later prompt extras depend on earlier recall or load results. - Do not expose secrets or private files from workdir extras. +- Explicitly loaded skill instructions belong in normal tool-result history; this hook may recall candidate skills, but must not reinject loaded skill bodies through prompt extras every turn. ## Work Guidance diff --git a/extensions/python/message_loop_prompts_after/_65_include_loaded_skills.py b/extensions/python/message_loop_prompts_after/_65_include_loaded_skills.py index b3f88f988..261a6b611 100644 --- a/extensions/python/message_loop_prompts_after/_65_include_loaded_skills.py +++ b/extensions/python/message_loop_prompts_after/_65_include_loaded_skills.py @@ -9,30 +9,17 @@ class IncludeLoadedSkills(Extension): if not self.agent: return - extras = loop_data.extras_persistent - - # Get loaded skills names + loop_data.extras_persistent.pop("loaded_skills", None) skill_names = self.agent.data.get(DATA_NAME_LOADED_SKILLS) if not skill_names: return - # load skill text here - content = "" + # `skills_tool load` now appends full skill instructions as a normal + # tool-result history message. Keep this legacy ledger pruned, but do + # not reinject loaded skills through prompt extras every turn. visible_skill_names = [] for skill_name in skill_names: if not skills.find_skill(skill_name, agent=self.agent): continue visible_skill_names.append(skill_name) - skill_data = skills.load_skill_for_agent(skill_name=skill_name, agent=self.agent) - content += "\n\n" + skill_data self.agent.data[DATA_NAME_LOADED_SKILLS] = visible_skill_names - content = content.strip() - if not content: - return - - - # Inject into extras - extras["loaded_skills"] = self.agent.read_prompt( - "agent.system.skills.loaded.md", - skills=content, - ) diff --git a/prompts/agent.system.tool.skills.md b/prompts/agent.system.tool.skills.md index 22c1ba0b4..c9e5ad4f7 100644 --- a/prompts/agent.system.tool.skills.md +++ b/prompts/agent.system.tool.skills.md @@ -5,7 +5,7 @@ common args: action skill_name query file_path workflow: - action `search`: find candidate skills by keywords or trigger phrases from the current task - action `list`: discover available skills -- action `load`: load one skill by `skill_name` +- action `load`: append one skill's full instructions to chat history by `skill_name` - action `read_file`: open one file inside a loaded skill directory if the user says "find/search a skill", call `search` before `load` even when the likely skill name seems obvious `read_file` requires both `skill_name` and `file_path`; load the skill first, then read `SKILL.md` or the named relative file diff --git a/tests/test_tool_action_contracts.py b/tests/test_tool_action_contracts.py index 8bd638ecb..f19614b2d 100644 --- a/tests/test_tool_action_contracts.py +++ b/tests/test_tool_action_contracts.py @@ -38,6 +38,7 @@ class _FakeAgent: def __init__(self) -> None: self.data = {} self.context = types.SimpleNamespace(id="ctx") + self.history = types.SimpleNamespace(output=lambda: []) def read_prompt(self, _name: str, **kwargs) -> str: return f"deleted {kwargs.get('memory_count', 0)}" @@ -74,6 +75,9 @@ def _load_skills_tool(monkeypatch, skill_root: Path): skills_stub.list_skills = lambda *args, **kwargs: [fake_skill] skills_stub.search_skills = lambda *args, **kwargs: [fake_skill] skills_stub.find_skill = lambda *args, **kwargs: fake_skill + skills_stub.load_skill_for_agent = ( + lambda *args, **kwargs: "Skill: browser-form-workflows\n\nInstructions:\nUse labels before typing." + ) monkeypatch.setitem(sys.modules, "helpers.skills", skills_stub) print_style_stub = types.ModuleType("helpers.print_style") @@ -146,6 +150,102 @@ def test_skills_tool_accepts_action_alias_for_search(monkeypatch, tmp_path: Path assert "browser-form-workflows" in response.message +def test_skills_tool_load_appends_skill_instructions_as_tool_result( + monkeypatch, tmp_path: Path +): + module = _load_skills_tool(monkeypatch, tmp_path) + agent = _FakeAgent() + tool = module.SkillsTool( + agent, + "skills_tool", + None, + {"action": "load", "skill_name": "browser-form-workflows"}, + "", + None, + ) + + response = asyncio.run(tool.execute(**tool.args)) + + assert "Skill: browser-form-workflows" in response.message + assert response.additional["skill_instructions"]["name"] == "browser-form-workflows" + assert response.additional["skill_instructions"]["content_included"] is True + assert agent.data["loaded_skills"] == ["browser-form-workflows"] + + +def test_skills_tool_load_omits_duplicate_visible_skill_revision( + monkeypatch, tmp_path: Path +): + module = _load_skills_tool(monkeypatch, tmp_path) + agent = _FakeAgent() + tool = module.SkillsTool( + agent, + "skills_tool", + None, + {"action": "load", "skill_name": "browser-form-workflows"}, + "", + None, + ) + first = asyncio.run(tool.execute(**tool.args)) + loaded_message = { + "ai": False, + "content": {"skill_instructions": first.additional["skill_instructions"]}, + } + agent.history = types.SimpleNamespace(output=lambda: [loaded_message]) + + second = asyncio.run(tool.execute(**tool.args)) + + assert "already loaded in visible chat history" in second.message + assert "Instructions:\nUse labels before typing." not in second.message + assert second.additional["skill_instructions"]["content_included"] is False + assert second.additional["skill_instructions"]["already_loaded"] is True + assert agent.data["loaded_skills"] == ["browser-form-workflows"] + + +def test_skills_tool_load_reloads_when_prior_skill_is_not_model_visible( + monkeypatch, tmp_path: Path +): + module = _load_skills_tool(monkeypatch, tmp_path) + agent = _FakeAgent() + tool = module.SkillsTool( + agent, + "skills_tool", + None, + {"action": "load", "skill_name": "browser-form-workflows"}, + "", + None, + ) + first = asyncio.run(tool.execute(**tool.args)) + hidden_message = types.SimpleNamespace( + summary="", + content={"skill_instructions": first.additional["skill_instructions"]}, + ) + agent.history = types.SimpleNamespace( + all_messages=lambda: [hidden_message], + output=lambda: [ + { + "ai": False, + "content": "Earlier history was summarized and no skill body is visible.", + } + ], + ) + + second = asyncio.run(tool.execute(**tool.args)) + + assert "Skill: browser-form-workflows" in second.message + assert second.additional["skill_instructions"]["content_included"] is True + + +def test_loaded_skills_extension_no_longer_reinjects_skill_bodies(): + project_root = Path(__file__).resolve().parents[1] + extension = ( + project_root + / "extensions/python/message_loop_prompts_after/_65_include_loaded_skills.py" + ).read_text(encoding="utf-8") + + assert 'extras["loaded_skills"]' not in extension + assert "load_skill_for_agent" not in extension + + def test_skills_tool_read_file_action_reads_inside_skill_dir( monkeypatch, tmp_path: Path ): diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 3c566fa67..06fc20d15 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -1,5 +1,6 @@ from __future__ import annotations +import hashlib from pathlib import Path from typing import List @@ -111,7 +112,7 @@ class SkillsTool(Tool): skill_name = self._normalize_skill_name( str(kwargs.get("skill_name") or self.args.get("skill_name") or "") ) - return Response(message=self._load(skill_name), break_loop=False) + return self._load(skill_name) if action == "read_file": skill_name = self._normalize_skill_name( str(kwargs.get("skill_name") or self.args.get("skill_name") or "") @@ -185,11 +186,14 @@ class SkillsTool(Tool): ) return "\n".join(lines) - def _load(self, skill_name: str) -> str: + def _load(self, skill_name: str) -> Response: skill_name = self._normalize_skill_name(skill_name) if not skill_name: - return "Error: 'skill_name' is required for action=load." + return Response( + message="Error: 'skill_name' is required for action=load.", + break_loop=False, + ) # Verify skill exists skill = skills_helper.find_skill( @@ -198,9 +202,29 @@ class SkillsTool(Tool): agent=self.agent, ) if not skill: - return f"Error: skill not found: {skill_name!r}. Try skills_tool action=list or action=search." + return Response( + message=( + f"Error: skill not found: {skill_name!r}. " + "Try skills_tool action=list or action=search." + ), + break_loop=False, + ) - # Store skill name for fresh loading each turn + skill_data = skills_helper.load_skill_for_agent( + skill_name=skill.name, + agent=self.agent, + ) + revision = self._skill_revision(skill_data) + metadata = { + "name": skill.name, + "path": str(skill.path), + "revision": revision, + "source": "skills_tool:load", + "content_included": True, + } + + # Keep the old ledger for UI/backwards compatibility. The skill body now + # lives in normal tool-result history, not in per-turn prompt extras. if not self.agent.data.get(DATA_NAME_LOADED_SKILLS): self.agent.data[DATA_NAME_LOADED_SKILLS] = [] loaded = self.agent.data[DATA_NAME_LOADED_SKILLS] @@ -209,7 +233,52 @@ class SkillsTool(Tool): loaded.append(skill.name) self.agent.data[DATA_NAME_LOADED_SKILLS] = loaded[-max_loaded_skills():] - return f"Loaded skill '{skill.name}' into EXTRAS." + if self._visible_skill_revision_loaded(skill.name, revision): + return Response( + message=( + f"Skill '{skill.name}' is already loaded in visible " + "chat history for this revision." + ), + break_loop=False, + additional={ + "skill_instructions": { + **metadata, + "content_included": False, + "already_loaded": True, + } + }, + ) + + return Response( + message=skill_data, + break_loop=False, + additional={"skill_instructions": metadata}, + ) + + @staticmethod + def _skill_revision(skill_data: str) -> str: + return hashlib.sha256(skill_data.encode("utf-8")).hexdigest()[:16] + + def _visible_skill_revision_loaded(self, skill_name: str, revision: str) -> bool: + history_obj = getattr(self.agent, "history", None) + output = getattr(history_obj, "output", None) + if not callable(output): + return False + + for message in output(): + if not isinstance(message, dict): + continue + content = message.get("content") + if not isinstance(content, dict): + continue + meta = content.get("skill_instructions") + if not isinstance(meta, dict): + continue + if not meta.get("content_included"): + continue + if meta.get("name") == skill_name and meta.get("revision") == revision: + return True + return False def _read_file(self, skill_name: str, file_path: str) -> str: if not skill_name: diff --git a/tools/skills_tool.py.dox.md b/tools/skills_tool.py.dox.md index e93b91e50..35d014984 100644 --- a/tools/skills_tool.py.dox.md +++ b/tools/skills_tool.py.dox.md @@ -25,12 +25,15 @@ - Update this file whenever tool arguments, output shape, `break_loop` behavior, intervention handling, prompt instructions, or side effects change. - `SkillsTool` is a `Tool`. - `SkillsTool` defines `execute(...)`. -- Observed side-effect areas: filesystem reads, filesystem deletion, settings/state persistence. -- Imported dependency areas include: `__future__`, `helpers`, `helpers.print_style`, `helpers.tool`, `pathlib`, `typing`. +- `load` returns the full formatted skill instructions as the tool result so the instructions are appended once through normal message history instead of being reinjected through prompt extras. +- `load` includes a `skill_instructions` metadata sidecar in the tool-result content with skill name, path, revision, source, and whether full content was included. +- Duplicate `load` calls may omit the full body only when the same skill revision is still present in model-visible `history.output()` content. +- Observed side-effect areas: filesystem reads, filesystem deletion, settings/state persistence, chat history content. +- Imported dependency areas include: `__future__`, `hashlib`, `helpers`, `helpers.print_style`, `helpers.tool`, `pathlib`, `typing`. ## Key Concepts -- Important called helpers/classes observed in the source: `str.strip.lower.replace`, `skill_name.strip`, `super.get_log_object`, `self._normalize_skill_name`, `self.get_log_object`, `skills_helper.list_skills`, `join`, `skills_helper.search_skills`, `skills_helper.find_skill`, `skill.path.resolve`, `Path`, `resolved.read_text`, `skill_name.startswith`, `skill_name.endswith`, `self._current_action`, `self.agent.context.log.log`, `Response`, `strip`, `loaded.remove`, `target.is_absolute`. +- Important called helpers/classes observed in the source: `str.strip.lower.replace`, `skill_name.strip`, `super.get_log_object`, `self._normalize_skill_name`, `self.get_log_object`, `skills_helper.list_skills`, `join`, `skills_helper.search_skills`, `skills_helper.find_skill`, `skills_helper.load_skill_for_agent`, `hashlib.sha256`, `skill.path.resolve`, `Path`, `resolved.read_text`, `skill_name.startswith`, `skill_name.endswith`, `self._current_action`, `self.agent.context.log.log`, `Response`, `strip`, `loaded.remove`, `target.is_absolute`. - Keep request/response, tool, or helper semantics documented here at the same time as source changes. ## Work Guidance