From 7f5bf641bb4de42cd175f69bb9040147524a3b9f Mon Sep 17 00:00:00 2001 From: Spherrrical Date: Wed, 13 May 2026 15:44:16 -0700 Subject: [PATCH 1/2] feat(skills): add Agent Skills support with orchestrator-driven activation --- cli/planoai/config_generator.py | 137 +++++ cli/planoai/init_cmd.py | 6 + cli/planoai/main.py | 2 + cli/planoai/skills.py | 420 ++++++++++++++++ cli/planoai/skills_cmd.py | 471 ++++++++++++++++++ cli/planoai/templates/skills_routing.yaml | 71 +++ cli/planoai/templates/template_sync_map.yaml | 5 + cli/test/test_skills.py | 394 +++++++++++++++ cli/test/test_skills_cmd.py | 235 +++++++++ config/plano_config_schema.yaml | 57 +++ .../src/handlers/agents/selector.rs | 10 +- crates/brightstaff/src/handlers/llm/mod.rs | 62 ++- .../src/handlers/llm/model_selection.rs | 78 ++- crates/brightstaff/src/main.rs | 3 +- crates/brightstaff/src/router/orchestrator.rs | 148 +++++- .../src/router/orchestrator_model.rs | 25 +- .../src/router/orchestrator_model_v1.rs | 246 +++++++-- crates/brightstaff/src/router/stress_tests.rs | 2 + crates/common/src/configuration.rs | 86 ++++ crates/common/src/lib.rs | 1 + crates/common/src/skills_runtime.rs | 215 ++++++++ crates/prompt_gateway/src/stream_context.rs | 22 +- docs/source/index.rst | 1 + docs/source/resources/skills.rst | 177 +++++++ 24 files changed, 2777 insertions(+), 97 deletions(-) create mode 100644 cli/planoai/skills.py create mode 100644 cli/planoai/skills_cmd.py create mode 100644 cli/planoai/templates/skills_routing.yaml create mode 100644 cli/test/test_skills.py create mode 100644 cli/test/test_skills_cmd.py create mode 100644 crates/common/src/skills_runtime.rs create mode 100644 docs/source/resources/skills.rst diff --git a/cli/planoai/config_generator.py b/cli/planoai/config_generator.py index cb07767e0..7c9e8b951 100644 --- a/cli/planoai/config_generator.py +++ b/cli/planoai/config_generator.py @@ -1,6 +1,7 @@ import json import os import uuid +from pathlib import Path from planoai.utils import convert_legacy_listeners from jinja2 import Environment, FileSystemLoader import yaml @@ -8,6 +9,14 @@ from urllib.parse import urlparse from copy import deepcopy from planoai.consts import DEFAULT_OTEL_TRACING_GRPC_ENDPOINT +from planoai.skills import ( + MAX_CATALOG_BYTES, + Skill, + discover_skills, + find_project_root, + is_project_trusted, + total_catalog_size, +) SUPPORTED_PROVIDERS_WITH_BASE_URL = [ "azure_openai", @@ -162,6 +171,127 @@ def _version_tuple(version_string): return tuple(out) +def materialize_skills_in_config(config_yaml: dict, project_root: Path) -> None: + """Discover and inline Agent Skills referenced by `config_yaml`. + + Mutates `config_yaml` in place. The user's source config may declare + `skills:` as a list of strings (skill names) or omit it entirely. After + this call, `config_yaml["skills"]` is either absent or a list of fully + materialized objects with `name`, `description`, `path`, `body`, etc. + + Project-scope skills under `/.plano/skills/` are only loaded + when the project has been marked trusted via `planoai skills trust`. + + Per-route `routing_preferences[].skills` allow-lists are preserved as-is + so brightstaff can scope the catalog when that route is selected. + """ + requested = config_yaml.get("skills") + user_only = not is_project_trusted(project_root) + + discovered, diagnostics = discover_skills( + project_root=project_root, include_user_scope=True + ) + for diag in diagnostics: + prefix = "error" if diag.severity == "error" else "warning" + print(f"[skills] {prefix}: {diag.path}: {diag.message}") + + if user_only: + project_skills = [s for s in discovered if s.scope == "project"] + if project_skills: + print( + "[skills] note: project-scope skills are present but the project is " + "not trusted yet; run `planoai skills trust` to enable them." + ) + # Keep all non-project scopes (user + agents) — both are user-tier and + # auto-trusted, so they always load regardless of project trust state. + discovered = [s for s in discovered if s.scope != "project"] + + skills_by_name: dict[str, Skill] = {s.name: s for s in discovered} + + if requested is None: + # Default: auto-include every discovered skill. + selected: list[Skill] = list(discovered) + else: + if not isinstance(requested, list): + raise Exception("`skills:` must be a list of strings or skill objects") + selected = [] + seen: set[str] = set() + for entry in requested: + if isinstance(entry, str): + name = entry + elif isinstance(entry, dict): + name = entry.get("name") + if not isinstance(name, str): + raise Exception( + "skill entries with object form must include a string `name`" + ) + else: + raise Exception( + f"unsupported entry in `skills:` (expected str or mapping, got {type(entry).__name__})" + ) + if name in seen: + continue + seen.add(name) + skill = skills_by_name.get(name) + if skill is None: + print( + f"[skills] warning: skill '{name}' is declared in config but no " + f"SKILL.md was discovered under .plano/skills/ or ~/.plano/skills/" + ) + continue + selected.append(skill) + + if not selected: + config_yaml.pop("skills", None) + _strip_unknown_route_skills(config_yaml, set()) + return + + catalog_bytes = total_catalog_size(selected) + if catalog_bytes > MAX_CATALOG_BYTES: + print( + f"[skills] warning: skill catalog size is {catalog_bytes} bytes, " + f"above the recommended cap of {MAX_CATALOG_BYTES}. Consider trimming " + f"`routing_preferences[].skills` to the smallest useful set per route." + ) + + config_yaml["skills"] = [s.to_dict() for s in selected] + _strip_unknown_route_skills(config_yaml, {s.name for s in selected}) + + +def _strip_unknown_route_skills(config_yaml: dict, known: set) -> None: + """Drop unknown skill names from `routing_preferences[*].skills` allow-lists. + + The orchestrator only ever sees skills referenced under some + `routing_preferences[].skills`; an unknown name there would render the + `` block with a stale entry the runtime can't resolve, so filter + them out here with a warning instead. + """ + routes = config_yaml.get("routing_preferences") + if not isinstance(routes, list): + return + for route in routes: + if not isinstance(route, dict): + continue + allow = route.get("skills") + if not isinstance(allow, list): + continue + filtered = [] + for name in allow: + if not isinstance(name, str): + continue + if name in known: + filtered.append(name) + else: + print( + f"[skills] warning: routing_preference '{route.get('name')}' " + f"references unknown skill '{name}'; dropping from allow-list." + ) + if filtered: + route["skills"] = filtered + else: + route.pop("skills", None) + + def validate_and_render_schema(): ENVOY_CONFIG_TEMPLATE_FILE = os.getenv( "ENVOY_CONFIG_TEMPLATE_FILE", "envoy.template.yaml" @@ -196,6 +326,13 @@ def validate_and_render_schema(): _ = yaml.safe_load(plano_config_schema) inferred_clusters = {} + # Materialize Agent Skills before further processing so the rest of the + # pipeline (Jinja2 envoy template, dump to plano_config_rendered.yaml) sees + # the inlined body / description / path. + plano_config_path = Path(PLANO_CONFIG_FILE).resolve() + project_root = find_project_root(plano_config_path.parent) + materialize_skills_in_config(config_yaml, project_root) + # Convert legacy llm_providers to model_providers if "llm_providers" in config_yaml: if "model_providers" in config_yaml: diff --git a/cli/planoai/init_cmd.py b/cli/planoai/init_cmd.py index 66cb5222e..a4b77ccad 100644 --- a/cli/planoai/init_cmd.py +++ b/cli/planoai/init_cmd.py @@ -64,6 +64,12 @@ def _load_template_yaml(filename: str) -> str: description="stateful responses with memory-backed storage", yaml_text=_load_template_yaml("conversational_state_v1_responses.yaml"), ), + Template( + id="skills_routing", + title="Agent Skills Routing", + description="install Agent Skills (agentskills.io) and let Plano-Orchestrator route to them", + yaml_text=_load_template_yaml("skills_routing.yaml"), + ), ] diff --git a/cli/planoai/main.py b/cli/planoai/main.py index ea43a1a8a..42e1dd020 100644 --- a/cli/planoai/main.py +++ b/cli/planoai/main.py @@ -39,6 +39,7 @@ from planoai.trace_cmd import trace as trace_cmd, start_trace_listener_background from planoai.chatgpt_cmd import chatgpt as chatgpt_cmd from planoai.obs_cmd import obs as obs_cmd +from planoai.skills_cmd import skills as skills_cmd from planoai.consts import ( DEFAULT_OTEL_TRACING_GRPC_ENDPOINT, DEFAULT_NATIVE_OTEL_TRACING_GRPC_ENDPOINT, @@ -746,6 +747,7 @@ def cli_agent(type, file, path, settings): main.add_command(trace_cmd, name="trace") main.add_command(chatgpt_cmd, name="chatgpt") main.add_command(obs_cmd, name="obs") +main.add_command(skills_cmd, name="skills") if __name__ == "__main__": main() diff --git a/cli/planoai/skills.py b/cli/planoai/skills.py new file mode 100644 index 000000000..f8a4a9829 --- /dev/null +++ b/cli/planoai/skills.py @@ -0,0 +1,420 @@ +"""Agent Skills discovery for Plano. + +Parses SKILL.md files from .plano/skills/ (project scope) and ~/.plano/skills/ +(user scope) following the Agent Skills specification: +https://agentskills.io/specification.md + +The parser is intentionally lenient (per the "Adding skills support" guide): +warn on cosmetic issues but only skip a skill when its YAML is unparseable or +its required `description` field is missing. +""" + +from __future__ import annotations + +import json +import os +import re +from dataclasses import dataclass, field +from pathlib import Path +from typing import Iterable + +import yaml + +from planoai.utils import getLogger + +log = getLogger(__name__) + +PROJECT_SKILLS_DIR = Path(".plano") / "skills" +USER_SKILLS_DIR = Path(os.path.expanduser("~/.plano/skills")) +# Universal Agent Skills install location used by `npx skills add` (vercel-labs/add-skill). +# Auto-trusted: same security posture as ~/.plano/skills, no project trust needed. +AGENTS_SKILLS_DIR = Path(os.path.expanduser("~/.agents/skills")) + +MAX_CATALOG_BYTES = 5 * 1024 + +MAX_DIRS_SCANNED = 2000 + +_NAME_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") + + +def trusted_projects_file() -> Path: + """Resolve `~/.plano/trusted_projects.json` at call time. + + Lazy so tests can override $HOME and have the new path picked up; module + import time would freeze it to the developer's actual home directory. + """ + return Path(os.path.expanduser("~/.plano/trusted_projects.json")) + + +def is_project_trusted(project_root: Path) -> bool: + """Return True if `project_root` is listed in `~/.plano/trusted_projects.json`. + + Project-scope skills come from arbitrary repos and are gated on this trust + decision (set with `planoai skills trust`). Single source of truth, shared + between the `skills_cmd` CLI surface and the render pipeline. + """ + path = trusted_projects_file() + if not path.exists(): + return False + try: + with path.open("r", encoding="utf-8") as fh: + data = json.load(fh) + except (OSError, json.JSONDecodeError): + return False + trusted = data.get("trusted_projects", []) if isinstance(data, dict) else [] + resolved = str(project_root.resolve()) + return resolved in {str(Path(p).resolve()) for p in trusted} + + +@dataclass(frozen=True) +class SkillDiagnostic: + severity: str # "warn" or "error" + message: str + path: Path + + +@dataclass +class Skill: + name: str + description: str + location: Path + base_dir: Path + body: str + scope: str + compatibility: str | None = None + license: str | None = None + metadata: dict = field(default_factory=dict) + allowed_tools: str | None = None + + def to_dict(self) -> dict: + """Serialize to a YAML-friendly dict for embedding in rendered config.""" + return { + "name": self.name, + "description": self.description, + "path": str(self.location), + "base_dir": str(self.base_dir), + "scope": self.scope, + "body": self.body, + "compatibility": self.compatibility, + "license": self.license, + "metadata": dict(self.metadata) if self.metadata else None, + "allowed_tools": self.allowed_tools, + } + + +def find_project_root(start: Path | None = None) -> Path: + """Walk up from `start` looking for `.plano/`, then `.git/`. + + Falls back to `start` (or cwd) if nothing is found. This matches how + `npx skills add` chooses a project root. + """ + base = Path(start or Path.cwd()).resolve() + cur = base + while cur != cur.parent: + if (cur / ".plano").exists(): + return cur + cur = cur.parent + + cur = base + while cur != cur.parent: + if (cur / ".git").exists(): + return cur + cur = cur.parent + + return base + + +def parse_skill_md(path: Path) -> tuple[Skill | None, list[SkillDiagnostic]]: + """Parse a single SKILL.md file leniently.""" + diagnostics: list[SkillDiagnostic] = [] + try: + text = path.read_text(encoding="utf-8") + except OSError as exc: + diagnostics.append( + SkillDiagnostic("error", f"failed to read SKILL.md: {exc}", path) + ) + return None, diagnostics + + frontmatter, body = _split_frontmatter(text) + if frontmatter is None: + diagnostics.append(SkillDiagnostic("error", "missing YAML frontmatter", path)) + return None, diagnostics + + data = _parse_yaml_lenient(frontmatter, path, diagnostics) + if data is None: + return None, diagnostics + + description = data.get("description") + if not isinstance(description, str) or not description.strip(): + diagnostics.append( + SkillDiagnostic( + "error", "skill is missing required 'description' field", path + ) + ) + return None, diagnostics + + parent_name = path.parent.name + name = data.get("name") + if not isinstance(name, str) or not name.strip(): + diagnostics.append( + SkillDiagnostic( + "warn", + f"missing 'name' field; falling back to parent directory '{parent_name}'", + path, + ) + ) + name = parent_name + + name = name.strip() + + if len(name) > 64: + diagnostics.append( + SkillDiagnostic("warn", "skill name exceeds 64 characters", path) + ) + + if not _NAME_PATTERN.match(name): + diagnostics.append( + SkillDiagnostic( + "warn", + f"skill name '{name}' violates spec naming rules " + "(lowercase alphanumeric + hyphens, no leading/trailing/double hyphens)", + path, + ) + ) + + if name != parent_name: + diagnostics.append( + SkillDiagnostic( + "warn", + f"skill name '{name}' does not match parent directory '{parent_name}'", + path, + ) + ) + + metadata_raw = data.get("metadata") + metadata = {} + if isinstance(metadata_raw, dict): + metadata = {str(k): str(v) for k, v in metadata_raw.items()} + + skill = Skill( + name=name, + description=description.strip(), + location=path.resolve(), + base_dir=path.parent.resolve(), + body=body, + scope="project", # may be overridden by caller + compatibility=_string_field(data.get("compatibility")), + license=_string_field(data.get("license")), + metadata=metadata, + allowed_tools=_string_field(data.get("allowed-tools")), + ) + return skill, diagnostics + + +def _split_frontmatter(text: str) -> tuple[str | None, str]: + if not text.startswith("---"): + return None, text + + m = re.match(r"^---\s*\r?\n(.*?)\r?\n---\s*(?:\r?\n)?(.*)$", text, re.DOTALL) + if not m: + return None, text + return m.group(1), m.group(2).strip("\n") + + +def _parse_yaml_lenient( + frontmatter: str, path: Path, diagnostics: list[SkillDiagnostic] +) -> dict | None: + try: + data = yaml.safe_load(frontmatter) + except yaml.YAMLError as exc: + retried = _retry_quote_problem_fields(frontmatter) + if retried is None: + diagnostics.append( + SkillDiagnostic("error", f"YAML parse error: {exc}", path) + ) + return None + try: + data = yaml.safe_load(retried) + except yaml.YAMLError as exc2: + diagnostics.append( + SkillDiagnostic( + "error", f"YAML parse error (after retry): {exc2}", path + ) + ) + return None + + if not isinstance(data, dict): + diagnostics.append( + SkillDiagnostic("error", "frontmatter is not a YAML mapping", path) + ) + return None + return data + + +_PROBLEM_FIELDS = ("description", "compatibility") + + +def _retry_quote_problem_fields(frontmatter: str) -> str | None: + """Wrap unquoted values for fields prone to YAML colon-collisions in quotes.""" + lines = frontmatter.splitlines() + out: list[str] = [] + changed = False + for line in lines: + m = re.match(r"^(\w[\w-]*)\s*:\s*(.*)$", line) + if m and m.group(1) in _PROBLEM_FIELDS: + key = m.group(1) + value = m.group(2).rstrip() + if value and not ( + (value.startswith("'") and value.endswith("'")) + or (value.startswith('"') and value.endswith('"')) + ): + escaped = value.replace("\\", "\\\\").replace('"', '\\"') + out.append(f'{key}: "{escaped}"') + changed = True + continue + out.append(line) + if not changed: + return None + return "\n".join(out) + + +def _string_field(value) -> str | None: + if value is None: + return None + s = str(value).strip() + return s or None + + +def _iter_skill_dirs(root: Path) -> Iterable[Path]: + if not root.exists() or not root.is_dir(): + return + + try: + children = sorted(root.iterdir(), key=lambda p: p.name) + except OSError: + return + + count = 0 + for child in children: + count += 1 + if count > MAX_DIRS_SCANNED: + log.warning( + "exceeded max scan budget (%d) while looking for skills in %s", + MAX_DIRS_SCANNED, + root, + ) + break + if not child.is_dir(): + continue + if child.name.startswith("."): + continue + yield child + + +def discover_skills( + project_root: Path | None = None, + include_user_scope: bool = True, +) -> tuple[list[Skill], list[SkillDiagnostic]]: + """Discover all skills available to the current project. + + Precedence (highest first): project > user > agents. Project-scope + skills shadow lower tiers with the same name; user-scope shadows + agents-scope. Both ``~/.plano/skills/`` (Plano-native) and + ``~/.agents/skills/`` (the universal Agent Skills install location used + by ``npx skills add``) are treated as auto-trusted user-tier scopes. + + Returns ``(skills, diagnostics)`` sorted by name. + """ + project_root = find_project_root(project_root) + project_dir = project_root / PROJECT_SKILLS_DIR + + skills_by_name: dict[str, Skill] = {} + diagnostics: list[SkillDiagnostic] = [] + + if include_user_scope: + # Load lowest precedence first so higher tiers shadow. + for skill_dir in _iter_skill_dirs(AGENTS_SKILLS_DIR): + skill_md = skill_dir / "SKILL.md" + if not skill_md.exists(): + continue + skill, diags = parse_skill_md(skill_md) + diagnostics.extend(diags) + if skill is not None: + skill = _set_scope(skill, "agents") + skills_by_name[skill.name] = skill + + for skill_dir in _iter_skill_dirs(USER_SKILLS_DIR): + skill_md = skill_dir / "SKILL.md" + if not skill_md.exists(): + continue + skill, diags = parse_skill_md(skill_md) + diagnostics.extend(diags) + if skill is None: + continue + skill = _set_scope(skill, "user") + existing = skills_by_name.get(skill.name) + if existing is not None and existing.scope == "agents": + diagnostics.append( + SkillDiagnostic( + "warn", + f"user-scope skill '{skill.name}' shadows ~/.agents/skills entry at {existing.location}", + skill.location, + ) + ) + skills_by_name[skill.name] = skill + + for skill_dir in _iter_skill_dirs(project_dir): + skill_md = skill_dir / "SKILL.md" + if not skill_md.exists(): + continue + skill, diags = parse_skill_md(skill_md) + diagnostics.extend(diags) + if skill is None: + continue + skill = _set_scope(skill, "project") + existing = skills_by_name.get(skill.name) + if existing is not None and existing.scope in ("user", "agents"): + diagnostics.append( + SkillDiagnostic( + "warn", + f"project-scope skill '{skill.name}' shadows {existing.scope}-scope skill at {existing.location}", + skill.location, + ) + ) + skills_by_name[skill.name] = skill + + return sorted(skills_by_name.values(), key=lambda s: s.name), diagnostics + + +def _set_scope(skill: Skill, scope: str) -> Skill: + return Skill( + name=skill.name, + description=skill.description, + location=skill.location, + base_dir=skill.base_dir, + body=skill.body, + scope=scope, + compatibility=skill.compatibility, + license=skill.license, + metadata=skill.metadata, + allowed_tools=skill.allowed_tools, + ) + + +def total_catalog_size(skills: Iterable[Skill]) -> int: + """Approximate byte size of the catalog the orchestrator will receive.""" + return sum(len(s.name) + len(s.description) for s in skills) + + +def filter_skills_by_allow_list( + skills: Iterable[Skill], allow_list: Iterable[str] | None +) -> list[Skill]: + """Filter skills to those whose `name` appears in `allow_list`. + + If `allow_list` is None, returns all skills. Unknown names are silently + dropped — callers warn at config-validation time. + """ + if allow_list is None: + return list(skills) + allowed = set(allow_list) + return [s for s in skills if s.name in allowed] diff --git a/cli/planoai/skills_cmd.py b/cli/planoai/skills_cmd.py new file mode 100644 index 000000000..ff6b44c06 --- /dev/null +++ b/cli/planoai/skills_cmd.py @@ -0,0 +1,471 @@ +"""`planoai skills` command group. + +Installs Agent Skills (https://agentskills.io) and surfaces them to Plano. + +Three discovery scopes are supported, in descending precedence: + +* ``/.plano/skills/`` -- repo-pinned skills. Loaded only when the + project has been marked trusted via ``planoai skills trust`` (skill content + is injected into the orchestrator prompt, so we gate on trust). +* ``~/.plano/skills/`` -- Plano-native user-scope. Always trusted. +* ``~/.agents/skills/`` -- universal Agent Skills location used by + ``npx skills add``. Always trusted; lets the upstream skills CLI work + out of the box without any Plano-specific awareness. + +``planoai skills add`` tries ``npx skills add`` first (the upstream CLI from +https://github.com/vercel-labs/add-skill), which writes to +``~/.agents/skills/`` and is picked up automatically thanks to the +agents-scope above. Falls back to ``git clone`` into ``.plano/skills/`` when +``npx`` is unavailable. +""" + +from __future__ import annotations + +import json +import os +import re +import shutil +import subprocess +import sys +from dataclasses import dataclass +from datetime import datetime, timezone +from pathlib import Path + +import rich_click as click +from rich.console import Console +from rich.table import Table + +from planoai.consts import PLANO_COLOR +from planoai.skills import ( + PROJECT_SKILLS_DIR, + Skill, + discover_skills, + find_project_root, + is_project_trusted, + trusted_projects_file, +) +from planoai.utils import getLogger + +log = getLogger(__name__) + +_OWNER_REPO_PATTERN = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") + + +@dataclass +class _InstallTarget: + owner: str + repo: str + ref: str | None = None # optional branch / tag / commit (e.g. "owner/repo@v1") + + @property + def slug(self) -> str: + return f"{self.owner}/{self.repo}" + + @property + def url(self) -> str: + return f"https://github.com/{self.owner}/{self.repo}.git" + + +def _console() -> Console: + return Console() + + +def _ensure_skills_dir(project_root: Path) -> Path: + skills_dir = project_root / PROJECT_SKILLS_DIR + skills_dir.mkdir(parents=True, exist_ok=True) + return skills_dir + + +def _parse_install_target(raw: str) -> _InstallTarget: + spec = raw.strip() + ref: str | None = None + if "@" in spec: + spec, _, ref_value = spec.partition("@") + ref = ref_value.strip() or None + if not _OWNER_REPO_PATTERN.match(spec): + raise click.BadParameter( + f"expected '/' (optionally suffixed with '@'), got '{raw}'" + ) + owner, repo = spec.split("/", 1) + return _InstallTarget(owner=owner, repo=repo, ref=ref) + + +def _has_npx() -> bool: + return shutil.which("npx") is not None + + +def _has_git() -> bool: + return shutil.which("git") is not None + + +def _mark_project_trusted(project_root: Path) -> None: + path = trusted_projects_file() + path.parent.mkdir(parents=True, exist_ok=True) + existing: dict = {} + if path.exists(): + try: + with path.open("r", encoding="utf-8") as fh: + existing = json.load(fh) or {} + except (OSError, json.JSONDecodeError): + existing = {} + trusted = set(existing.get("trusted_projects", []) or []) + trusted.add(str(project_root.resolve())) + existing["trusted_projects"] = sorted(trusted) + with path.open("w", encoding="utf-8") as fh: + json.dump(existing, fh, indent=2) + + +def _read_manifest(skills_dir: Path) -> dict: + manifest_path = skills_dir / ".skills.json" + if not manifest_path.exists(): + return {"skills": {}} + try: + with manifest_path.open("r", encoding="utf-8") as fh: + data = json.load(fh) + except (OSError, json.JSONDecodeError): + return {"skills": {}} + if not isinstance(data, dict): + return {"skills": {}} + data.setdefault("skills", {}) + return data + + +def _write_manifest(skills_dir: Path, manifest: dict) -> None: + manifest_path = skills_dir / ".skills.json" + with manifest_path.open("w", encoding="utf-8") as fh: + json.dump(manifest, fh, indent=2, sort_keys=True) + + +def _record_install( + skills_dir: Path, name: str, target: _InstallTarget, source: str +) -> None: + manifest = _read_manifest(skills_dir) + manifest["skills"][name] = { + "source": source, + "repo": target.slug, + "ref": target.ref, + "installed_at": datetime.now(timezone.utc).isoformat(), + } + _write_manifest(skills_dir, manifest) + + +def _remove_from_manifest(skills_dir: Path, name: str) -> None: + manifest = _read_manifest(skills_dir) + manifest["skills"].pop(name, None) + _write_manifest(skills_dir, manifest) + + +def _install_via_npx( + target: _InstallTarget, project_root: Path, console: Console +) -> bool: + """Try to install with `npx skills add`. Returns True on success.""" + env = os.environ.copy() + env.setdefault("SKILLS_NO_TELEMETRY", "1") + arg = target.slug if target.ref is None else f"{target.slug}@{target.ref}" + cmd = ["npx", "--yes", "skills", "add", arg] + console.print( + f"[dim]Running:[/dim] [cyan]{' '.join(cmd)}[/cyan] [dim](cwd={project_root})[/dim]" + ) + try: + result = subprocess.run( + cmd, + cwd=project_root, + env=env, + check=False, + ) + except FileNotFoundError: + return False + return result.returncode == 0 + + +def _install_via_git( + target: _InstallTarget, + project_root: Path, + skills_dir: Path, + console: Console, +) -> bool: + if not _has_git(): + console.print("[red]X[/red] git is not installed; cannot fall back from npx") + return False + + dest = skills_dir / target.repo + if dest.exists(): + console.print( + f"[yellow]![/yellow] {dest} already exists. " + "Remove it first with [cyan]planoai skills remove[/cyan] before reinstalling." + ) + return False + + cmd = ["git", "clone", "--depth", "1"] + if target.ref: + cmd.extend(["--branch", target.ref]) + cmd.extend([target.url, str(dest)]) + console.print( + f"[dim]Running:[/dim] [cyan]{' '.join(cmd)}[/cyan] [dim](cwd={project_root})[/dim]" + ) + try: + result = subprocess.run(cmd, cwd=project_root, check=False) + except FileNotFoundError: + console.print("[red]X[/red] git binary not found") + return False + if result.returncode != 0: + return False + + shutil.rmtree(dest / ".git", ignore_errors=True) + + if not (dest / "SKILL.md").exists(): + console.print( + f"[red]X[/red] {target.slug} does not contain a SKILL.md at its repo root; " + "this does not appear to be a valid Agent Skill." + ) + shutil.rmtree(dest, ignore_errors=True) + return False + return True + + +def _print_skills_table(console: Console, skills: list[Skill]) -> None: + if not skills: + console.print( + f"[dim]No skills installed.[/dim] Try [cyan]planoai skills add owner/repo[/cyan]." + ) + return + table = Table(title="Installed Agent Skills", border_style="dim") + table.add_column("Name", style=f"bold {PLANO_COLOR}") + table.add_column("Scope") + table.add_column("Description") + table.add_column("Path", style="dim") + for s in skills: + desc = s.description.splitlines()[0] + if len(desc) > 80: + desc = desc[:77] + "..." + table.add_row(s.name, s.scope, desc, str(s.location)) + console.print(table) + + +@click.group(name="skills") +def skills(): + """Manage Agent Skills (agentskills.io) for this Plano project.""" + + +@skills.command(name="add") +@click.argument("target", required=True) +@click.option( + "--path", + default=".", + help="Project directory (defaults to the directory containing .plano/ or .git/).", +) +def add_cmd(target: str, path: str): + """Install an Agent Skill from a GitHub repo into .plano/skills/. + + TARGET should be `owner/repo` (optionally suffixed with `@ref` for a branch + or tag). + """ + console = _console() + install_target = _parse_install_target(target) + project_root = find_project_root(Path(path).resolve()) + skills_dir = _ensure_skills_dir(project_root) + + console.print( + f"[bold {PLANO_COLOR}]Installing skill[/bold {PLANO_COLOR}] " + f"[cyan]{install_target.slug}[/cyan] -> [dim]{skills_dir}[/dim]" + ) + + # Snapshot what's already discoverable so we can diff after install and + # surface every newly-added skill regardless of which scope it landed in + # (project for git fallback, agents for `npx skills add`, etc.) and + # regardless of how the installed skill name maps to the repo name (e.g. + # multi-skill repos like openai/skills -> ~/.agents/skills/pdf). + before, _ = discover_skills(project_root=project_root, include_user_scope=True) + before_keys = {(s.name, str(s.base_dir)) for s in before} + + used_source: str + success = False + if _has_npx(): + success = _install_via_npx(install_target, project_root, console) + used_source = "npx-skills" + if not success: + console.print( + "[yellow]![/yellow] npx skills add did not succeed; " + "falling back to direct git clone." + ) + if not success: + success = _install_via_git(install_target, project_root, skills_dir, console) + used_source = "git" + + if not success: + console.print( + f"[red]X[/red] Failed to install [cyan]{install_target.slug}[/cyan]" + ) + sys.exit(1) + + discovered, diagnostics = discover_skills( + project_root=project_root, include_user_scope=True + ) + for diag in diagnostics: + if diag.severity == "error": + console.print(f"[red]X[/red] {diag.path}: {diag.message}") + else: + console.print(f"[yellow]![/yellow] {diag.path}: {diag.message}") + + newly_installed = [ + s for s in discovered if (s.name, str(s.base_dir)) not in before_keys + ] + if newly_installed: + for s in newly_installed: + if s.scope == "project": + _record_install(skills_dir, s.name, install_target, used_source) + try: + display_path = s.location.relative_to(project_root) + except ValueError: + display_path = s.location + console.print( + f"[green]+[/green] Installed [bold]{s.name}[/bold] " + f"[dim]({display_path}, scope={s.scope})[/dim]" + ) + if any( + s.scope == "project" for s in newly_installed + ) and not is_project_trusted(project_root): + console.print( + "\n[dim]Project-scope skills are not auto-loaded until this project is " + "trusted. Run[/dim] [cyan]planoai skills trust[/cyan] [dim]to enable them.[/dim]" + ) + else: + console.print( + "[yellow]![/yellow] Install reported success but no new SKILL.md " + "was discovered under .plano/skills, ~/.plano/skills, or " + "~/.agents/skills. Check the repo structure or pass a " + "single-skill repo." + ) + + +@skills.command(name="list") +@click.option( + "--path", + default=".", + help="Project directory (defaults to the directory containing .plano/ or .git/).", +) +@click.option( + "--no-user-scope", + is_flag=True, + default=False, + help="Skip user-scope skills under ~/.plano/skills and ~/.agents/skills.", +) +def list_cmd(path: str, no_user_scope: bool): + """List discovered Agent Skills across project / user / agents scopes.""" + console = _console() + project_root = find_project_root(Path(path).resolve()) + discovered, diagnostics = discover_skills( + project_root=project_root, include_user_scope=not no_user_scope + ) + _print_skills_table(console, discovered) + + if diagnostics: + console.print() + for diag in diagnostics: + color = "red" if diag.severity == "error" else "yellow" + marker = "X" if diag.severity == "error" else "!" + console.print(f"[{color}]{marker}[/{color}] {diag.path}: {diag.message}") + + +@skills.command(name="remove") +@click.argument("name", required=True) +@click.option( + "--path", + default=".", + help="Project directory (defaults to the directory containing .plano/ or .git/).", +) +def remove_cmd(name: str, path: str): + """Remove a project-scope skill from .plano/skills/. + + User-scope skills under ~/.plano/skills or ~/.agents/skills must be + removed with their respective installer (`npx skills remove ` for + the latter); planoai will not touch directories outside the project. + """ + console = _console() + project_root = find_project_root(Path(path).resolve()) + skills_dir = project_root / PROJECT_SKILLS_DIR + if not skills_dir.exists(): + console.print(f"[red]X[/red] no skills directory at {skills_dir}") + sys.exit(1) + + target_dir = skills_dir / name + if not target_dir.exists(): + discovered, _ = discover_skills( + project_root=project_root, include_user_scope=True + ) + project_match = next( + (s for s in discovered if s.name == name and s.scope == "project"), None + ) + if project_match is None: + other = next((s for s in discovered if s.name == name), None) + if other is not None: + console.print( + f"[red]X[/red] '{name}' is installed in {other.scope} scope at " + f"{other.base_dir}; planoai only removes project-scope skills. " + "Use the upstream installer (e.g. `npx skills remove`) for that one." + ) + else: + console.print( + f"[red]X[/red] no project-scope skill named '{name}' under {skills_dir}" + ) + sys.exit(1) + target_dir = project_match.base_dir + + if not target_dir.resolve().is_relative_to(skills_dir.resolve()): + console.print( + f"[red]X[/red] refusing to delete {target_dir} (outside {skills_dir})" + ) + sys.exit(1) + + shutil.rmtree(target_dir, ignore_errors=False) + _remove_from_manifest(skills_dir, name) + console.print(f"[green]+[/green] Removed [bold]{name}[/bold]") + + +@skills.command(name="trust") +@click.option( + "--path", + default=".", + help="Project directory to mark as trusted.", +) +@click.option( + "--revoke", + is_flag=True, + default=False, + help="Revoke trust instead of granting it.", +) +def trust_cmd(path: str, revoke: bool): + """Mark this project's .plano/skills/ as trusted for auto-loading. + + Project-scope skills come from the working directory's repo, which may + be untrusted. Plano refuses to inject their contents into the + orchestrator prompt until you trust the project. + """ + console = _console() + project_root = find_project_root(Path(path).resolve()) + + if revoke: + path = trusted_projects_file() + if not path.exists(): + console.print("[dim]No trusted projects to revoke.[/dim]") + return + try: + with path.open("r", encoding="utf-8") as fh: + data = json.load(fh) or {} + except (OSError, json.JSONDecodeError): + data = {} + trusted = { + str(Path(p).resolve()) for p in data.get("trusted_projects", []) or [] + } + trusted.discard(str(project_root.resolve())) + data["trusted_projects"] = sorted(trusted) + with path.open("w", encoding="utf-8") as fh: + json.dump(data, fh, indent=2) + console.print(f"[green]+[/green] Revoked trust for [bold]{project_root}[/bold]") + return + + _mark_project_trusted(project_root) + console.print( + f"[green]+[/green] Trusted [bold]{project_root}[/bold].\n" + f"[dim]Project-scope skills under .plano/skills/ will now be loaded at startup.[/dim]" + ) diff --git a/cli/planoai/templates/skills_routing.yaml b/cli/planoai/templates/skills_routing.yaml new file mode 100644 index 000000000..bd041cb1d --- /dev/null +++ b/cli/planoai/templates/skills_routing.yaml @@ -0,0 +1,71 @@ +version: v0.3.0 + +# This template wires Agent Skills (https://agentskills.io) into Plano so the +# built-in Plano-Orchestrator can decide *per request* which skill(s) to attach +# to the selected route. +# +# 1. Install skills locally: +# planoai skills add owner/pdf-processing +# planoai skills add owner/code-review +# Skills land under .plano/skills//SKILL.md. +# +# 2. (Required for project-scope skills) Mark the project trusted so its +# skills auto-load at startup: +# planoai skills trust +# +# 3. Reference the installed skills under `routing_preferences[].skills`. +# During the intent step Plano-Orchestrator receives both a and a +# XML block; it picks a route and zero or more skills, and the +# SKILL.md bodies of the chosen skills are injected into the upstream +# system prompt for that turn. + +model_providers: + - model: anthropic/claude-sonnet-4-5 + default: true + access_key: $ANTHROPIC_API_KEY + + - model: openai/gpt-4.1-2025-04-14 + access_key: $OPENAI_API_KEY + +# Catalog of skills available to this project. Entries are skill names +# (resolved against .plano/skills//) — the CLI inlines each SKILL.md +# body into the rendered config at `planoai up` time. +# +# Omit `skills:` entirely to auto-include every discovered skill. +skills: + - pdf-processing + - code-review + +listeners: + - type: model + name: model_listener + port: 12000 + +# Routing preferences double as Plano-Orchestrator's catalog. Attach +# `skills:` to a route to make those skills eligible for activation when the +# orchestrator picks that route. +routing_preferences: + - name: code review + description: | + Reviewing pull requests, analyzing diffs, and suggesting improvements + to existing code. + models: + - anthropic/claude-sonnet-4-5 + - openai/gpt-4.1-2025-04-14 + skills: + - code-review + + - name: document understanding + description: | + Summarizing PDFs and other long-form documents, extracting structured + data such as tables, line items, or signatures. + models: + - anthropic/claude-sonnet-4-5 + - openai/gpt-4.1-2025-04-14 + skills: + - pdf-processing + selection_policy: + prefer: cheapest + +tracing: + random_sampling: 100 diff --git a/cli/planoai/templates/template_sync_map.yaml b/cli/planoai/templates/template_sync_map.yaml index 4d601f6c0..56de29495 100644 --- a/cli/planoai/templates/template_sync_map.yaml +++ b/cli/planoai/templates/template_sync_map.yaml @@ -27,3 +27,8 @@ templates: template_file: conversational_state_v1_responses.yaml demo_configs: [] transform: none + + - template_id: skills_routing + template_file: skills_routing.yaml + demo_configs: [] + transform: none diff --git a/cli/test/test_skills.py b/cli/test/test_skills.py new file mode 100644 index 000000000..1188616b6 --- /dev/null +++ b/cli/test/test_skills.py @@ -0,0 +1,394 @@ +"""Tests for cli/planoai/skills.py and the config-rendering hooks that +materialize SKILL.md bodies into the rendered plano config. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from unittest import mock + +import pytest + +from planoai.skills import ( + AGENTS_SKILLS_DIR, + PROJECT_SKILLS_DIR, + USER_SKILLS_DIR, + Skill, + discover_skills, + parse_skill_md, + total_catalog_size, +) + + +@pytest.fixture(autouse=True) +def _isolate_user_scopes(tmp_path, monkeypatch): + """Default both user-tier scopes to non-existent dirs so the dev's real + ~/.plano/skills and ~/.agents/skills cannot bleed into tests. + """ + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "_no_user_skills") + monkeypatch.setattr( + "planoai.skills.AGENTS_SKILLS_DIR", tmp_path / "_no_agents_skills" + ) + + +def _write_skill( + base: Path, + name: str, + description: str = "Process PDFs. Use when handling PDF files.", + body: str = "# Body\n\nDo the thing.", + extra_frontmatter: str = "", +) -> Path: + skill_dir = base / name + skill_dir.mkdir(parents=True, exist_ok=True) + frontmatter = f"name: {name}\ndescription: {description}\n{extra_frontmatter}" + (skill_dir / "SKILL.md").write_text( + f"---\n{frontmatter}---\n\n{body}", + encoding="utf-8", + ) + return skill_dir / "SKILL.md" + + +def test_parse_skill_md_minimal(tmp_path): + skill_md = _write_skill(tmp_path, "pdf-processing") + + skill, diagnostics = parse_skill_md(skill_md) + + assert skill is not None + assert skill.name == "pdf-processing" + assert skill.description.startswith("Process PDFs") + assert "Do the thing." in skill.body + assert skill.location == skill_md.resolve() + assert skill.base_dir == skill_md.parent.resolve() + # No warnings or errors for a well-formed skill. + assert diagnostics == [] + + +def test_parse_skill_md_lenient_when_name_mismatches_directory(tmp_path): + skill_dir = tmp_path / "some-dir" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: different-name\ndescription: ok\n---\n\nbody", + encoding="utf-8", + ) + + skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md") + + assert skill is not None + assert skill.name == "different-name" + assert any( + "does not match parent directory" in d.message for d in diagnostics + ), diagnostics + + +def test_parse_skill_md_warns_on_invalid_name(tmp_path): + skill_dir = tmp_path / "Bad-Name" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: Bad-Name\ndescription: ok\n---\n\nbody", + encoding="utf-8", + ) + + skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md") + assert skill is not None + assert any("violates spec naming rules" in d.message for d in diagnostics) + + +def test_parse_skill_md_recovers_from_unquoted_colons(tmp_path): + skill_dir = tmp_path / "code-review" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: code-review\n" + "description: Use this skill when: the user asks about code review\n" + "---\n\nbody", + encoding="utf-8", + ) + + skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md") + # Lenient parse retries with quoted values and succeeds. + assert skill is not None + assert skill.name == "code-review" + assert "Use this skill when:" in skill.description + + +def test_parse_skill_md_rejects_when_description_missing(tmp_path): + skill_dir = tmp_path / "broken" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: broken\n---\n\nbody", + encoding="utf-8", + ) + + skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md") + assert skill is None + assert any(d.severity == "error" for d in diagnostics) + + +def test_parse_skill_md_rejects_when_frontmatter_missing(tmp_path): + skill_dir = tmp_path / "no-frontmatter" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("just markdown", encoding="utf-8") + + skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md") + assert skill is None + assert any("frontmatter" in d.message for d in diagnostics) + + +def test_discover_skills_project_only(tmp_path, monkeypatch): + (tmp_path / ".plano").mkdir() + project_skills_dir = tmp_path / ".plano" / "skills" + project_skills_dir.mkdir() + _write_skill(project_skills_dir, "pdf-processing") + _write_skill(project_skills_dir, "code-review") + + skills, diagnostics = discover_skills( + project_root=tmp_path, include_user_scope=False + ) + + names = sorted(s.name for s in skills) + assert names == ["code-review", "pdf-processing"] + assert all(s.scope == "project" for s in skills) + + +def test_discover_skills_picks_up_agents_scope(tmp_path, monkeypatch): + """`npx skills add` writes into ~/.agents/skills/. That directory + must be discovered as a user-tier (auto-trusted) scope so the upstream + CLI works without Plano-specific awareness. + """ + agents_dir = tmp_path / "fake-home" / ".agents" / "skills" + agents_dir.mkdir(parents=True) + _write_skill(agents_dir, "pdf", description="agents-scope description") + + monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir) + + (tmp_path / ".plano").mkdir() + (tmp_path / ".plano" / "skills").mkdir() + + skills, _ = discover_skills(project_root=tmp_path, include_user_scope=True) + by_name = {s.name: s for s in skills} + assert "pdf" in by_name + assert by_name["pdf"].scope == "agents" + assert by_name["pdf"].description == "agents-scope description" + + +def test_discover_skills_user_scope_shadows_agents_scope(tmp_path, monkeypatch): + """When the same skill name exists in both ~/.plano/skills and + ~/.agents/skills, the Plano-native one wins and a diagnostic is emitted. + + Project root lives in its own subtree (with no .plano/ ancestor) so it + cannot collide with the patched user-scope dir. + """ + home = tmp_path / "fake-home" + home.mkdir() + agents_dir = home / ".agents" / "skills" + agents_dir.mkdir(parents=True) + _write_skill(agents_dir, "pdf", description="agents copy") + monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir) + + user_dir = home / ".plano" / "skills" + user_dir.mkdir(parents=True) + _write_skill(user_dir, "pdf", description="user copy") + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", user_dir) + + project_root = tmp_path / "elsewhere" / "proj" + project_root.mkdir(parents=True) + skills, diagnostics = discover_skills( + project_root=project_root, include_user_scope=True + ) + by_name = {s.name: s for s in skills} + assert by_name["pdf"].scope == "user" + assert by_name["pdf"].description == "user copy" + assert any("shadows ~/.agents/skills" in d.message for d in diagnostics) + + +def test_discover_skills_project_overrides_user_scope(tmp_path, monkeypatch): + user_skills_dir = tmp_path / "fake-home" / ".plano" / "skills" + user_skills_dir.mkdir(parents=True) + _write_skill(user_skills_dir, "shared", description="user-scope description") + + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", user_skills_dir) + + (tmp_path / ".plano").mkdir() + project_skills_dir = tmp_path / ".plano" / "skills" + project_skills_dir.mkdir() + _write_skill(project_skills_dir, "shared", description="project-scope description") + _write_skill(project_skills_dir, "only-project") + + skills, diagnostics = discover_skills( + project_root=tmp_path, include_user_scope=True + ) + + by_name = {s.name: s for s in skills} + assert by_name["shared"].scope == "project" + assert by_name["shared"].description == "project-scope description" + assert by_name["only-project"].scope == "project" + assert any("shadows user-scope skill" in d.message for d in diagnostics) + + +def test_total_catalog_size_counts_name_and_description(): + skills = [ + Skill( + name="a", + description="d1", + location=Path("/x"), + base_dir=Path("/"), + body="b", + scope="project", + ), + Skill( + name="bb", + description="dd", + location=Path("/y"), + base_dir=Path("/"), + body="b", + scope="project", + ), + ] + assert total_catalog_size(skills) == (1 + 2) + (2 + 2) + + +def test_materialize_skills_in_config_default_inlines_bodies(tmp_path, monkeypatch): + project_root = tmp_path + (project_root / ".plano").mkdir() + project_skills_dir = project_root / ".plano" / "skills" + project_skills_dir.mkdir() + _write_skill( + project_skills_dir, + "pdf-processing", + description="Process PDFs.", + body="# Body\nfollow these steps.", + ) + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + # Mark the project trusted so .plano/skills is loaded. + trusted = fake_home / ".plano" / "trusted_projects.json" + trusted.parent.mkdir(parents=True, exist_ok=True) + trusted.write_text( + json.dumps({"trusted_projects": [str(project_root.resolve())]}), + encoding="utf-8", + ) + monkeypatch.setattr( + "planoai.skills.USER_SKILLS_DIR", + fake_home / ".plano" / "skills", + ) + + from planoai.config_generator import materialize_skills_in_config + + config_yaml = {"version": "v0.4.0"} + materialize_skills_in_config(config_yaml, project_root) + + assert "skills" in config_yaml + materialized = config_yaml["skills"] + assert len(materialized) == 1 + entry = materialized[0] + assert entry["name"] == "pdf-processing" + assert "follow these steps." in entry["body"] + assert entry["scope"] == "project" + + +def test_materialize_skills_in_config_skips_untrusted_project_skills( + tmp_path, monkeypatch +): + project_root = tmp_path + (project_root / ".plano").mkdir() + project_skills_dir = project_root / ".plano" / "skills" + project_skills_dir.mkdir() + _write_skill(project_skills_dir, "pdf-processing") + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + monkeypatch.setattr( + "planoai.skills.USER_SKILLS_DIR", + fake_home / ".plano" / "skills", + ) + + from planoai.config_generator import materialize_skills_in_config + + config_yaml = {"version": "v0.4.0"} + materialize_skills_in_config(config_yaml, project_root) + # Untrusted -> project skills are not loaded. + assert "skills" not in config_yaml + + +def test_materialize_skills_in_config_loads_agents_scope_without_trust( + tmp_path, monkeypatch +): + """Even with no project trust, skills installed by `npx skills add` into + ~/.agents/skills/ must materialize into the rendered config — that + directory is the universal Agent Skills install location and is + user-tier, not project-tier. + """ + project_root = tmp_path / "proj" + (project_root / ".plano").mkdir(parents=True) + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + monkeypatch.setattr( + "planoai.skills.USER_SKILLS_DIR", + fake_home / ".plano" / "skills", + ) + + agents_dir = fake_home / ".agents" / "skills" + agents_dir.mkdir(parents=True) + _write_skill(agents_dir, "pdf", body="# Body\nhandle the pdf.") + monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir) + + from planoai.config_generator import materialize_skills_in_config + + config_yaml = {"version": "v0.4.0"} + materialize_skills_in_config(config_yaml, project_root) + + assert "skills" in config_yaml + materialized = config_yaml["skills"] + assert len(materialized) == 1 + assert materialized[0]["name"] == "pdf" + assert materialized[0]["scope"] == "agents" + assert "handle the pdf." in materialized[0]["body"] + + +def test_materialize_skills_in_config_respects_allow_list(tmp_path, monkeypatch): + project_root = tmp_path + (project_root / ".plano").mkdir() + skills_dir = project_root / ".plano" / "skills" + skills_dir.mkdir() + _write_skill(skills_dir, "skill-a") + _write_skill(skills_dir, "skill-b") + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + trusted = fake_home / ".plano" / "trusted_projects.json" + trusted.parent.mkdir(parents=True, exist_ok=True) + trusted.write_text( + json.dumps({"trusted_projects": [str(project_root.resolve())]}), + encoding="utf-8", + ) + monkeypatch.setattr( + "planoai.skills.USER_SKILLS_DIR", + fake_home / ".plano" / "skills", + ) + + from planoai.config_generator import materialize_skills_in_config + + config_yaml = { + "version": "v0.4.0", + "skills": ["skill-a"], + "routing_preferences": [ + { + "name": "demo route", + "description": "demo", + "models": ["openai/gpt-4o"], + "skills": ["skill-a", "does-not-exist"], + } + ], + } + materialize_skills_in_config(config_yaml, project_root) + + assert [s["name"] for s in config_yaml["skills"]] == ["skill-a"] + # Unknown allow-list entries are pruned but the known one is kept. + assert config_yaml["routing_preferences"][0]["skills"] == ["skill-a"] diff --git a/cli/test/test_skills_cmd.py b/cli/test/test_skills_cmd.py new file mode 100644 index 000000000..e072c0258 --- /dev/null +++ b/cli/test/test_skills_cmd.py @@ -0,0 +1,235 @@ +"""CLI tests for the `planoai skills` command group.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from unittest import mock + +import pytest +from click.testing import CliRunner + +from planoai.skills_cmd import skills + + +def _seed_project(tmp_path: Path) -> Path: + """Create a project that find_project_root will pick up via .plano/.""" + project = tmp_path / "project" + project.mkdir() + (project / ".plano").mkdir() + return project + + +@pytest.fixture(autouse=True) +def _isolate_user_scopes(tmp_path, monkeypatch): + """Default both user-tier scopes to non-existent dirs so the dev's real + ~/.plano/skills and ~/.agents/skills cannot bleed into the test sandbox. + Individual tests can override these via further monkeypatching. + """ + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-user-skills") + monkeypatch.setattr( + "planoai.skills.AGENTS_SKILLS_DIR", tmp_path / "no-agents-skills" + ) + + +def _write_skill(base: Path, name: str, description: str = "demo skill") -> None: + skill_dir = base / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: {description}\n---\n\nbody", + encoding="utf-8", + ) + + +def test_list_empty(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + # Isolate user-scope skills dir. + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home") + + runner = CliRunner() + result = runner.invoke(skills, ["list"]) + + assert result.exit_code == 0, result.output + assert "No skills installed" in result.output + + +def test_list_shows_project_skills(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home") + + _write_skill(project / ".plano" / "skills", "pdf-processing") + _write_skill(project / ".plano" / "skills", "code-review") + + runner = CliRunner() + result = runner.invoke(skills, ["list", "--no-user-scope"]) + + assert result.exit_code == 0, result.output + assert "pdf-processing" in result.output + assert "code-review" in result.output + + +def test_remove_deletes_skill_dir(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home") + + skills_dir = project / ".plano" / "skills" + _write_skill(skills_dir, "pdf-processing") + (skills_dir / ".skills.json").write_text( + json.dumps( + { + "skills": { + "pdf-processing": { + "source": "git", + "repo": "owner/pdf-processing", + } + } + } + ), + encoding="utf-8", + ) + + runner = CliRunner() + result = runner.invoke(skills, ["remove", "pdf-processing"]) + + assert result.exit_code == 0, result.output + assert not (skills_dir / "pdf-processing").exists() + manifest = json.loads((skills_dir / ".skills.json").read_text(encoding="utf-8")) + assert "pdf-processing" not in manifest["skills"] + + +def test_remove_unknown_skill_errors(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home") + (project / ".plano" / "skills").mkdir() + + runner = CliRunner() + result = runner.invoke(skills, ["remove", "nope"]) + assert result.exit_code != 0 + + +def test_add_falls_back_to_git_when_no_npx(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + + # Force the npx branch off and stub git clone to create a SKILL.md. + monkeypatch.setattr("planoai.skills_cmd._has_npx", lambda: False) + monkeypatch.setattr("planoai.skills_cmd._has_git", lambda: True) + + def fake_subprocess_run(cmd, **kwargs): + # cmd is like ["git", "clone", ..., url, dest] + dest = Path(cmd[-1]) + dest.mkdir(parents=True, exist_ok=True) + (dest / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: example\n---\n\nbody", + encoding="utf-8", + ) + (dest / ".git").mkdir() + return mock.Mock(returncode=0) + + monkeypatch.setattr("planoai.skills_cmd.subprocess.run", fake_subprocess_run) + + runner = CliRunner() + result = runner.invoke(skills, ["add", "owner/my-skill"]) + assert result.exit_code == 0, result.output + assert (project / ".plano" / "skills" / "my-skill" / "SKILL.md").exists() + # Trust hint should be shown for untrusted projects with project-scope installs. + assert "planoai skills trust" in result.output + + +def test_add_discovers_skill_installed_into_agents_scope_by_npx(tmp_path, monkeypatch): + """`npx skills add` writes to ~/.agents/skills/; planoai must + pick it up from that universal scope and *not* nag about trust. + """ + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + + agents_dir = tmp_path / "agents" / "skills" + agents_dir.mkdir(parents=True) + monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir) + + # Pretend npx is on $PATH and succeeds, dropping the skill in ~/.agents/skills + # rather than .plano/skills (which is what the upstream CLI actually does). + monkeypatch.setattr("planoai.skills_cmd._has_npx", lambda: True) + + def fake_install_via_npx(target, project_root, console): + skill_dir = agents_dir / "pdf" + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: pdf\ndescription: process pdfs\n---\n\nbody", + encoding="utf-8", + ) + return True + + monkeypatch.setattr("planoai.skills_cmd._install_via_npx", fake_install_via_npx) + + runner = CliRunner() + result = runner.invoke(skills, ["add", "openai/skills"]) + + assert result.exit_code == 0, result.output + assert "scope=agents" in result.output + assert "planoai skills trust" not in result.output + + +def test_list_includes_agents_scope_entries(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + + agents_dir = tmp_path / "agents-skills" + agents_dir.mkdir() + monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir) + _write_skill(agents_dir, "pdf") + + runner = CliRunner() + result = runner.invoke(skills, ["list"]) + + assert result.exit_code == 0, result.output + assert "pdf" in result.output + assert "agents" in result.output + + +def test_remove_rejects_agents_scope_skill(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + (project / ".plano" / "skills").mkdir() + + agents_dir = tmp_path / "agents-skills" + agents_dir.mkdir() + monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir) + _write_skill(agents_dir, "pdf") + + runner = CliRunner() + result = runner.invoke(skills, ["remove", "pdf"]) + + assert result.exit_code != 0 + assert "npx skills remove" in result.output + + +def test_trust_marks_project(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + + runner = CliRunner() + result = runner.invoke(skills, ["trust"]) + + assert result.exit_code == 0, result.output + trusted_file = fake_home / ".plano" / "trusted_projects.json" + assert trusted_file.exists() + data = json.loads(trusted_file.read_text(encoding="utf-8")) + assert str(project.resolve()) in data["trusted_projects"] + + +def test_add_rejects_invalid_target(tmp_path, monkeypatch): + project = _seed_project(tmp_path) + monkeypatch.chdir(project) + runner = CliRunner() + result = runner.invoke(skills, ["add", "not-a-spec"]) + assert result.exit_code != 0 diff --git a/config/plano_config_schema.yaml b/config/plano_config_schema.yaml index 9560b4376..f1b4a7e54 100644 --- a/config/plano_config_schema.yaml +++ b/config/plano_config_schema.yaml @@ -316,6 +316,52 @@ properties: description: "Maximum token length for the orchestrator/routing model context window. Default is 8192." system_prompt: type: string + skills: + type: array + description: | + Agent Skills (https://agentskills.io) registered for this Plano project. + Each entry may be a string (the skill name, resolved against + .plano/skills// or ~/.plano/skills//) or an object with name + + optional inline metadata. The Python CLI auto-populates body/path + during config rendering. Skills are attached to routes via + `routing_preferences[].skills`; when omitted there, the orchestrator + sees every entry declared (or auto-discovered) here. + items: + oneOf: + - type: string + - type: object + properties: + name: + type: string + description: + type: string + path: + type: string + description: "Absolute path to the SKILL.md file (set by the CLI at render time)." + base_dir: + type: string + description: "Absolute path to the skill directory (set by the CLI at render time)." + body: + type: string + description: "Markdown body of SKILL.md (inlined at render time so WASM does not need filesystem access)." + scope: + type: string + enum: + - project + - user + compatibility: + type: string + license: + type: string + metadata: + type: object + additionalProperties: + type: string + allowed_tools: + type: string + additionalProperties: false + required: + - name prompt_targets: type: array items: @@ -549,6 +595,17 @@ properties: items: type: string minItems: 1 + skills: + type: array + description: | + Agent Skills associated with this routing preference. When + Plano-Orchestrator selects this route, the listed skills are also + considered for activation and their SKILL.md bodies are injected + into the upstream system prompt. Skill names must match an entry + in the top-level `skills:` catalog or be discoverable under + `.plano/skills/` or `~/.plano/skills/`. + items: + type: string selection_policy: type: object properties: diff --git a/crates/brightstaff/src/handlers/agents/selector.rs b/crates/brightstaff/src/handlers/agents/selector.rs index e04671635..48cb811fb 100644 --- a/crates/brightstaff/src/handlers/agents/selector.rs +++ b/crates/brightstaff/src/handlers/agents/selector.rs @@ -132,11 +132,15 @@ impl AgentSelector { .determine_orchestration(messages, Some(usage_preferences), request_id) .await { - Ok(Some(routes)) => { - debug!(count = routes.len(), "determined agents via orchestration"); + Ok(Some(selection)) => { + debug!( + count = selection.routes.len(), + skill_count = selection.skills.len(), + "determined agents via orchestration" + ); let mut selected_agents = Vec::new(); - for (route_name, agent_name) in routes { + for (route_name, agent_name) in selection.routes { debug!(route = %route_name, agent = %agent_name, "processing route"); let selected_agent = agents .iter() diff --git a/crates/brightstaff/src/handlers/llm/mod.rs b/crates/brightstaff/src/handlers/llm/mod.rs index 3336209fb..425151a73 100644 --- a/crates/brightstaff/src/handlers/llm/mod.rs +++ b/crates/brightstaff/src/handlers/llm/mod.rs @@ -37,7 +37,7 @@ use crate::tracing::{ collect_custom_trace_attributes, llm as tracing_llm, operation_component, plano as tracing_plano, set_service_name, }; -use model_selection::router_chat_get_upstream_model; +use model_selection::{inject_activated_skills_into_request, router_chat_get_upstream_model}; const PERPLEXITY_PROVIDER_PREFIX: &str = "perplexity/"; @@ -282,26 +282,16 @@ async fn llm_chat_inner( Err(response) => return Ok(response), }; - // Serialize request for upstream BEFORE router consumes it - let client_request_bytes_for_upstream: Bytes = - match ProviderRequestType::to_bytes(&client_request) { - Ok(bytes) => bytes.into(), - Err(err) => { - warn!(error = %err, "failed to serialize request for upstream"); - let mut r = Response::new(full(format!("Failed to serialize request: {}", err))); - *r.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; - return Ok(r); - } - }; - - // --- Phase 3: Route the request (or use pinned model from session cache) --- - let resolved_model = if let Some(cached_model) = pinned_model { + // Route the request (or use pinned model from session cache) BEFORE + // serializing for upstream — skill body injection happens here, so the + // upstream bytes must be produced after routing returns. + let (resolved_model, activated_skills) = if let Some(cached_model) = pinned_model { info!( session_id = %session_id.as_deref().unwrap_or(""), model = %cached_model, "using pinned routing decision from cache" ); - cached_model + (cached_model, Vec::new()) } else { let routing_span = info_span!( "routing", @@ -313,11 +303,16 @@ async fn llm_chat_inner( route.selected_model = tracing::field::Empty, routing.determination_ms = tracing::field::Empty, ); + // The router consumes the request (it converts it to OpenAI format + // internally to extract conversation messages). Clone so we can + // still mutate the original below when Plano-Orchestrator activates + // any Agent Skills. + let request_for_routing = client_request.clone(); let routing_result = match async { set_service_name(operation_component::ROUTING); router_chat_get_upstream_model( Arc::clone(&state.orchestrator_service), - client_request, + request_for_routing, &request_path, &request_id, inline_routing_preferences, @@ -335,8 +330,11 @@ async fn llm_chat_inner( } }; - let (router_selected_model, route_name) = - (routing_result.model_name, routing_result.route_name); + let (router_selected_model, route_name, activated) = ( + routing_result.model_name, + routing_result.route_name, + routing_result.activated_skills, + ); let model = if router_selected_model != "none" { router_selected_model } else { @@ -362,10 +360,34 @@ async fn llm_chat_inner( .await; } - model + (model, activated) }; tracing::Span::current().record(tracing_llm::MODEL_NAME, resolved_model.as_str()); + // If Plano-Orchestrator activated any Agent Skills for this route, inject + // their SKILL.md bodies into the system prompt before we hand the bytes + // off to the upstream provider. + if !activated_skills.is_empty() { + info!( + count = activated_skills.len(), + skills = ?activated_skills.iter().map(|s| s.name.as_str()).collect::>(), + "injecting activated Agent Skills into upstream system prompt" + ); + inject_activated_skills_into_request(&mut client_request, &activated_skills); + } + + // Serialize request for upstream AFTER potential skill injection. + let client_request_bytes_for_upstream: Bytes = + match ProviderRequestType::to_bytes(&client_request) { + Ok(bytes) => bytes.into(), + Err(err) => { + warn!(error = %err, "failed to serialize request for upstream"); + let mut r = Response::new(full(format!("Failed to serialize request: {}", err))); + *r.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; + return Ok(r); + } + }; + // --- Phase 4: Forward to upstream and stream back --- send_upstream( &state.http_client, diff --git a/crates/brightstaff/src/handlers/llm/model_selection.rs b/crates/brightstaff/src/handlers/llm/model_selection.rs index a1378d86d..5418c5845 100644 --- a/crates/brightstaff/src/handlers/llm/model_selection.rs +++ b/crates/brightstaff/src/handlers/llm/model_selection.rs @@ -1,5 +1,9 @@ -use common::configuration::TopLevelRoutingPreference; +use common::configuration::{SkillRef, TopLevelRoutingPreference}; +use common::skills_runtime::augment_system_prompt_with_skills; +use hermesllm::apis::openai::{Message, MessageContent, Role}; use hermesllm::clients::endpoints::SupportedUpstreamAPIs; +use hermesllm::providers::request::ProviderRequest; +use hermesllm::transforms::lib::ExtractText; use hermesllm::ProviderRequestType; use hyper::StatusCode; use std::sync::Arc; @@ -29,6 +33,10 @@ pub struct RoutingResult { /// Full ranked list — use subsequent entries as fallbacks on 429/5xx. pub models: Vec, pub route_name: Option, + /// Agent Skills activated by Plano-Orchestrator for this request. + /// Their `body` field (the SKILL.md content) is prepended to the + /// upstream system prompt by the caller in `send_upstream`. + pub activated_skills: Vec, } pub struct RoutingError { @@ -128,8 +136,8 @@ pub async fn router_chat_get_upstream_model( match routing_result { Ok(route) => match route { - Some((route_name, ranked_models)) => { - let model_name = ranked_models.first().cloned().unwrap_or_default(); + Some(decision) => { + let model_name = decision.models.first().cloned().unwrap_or_default(); current_span.record("route.selected_model", model_name.as_str()); bs_metrics::record_router_decision( route_label, @@ -139,8 +147,9 @@ pub async fn router_chat_get_upstream_model( ); Ok(RoutingResult { model_name, - models: ranked_models, - route_name: Some(route_name), + models: decision.models, + route_name: Some(decision.route_name), + activated_skills: decision.activated_skills, }) } None => { @@ -159,6 +168,7 @@ pub async fn router_chat_get_upstream_model( model_name: "none".to_string(), models: vec!["none".to_string()], route_name: None, + activated_skills: Vec::new(), }) } }, @@ -172,3 +182,61 @@ pub async fn router_chat_get_upstream_model( } } } + +/// Prepend the bodies of `activated_skills` to the system prompt of the +/// upstream request so the chosen LLM has access to each skill's instructions. +/// Works across every provider variant by going through the OpenAI message +/// shape (`get_messages`/`set_messages`). +/// +/// When there is already a leading system message we augment it in place; +/// otherwise a new system message is inserted at position 0. No-op when +/// `activated_skills` is empty. +pub fn inject_activated_skills_into_request( + client_request: &mut ProviderRequestType, + activated_skills: &[SkillRef], +) { + if activated_skills.is_empty() { + return; + } + + let skill_refs: Vec<&SkillRef> = activated_skills.iter().collect(); + + let mut messages = client_request.get_messages(); + + let (system_idx, base_text) = match messages.iter().position(|m| m.role == Role::System) { + Some(idx) => { + let text = messages[idx] + .content + .as_ref() + .map(|c| c.extract_text()) + .unwrap_or_default(); + (Some(idx), Some(text)) + } + None => (None, None), + }; + + let augmented = augment_system_prompt_with_skills(base_text, &skill_refs); + let Some(augmented_text) = augmented else { + return; + }; + + match system_idx { + Some(idx) => { + messages[idx].content = Some(MessageContent::Text(augmented_text)); + } + None => { + messages.insert( + 0, + Message { + role: Role::System, + content: Some(MessageContent::Text(augmented_text)), + name: None, + tool_calls: None, + tool_call_id: None, + }, + ); + } + } + + client_request.set_messages(&messages); +} diff --git a/crates/brightstaff/src/main.rs b/crates/brightstaff/src/main.rs index b1e17e42b..e5c503aa4 100644 --- a/crates/brightstaff/src/main.rs +++ b/crates/brightstaff/src/main.rs @@ -314,11 +314,12 @@ async fn init_app_state( .orchestrator_model_context_length .unwrap_or(brightstaff::router::orchestrator_model_v1::MAX_TOKEN_LEN); - let orchestrator_service = Arc::new(OrchestratorService::with_routing( + let orchestrator_service = Arc::new(OrchestratorService::with_routing_and_skills( format!("{llm_provider_url}{CHAT_COMPLETIONS_PATH}"), orchestrator_model_name, orchestrator_llm_provider, config.routing_preferences.clone(), + config.skills.clone(), metrics_service, session_ttl_seconds, session_cache, diff --git a/crates/brightstaff/src/router/orchestrator.rs b/crates/brightstaff/src/router/orchestrator.rs index 2d7b25dee..31bf8e997 100644 --- a/crates/brightstaff/src/router/orchestrator.rs +++ b/crates/brightstaff/src/router/orchestrator.rs @@ -1,7 +1,9 @@ use std::{borrow::Cow, collections::HashMap, sync::Arc, time::Duration}; use common::{ - configuration::{AgentUsagePreference, OrchestrationPreference, TopLevelRoutingPreference}, + configuration::{ + AgentUsagePreference, OrchestrationPreference, SkillRef, TopLevelRoutingPreference, + }, consts::{ARCH_PROVIDER_HINT_HEADER, REQUEST_ID_HEADER}, }; use hermesllm::apis::openai::Message; @@ -13,7 +15,7 @@ use tracing::{debug, info}; use super::http::{self, post_and_extract_content}; use super::model_metrics::ModelMetricsService; -use super::orchestrator_model::OrchestratorModel; +use super::orchestrator_model::{OrchestratorModel, OrchestratorSelection}; use crate::metrics as bs_metrics; use crate::metrics::labels as metric_labels; @@ -30,12 +32,27 @@ pub struct OrchestratorService { orchestrator_model: Arc, orchestrator_provider_name: String, top_level_preferences: HashMap, + /// Agent Skills catalog (deduplicated by name) attached to any + /// `routing_preferences[].skills` list. Empty when no route has skills. + skills_catalog: Vec, metrics_service: Option>, session_cache: Option>, session_ttl: Duration, tenant_header: Option, } +/// Result of `determine_route`: which route was picked, the ranked candidate +/// models for that route, and the Agent Skill bodies the orchestrator chose +/// to activate alongside it. Skills are resolved against +/// `routing_preferences[].skills`, so unknown / cross-route names are +/// silently dropped. +#[derive(Debug, Clone, Default)] +pub struct RouteDecision { + pub route_name: String, + pub models: Vec, + pub activated_skills: Vec, +} + #[derive(Debug, Error)] pub enum OrchestrationError { #[error(transparent)] @@ -66,6 +83,7 @@ impl OrchestratorService { orchestrator_model, orchestrator_provider_name, top_level_preferences: HashMap::new(), + skills_catalog: Vec::new(), metrics_service: None, session_cache: None, session_ttl: Duration::from_secs(DEFAULT_SESSION_TTL_SECONDS), @@ -84,14 +102,53 @@ impl OrchestratorService { session_cache: Arc, tenant_header: Option, max_token_length: usize, + ) -> Self { + Self::with_routing_and_skills( + orchestrator_url, + orchestration_model_name, + orchestrator_provider_name, + top_level_prefs, + None, + metrics_service, + session_ttl_seconds, + session_cache, + tenant_header, + max_token_length, + ) + } + + /// Like `with_routing`, but also seeds the orchestrator with a catalog of + /// Agent Skills referenced by `routing_preferences[].skills`. The + /// orchestrator gets a `` block in its system prompt and may + /// select zero or more skills alongside the picked route; this enables + /// the LLM handler to inject the chosen SKILL.md bodies into the + /// upstream request. + #[allow(clippy::too_many_arguments)] + pub fn with_routing_and_skills( + orchestrator_url: String, + orchestration_model_name: String, + orchestrator_provider_name: String, + top_level_prefs: Option>, + skills_catalog: Option>, + metrics_service: Option>, + session_ttl_seconds: Option, + session_cache: Arc, + tenant_header: Option, + max_token_length: usize, ) -> Self { let top_level_preferences: HashMap = top_level_prefs .map_or_else(HashMap::new, |prefs| { prefs.into_iter().map(|p| (p.name.clone(), p)).collect() }); - let orchestrator_model = Arc::new(orchestrator_model_v1::OrchestratorModelV1::new( + let skills_catalog = build_skills_catalog_for_routes( + skills_catalog.as_deref().unwrap_or(&[]), + &top_level_preferences, + ); + + let orchestrator_model = Arc::new(orchestrator_model_v1::OrchestratorModelV1::with_skills( HashMap::new(), + skills_catalog.clone(), orchestration_model_name, max_token_length, )); @@ -105,6 +162,7 @@ impl OrchestratorService { orchestrator_model, orchestrator_provider_name, top_level_preferences, + skills_catalog, metrics_service, session_cache: Some(session_cache), session_ttl, @@ -170,7 +228,7 @@ impl OrchestratorService { messages: &[Message], inline_routing_preferences: Option>, request_id: &str, - ) -> Result)>> { + ) -> Result> { if messages.is_empty() { return Ok(None); } @@ -206,9 +264,13 @@ impl OrchestratorService { ) .await?; - let result = if let Some(ref routes) = orchestration_result { - if routes.len() > 1 { - let all_routes: Vec<&str> = routes.iter().map(|(name, _)| name.as_str()).collect(); + let result = if let Some(ref selection) = orchestration_result { + if selection.routes.len() > 1 { + let all_routes: Vec<&str> = selection + .routes + .iter() + .map(|(name, _)| name.as_str()) + .collect(); info!( routes = ?all_routes, using = %all_routes.first().unwrap_or(&"none"), @@ -216,7 +278,7 @@ impl OrchestratorService { ); } - if let Some((route_name, _)) = routes.first() { + if let Some((route_name, _)) = selection.routes.first() { let top_pref = inline_top_map .as_ref() .and_then(|m| m.get(route_name)) @@ -227,7 +289,16 @@ impl OrchestratorService { Some(svc) => svc.rank_models(&pref.models, &pref.selection_policy).await, None => pref.models.clone(), }; - Some((route_name.clone(), ranked)) + let activated_skills = resolve_activated_skills( + &self.skills_catalog, + pref.skills.as_deref().unwrap_or(&[]), + &selection.skills, + ); + Some(RouteDecision { + route_name: route_name.clone(), + models: ranked, + activated_skills, + }) } else { None } @@ -239,7 +310,7 @@ impl OrchestratorService { }; info!( - selected_model = ?result, + selected_route = ?result.as_ref().map(|r| (&r.route_name, r.models.first(), r.activated_skills.iter().map(|s| s.name.as_str()).collect::>())), "plano-orchestrator determined route" ); @@ -253,7 +324,7 @@ impl OrchestratorService { messages: &[Message], usage_preferences: Option>, request_id: Option, - ) -> Result>> { + ) -> Result> { if messages.is_empty() { return Ok(None); } @@ -328,6 +399,61 @@ impl OrchestratorService { } } +/// Build the orchestrator-visible skills catalog (deduplicated by name) from +/// the union of every skill name referenced under +/// `routing_preferences[].skills`. Skills that are not referenced by any +/// route are excluded — they would just clutter the prompt with no way for +/// the orchestrator to attach them to a route. +fn build_skills_catalog_for_routes( + catalog: &[SkillRef], + routes: &HashMap, +) -> Vec { + let mut referenced: std::collections::HashSet<&str> = std::collections::HashSet::new(); + for route in routes.values() { + if let Some(names) = route.skills.as_ref() { + for name in names { + referenced.insert(name.as_str()); + } + } + } + + let mut out: Vec = Vec::new(); + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + for skill in catalog { + if referenced.contains(skill.name.as_str()) && seen.insert(skill.name.clone()) { + out.push(skill.clone()); + } + } + out +} + +/// Filter the orchestrator-selected skill names down to the SKILL.md bodies +/// allowed for the chosen route, preserving the order the orchestrator +/// returned. Unknown names (either not in the catalog or not allowed by the +/// route) are silently dropped; the orchestrator can hallucinate. +fn resolve_activated_skills( + catalog: &[SkillRef], + route_allowlist: &[String], + selected: &[String], +) -> Vec { + let allowed: std::collections::HashSet<&str> = + route_allowlist.iter().map(String::as_str).collect(); + let mut out: Vec = Vec::with_capacity(selected.len()); + let mut taken: std::collections::HashSet<&str> = std::collections::HashSet::new(); + for name in selected { + if !allowed.contains(name.as_str()) { + continue; + } + if !taken.insert(name.as_str()) { + continue; + } + if let Some(skill) = catalog.iter().find(|s| &s.name == name) { + out.push(skill.clone()); + } + } + out +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/brightstaff/src/router/orchestrator_model.rs b/crates/brightstaff/src/router/orchestrator_model.rs index a6b32b8e9..25309ddec 100644 --- a/crates/brightstaff/src/router/orchestrator_model.rs +++ b/crates/brightstaff/src/router/orchestrator_model.rs @@ -10,20 +10,37 @@ pub enum OrchestratorModelError { pub type Result = std::result::Result; +/// The result of running Plano-Orchestrator over a conversation: zero or more +/// selected routes (each mapped to its upstream model name) plus zero or more +/// selected Agent Skills. Skills are filtered down by the consumer to the +/// catalog defined under `routing_preferences[].skills` for the chosen route. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct OrchestratorSelection { + pub routes: Vec<(String, String)>, + pub skills: Vec, +} + +impl OrchestratorSelection { + pub fn is_empty(&self) -> bool { + self.routes.is_empty() && self.skills.is_empty() + } +} + /// OrchestratorModel trait for handling orchestration requests. -/// Returns multiple routes as the model output format is: -/// {"route": ["route_name_1", "route_name_2", ...]} +/// Returns multiple routes and skills as the model output format is: +/// {"route": ["route_name_1", ...], "skills": ["skill_name_1", ...]} pub trait OrchestratorModel: Send + Sync { fn generate_request( &self, messages: &[Message], usage_preferences: &Option>, ) -> ChatCompletionsRequest; - /// Returns a vector of (route_name, model_name) tuples for all matched routes. + /// Parses the orchestrator's raw model output into selected routes (each + /// mapped to a model) and selected skill names. fn parse_response( &self, content: &str, usage_preferences: &Option>, - ) -> Result>>; + ) -> Result>; fn get_model_name(&self) -> String; } diff --git a/crates/brightstaff/src/router/orchestrator_model_v1.rs b/crates/brightstaff/src/router/orchestrator_model_v1.rs index 693aacc28..80bb241d6 100644 --- a/crates/brightstaff/src/router/orchestrator_model_v1.rs +++ b/crates/brightstaff/src/router/orchestrator_model_v1.rs @@ -1,12 +1,12 @@ use std::collections::HashMap; -use common::configuration::{AgentUsagePreference, OrchestrationPreference}; +use common::configuration::{AgentUsagePreference, OrchestrationPreference, SkillRef}; use hermesllm::apis::openai::{ChatCompletionsRequest, Message, MessageContent, Role}; use hermesllm::transforms::lib::ExtractText; use serde::{ser::Serialize as SerializeTrait, Deserialize, Serialize}; use tracing::{debug, warn}; -use super::orchestrator_model::{OrchestratorModel, OrchestratorModelError}; +use super::orchestrator_model::{OrchestratorModel, OrchestratorModelError, OrchestratorSelection}; pub const MAX_TOKEN_LEN: usize = 8192; // Default max token length for the orchestration model @@ -138,10 +138,47 @@ Return your answer strictly in JSON as follows: If no routes are needed, return an empty list for `route`. "#; +/// System prompt used when one or more Agent Skills are attached to candidate +/// routes. Adds a `` block alongside `` and asks the model to +/// also pick zero or more skills that should be loaded into the downstream +/// LLM's system prompt. +pub const ARCH_ORCHESTRATOR_V1_SYSTEM_PROMPT_WITH_SKILLS: &str = r#" +You are a helpful assistant that selects the most suitable routes and Agent Skills based on user intent. +You are provided with a list of available routes enclosed within XML tags: + +{routes} + + +You are provided with a list of available Agent Skills enclosed within XML tags: + +{skills} + + +You are also given the conversation context enclosed within XML tags: + +{conversation} + + +## Instructions +1. Analyze the latest user intent from the conversation. +2. Compare it against the available routes to find which routes can help fulfill the request. +3. Independently compare it against the available skills; pick the skills whose descriptions match what the user is trying to do. Skills can be combined with any route. Activating a skill loads detailed instructions into the next response's system prompt. +4. Respond only with exact names from and . +5. If no routes or skills can help, return empty lists. + +## Response Format +Return your answer strictly in JSON as follows: +{{"route": ["route_name_1", "..."], "skills": ["skill_name_1", "..."]}} +Use empty lists for `route` and/or `skills` when nothing applies. +"#; + pub type Result = std::result::Result; pub struct OrchestratorModelV1 { agent_orchestration_json_str: String, agent_orchestration_to_model_map: HashMap, + /// Pre-rendered `` block (one JSON entry per skill, name + + /// description). Empty when no skills are attached to any route. + skills_catalog_json_str: String, orchestration_model: String, max_token_length: usize, } @@ -151,10 +188,27 @@ impl OrchestratorModelV1 { agent_orchestrations: HashMap>, orchestration_model: String, max_token_length: usize, + ) -> Self { + Self::with_skills( + agent_orchestrations, + Vec::new(), + orchestration_model, + max_token_length, + ) + } + + /// Like `new`, but additionally seeds the orchestrator with an Agent + /// Skills catalog. When `skills_catalog` is empty the orchestrator uses + /// the routes-only system prompt; otherwise it asks the model to also + /// pick zero or more skills from the catalog. + pub fn with_skills( + agent_orchestrations: HashMap>, + skills_catalog: Vec, + orchestration_model: String, + max_token_length: usize, ) -> Self { let agent_orchestration_values: Vec = agent_orchestrations.values().flatten().cloned().collect(); - // Format routes: each route as JSON on its own line with standard spacing let agent_orchestration_json_str = agent_orchestration_values .iter() .map(to_spaced_json) @@ -165,19 +219,48 @@ impl OrchestratorModelV1 { .flat_map(|(model, prefs)| prefs.iter().map(|pref| (pref.name.clone(), model.clone()))) .collect(); + let skills_catalog_json_str = render_skills_catalog(&skills_catalog); + OrchestratorModelV1 { orchestration_model, max_token_length, agent_orchestration_json_str, agent_orchestration_to_model_map, + skills_catalog_json_str, } } } +/// JSON shape suitable for the `` block in the orchestrator prompt: +/// `{"name": "...", "description": "..."}`. Only metadata that helps the +/// orchestrator pick a skill is included; the full SKILL.md body is injected +/// separately, after a skill has been selected. +#[derive(Debug, Clone, Serialize)] +struct SkillCatalogEntry<'a> { + name: &'a str, + description: &'a str, +} + +fn render_skills_catalog(skills: &[SkillRef]) -> String { + skills + .iter() + .map(|s| SkillCatalogEntry { + name: &s.name, + description: s.catalog_description(), + }) + .map(|entry| to_spaced_json(&entry)) + .collect::>() + .join("\n") +} + #[derive(Debug, Clone, Serialize, Deserialize)] struct AgentOrchestratorResponse { - /// The route field now expects an array of route names: ["route_name_1", "route_name_2", ...] + /// The route field expects an array of route names: ["route_name_1", "route_name_2", ...]. pub route: Option>, + /// Optional array of Agent Skill names the orchestrator chose to activate. + /// Absent or empty when no skills should be loaded for this turn. + #[serde(default)] + pub skills: Option>, } const TOKEN_LENGTH_DIVISOR: usize = 4; // Approximate token length divisor for UTF-8 characters @@ -209,7 +292,13 @@ impl OrchestratorModel for OrchestratorModelV1 { // Ensure the conversation does not exceed the configured token budget. // We use `len() / TOKEN_LENGTH_DIVISOR` as a cheap token estimate to // avoid running a real tokenizer on the hot path. - let mut token_count = ARCH_ORCHESTRATOR_V1_SYSTEM_PROMPT.len() / TOKEN_LENGTH_DIVISOR; + let template_len = if self.skills_catalog_json_str.is_empty() { + ARCH_ORCHESTRATOR_V1_SYSTEM_PROMPT.len() + } else { + ARCH_ORCHESTRATOR_V1_SYSTEM_PROMPT_WITH_SKILLS.len() + + self.skills_catalog_json_str.len() + }; + let mut token_count = template_len / TOKEN_LENGTH_DIVISOR; let mut selected_messages_list_reversed: Vec = vec![]; for (selected_messsage_count, message) in messages_vec.iter().rev().enumerate() { let message_text = message.content.extract_text(); @@ -289,14 +378,16 @@ impl OrchestratorModel for OrchestratorModelV1 { // Generate the orchestrator request message based on the usage preferences. // If preferences are passed in request then we use them; // Otherwise, we use the default orchestration modelpreferences. - let orchestrator_message = - match convert_to_orchestrator_preferences(usage_preferences_from_request) { - Some(prefs) => generate_orchestrator_message(&prefs, &selected_conversation_list), - None => generate_orchestrator_message( - &self.agent_orchestration_json_str, - &selected_conversation_list, - ), - }; + let routes_block = match convert_to_orchestrator_preferences(usage_preferences_from_request) + { + Some(prefs) => prefs, + None => self.agent_orchestration_json_str.clone(), + }; + let orchestrator_message = generate_orchestrator_message( + &routes_block, + &self.skills_catalog_json_str, + &selected_conversation_list, + ); ChatCompletionsRequest { model: self.orchestration_model.clone(), @@ -316,7 +407,7 @@ impl OrchestratorModel for OrchestratorModelV1 { &self, content: &str, usage_preferences: &Option>, - ) -> Result>> { + ) -> Result> { if content.is_empty() { return Ok(None); } @@ -326,20 +417,21 @@ impl OrchestratorModel for OrchestratorModelV1 { let selected_routes = orchestrator_response.route.unwrap_or_default(); - // Filter out empty routes let valid_routes: Vec = selected_routes .into_iter() .filter(|route| !route.is_empty()) .collect(); - if valid_routes.is_empty() { - return Ok(None); - } + let selected_skills: Vec = orchestrator_response + .skills + .unwrap_or_default() + .into_iter() + .filter(|s| !s.is_empty()) + .collect(); - let mut result: Vec<(String, String)> = Vec::new(); + let mut routes: Vec<(String, String)> = Vec::new(); if let Some(usage_preferences) = usage_preferences { - // If usage preferences are defined, we need to find the model that matches each selected route for selected_route in valid_routes { let model_name: Option = usage_preferences .iter() @@ -351,7 +443,7 @@ impl OrchestratorModel for OrchestratorModelV1 { .map(|pref| pref.model.clone()); if let Some(model_name) = model_name { - result.push((selected_route, model_name)); + routes.push((selected_route, model_name)); } else { warn!( route = %selected_route, @@ -361,14 +453,13 @@ impl OrchestratorModel for OrchestratorModelV1 { } } } else { - // If no usage preferences are passed in request then use the default orchestration model preferences for selected_route in valid_routes { if let Some(model) = self .agent_orchestration_to_model_map .get(&selected_route) .cloned() { - result.push((selected_route, model)); + routes.push((selected_route, model)); } else { warn!( route = %selected_route, @@ -379,11 +470,14 @@ impl OrchestratorModel for OrchestratorModelV1 { } } - if result.is_empty() { + if routes.is_empty() && selected_skills.is_empty() { return Ok(None); } - Ok(Some(result)) + Ok(Some(OrchestratorSelection { + routes, + skills: selected_skills, + })) } fn get_model_name(&self) -> String { @@ -391,7 +485,11 @@ impl OrchestratorModel for OrchestratorModelV1 { } } -fn generate_orchestrator_message(prefs: &str, selected_conversation_list: &Vec) -> String { +fn generate_orchestrator_message( + prefs: &str, + skills_catalog: &str, + selected_conversation_list: &Vec, +) -> String { // Format conversation with 4-space indentation (equivalent to Python's json.dumps(obj, indent=4)) let formatter = serde_json::ser::PrettyFormatter::with_indent(b" "); let mut conversation_buf = Vec::new(); @@ -399,9 +497,19 @@ fn generate_orchestrator_message(prefs: &str, selected_conversation_list: &Vec OrchestratorSelection { + OrchestratorSelection { + routes: pairs + .iter() + .map(|(r, m)| (r.to_string(), m.to_string())) + .collect(), + skills: Vec::new(), + } + } + // Case 1: Valid JSON with single route in array let input = r#"{"route": ["Image generation"]}"#; let result = orchestrator.parse_response(input, &None).unwrap(); - assert_eq!( - result, - Some(vec![("Image generation".to_string(), "gpt-4o".to_string())]) - ); + assert_eq!(result, Some(routes(&[("Image generation", "gpt-4o")]))); // Case 2: Valid JSON with multiple routes in array let input = r#"{"route": ["Image generation", "Code generation"]}"#; let result = orchestrator.parse_response(input, &None).unwrap(); assert_eq!( result, - Some(vec![ - ("Image generation".to_string(), "gpt-4o".to_string()), - ("Code generation".to_string(), "gpt-4o".to_string()) - ]) + Some(routes(&[ + ("Image generation", "gpt-4o"), + ("Code generation", "gpt-4o") + ])) ); // Case 3: Valid JSON with empty array @@ -1396,14 +1511,65 @@ If no routes are needed, return an empty list for `route`. // Case 7: Single quotes and \n in JSON let input = "{'route': ['Image generation']}\\n"; let result = orchestrator.parse_response(input, &None).unwrap(); - assert_eq!( - result, - Some(vec![("Image generation".to_string(), "gpt-4o".to_string())]) - ); + assert_eq!(result, Some(routes(&[("Image generation", "gpt-4o")]))); // Case 8: Array with unknown route (not in orchestrations map) let input = r#"{"route": ["Unknown route"]}"#; let result = orchestrator.parse_response(input, &None).unwrap(); assert_eq!(result, None); + + // Case 9: Routes plus selected skills are propagated through. + let input = r#"{"route": ["Image generation"], "skills": ["pdf-processing"]}"#; + let result = orchestrator.parse_response(input, &None).unwrap().unwrap(); + assert_eq!(result.routes.len(), 1); + assert_eq!(result.skills, vec!["pdf-processing".to_string()]); + + // Case 10: Skills-only selection (no routes) still surfaces as Some. + let input = r#"{"route": [], "skills": ["pdf-processing"]}"#; + let result = orchestrator.parse_response(input, &None).unwrap().unwrap(); + assert!(result.routes.is_empty()); + assert_eq!(result.skills, vec!["pdf-processing".to_string()]); + } + + #[test] + fn test_system_prompt_with_skills_block() { + let orchestrator = OrchestratorModelV1::with_skills( + HashMap::from([( + "gpt-4o".to_string(), + vec![OrchestrationPreference { + name: "Image generation".to_string(), + description: "generating image".to_string(), + }], + )]), + vec![SkillRef { + name: "pdf-processing".to_string(), + description: "Extract structured data from PDFs.".to_string(), + path: None, + base_dir: None, + body: None, + scope: None, + compatibility: None, + license: None, + metadata: None, + allowed_tools: None, + }], + "test-model".to_string(), + usize::MAX, + ); + + let conversation: Vec = serde_json::from_str( + r#"[{"role": "user", "content": "extract the invoice totals from this pdf"}]"#, + ) + .unwrap(); + let req = orchestrator.generate_request(&conversation, &None); + let prompt = req.messages[0].content.extract_text(); + + assert!(prompt.contains("")); + assert!(prompt.contains("")); + assert!(prompt.contains(r#""name": "pdf-processing""#)); + assert!(prompt.contains("Extract structured data from PDFs.")); + // Response format documentation must mention the `skills` array + // so the orchestrator emits it. + assert!(prompt.contains(r#""skills""#)); } } diff --git a/crates/brightstaff/src/router/stress_tests.rs b/crates/brightstaff/src/router/stress_tests.rs index 6c3ffefd4..73bcc8e7a 100644 --- a/crates/brightstaff/src/router/stress_tests.rs +++ b/crates/brightstaff/src/router/stress_tests.rs @@ -33,6 +33,7 @@ mod tests { "openai/gpt-4o".to_string(), "openai/gpt-4o-mini".to_string(), ], + skills: None, selection_policy: SelectionPolicy { prefer: SelectionPreference::None, }, @@ -44,6 +45,7 @@ mod tests { "anthropic/claude-3-sonnet".to_string(), "openai/gpt-4o-mini".to_string(), ], + skills: None, selection_policy: SelectionPolicy { prefer: SelectionPreference::None, }, diff --git a/crates/common/src/configuration.rs b/crates/common/src/configuration.rs index 37492904d..8b936d62f 100644 --- a/crates/common/src/configuration.rs +++ b/crates/common/src/configuration.rs @@ -163,6 +163,12 @@ pub struct TopLevelRoutingPreference { pub name: String, pub description: String, pub models: Vec, + /// Agent Skills associated with this route. When Plano-Orchestrator + /// selects this route, every skill listed here is also offered to the + /// orchestrator in the `` block; selected skills have their + /// SKILL.md body prepended to the upstream system prompt. + #[serde(default)] + pub skills: Option>, #[serde(default)] pub selection_policy: SelectionPolicy, } @@ -224,6 +230,17 @@ pub struct Configuration { pub state_storage: Option, pub routing_preferences: Option>, pub model_metrics_sources: Option>, + /// Agent Skills (https://agentskills.io) installed for this project. + /// + /// The Plano CLI discovers `.plano/skills//SKILL.md` files at render + /// time and materializes them into this list with `body` already loaded so + /// downstream consumers do not need filesystem access. Skills are scoped + /// to specific routes via `routing_preferences[].skills`; Plano-Orchestrator + /// receives a `` block alongside `` for any skills attached + /// to candidate routes, and selected skills have their SKILL.md body + /// injected into the upstream system prompt. + #[serde(default)] + pub skills: Option>, } #[derive(Debug, Clone, Serialize, Deserialize, Default)] @@ -611,6 +628,45 @@ pub struct PromptTarget { pub auto_llm_dispatch_on_response: Option, } +/// An Agent Skill (https://agentskills.io) as materialized by the Plano CLI. +/// +/// At runtime brightstaff and the WASM filters reason over the catalog +/// (`name` + `description`) and, when a skill is selected, inject the +/// pre-loaded `body` into the downstream system prompt. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct SkillRef { + pub name: String, + pub description: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub base_dir: Option, + /// Full SKILL.md markdown body (post-frontmatter). Inlined here at render + /// time so the WASM sandbox does not need filesystem access. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub body: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub scope: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub compatibility: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub license: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub metadata: Option>, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub allowed_tools: Option, +} + +impl SkillRef { + /// Best-effort short summary suitable for the `` block sent to + /// Plano-Orchestrator: only the public-facing description, never the + /// full SKILL.md body. The body is injected separately, after a skill + /// has been selected. + pub fn catalog_description(&self) -> &str { + &self.description + } +} + // convert PromptTarget to ChatCompletionTool impl From<&PromptTarget> for ChatCompletionTool { fn from(val: &PromptTarget) -> Self { @@ -807,4 +863,34 @@ disable_signals: false let overrides: super::Overrides = serde_yaml::from_str(yaml_missing).unwrap(); assert_eq!(overrides.disable_signals, None); } + + #[test] + fn test_top_level_routing_preference_skills_deserialize() { + let yaml = r#" +name: code review +description: reviewing, analyzing, and suggesting improvements to existing code +models: + - openai/gpt-4o +skills: + - code-review-skill +"#; + let pref: super::TopLevelRoutingPreference = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(pref.name, "code review"); + assert_eq!( + pref.skills.as_deref(), + Some(&["code-review-skill".to_string()][..]) + ); + } + + #[test] + fn test_top_level_routing_preference_skills_optional() { + let yaml = r#" +name: code generation +description: generating new code +models: + - openai/gpt-4o +"#; + let pref: super::TopLevelRoutingPreference = serde_yaml::from_str(yaml).unwrap(); + assert!(pref.skills.is_none()); + } } diff --git a/crates/common/src/lib.rs b/crates/common/src/lib.rs index aba27b9b2..eb8beb5b5 100644 --- a/crates/common/src/lib.rs +++ b/crates/common/src/lib.rs @@ -8,6 +8,7 @@ pub mod path; pub mod pii; pub mod ratelimit; pub mod routing; +pub mod skills_runtime; pub mod stats; pub mod tokenizer; pub mod traces; diff --git a/crates/common/src/skills_runtime.rs b/crates/common/src/skills_runtime.rs new file mode 100644 index 000000000..4e9d4aabe --- /dev/null +++ b/crates/common/src/skills_runtime.rs @@ -0,0 +1,215 @@ +//! Runtime helpers for handling Agent Skills selected by Plano-Orchestrator. +//! +//! These functions live in `common` (rather than `brightstaff` or a WASM +//! crate) so they can be unit-tested on the native target without dragging +//! in proxy-wasm host-call symbols or tokio runtime dependencies. + +use crate::configuration::{SkillRef, TopLevelRoutingPreference}; + +/// Filter `skills` down to the subset attached to `route_name` via +/// `routing_preferences[].skills`. When the selected route has no `skills:` +/// list, returns an empty vector — skills are scoped to routes, not global. +/// +/// `routing_preferences` is the source of truth for which skills are even +/// eligible for the orchestrator to activate on a given route. +pub fn skills_for_route<'a>( + skills: &'a [SkillRef], + routing_preferences: &[TopLevelRoutingPreference], + route_name: &str, +) -> Vec<&'a SkillRef> { + let Some(route) = routing_preferences.iter().find(|p| p.name == route_name) else { + return Vec::new(); + }; + let Some(allow) = route.skills.as_ref() else { + return Vec::new(); + }; + let mut out: Vec<&SkillRef> = Vec::with_capacity(allow.len()); + for name in allow { + if let Some(skill) = skills.iter().find(|s| &s.name == name) { + out.push(skill); + } + } + out +} + +/// Resolve a list of orchestrator-selected skill names to their `SkillRef`s. +/// Unknown names are dropped silently — the orchestrator can hallucinate. +/// Results are deduplicated by name, preserving the order Plano-Orchestrator +/// returned. +pub fn resolve_selected_skills<'a>( + skills: &'a [SkillRef], + selected_names: &[String], +) -> Vec<&'a SkillRef> { + let mut out: Vec<&SkillRef> = Vec::with_capacity(selected_names.len()); + for name in selected_names { + if out.iter().any(|s| &s.name == name) { + continue; + } + if let Some(skill) = skills.iter().find(|s| &s.name == name) { + out.push(skill); + } + } + out +} + +/// Append the bodies of activated skills to a system prompt, wrapped in +/// `` tags so a future context-management pass can +/// recognize and recompact them. +/// +/// Returns `None` only if no base system prompt was supplied and no skills +/// were activated. When skills are present the wrapper text always appears so +/// the downstream model receives a clear, well-structured instruction block. +pub fn augment_system_prompt_with_skills( + base_system_prompt: Option, + activated_skills: &[&SkillRef], +) -> Option { + if activated_skills.is_empty() { + return base_system_prompt; + } + let mut buf = String::new(); + if let Some(base) = base_system_prompt { + if !base.is_empty() { + buf.push_str(&base); + buf.push('\n'); + buf.push('\n'); + } + } + buf.push_str( + "The following Agent Skills have been activated for this request. \ + Follow their instructions when relevant; resolve relative paths \ + against each skill's base directory.\n\n", + ); + for skill in activated_skills { + buf.push_str(&format!("\n"); + if let Some(body) = skill.body.as_deref() { + buf.push_str(body.trim_end()); + buf.push('\n'); + } else { + buf.push_str(&format!("(skill description) {}\n", skill.description)); + } + buf.push_str("\n\n"); + } + Some(buf.trim_end().to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::configuration::SelectionPolicy; + + fn skill(name: &str, body: &str) -> SkillRef { + SkillRef { + name: name.to_string(), + description: format!("desc for {}", name), + path: Some(format!("/skills/{}/SKILL.md", name)), + base_dir: Some(format!("/skills/{}", name)), + body: Some(body.to_string()), + scope: Some("project".to_string()), + compatibility: None, + license: None, + metadata: None, + allowed_tools: None, + } + } + + fn route(name: &str, skill_names: Option>) -> TopLevelRoutingPreference { + TopLevelRoutingPreference { + name: name.to_string(), + description: format!("desc for {}", name), + models: vec!["openai/gpt-4o".to_string()], + skills: skill_names.map(|v| v.into_iter().map(String::from).collect()), + selection_policy: SelectionPolicy::default(), + } + } + + #[test] + fn skills_for_route_returns_attached_skills() { + let catalog = vec![ + skill("pdf-processing", "extract"), + skill("code-review", "review"), + ]; + let routes = vec![ + route("code review", Some(vec!["code-review"])), + route("doc work", Some(vec!["pdf-processing"])), + ]; + let resolved = skills_for_route(&catalog, &routes, "code review"); + assert_eq!(resolved.len(), 1); + assert_eq!(resolved[0].name, "code-review"); + } + + #[test] + fn skills_for_route_empty_when_route_has_no_skills_list() { + let catalog = vec![skill("pdf-processing", "extract")]; + let routes = vec![route("code review", None)]; + let resolved = skills_for_route(&catalog, &routes, "code review"); + assert!(resolved.is_empty()); + } + + #[test] + fn skills_for_route_empty_when_route_missing() { + let catalog = vec![skill("pdf-processing", "extract")]; + let routes = vec![route("code review", Some(vec!["pdf-processing"]))]; + let resolved = skills_for_route(&catalog, &routes, "no-such-route"); + assert!(resolved.is_empty()); + } + + #[test] + fn skills_for_route_drops_unknown_skill_names() { + let catalog = vec![skill("pdf-processing", "extract")]; + let routes = vec![route( + "code review", + Some(vec!["pdf-processing", "ghost-skill"]), + )]; + let resolved = skills_for_route(&catalog, &routes, "code review"); + assert_eq!(resolved.len(), 1); + assert_eq!(resolved[0].name, "pdf-processing"); + } + + #[test] + fn resolve_selected_skills_drops_unknown_and_dedupes() { + let catalog = vec![ + skill("pdf-processing", "extract"), + skill("code-review", "review"), + ]; + let names = vec![ + "code-review".to_string(), + "ghost".to_string(), + "code-review".to_string(), + "pdf-processing".to_string(), + ]; + let resolved = resolve_selected_skills(&catalog, &names); + assert_eq!(resolved.len(), 2); + assert_eq!(resolved[0].name, "code-review"); + assert_eq!(resolved[1].name, "pdf-processing"); + } + + #[test] + fn augment_passthrough_with_no_skills() { + let augmented = augment_system_prompt_with_skills(Some("you are helpful".to_string()), &[]); + assert_eq!(augmented.as_deref(), Some("you are helpful")); + } + + #[test] + fn augment_includes_skill_bodies() { + let s = skill("pdf-processing", "extract text and tables"); + let augmented = + augment_system_prompt_with_skills(Some("you are helpful".to_string()), &[&s]) + .expect("augmented"); + assert!(augmented.starts_with("you are helpful")); + assert!(augmented.contains(" {} - Some(system_prompt) => { - let system_prompt_message = Message { - role: SYSTEM_ROLE.to_string(), - content: Some(ContentType::Text(system_prompt.clone())), - model: None, - tool_calls: None, - tool_call_id: None, - }; - messages.push(system_prompt_message); - } + if let Some(system_prompt) = self.system_prompt.as_ref().clone() { + let system_prompt_message = Message { + role: SYSTEM_ROLE.to_string(), + content: Some(ContentType::Text(system_prompt)), + model: None, + tool_calls: None, + tool_call_id: None, + }; + messages.push(system_prompt_message); } messages.append( diff --git a/docs/source/index.rst b/docs/source/index.rst index 7a2e5b603..cfe210d93 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -63,4 +63,5 @@ Built by contributors to the widely adopted `Envoy Proxy `_ — lightweight, +markdown-defined capabilities — and let Plano-Orchestrator decide *per request* +which skills to attach to the downstream LLM call. Skills attach to entries in +``routing_preferences``: when the orchestrator picks a route, it also picks +zero or more skills from that route's allow-list, and brightstaff injects +each selected ``SKILL.md`` body into the upstream system prompt before +forwarding the request. + +Why use this? +------------- + +- **Modular instructions.** Ship a skill (markdown + scripts + assets) rather + than baking 500-token instructions into every system prompt. +- **Progressive disclosure.** Skill names and one-line descriptions are + always visible to the orchestrator; full instructions load only when a + skill is activated. +- **Per-route scoping.** A ``skills:`` list on a ``routing_preferences`` + entry constrains which skills can be activated for that route. + +Install a skill +--------------- + +.. code-block:: bash + + # via the upstream Agent Skills CLI (recommended for multi-skill repos) + planoai skills add openai/skills + + # planoai falls back to a direct git clone if `npx` is unavailable; this + # path expects a single-skill repo with a SKILL.md at the root. + planoai skills add owner/code-review + +Where do skills end up? +~~~~~~~~~~~~~~~~~~~~~~~ + +Plano looks for skills across three scopes (highest precedence first): + +============== ============================== =========================== ===================================== +Scope Location Trust required Typical installer +============== ============================== =========================== ===================================== +``project`` ``.plano/skills//`` Yes — ``planoai skills ``planoai skills add`` (git fallback) + trust`` +``user`` ``~/.plano/skills//`` No (auto-trusted) manual +``agents`` ``~/.agents/skills//`` No (auto-trusted) ``npx skills add`` / upstream CLI +============== ============================== =========================== ===================================== + +The ``agents`` scope is the universal Agent Skills install location used by +``npx skills add`` (see https://github.com/vercel-labs/add-skill). Because +``npx skills add`` doesn't know about Plano, it never writes into +``.plano/skills/``; instead it drops the skill under ``~/.agents/skills/`` +and symlinks it into every recognised agent (Claude Code, Cursor, …). Plano +treats that directory as an auto-trusted user-tier scope, so anything +installed there is picked up automatically — no ``planoai skills trust`` +needed. + +A ``.plano/skills/.skills.json`` manifest is maintained only for installs +that land in project scope (the git fallback). The ``agents`` scope owns +its own bookkeeping in ``~/.agents/``. + +Trust the project +----------------- + +Project-level skills are loaded only after you mark the project trusted. This +matches the recommendation in the `Adding skills support guide +`_: + +.. code-block:: bash + + planoai skills trust + + # revoke trust later if needed + planoai skills trust --revoke + +Skills under ``~/.plano/skills/`` and ``~/.agents/skills/`` are always +trusted and ignore this setting. + +Discover and remove +------------------- + +.. code-block:: bash + + planoai skills list + planoai skills remove pdf-processing + +Configure routing +----------------- + +Reference installed skills from your ``config.yaml`` in two places: + +1. The top-level ``skills:`` catalog (optional — omit to auto-include every + discovered skill). +2. Each ``routing_preferences`` entry that should make a skill eligible for + activation. The orchestrator's ```` block is built from the union + of every ``routing_preferences[].skills`` list; skills not referenced by + any route are silently dropped. + +.. code-block:: yaml + + skills: + - pdf-processing + - code-review + + routing_preferences: + - name: code review + description: | + Reviewing pull requests, analyzing diffs, and suggesting improvements + to existing code. + models: + - anthropic/claude-sonnet-4-5 + - openai/gpt-4.1-2025-04-14 + skills: + - code-review + + - name: document understanding + description: | + Summarizing PDFs and other long-form documents, extracting structured + data such as tables, line items, or signatures. + models: + - anthropic/claude-sonnet-4-5 + skills: + - pdf-processing + selection_policy: + prefer: cheapest + +When ``planoai up`` runs, the CLI walks ``.plano/skills/`` and +``~/.plano/skills/``, parses each ``SKILL.md``, and inlines the markdown body +into the rendered Plano config so the brightstaff orchestrator can attach it +to the request without any filesystem access. + +How routing works +----------------- + +At request time: + +1. The brightstaff routing service builds an ```` block in the + Plano-Orchestrator prompt — alongside the existing ```` block — + listing every skill referenced by ``routing_preferences[].skills`` with + its name and short description. +2. The orchestrator replies with JSON of the form + ``{"route": ["..."], "skills": ["..."]}``. +3. brightstaff resolves each selected skill name against the chosen route's + ``skills:`` allow-list. Names that aren't allowed for that route (or are + not in the catalog) are dropped. +4. The activated ``SkillRef`` bodies are prepended to the upstream request's + system prompt — wrapped in + ``…`` tags — and + the request is forwarded to the chosen model. + +If the orchestrator picks only skills and no route, the request falls back +to the originally-requested model (or the default) and the skill bodies are +injected the same way. + +Bootstrap from the template +--------------------------- + +A ready-made template wires the moving pieces together: + +.. code-block:: bash + + planoai init --template skills_routing + +Out of scope +------------ + +- Hot-reload of ``.plano/skills/`` while Plano is running — re-run + ``planoai up`` to pick up new skills. +- Server-side execution of bundled ``scripts/`` from skills. The upstream + client runs scripts as part of the progressive-disclosure model from the + `specification `_. +- Subagent delegation per skill. See the + `client-implementation guide + `_ + for the advanced pattern. From 5e8d27fd3c088d40a1c1ffb8697f784a4ce10ccf Mon Sep 17 00:00:00 2001 From: Spherrrical Date: Mon, 18 May 2026 12:39:21 -0700 Subject: [PATCH 2/2] fix(skills): honor skills-only orchestrator decisions, dedupe runtime helpers, warn on dropped picks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the code-review findings on 7f5bf641: - Honor skills-only decisions: RouteDecision.route_name is now Option and the orchestrator emits a decision when routes is empty but skills is non-empty. The LLM handler falls back to the originally-requested model and still injects activated skill bodies, matching the contract in docs/source/resources/skills.rst. - Warn on allow-list misses: resolve_for_route now returns a SkillResolution that splits drops into "not allow-listed for this route" vs "not in catalog (hallucinated)". brightstaff logs each bucket so misconfigured routing_preferences[].skills lists become visible instead of vanishing silently. - Consolidate runtime: common::skills_runtime is now the single source of truth (referenced_skills_catalog, resolve_for_route, resolve_selected_skills, augment_system_prompt_with_skills). brightstaff drops its local re-implementations and calls into common. - Tests: 11 new tests in common::skills_runtime (catalog union, allow-list intersection, dedup, hallucination handling, XML escaping, body size cap) and 6 new tests in brightstaff::handlers::llm::model_selection cover inject_activated_skills_into_request, including the first-system-message rule and the Parts->Text flatten — both now documented on the function. - Cap skill body size at 32 KiB with a UTF-8-safe tail-trim + marker so an oversized SKILL.md cannot blow the downstream context window. - XML-escape skill name and base_dir in the wrapper as defense-in-depth (names are validated upstream, but the wrapper sits inside the system prompt). - Bound find_project_root at \$HOME plus a 30-parent depth cap so CLI invocations outside HOME no longer walk to /. --- cli/planoai/skills.py | 51 ++- .../src/handlers/llm/model_selection.rs | 227 +++++++++++- crates/brightstaff/src/router/orchestrator.rs | 174 ++++++---- crates/common/src/skills_runtime.rs | 326 ++++++++++++++---- 4 files changed, 639 insertions(+), 139 deletions(-) diff --git a/cli/planoai/skills.py b/cli/planoai/skills.py index f8a4a9829..367fde859 100644 --- a/cli/planoai/skills.py +++ b/cli/planoai/skills.py @@ -102,24 +102,55 @@ def to_dict(self) -> dict: } -def find_project_root(start: Path | None = None) -> Path: - """Walk up from `start` looking for `.plano/`, then `.git/`. +_MAX_PROJECT_ROOT_WALK_DEPTH = 30 + - Falls back to `start` (or cwd) if nothing is found. This matches how - `npx skills add` chooses a project root. +def find_project_root(start: Path | None = None) -> Path: + """Walk up from ``start`` looking for ``.plano/``, then ``.git/``. + + The walk is bounded so a CLI invocation in a deeply-nested or + pathological directory does not iterate all the way to ``/`` on every + call. Two bounds apply, whichever fires first: + + * **$HOME**: when ``start`` is inside the user's home directory, the + walk stops at ``$HOME`` itself. We never inspect siblings of + ``$HOME`` like ``/Users`` — picking up a stray ``.git/`` there would + be more surprising than helpful. + * **Hard depth cap** (``_MAX_PROJECT_ROOT_WALK_DEPTH`` parents): a + defensive fallback for paths outside ``$HOME`` (e.g. ``/tmp/...``) + so we still terminate quickly on absurdly deep trees. + + Falls back to ``start`` (or cwd) if nothing is found. This matches how + ``npx skills add`` chooses a project root. """ base = Path(start or Path.cwd()).resolve() - cur = base - while cur != cur.parent: + + try: + home = Path(os.path.expanduser("~")).resolve() + except (OSError, RuntimeError): + home = None + + def _ancestors(start_dir: Path) -> list[Path]: + out: list[Path] = [] + cur = start_dir + for _ in range(_MAX_PROJECT_ROOT_WALK_DEPTH + 1): + out.append(cur) + if home is not None and cur == home: + break + if cur == cur.parent: + break + cur = cur.parent + return out + + ancestors = _ancestors(base) + + for cur in ancestors: if (cur / ".plano").exists(): return cur - cur = cur.parent - cur = base - while cur != cur.parent: + for cur in ancestors: if (cur / ".git").exists(): return cur - cur = cur.parent return base diff --git a/crates/brightstaff/src/handlers/llm/model_selection.rs b/crates/brightstaff/src/handlers/llm/model_selection.rs index 5418c5845..088bb288e 100644 --- a/crates/brightstaff/src/handlers/llm/model_selection.rs +++ b/crates/brightstaff/src/handlers/llm/model_selection.rs @@ -137,6 +137,34 @@ pub async fn router_chat_get_upstream_model( match routing_result { Ok(route) => match route { Some(decision) => { + // Skills-only decision (no route, no models) -> fall through + // to the "none" sentinel so the original / aliased model is + // used, but propagate activated_skills so they still get + // injected. Documented at docs/source/resources/skills.rst. + if decision.route_name.is_none() && decision.models.is_empty() { + current_span.record("route.selected_model", "none"); + info!( + skills = ?decision + .activated_skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + "no route determined; activating skills against default model" + ); + bs_metrics::record_router_decision( + route_label, + "none", + true, + determination_elapsed, + ); + return Ok(RoutingResult { + model_name: "none".to_string(), + models: vec!["none".to_string()], + route_name: None, + activated_skills: decision.activated_skills, + }); + } + let model_name = decision.models.first().cloned().unwrap_or_default(); current_span.record("route.selected_model", model_name.as_str()); bs_metrics::record_router_decision( @@ -148,7 +176,7 @@ pub async fn router_chat_get_upstream_model( Ok(RoutingResult { model_name, models: decision.models, - route_name: Some(decision.route_name), + route_name: decision.route_name, activated_skills: decision.activated_skills, }) } @@ -186,11 +214,29 @@ pub async fn router_chat_get_upstream_model( /// Prepend the bodies of `activated_skills` to the system prompt of the /// upstream request so the chosen LLM has access to each skill's instructions. /// Works across every provider variant by going through the OpenAI message -/// shape (`get_messages`/`set_messages`). +/// shape (`get_messages` / `set_messages`). +/// +/// # Behavior contract /// -/// When there is already a leading system message we augment it in place; -/// otherwise a new system message is inserted at position 0. No-op when -/// `activated_skills` is empty. +/// * **No-op** when `activated_skills` is empty. +/// * **Augments the first system message in place** when one is present at +/// any position in `messages` (typically index 0, but we look for the +/// first `Role::System` rather than assuming). Subsequent system messages +/// (rare but legal for some providers) are left untouched. We pick "first" +/// so the skill content appears as early in the prompt as possible — +/// models weight earlier system content more heavily and an Anthropic +/// tools+system combo is conventionally a single leading block. +/// * **Inserts a new leading system message** at index 0 when no system +/// message exists in the request. +/// * **Flattens `MessageContent::Parts` system content to a single +/// `MessageContent::Text`** when extracting the base prompt. This is +/// intentional: every supported upstream API accepts text in system +/// messages, and the alternative — preserving each `ContentPart` and +/// appending a new text part — fails on providers that disallow +/// multi-part system content. The trade-off is that non-text system parts +/// (e.g. images attached to a system message, which no production +/// provider supports anyway) are dropped on the floor. Verified by +/// `flattens_parts_system_content` below. pub fn inject_activated_skills_into_request( client_request: &mut ProviderRequestType, activated_skills: &[SkillRef], @@ -240,3 +286,174 @@ pub fn inject_activated_skills_into_request( client_request.set_messages(&messages); } + +#[cfg(test)] +mod tests { + use super::*; + use hermesllm::apis::openai::{ChatCompletionsRequest, ContentPart}; + + fn req_with_messages(msgs: Vec) -> ProviderRequestType { + ProviderRequestType::ChatCompletionsRequest(ChatCompletionsRequest { + model: "test".to_string(), + messages: msgs, + ..Default::default() + }) + } + + fn user_msg(text: &str) -> Message { + Message { + role: Role::User, + content: Some(MessageContent::Text(text.to_string())), + name: None, + tool_calls: None, + tool_call_id: None, + } + } + + fn system_msg(text: &str) -> Message { + Message { + role: Role::System, + content: Some(MessageContent::Text(text.to_string())), + name: None, + tool_calls: None, + tool_call_id: None, + } + } + + fn skill(name: &str, body: &str) -> SkillRef { + SkillRef { + name: name.to_string(), + description: format!("desc for {name}"), + path: None, + base_dir: None, + body: Some(body.to_string()), + scope: Some("project".to_string()), + compatibility: None, + license: None, + metadata: None, + allowed_tools: None, + } + } + + fn first_system_text(req: &ProviderRequestType) -> String { + req.get_messages() + .iter() + .find(|m| m.role == Role::System) + .and_then(|m| m.content.as_ref()) + .map(|c| c.extract_text()) + .unwrap_or_default() + } + + #[test] + fn injects_new_system_message_when_none_present() { + let mut req = req_with_messages(vec![user_msg("hi")]); + inject_activated_skills_into_request(&mut req, &[skill("pdf", "process pdfs")]); + let messages = req.get_messages(); + assert_eq!(messages.len(), 2); + assert_eq!(messages[0].role, Role::System); + let txt = first_system_text(&req); + assert!(txt.contains(" { + assert!(t.contains("be brief")); + assert!(t.contains(" and polite")); + assert!(t.contains(" panic!("expected flattened text, got Parts"), + } + } + + #[test] + fn injects_in_orchestrator_order_for_multiple_skills() { + let mut req = req_with_messages(vec![user_msg("hi")]); + inject_activated_skills_into_request( + &mut req, + &[skill("first", "alpha-body"), skill("second", "beta-body")], + ); + let txt = first_system_text(&req); + let first_pos = txt.find("alpha-body").expect("first skill body present"); + let second_pos = txt.find("beta-body").expect("second skill body present"); + assert!( + first_pos < second_pos, + "skills should appear in the order they were activated" + ); + } +} diff --git a/crates/brightstaff/src/router/orchestrator.rs b/crates/brightstaff/src/router/orchestrator.rs index 31bf8e997..f27001c8f 100644 --- a/crates/brightstaff/src/router/orchestrator.rs +++ b/crates/brightstaff/src/router/orchestrator.rs @@ -5,13 +5,14 @@ use common::{ AgentUsagePreference, OrchestrationPreference, SkillRef, TopLevelRoutingPreference, }, consts::{ARCH_PROVIDER_HINT_HEADER, REQUEST_ID_HEADER}, + skills_runtime::{referenced_skills_catalog, resolve_for_route, resolve_selected_skills}, }; use hermesllm::apis::openai::Message; use hyper::header; use opentelemetry::global; use opentelemetry_http::HeaderInjector; use thiserror::Error; -use tracing::{debug, info}; +use tracing::{debug, info, warn}; use super::http::{self, post_and_extract_content}; use super::model_metrics::ModelMetricsService; @@ -41,14 +42,27 @@ pub struct OrchestratorService { tenant_header: Option, } -/// Result of `determine_route`: which route was picked, the ranked candidate -/// models for that route, and the Agent Skill bodies the orchestrator chose -/// to activate alongside it. Skills are resolved against -/// `routing_preferences[].skills`, so unknown / cross-route names are -/// silently dropped. +/// Result of `determine_route`: which route was picked (if any), the +/// ranked candidate models for that route, and the Agent Skill bodies the +/// orchestrator chose to activate alongside it. +/// +/// Two valid shapes: +/// +/// * **Route + skills (typical):** `route_name = Some(...)`, `models` +/// non-empty, `activated_skills` may be non-empty. Skills are resolved +/// against `routing_preferences[].skills`, so picks that aren't +/// allow-listed for the route are dropped with a `warn!`. +/// * **Skills-only:** `route_name = None`, `models` empty, +/// `activated_skills` non-empty. The orchestrator decided no route +/// needed to change but the user's intent matches one or more skills. +/// Per `docs/source/resources/skills.rst`, the request falls back to the +/// originally-requested model and the skill bodies are injected the +/// same way. Allow-list filtering uses the catalog union (effectively +/// the catalog itself, which is pre-filtered to skills referenced by +/// some route). #[derive(Debug, Clone, Default)] pub struct RouteDecision { - pub route_name: String, + pub route_name: Option, pub models: Vec, pub activated_skills: Vec, } @@ -141,7 +155,7 @@ impl OrchestratorService { prefs.into_iter().map(|p| (p.name.clone(), p)).collect() }); - let skills_catalog = build_skills_catalog_for_routes( + let skills_catalog = referenced_skills_catalog( skills_catalog.as_deref().unwrap_or(&[]), &top_level_preferences, ); @@ -279,6 +293,7 @@ impl OrchestratorService { } if let Some((route_name, _)) = selection.routes.first() { + // Route + (optional) skills path. let top_pref = inline_top_map .as_ref() .and_then(|m| m.get(route_name)) @@ -289,19 +304,43 @@ impl OrchestratorService { Some(svc) => svc.rank_models(&pref.models, &pref.selection_policy).await, None => pref.models.clone(), }; - let activated_skills = resolve_activated_skills( + let resolution = resolve_for_route( &self.skills_catalog, pref.skills.as_deref().unwrap_or(&[]), &selection.skills, ); + log_skill_drops(route_name, &resolution); + let activated_skills: Vec = + resolution.activated.into_iter().cloned().collect(); Some(RouteDecision { - route_name: route_name.clone(), + route_name: Some(route_name.clone()), models: ranked, activated_skills, }) } else { None } + } else if !selection.skills.is_empty() { + // Skills-only path: orchestrator picked no route but flagged + // skills. Per the documented contract the request still goes + // through with the originally-requested model and the skill + // bodies are injected. The catalog itself is the effective + // allow-list (it's already the union across every route's + // allow-list, so anything in it was deemed safe to expose). + let activated: Vec = + resolve_selected_skills(&self.skills_catalog, &selection.skills) + .into_iter() + .cloned() + .collect(); + if activated.is_empty() { + None + } else { + Some(RouteDecision { + route_name: None, + models: Vec::new(), + activated_skills: activated, + }) + } } else { None } @@ -399,59 +438,28 @@ impl OrchestratorService { } } -/// Build the orchestrator-visible skills catalog (deduplicated by name) from -/// the union of every skill name referenced under -/// `routing_preferences[].skills`. Skills that are not referenced by any -/// route are excluded — they would just clutter the prompt with no way for -/// the orchestrator to attach them to a route. -fn build_skills_catalog_for_routes( - catalog: &[SkillRef], - routes: &HashMap, -) -> Vec { - let mut referenced: std::collections::HashSet<&str> = std::collections::HashSet::new(); - for route in routes.values() { - if let Some(names) = route.skills.as_ref() { - for name in names { - referenced.insert(name.as_str()); - } - } - } - - let mut out: Vec = Vec::new(); - let mut seen: std::collections::HashSet = std::collections::HashSet::new(); - for skill in catalog { - if referenced.contains(skill.name.as_str()) && seen.insert(skill.name.clone()) { - out.push(skill.clone()); - } +/// Emit `warn!` for any skill names the orchestrator selected but the +/// resolver dropped. Surfacing these is critical for debuggability — a +/// silently-dropped skill is hard to diagnose, and the most common causes +/// (forgetting to add a skill to a route's allow-list, or the orchestrator +/// hallucinating a name) are both fixable once visible. +fn log_skill_drops(route_name: &str, resolution: &common::skills_runtime::SkillResolution<'_>) { + if !resolution.dropped_not_allowed.is_empty() { + warn!( + route = %route_name, + skills = ?resolution.dropped_not_allowed, + "orchestrator selected Agent Skills that are not on this route's allow-list; \ + dropping (add them to routing_preferences[].skills if you want this route to use them)" + ); } - out -} - -/// Filter the orchestrator-selected skill names down to the SKILL.md bodies -/// allowed for the chosen route, preserving the order the orchestrator -/// returned. Unknown names (either not in the catalog or not allowed by the -/// route) are silently dropped; the orchestrator can hallucinate. -fn resolve_activated_skills( - catalog: &[SkillRef], - route_allowlist: &[String], - selected: &[String], -) -> Vec { - let allowed: std::collections::HashSet<&str> = - route_allowlist.iter().map(String::as_str).collect(); - let mut out: Vec = Vec::with_capacity(selected.len()); - let mut taken: std::collections::HashSet<&str> = std::collections::HashSet::new(); - for name in selected { - if !allowed.contains(name.as_str()) { - continue; - } - if !taken.insert(name.as_str()) { - continue; - } - if let Some(skill) = catalog.iter().find(|s| &s.name == name) { - out.push(skill.clone()); - } + if !resolution.dropped_unknown.is_empty() { + warn!( + route = %route_name, + skills = ?resolution.dropped_unknown, + "orchestrator selected Agent Skills that are not in the runtime catalog \ + (likely hallucinated or removed)" + ); } - out } #[cfg(test)] @@ -536,6 +544,50 @@ mod tests { assert!(svc.get_cached_route("s3", None).await.is_some()); } + // ---- RouteDecision construction ---- + + fn skill_ref(name: &str) -> SkillRef { + SkillRef { + name: name.to_string(), + description: format!("desc for {name}"), + path: None, + base_dir: None, + body: Some(format!("body for {name}")), + scope: Some("project".to_string()), + compatibility: None, + license: None, + metadata: None, + allowed_tools: None, + } + } + + #[test] + fn route_decision_holds_optional_route_name_for_skills_only_path() { + // Regression guard for the docs promise at skills.rst:153-155: a + // skills-only decision must be representable, with no route_name and + // empty models, so the LLM handler falls back to the original model. + let decision = RouteDecision { + route_name: None, + models: Vec::new(), + activated_skills: vec![skill_ref("pdf")], + }; + assert!(decision.route_name.is_none()); + assert!(decision.models.is_empty()); + assert_eq!(decision.activated_skills.len(), 1); + } + + #[test] + fn log_skill_drops_does_not_panic_on_empty_resolution() { + // The logger is fire-and-forget. We can't easily assert on the + // emitted warns here without setting up a tracing subscriber, so the + // contract under test is: empty resolutions are silent (no warn + // attempt). Confidence in the warn paths comes from + // common::skills_runtime tests for resolve_for_route, which is the + // function whose dropped_* lists drive this logger. + let empty = common::skills_runtime::SkillResolution::default(); + log_skill_drops("any", &empty); + } + #[tokio::test] async fn test_cache_update_existing_session_does_not_evict() { let svc = make_orchestrator_service(600, 2); diff --git a/crates/common/src/skills_runtime.rs b/crates/common/src/skills_runtime.rs index 4e9d4aabe..e8bd69ee7 100644 --- a/crates/common/src/skills_runtime.rs +++ b/crates/common/src/skills_runtime.rs @@ -4,45 +4,117 @@ //! crate) so they can be unit-tested on the native target without dragging //! in proxy-wasm host-call symbols or tokio runtime dependencies. +use std::collections::{HashMap, HashSet}; + use crate::configuration::{SkillRef, TopLevelRoutingPreference}; -/// Filter `skills` down to the subset attached to `route_name` via -/// `routing_preferences[].skills`. When the selected route has no `skills:` -/// list, returns an empty vector — skills are scoped to routes, not global. -/// -/// `routing_preferences` is the source of truth for which skills are even -/// eligible for the orchestrator to activate on a given route. -pub fn skills_for_route<'a>( - skills: &'a [SkillRef], - routing_preferences: &[TopLevelRoutingPreference], - route_name: &str, -) -> Vec<&'a SkillRef> { - let Some(route) = routing_preferences.iter().find(|p| p.name == route_name) else { - return Vec::new(); - }; - let Some(allow) = route.skills.as_ref() else { - return Vec::new(); - }; - let mut out: Vec<&SkillRef> = Vec::with_capacity(allow.len()); - for name in allow { - if let Some(skill) = skills.iter().find(|s| &s.name == name) { - out.push(skill); +/// Upper bound on the byte length of a single skill body the runtime will +/// inject into an upstream system prompt. SKILL.md files are typically a few +/// kilobytes; this guard keeps a single oversized or malicious skill from +/// blowing the downstream model's context window. Bodies longer than this +/// are tail-trimmed with a marker line. ~32 KiB ≈ 8K tokens at the +/// 4-bytes-per-token heuristic used elsewhere in brightstaff. +pub const MAX_SKILL_BODY_BYTES: usize = 32 * 1024; + +const SKILL_BODY_TRUNCATION_MARKER: &str = "\n…[skill body truncated]\n"; + +/// Outcome of resolving a list of orchestrator-selected skill names against +/// a route's `skills:` allow-list and the runtime catalog. Callers should +/// emit `warn!` for each name in `dropped_not_allowed` / `dropped_unknown` +/// so misconfigured allow-lists and hallucinated picks are observable. +#[derive(Debug, Default)] +pub struct SkillResolution<'a> { + /// Skills that survived both the allow-list and catalog filters, in + /// orchestrator-selected order with duplicates removed. + pub activated: Vec<&'a SkillRef>, + /// Names the orchestrator selected that are NOT in the chosen route's + /// `skills:` allow-list. Typically a cross-route mention — the model + /// pulled a skill name from the global catalog that this route did not + /// expose. Callers should `warn!`. + pub dropped_not_allowed: Vec, + /// Names that ARE allow-listed for the route but are missing from the + /// runtime catalog (skill removed / never installed / hallucinated). + pub dropped_unknown: Vec, +} + +/// Build the orchestrator-visible skills catalog from the union of every +/// skill name referenced under `routing_preferences[].skills`. Skills not +/// referenced by any route are excluded — they would just clutter the +/// `` block with no way for the orchestrator to attach them. The +/// result preserves `catalog` order and is deduplicated by name. +pub fn referenced_skills_catalog( + catalog: &[SkillRef], + routes: &HashMap, +) -> Vec { + let mut referenced: HashSet<&str> = HashSet::new(); + for route in routes.values() { + if let Some(names) = route.skills.as_ref() { + for name in names { + referenced.insert(name.as_str()); + } + } + } + + let mut out: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); + for skill in catalog { + if referenced.contains(skill.name.as_str()) && seen.insert(skill.name.clone()) { + out.push(skill.clone()); } } out } -/// Resolve a list of orchestrator-selected skill names to their `SkillRef`s. -/// Unknown names are dropped silently — the orchestrator can hallucinate. -/// Results are deduplicated by name, preserving the order Plano-Orchestrator -/// returned. +/// Filter `selected` skill names to those that are both (a) allow-listed +/// for the chosen route via `route_allowlist` and (b) present in `catalog`, +/// preserving orchestrator order and deduplicating. Drops are reported on +/// the `SkillResolution` struct so callers can `warn!` and surface +/// misconfiguration without re-walking the lists. +pub fn resolve_for_route<'a>( + catalog: &'a [SkillRef], + route_allowlist: &[String], + selected: &[String], +) -> SkillResolution<'a> { + let allowed: HashSet<&str> = route_allowlist.iter().map(String::as_str).collect(); + let mut activated: Vec<&SkillRef> = Vec::with_capacity(selected.len()); + let mut taken: HashSet<&str> = HashSet::new(); + let mut dropped_not_allowed: Vec = Vec::new(); + let mut dropped_unknown: Vec = Vec::new(); + for name in selected { + if !taken.insert(name.as_str()) { + continue; + } + if !allowed.contains(name.as_str()) { + dropped_not_allowed.push(name.clone()); + continue; + } + match catalog.iter().find(|s| &s.name == name) { + Some(skill) => activated.push(skill), + None => dropped_unknown.push(name.clone()), + } + } + SkillResolution { + activated, + dropped_not_allowed, + dropped_unknown, + } +} + +/// Resolve a list of orchestrator-selected skill names to their `SkillRef`s +/// directly against the catalog, without any per-route allow-list. Use this +/// for the "skills-only" path documented in `docs/source/resources/skills.rst` +/// where the orchestrator returns skills but no route — the catalog itself +/// (already pre-filtered to skills referenced by SOME route via +/// `referenced_skills_catalog`) is the effective allow-list. Unknown names +/// are dropped silently; results are deduplicated by name preserving order. pub fn resolve_selected_skills<'a>( skills: &'a [SkillRef], selected_names: &[String], ) -> Vec<&'a SkillRef> { let mut out: Vec<&SkillRef> = Vec::with_capacity(selected_names.len()); + let mut seen: HashSet<&str> = HashSet::new(); for name in selected_names { - if out.iter().any(|s| &s.name == name) { + if !seen.insert(name.as_str()) { continue; } if let Some(skill) = skills.iter().find(|s| &s.name == name) { @@ -53,12 +125,22 @@ pub fn resolve_selected_skills<'a>( } /// Append the bodies of activated skills to a system prompt, wrapped in -/// `` tags so a future context-management pass can -/// recognize and recompact them. +/// `…` tags so a +/// future context-management pass can recognize and recompact them. +/// +/// Behavior contract (relied on by `brightstaff::handlers::llm::model_selection`): /// -/// Returns `None` only if no base system prompt was supplied and no skills -/// were activated. When skills are present the wrapper text always appears so -/// the downstream model receives a clear, well-structured instruction block. +/// * Returns `None` only when no base prompt was supplied **and** no skills +/// were activated. Otherwise always returns `Some`. +/// * The base prompt (if any) is kept verbatim and the skill block is +/// appended after a blank line. +/// * Each skill body is tail-trimmed at `MAX_SKILL_BODY_BYTES` bytes (UTF-8 +/// boundary safe) with a truncation marker, so a single oversized +/// SKILL.md cannot blow the downstream context window. +/// * `name` and `base_dir` are XML-attribute-escaped (`&`, `<`, `>`, `"`) +/// so a maliciously named skill cannot break out of the wrapper. Skill +/// names are already validated upstream, but defense-in-depth matters +/// here because the wrapper is part of the LLM's system prompt. pub fn augment_system_prompt_with_skills( base_system_prompt: Option, activated_skills: &[&SkillRef], @@ -80,22 +162,66 @@ pub fn augment_system_prompt_with_skills( against each skill's base directory.\n\n", ); for skill in activated_skills { - buf.push_str(&format!("\n"); if let Some(body) = skill.body.as_deref() { - buf.push_str(body.trim_end()); + buf.push_str(&truncate_skill_body(body)); buf.push('\n'); } else { - buf.push_str(&format!("(skill description) {}\n", skill.description)); + buf.push_str(&format!( + "(skill description) {}\n", + xml_attr_escape(&skill.description) + )); } buf.push_str("\n\n"); } Some(buf.trim_end().to_string()) } +/// Escape a string for use inside an XML attribute value (double-quoted). +/// Quotes `&`, `<`, `>`, and `"`; leaves single quotes alone since the +/// wrapper always uses double quotes. +fn xml_attr_escape(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for ch in s.chars() { + match ch { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + _ => out.push(ch), + } + } + out +} + +/// Tail-trim `body` to at most `MAX_SKILL_BODY_BYTES` bytes, respecting +/// UTF-8 character boundaries. Appends a marker so the downstream model +/// can tell content was dropped. Pass-through for short bodies. +fn truncate_skill_body(body: &str) -> String { + let trimmed = body.trim_end(); + if trimmed.len() <= MAX_SKILL_BODY_BYTES { + return trimmed.to_string(); + } + // Reserve room for the marker so the final length is still within the + // budget even when the marker is added. + let budget = MAX_SKILL_BODY_BYTES.saturating_sub(SKILL_BODY_TRUNCATION_MARKER.len()); + let mut end = budget; + while end > 0 && !trimmed.is_char_boundary(end) { + end -= 1; + } + let mut out = String::with_capacity(end + SKILL_BODY_TRUNCATION_MARKER.len()); + out.push_str(&trimmed[..end]); + out.push_str(SKILL_BODY_TRUNCATION_MARKER); + out +} + #[cfg(test)] mod tests { use super::*; @@ -126,49 +252,93 @@ mod tests { } } + fn routes_map( + routes: Vec, + ) -> HashMap { + routes.into_iter().map(|r| (r.name.clone(), r)).collect() + } + + // --- referenced_skills_catalog --- + #[test] - fn skills_for_route_returns_attached_skills() { + fn referenced_catalog_is_union_across_routes() { let catalog = vec![ - skill("pdf-processing", "extract"), + skill("pdf", "extract"), skill("code-review", "review"), + skill("never-used", "x"), ]; - let routes = vec![ - route("code review", Some(vec!["code-review"])), - route("doc work", Some(vec!["pdf-processing"])), - ]; - let resolved = skills_for_route(&catalog, &routes, "code review"); - assert_eq!(resolved.len(), 1); - assert_eq!(resolved[0].name, "code-review"); + let routes = routes_map(vec![ + route("docs", Some(vec!["pdf"])), + route("review", Some(vec!["code-review"])), + route("other", None), + ]); + let out = referenced_skills_catalog(&catalog, &routes); + let names: Vec<_> = out.iter().map(|s| s.name.as_str()).collect(); + assert!(names.contains(&"pdf")); + assert!(names.contains(&"code-review")); + assert!(!names.contains(&"never-used")); + } + + #[test] + fn referenced_catalog_deduplicates_when_multiple_routes_share_a_skill() { + let catalog = vec![skill("pdf", "extract")]; + let routes = routes_map(vec![ + route("a", Some(vec!["pdf"])), + route("b", Some(vec!["pdf"])), + ]); + let out = referenced_skills_catalog(&catalog, &routes); + assert_eq!(out.len(), 1); } + // --- resolve_for_route --- + #[test] - fn skills_for_route_empty_when_route_has_no_skills_list() { - let catalog = vec![skill("pdf-processing", "extract")]; - let routes = vec![route("code review", None)]; - let resolved = skills_for_route(&catalog, &routes, "code review"); - assert!(resolved.is_empty()); + fn resolve_for_route_keeps_allowlisted_skills_in_orchestrator_order() { + let catalog = vec![skill("a", ""), skill("b", ""), skill("c", "")]; + let allow = vec!["a".to_string(), "b".to_string(), "c".to_string()]; + let selected = vec!["c".to_string(), "a".to_string()]; + let r = resolve_for_route(&catalog, &allow, &selected); + let names: Vec<_> = r.activated.iter().map(|s| s.name.as_str()).collect(); + assert_eq!(names, vec!["c", "a"]); + assert!(r.dropped_not_allowed.is_empty()); + assert!(r.dropped_unknown.is_empty()); } #[test] - fn skills_for_route_empty_when_route_missing() { - let catalog = vec![skill("pdf-processing", "extract")]; - let routes = vec![route("code review", Some(vec!["pdf-processing"]))]; - let resolved = skills_for_route(&catalog, &routes, "no-such-route"); - assert!(resolved.is_empty()); + fn resolve_for_route_drops_cross_route_skill_into_not_allowed() { + let catalog = vec![skill("pdf", ""), skill("payment", "")]; + let allow = vec!["pdf".to_string()]; // route only allows pdf + let selected = vec!["pdf".to_string(), "payment".to_string()]; + let r = resolve_for_route(&catalog, &allow, &selected); + assert_eq!(r.activated.len(), 1); + assert_eq!(r.activated[0].name, "pdf"); + assert_eq!(r.dropped_not_allowed, vec!["payment".to_string()]); + assert!(r.dropped_unknown.is_empty()); } #[test] - fn skills_for_route_drops_unknown_skill_names() { - let catalog = vec![skill("pdf-processing", "extract")]; - let routes = vec![route( - "code review", - Some(vec!["pdf-processing", "ghost-skill"]), - )]; - let resolved = skills_for_route(&catalog, &routes, "code review"); - assert_eq!(resolved.len(), 1); - assert_eq!(resolved[0].name, "pdf-processing"); + fn resolve_for_route_drops_hallucinated_skill_into_unknown() { + let catalog = vec![skill("pdf", "")]; + let allow = vec!["pdf".to_string(), "imaginary".to_string()]; + let selected = vec!["pdf".to_string(), "imaginary".to_string()]; + let r = resolve_for_route(&catalog, &allow, &selected); + assert_eq!(r.activated.len(), 1); + assert_eq!(r.activated[0].name, "pdf"); + assert!(r.dropped_not_allowed.is_empty()); + assert_eq!(r.dropped_unknown, vec!["imaginary".to_string()]); } + #[test] + fn resolve_for_route_deduplicates_repeats() { + let catalog = vec![skill("pdf", "")]; + let allow = vec!["pdf".to_string()]; + let selected = vec!["pdf".to_string(), "pdf".to_string(), "pdf".to_string()]; + let r = resolve_for_route(&catalog, &allow, &selected); + assert_eq!(r.activated.len(), 1); + } + + // --- resolve_selected_skills (skills-only path) --- + #[test] fn resolve_selected_skills_drops_unknown_and_dedupes() { let catalog = vec![ @@ -187,6 +357,8 @@ mod tests { assert_eq!(resolved[1].name, "pdf-processing"); } + // --- augment_system_prompt_with_skills --- + #[test] fn augment_passthrough_with_no_skills() { let augmented = augment_system_prompt_with_skills(Some("you are helpful".to_string()), &[]); @@ -212,4 +384,32 @@ mod tests { assert!(augmented.contains("").unwrap(); + let body_section_start = augmented.find(">\n").unwrap() + 2; + let body_len = body_section_end - body_section_start; + assert!(body_len <= MAX_SKILL_BODY_BYTES + 64); + } }