fix(webhook): load URL-resolved profile's skills under multiplex
A `/p/<profile>/webhooks/<route>` request resolved the profile from the URL but ran the route script, prompt render and `skills:` lookup with no profile scope — the runner only enters `_profile_runtime_scope` later, around `handle_message` — so routed webhooks loaded the launch (default) profile's skills and logged "Skill not found" for the routed profile's own. - gateway/platforms/webhook.py: add `_profile_scope(profile)` (nullcontext when no prefix was resolved; `_profile_runtime_scope(get_profile_dir(p))` otherwise, same helper the runner uses) and wrap the script / render / skill-injection block in it. Bare routes are unchanged. - agent/skill_commands.py: `scan_skill_commands` scanned the import-time `SKILLS_DIR` (frozen to the launch home), so even a correctly scoped call listed default's skills; the #88023 home-keyed cache alone could not fix that. Use the call-time `_skills_dir()` there and at the two other SKILLS_DIR-relative sites in the module. - agent/skill_utils.py: `normalize_skill_lookup_name` used the same frozen root, so a routed profile's absolute skill_dir was rejected by `skill_view` ("must be a relative path within the skills directory"). Resolve against `_skills_dir()` — the root `skill_view` itself enforces. Fixes #67277 Co-authored-by: Juani Lezcano <tky.juani@gmail.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
This commit is contained in:
@@ -236,7 +236,7 @@ def _load_skill_payload(skill_identifier: str, task_id: str | None = None) -> tu
|
||||
return None
|
||||
|
||||
try:
|
||||
from tools.skills_tool import SKILLS_DIR, skill_view
|
||||
from tools.skills_tool import _skills_dir, skill_view
|
||||
from agent.skill_utils import normalize_skill_lookup_name
|
||||
|
||||
normalized = normalize_skill_lookup_name(raw_identifier)
|
||||
@@ -262,7 +262,7 @@ def _load_skill_payload(skill_identifier: str, task_id: str | None = None) -> tu
|
||||
skill_dir = Path(abs_skill_dir)
|
||||
elif skill_path:
|
||||
try:
|
||||
skill_dir = SKILLS_DIR / Path(skill_path).parent
|
||||
skill_dir = _skills_dir() / Path(skill_path).parent
|
||||
except Exception:
|
||||
skill_dir = None
|
||||
|
||||
@@ -317,7 +317,7 @@ def _build_skill_message(
|
||||
session_id: str | None = None,
|
||||
) -> str:
|
||||
"""Format a loaded skill into a user/system message payload."""
|
||||
from tools.skills_tool import SKILLS_DIR
|
||||
from tools.skills_tool import _skills_dir
|
||||
|
||||
content = str(loaded_skill.get("content") or "")
|
||||
|
||||
@@ -386,7 +386,7 @@ def _build_skill_message(
|
||||
|
||||
if supporting and skill_dir:
|
||||
try:
|
||||
skill_view_target = str(skill_dir.relative_to(SKILLS_DIR))
|
||||
skill_view_target = str(skill_dir.relative_to(_skills_dir()))
|
||||
except ValueError:
|
||||
# Skill is from an external dir — use the skill name instead
|
||||
skill_view_target = skill_dir.name
|
||||
@@ -441,7 +441,7 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]:
|
||||
# each naming the same skill as its own incumbent (#74574).
|
||||
commands: Dict[str, Dict[str, Any]] = {}
|
||||
try:
|
||||
from tools.skills_tool import SKILLS_DIR, _parse_frontmatter, skill_matches_platform, skill_matches_environment, _get_disabled_skill_names
|
||||
from tools.skills_tool import _skills_dir, _parse_frontmatter, skill_matches_platform, skill_matches_environment, _get_disabled_skill_names
|
||||
from agent.skill_utils import (
|
||||
get_external_skills_dirs,
|
||||
get_project_skills_dirs,
|
||||
@@ -456,8 +456,12 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]:
|
||||
# Project dirs iterate through the quarantine chokepoint.
|
||||
project_dirs = list(get_project_skills_dirs())
|
||||
dirs_to_scan = list(project_dirs)
|
||||
if SKILLS_DIR.exists():
|
||||
dirs_to_scan.append(SKILLS_DIR)
|
||||
# Resolve at call time: the import-time SKILLS_DIR is frozen to the
|
||||
# launch home, so a multiplexed profile scope (set_hermes_home_override)
|
||||
# would still scan the default profile's skills (#67277).
|
||||
skills_dir = _skills_dir()
|
||||
if skills_dir.exists():
|
||||
dirs_to_scan.append(skills_dir)
|
||||
dirs_to_scan.extend(get_external_skills_dirs())
|
||||
|
||||
for scan_dir in dirs_to_scan:
|
||||
|
||||
@@ -996,12 +996,15 @@ def normalize_skill_lookup_name(identifier: str) -> str:
|
||||
# Look the primary skills root up on tools.skills_tool at CALL time
|
||||
# (not via get_skills_dir()): callers and tests patch
|
||||
# ``tools.skills_tool.SKILLS_DIR`` and skill_view() itself resolves
|
||||
# against that module attribute, so normalization must agree with the
|
||||
# exact root skill_view() will enforce. Import deferred to avoid a
|
||||
# module cycle (tools.skills_tool imports agent.skill_utils).
|
||||
# against ``_skills_dir()`` — which honors that patch and otherwise
|
||||
# follows the live profile-scoped HERMES_HOME (the import-time
|
||||
# SKILLS_DIR is frozen to the launch home, #67277) — so normalization
|
||||
# must agree with the exact root skill_view() will enforce. Import
|
||||
# deferred to avoid a module cycle (tools.skills_tool imports
|
||||
# agent.skill_utils).
|
||||
try:
|
||||
from tools import skills_tool as _skills_tool
|
||||
primary_root = Path(_skills_tool.SKILLS_DIR)
|
||||
primary_root = _skills_tool._skills_dir()
|
||||
except Exception:
|
||||
primary_root = get_skills_dir()
|
||||
|
||||
|
||||
@@ -42,6 +42,7 @@ import subprocess
|
||||
import sys
|
||||
import time
|
||||
from collections import deque
|
||||
from contextlib import nullcontext
|
||||
from typing import Any, Deque, Dict, List, Optional
|
||||
|
||||
try:
|
||||
@@ -633,6 +634,21 @@ class WebhookAdapter(BasePlatformAdapter):
|
||||
effective_profile = request_profile or "default"
|
||||
return configured_profile == effective_profile
|
||||
|
||||
@staticmethod
|
||||
def _profile_scope(profile: Optional[str]):
|
||||
"""Enter the URL-resolved profile's runtime scope, or a no-op.
|
||||
|
||||
Only a resolved ``/p/<profile>/`` prefix enters a scope (same helper
|
||||
the runner wraps ``handle_message`` in); bare routes keep serving the
|
||||
launch profile exactly as before.
|
||||
"""
|
||||
if not profile or not isinstance(profile, str):
|
||||
return nullcontext()
|
||||
from gateway.run import _profile_runtime_scope
|
||||
from hermes_cli.profiles import get_profile_dir
|
||||
|
||||
return _profile_runtime_scope(get_profile_dir(profile))
|
||||
|
||||
async def _handle_webhook(self, request: "web.Request") -> "web.Response":
|
||||
"""POST /webhooks/{route_name} — receive and process a webhook event."""
|
||||
# Hot-reload dynamic subscriptions on each request (mtime-gated, cheap)
|
||||
@@ -784,63 +800,71 @@ class WebhookAdapter(BasePlatformAdapter):
|
||||
}
|
||||
)
|
||||
|
||||
if route_config.get("script"):
|
||||
# run_route_script shells out (subprocess.run, up to its timeout);
|
||||
# run it in a worker thread so it can't block the gateway event loop.
|
||||
keep, transformed_payload = await asyncio.to_thread(
|
||||
self._route_processor.run_route_script,
|
||||
route_config.get("script"),
|
||||
payload,
|
||||
# The route script, prompt render and skill lookup below read the
|
||||
# profile's home (skills/, config). The runner only enters the routed
|
||||
# profile's scope later, around handle_message, so without this they
|
||||
# ran against the launch (default) profile (#67277). Only a resolved
|
||||
# /p/<profile>/ enters a scope; bare routes are unchanged.
|
||||
with self._profile_scope(profile):
|
||||
if route_config.get("script"):
|
||||
# run_route_script shells out (subprocess.run, up to its
|
||||
# timeout); run it in a worker thread so it can't block the
|
||||
# gateway event loop. to_thread copies the contextvars, so
|
||||
# the profile scope follows it.
|
||||
keep, transformed_payload = await asyncio.to_thread(
|
||||
self._route_processor.run_route_script,
|
||||
route_config.get("script"),
|
||||
payload,
|
||||
)
|
||||
if not keep:
|
||||
logger.info(
|
||||
"[webhook] script ignored event=%s route=%s",
|
||||
event_type,
|
||||
route_name,
|
||||
)
|
||||
return web.json_response(
|
||||
{
|
||||
"status": "ignored",
|
||||
"reason": "script",
|
||||
"route": route_name,
|
||||
}
|
||||
)
|
||||
payload = transformed_payload or payload
|
||||
|
||||
# Format prompt from template
|
||||
prompt_template = route_config.get("prompt", "")
|
||||
prompt = self._render_prompt(
|
||||
prompt_template, payload, event_type, route_name
|
||||
)
|
||||
if not keep:
|
||||
logger.info(
|
||||
"[webhook] script ignored event=%s route=%s",
|
||||
event_type,
|
||||
route_name,
|
||||
)
|
||||
return web.json_response(
|
||||
{
|
||||
"status": "ignored",
|
||||
"reason": "script",
|
||||
"route": route_name,
|
||||
}
|
||||
)
|
||||
payload = transformed_payload or payload
|
||||
|
||||
# Format prompt from template
|
||||
prompt_template = route_config.get("prompt", "")
|
||||
prompt = self._render_prompt(
|
||||
prompt_template, payload, event_type, route_name
|
||||
)
|
||||
# Inject skill content if configured.
|
||||
# We call build_skill_invocation_message() directly rather than
|
||||
# using /skill-name slash commands — the gateway's command parser
|
||||
# would intercept those and break the flow.
|
||||
skills = route_config.get("skills", [])
|
||||
if skills:
|
||||
try:
|
||||
from agent.skill_commands import (
|
||||
build_skill_invocation_message,
|
||||
get_skill_commands,
|
||||
)
|
||||
|
||||
# Inject skill content if configured.
|
||||
# We call build_skill_invocation_message() directly rather than
|
||||
# using /skill-name slash commands — the gateway's command parser
|
||||
# would intercept those and break the flow.
|
||||
skills = route_config.get("skills", [])
|
||||
if skills:
|
||||
try:
|
||||
from agent.skill_commands import (
|
||||
build_skill_invocation_message,
|
||||
get_skill_commands,
|
||||
)
|
||||
|
||||
skill_cmds = get_skill_commands()
|
||||
for skill_name in skills:
|
||||
cmd_key = f"/{skill_name}"
|
||||
if cmd_key in skill_cmds:
|
||||
skill_content = build_skill_invocation_message(
|
||||
cmd_key, user_instruction=prompt
|
||||
)
|
||||
if skill_content:
|
||||
prompt = skill_content
|
||||
break # Load the first matching skill
|
||||
else:
|
||||
logger.warning(
|
||||
"[webhook] Skill '%s' not found", skill_name
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning("[webhook] Skill loading failed: %s", e)
|
||||
skill_cmds = get_skill_commands()
|
||||
for skill_name in skills:
|
||||
cmd_key = f"/{skill_name}"
|
||||
if cmd_key in skill_cmds:
|
||||
skill_content = build_skill_invocation_message(
|
||||
cmd_key, user_instruction=prompt
|
||||
)
|
||||
if skill_content:
|
||||
prompt = skill_content
|
||||
break # Load the first matching skill
|
||||
else:
|
||||
logger.warning(
|
||||
"[webhook] Skill '%s' not found", skill_name
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning("[webhook] Skill loading failed: %s", e)
|
||||
|
||||
# Build a unique delivery ID
|
||||
delivery_id = request.headers.get(
|
||||
|
||||
@@ -255,6 +255,41 @@ class TestScanSkillCommands:
|
||||
assert "/b-only" in profile_b_commands
|
||||
assert "/a-only" not in profile_b_commands
|
||||
|
||||
def test_get_skill_commands_scans_profile_skills_dir_not_frozen_import_dir(self, tmp_path):
|
||||
"""Under a profile home override the scan must read <profile>/skills/,
|
||||
not the launch home's import-time ``SKILLS_DIR`` (#67277): a
|
||||
multiplexed webhook routed to profile B otherwise sees default's skills.
|
||||
Deliberately does NOT patch ``tools.skills_tool.SKILLS_DIR``.
|
||||
"""
|
||||
import agent.skill_commands as sc_mod
|
||||
from agent.skill_commands import build_skill_invocation_message, get_skill_commands
|
||||
from hermes_constants import reset_hermes_home_override, set_hermes_home_override
|
||||
|
||||
profile_b = tmp_path / "profiles" / "b"
|
||||
_make_skill(profile_b / "skills", "b-only", body="Body of b-only.")
|
||||
(profile_b / "config.yaml").write_text("{}\n")
|
||||
|
||||
with (
|
||||
patch.object(sc_mod, "_skill_commands", {}),
|
||||
patch.object(sc_mod, "_skill_commands_platform", None),
|
||||
patch.object(sc_mod, "_skill_commands_home", None),
|
||||
):
|
||||
token = set_hermes_home_override(profile_b)
|
||||
try:
|
||||
commands = dict(get_skill_commands())
|
||||
assert "/b-only" in commands
|
||||
# Frozen SKILLS_DIR (the launch home) must not leak in.
|
||||
launch_dir = str(skills_tool_module._SKILLS_DIR_AT_IMPORT)
|
||||
assert not any(
|
||||
info["skill_dir"].startswith(launch_dir) for info in commands.values()
|
||||
)
|
||||
# And the absolute skill_dir round-trips through skill_view
|
||||
# (normalize_skill_lookup_name must use the same live root).
|
||||
msg = build_skill_invocation_message("/b-only", user_instruction="go")
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
assert msg is not None and "Body of b-only." in msg
|
||||
|
||||
def test_get_skill_commands_rescans_when_leaving_platform_scope(self, tmp_path, monkeypatch):
|
||||
"""Returning to no-platform-scope (CLI / cron / RL) after a gateway
|
||||
session must rescan so the unfiltered view is repopulated (#14536).
|
||||
|
||||
@@ -1011,6 +1011,63 @@ class TestMultiplexProfileWebhookAuthentication:
|
||||
)
|
||||
assert default_profile.status == 404
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_routed_profile_skills_resolve_under_that_profile(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
"""A /p/<profile>/ route's ``skills:`` must load from that profile's
|
||||
skills/ dir (#67277). Before the fix the lookup ran with no profile
|
||||
scope, so it scanned the launch profile and logged "Skill not found".
|
||||
"""
|
||||
import agent.skill_commands as sc_mod
|
||||
|
||||
worker = tmp_path / "profiles" / "worker"
|
||||
skill_dir = worker / "skills" / "worker-only"
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: worker-only\ndescription: w\n---\n\nBody of worker-only.\n"
|
||||
)
|
||||
(worker / "config.yaml").write_text("{}\n")
|
||||
(worker / ".env").write_text("")
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.profiles.get_profile_dir", lambda name: tmp_path / "profiles" / name
|
||||
)
|
||||
route_secret = "worker-route-secret-abc123"
|
||||
adapter = _make_adapter(
|
||||
routes={
|
||||
"gh": {
|
||||
"profile": "worker",
|
||||
"secret": route_secret,
|
||||
"prompt": "PR: {action}",
|
||||
"skills": ["worker-only"],
|
||||
}
|
||||
},
|
||||
host="127.0.0.1",
|
||||
)
|
||||
self._configure_profiles(adapter, tmp_path, monkeypatch)
|
||||
seen = []
|
||||
|
||||
async def _capture(event):
|
||||
seen.append(event)
|
||||
|
||||
adapter.handle_message = _capture
|
||||
body = b'{"action":"opened"}'
|
||||
headers = {
|
||||
"Content-Type": "application/json",
|
||||
"X-Hub-Signature-256": _github_signature(body, route_secret),
|
||||
}
|
||||
with (
|
||||
patch.object(sc_mod, "_skill_commands", {}),
|
||||
patch.object(sc_mod, "_skill_commands_home", None),
|
||||
):
|
||||
async with TestClient(TestServer(self._app(adapter))) as cli:
|
||||
resp = await cli.post("/p/worker/webhooks/gh", data=body, headers=headers)
|
||||
assert resp.status == 202
|
||||
await asyncio.sleep(0.05)
|
||||
assert len(seen) == 1
|
||||
assert seen[0].source.profile == "worker"
|
||||
assert "Body of worker-only." in seen[0].text
|
||||
|
||||
|
||||
def test_route_profile_validation_fails_closed():
|
||||
assert WebhookAdapter._route_allows_profile({}, None) is True
|
||||
|
||||
Reference in New Issue
Block a user