From 68bd786f390c2dd51a8ac3bfb32cb7177254eb4d Mon Sep 17 00:00:00 2001 From: Junyan Qin Date: Wed, 20 May 2026 22:37:20 +0800 Subject: [PATCH] fix(skill): re-inject skill index into local-agent system prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contributor's original PR (#1917) appended an ``Available Skills`` index to the system prompt before the LLM saw the user message, so the LLM could decide whether to activate a skill. ``7145447b`` removed the text-marker activation flow and, together with it, the entire system prompt injection — but the Tool Call replacement only put the available skills inside the ``activate`` tool's description. In practice the LLM ignores tool descriptions for selection and goes straight to native tools, so user-visible skill activation silently broke. Restore the injection, adapted for the Tool Call era: - SkillManager regains ``get_skill_index(bound_skills)`` and ``build_skill_aware_prompt_addition(bound_skills)``. The addendum carries only ``name (display_name): description`` for each pipeline-visible skill plus one instruction line pointing at the ``activate`` tool. No SKILL.md contents — KV cache stays clean - PreProcessor appends the addendum to the first system message (or inserts a new one) of ``query.prompt.messages`` for the local-agent runner. Handles plain-string and ContentElement[] bodies. Skips cleanly when no skills are visible - 3 new test_preproc cases: injection happens, bound-skills subset honoured, empty addendum touches nothing. 280 passed Co-Authored-By: Claude Opus 4.7 (1M context) --- src/langbot/pkg/pipeline/preproc/preproc.py | 63 +++++++++++++++---- src/langbot/pkg/skill/manager.py | 47 ++++++++++++++ tests/unit_tests/test_preproc.py | 70 +++++++++++++++++++++ 3 files changed, 167 insertions(+), 13 deletions(-) diff --git a/src/langbot/pkg/pipeline/preproc/preproc.py b/src/langbot/pkg/pipeline/preproc/preproc.py index 9cbe9141..8aa15750 100644 --- a/src/langbot/pkg/pipeline/preproc/preproc.py +++ b/src/langbot/pkg/pipeline/preproc/preproc.py @@ -248,30 +248,67 @@ class PreProcessor(stage.PipelineStage): query.prompt.messages = event_ctx.event.default_prompt query.messages = event_ctx.event.prompt - # =========== Store bound skills for runtime visibility checks =========== - # Skills are now activated through the `activate` tool (Tool Call mechanism), - # not through system prompt injection. This aligns with Claude Code's design. + # =========== Skill awareness for the local-agent runner =========== + # The actual activation goes through the ``activate`` Tool Call so the + # LLM doesn't see full SKILL.md instructions until it commits to a + # skill (Claude Code's progressive disclosure). But the LLM still has + # to KNOW which skills exist to make that choice, so we: + # 1. resolve the pipeline's bound skills and stash them in + # ``query.variables['_pipeline_bound_skills']`` for downstream + # visibility checks (skill loader, native exec workdir); + # 2. inject a short ``Available Skills`` index (name + description + # only) into the system prompt. The contributor's original PR + # relied on this injection; without it the LLM never discovers + # the skills are there and just calls native tools instead. if selected_runner == 'local-agent' and self.ap.skill_mgr: - # Get bound skills from pipeline extensions_preferences pipeline_data = await self.ap.pipeline_service.get_pipeline(query.pipeline_uuid) extensions_prefs = (pipeline_data or {}).get('extensions_preferences', {}) enable_all_skills = extensions_prefs.get('enable_all_skills', True) if enable_all_skills: - bound_skills = None # None = all skills available + bound_skills = None # None = all loaded skills are visible else: - # Get specific bound skill names bound_skills = extensions_prefs.get('skills', []) - # Store bound skills in query variables for runtime path visibility checks query.variables['_pipeline_bound_skills'] = bound_skills - loaded_count = len(self.ap.skill_mgr.skills) - self.ap.logger.debug( - f'Skills available for activate tool: ' - f'pipeline={query.pipeline_uuid} ' - f'loaded_skills={loaded_count} ' - f'bound_skills={bound_skills or "all"}' + skill_addition = self.ap.skill_mgr.build_skill_aware_prompt_addition( + bound_skills=bound_skills, ) + if skill_addition: + # Append to the first system message; create one if the + # prompt has none. Handles both plain-string and + # content-element (list) message bodies. + if query.prompt.messages and query.prompt.messages[0].role == 'system': + head = query.prompt.messages[0] + if isinstance(head.content, str): + head.content = head.content + skill_addition + elif isinstance(head.content, list): + appended = False + for ce in head.content: + if getattr(ce, 'type', None) == 'text': + ce.text = (ce.text or '') + skill_addition + appended = True + break + if not appended: + head.content.append(provider_message.ContentElement(type='text', text=skill_addition)) + else: + query.prompt.messages.insert( + 0, + provider_message.Message(role='system', content=skill_addition.strip()), + ) + self.ap.logger.debug( + f'Skill index injected into system prompt: ' + f'pipeline={query.pipeline_uuid} ' + f'bound_skills={bound_skills or "all"} ' + f'loaded_skills={len(self.ap.skill_mgr.skills)}' + ) + else: + self.ap.logger.debug( + f'No skills available for prompt injection: ' + f'pipeline={query.pipeline_uuid} ' + f'loaded_skills={len(self.ap.skill_mgr.skills)} ' + f'bound_skills={bound_skills}' + ) return entities.StageProcessResult(result_type=entities.ResultType.CONTINUE, new_query=query) diff --git a/src/langbot/pkg/skill/manager.py b/src/langbot/pkg/skill/manager.py index 92503a64..a053697f 100644 --- a/src/langbot/pkg/skill/manager.py +++ b/src/langbot/pkg/skill/manager.py @@ -86,3 +86,50 @@ class SkillManager: def get_skill_by_name(self, name: str) -> dict | None: """Get skill data by name.""" return self.skills.get(name) + + def get_skill_index(self, bound_skills: list[str] | None = None) -> str: + """Render the pipeline-visible skills as a short ``name: description`` + index suitable for the system prompt. + + ``bound_skills`` follows the same convention as + ``query.variables['_pipeline_bound_skills']``: ``None`` means every + loaded skill is exposed; an explicit list filters to that subset. + Returns an empty string when no skills are visible. + """ + lines: list[str] = [] + for skill in self.skills.values(): + name = skill.get('name') + if not name: + continue + if bound_skills is not None and name not in bound_skills: + continue + display = skill.get('display_name') or name + description = (skill.get('description') or '').strip().replace('\n', ' ') + lines.append(f'- {name} ({display}): {description}') + + if not lines: + return '' + return 'Available Skills:\n' + '\n'.join(lines) + + def build_skill_aware_prompt_addition(self, bound_skills: list[str] | None = None) -> str: + """Build the system-prompt addendum that makes the LLM aware of the + pipeline-visible skills. + + Only metadata (name + description) is injected — the full SKILL.md is + loaded later via the ``activate`` Tool Call, protecting KV cache and + matching Claude Code's progressive disclosure pattern. Returns an + empty string when no skills are visible (no prompt change at all). + """ + skill_index = self.get_skill_index(bound_skills) + if not skill_index: + return '' + return ( + '\n\n' + f'{skill_index}\n\n' + "When the user's request clearly matches one or more skills " + 'based on their descriptions above, call the `activate` tool with ' + 'the skill name to load its full instructions. Only the name and ' + 'description are visible here; the actual instructions arrive as ' + 'the tool result. If no skill is a clear match, respond normally ' + 'without activating any skill.' + ) diff --git a/tests/unit_tests/test_preproc.py b/tests/unit_tests/test_preproc.py index a5d411d3..3164f35b 100644 --- a/tests/unit_tests/test_preproc.py +++ b/tests/unit_tests/test_preproc.py @@ -132,3 +132,73 @@ async def test_preproc_disables_skill_authoring_tools_when_skill_service_missing assert result.result_type == entities_module.ResultType.CONTINUE app.tool_mgr.get_all_tools.assert_awaited_once_with(None, None, include_skill_authoring=False) + + +@pytest.mark.asyncio +async def test_preproc_injects_skill_index_into_system_prompt(): + """The Tool Call activation pattern still needs the LLM to know which + skills exist. PreProcessor must append the SkillManager's index + addendum to the first system message.""" + preproc_module, entities_module = _import_preproc_modules() + + app = _make_app(skill_service=SimpleNamespace()) + addendum = '\n\nAvailable Skills:\n- demo (demo): Demo skill.\n\nCall activate ...' + app.skill_mgr.build_skill_aware_prompt_addition = Mock(return_value=addendum) + + query = _make_query() + result = await stage_process_capture(preproc_module, app, query) + + assert result.result_type == entities_module.ResultType.CONTINUE + app.skill_mgr.build_skill_aware_prompt_addition.assert_called_once_with(bound_skills=None) + head = query.prompt.messages[0] + assert head.role == 'system' + assert head.content.endswith(addendum) + + +@pytest.mark.asyncio +async def test_preproc_respects_pipeline_bound_skills_subset(): + """When ``enable_all_skills`` is false the bound list is passed through + so the addendum only mentions skills allowed for this pipeline.""" + preproc_module, entities_module = _import_preproc_modules() + + app = _make_app(skill_service=SimpleNamespace()) + app.pipeline_service.get_pipeline = AsyncMock( + return_value={ + 'extensions_preferences': { + 'enable_all_skills': False, + 'skills': ['only-this'], + } + } + ) + app.skill_mgr.build_skill_aware_prompt_addition = Mock(return_value='') + + query = _make_query() + result = await stage_process_capture(preproc_module, app, query) + + assert result.result_type == entities_module.ResultType.CONTINUE + app.skill_mgr.build_skill_aware_prompt_addition.assert_called_once_with(bound_skills=['only-this']) + assert query.variables.get('_pipeline_bound_skills') == ['only-this'] + + +@pytest.mark.asyncio +async def test_preproc_skips_injection_when_addendum_is_empty(): + """No visible skills → system prompt is left untouched (no + ``Available Skills`` block appended).""" + preproc_module, entities_module = _import_preproc_modules() + + app = _make_app(skill_service=SimpleNamespace()) + app.skill_mgr.build_skill_aware_prompt_addition = Mock(return_value='') + + query = _make_query() + result = await stage_process_capture(preproc_module, app, query) + + assert result.result_type == entities_module.ResultType.CONTINUE + if query.prompt and query.prompt.messages: + assert 'Available Skills' not in (query.prompt.messages[0].content or '') + + +async def stage_process_capture(preproc_module, app, query): + """Run PreProcessor.process and return the result while keeping ``query`` + accessible to the assertions (process mutates query in place).""" + stage = preproc_module.PreProcessor(app) + return await stage.process(query, 'PreProcessor')