feat(skills): Agent Skills support with orchestrator-driven activation#954
feat(skills): Agent Skills support with orchestrator-driven activation#954Spherrrical wants to merge 2 commits into
Conversation
… helpers, warn on dropped picks Addresses the code-review findings on 7f5bf64: - Honor skills-only decisions: RouteDecision.route_name is now Option<String> 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 <skill_content> 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 /.
5e8d27f to
a0f77fd
Compare
|
Rebased onto current main (was 13 behind). No conflicts during rebase. Re-ran the full check matrix locally on the rebased tip (
Picked up the version bump to 0.4.25 automatically via the rebase. Spot-checked that nothing in our new files ( Ready for review. |
adilhafeez
left a comment
There was a problem hiding this comment.
From the code change what I understand is that you want some sort of map from route name to selected route + skills list. You can maintain list from route_name -> skills in memory and then model returns selected route you can attach route_name + skill from local map. We shouldn't introduce any changes in system prompt without a round of fine-tuning. As that will hurt the perf negatively.
| 1. The brightstaff routing service builds an ``<skills>`` block in the | ||
| Plano-Orchestrator prompt — alongside the existing ``<routes>`` block — | ||
| listing every skill referenced by ``routing_preferences[].skills`` with | ||
| its name and short description. |
There was a problem hiding this comment.
This will not work - this will instead hurt the model performance as model has not been trained with this prompt. Check with @nehcgs
| system prompt — wrapped in | ||
| ``<skill_content name="..." base_dir="...">…</skill_content>`` tags — and | ||
| the request is forwarded to the chosen model. |
There was a problem hiding this comment.
is that standard way to attach skills?
| # 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/<name>/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 <routes> and a | ||
| # <skills> 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. |
There was a problem hiding this comment.
add few lines explaining what does skill template does
| - template_id: skills_routing | ||
| template_file: skills_routing.yaml | ||
| demo_configs: [] | ||
| transform: none |
There was a problem hiding this comment.
this should be loaded dynamically
Summary
Integrates Agent Skills into Plano as a first-class capability: skills are discovered from
.plano/skills,~/.plano/skills, and the universal~/.agents/skills(wherenpx skills addinstalls), attached torouting_preferences[].skillsallow-lists, selected per-request by Plano-Orchestrator alongside the existing<routes>block, and injected as<skill_content>wrappers into the upstream system prompt — all native, no WASM hop, no synthetic tool calls.Two commits, second one addresses code-review feedback on the first:
7f5bf641— featurecli/planoai/):planoai skills add | list | remove | trust, three discovery scopes (projectrequires explicit trust;userandagentsare auto-trusted), lenientSKILL.mdparsing with diagnostics, snapshot-diff post-install attribution sonpx-installed skills are picked up regardless of where they land.config/plano_config_schema.yaml): documentsmodel_metrics_sources(cost / latency providers) sinceselection_policy.prefer: cheapest|fastestnow requires it at startup.routing_preferences[].skillsis a typed allow-list.crates/common/src/skills_runtime.rs,crates/brightstaff/):SkillRefconfig model, orchestrator V1 system prompt extended with a<skills>XML block (parsed as{"route": [...], "skills": [...]}),OrchestratorService::with_routing_and_skillsseeds the catalog from the union of every route's allow-list, and the LLM handler prepends activated skill bodies into the upstream request before serialization.docs/source/resources/skills.rst): end-to-end walkthrough, scope precedence table, and routing flow.cli/planoai/templates/skills_routing.yaml): ready-madeplanoai init --template skills_routingbootstrap.5e8d27f— review fixesRouteDecision.route_nameis nowOption<String>; when the orchestrator returns{"route": [], "skills": [...]}the request falls back to the originally-requested model and skills still inject, matching the contract documented inskills.rst:153-155.common::skills_runtime::resolve_for_routereturns aSkillResolutionthat splits drops intodropped_not_allowedvsdropped_unknown; brightstaff logs each via dedicatedwarn!s with the offending route + skill names — no more silent filtering.common::skills_runtimeownsreferenced_skills_catalog,resolve_for_route,resolve_selected_skills, andaugment_system_prompt_with_skills. brightstaff's parallel re-implementations are gone.inject_activated_skills_into_requestnow spells out the first-system-message rule and theParts → Textflatten; 6 new tests cover both. 11 new tests inskills_runtimecover catalog union, allow-list intersection, dedup, hallucination handling, XML escape, and body cap.MAX_SKILL_BODY_BYTES = 32 KiBper-skill UTF-8-safe tail-trim guards the downstream context window; skillname/base_dirare XML-attribute-escaped in the<skill_content>wrapper as defense-in-depth;cli/planoai/skills.py::find_project_rootbounds the walk at\$HOME(plus a 30-parent hard cap) so it no longer climbs to/.Wire shape (orchestrator system prompt, abbreviated)
Activated
SkillRefbodies are then wrapped into the upstream system prompt:Test plan
cd crates && cargo fmt --all -- --check(passed locally — pre-commit hook)cd crates && cargo clippy --locked --all-targets --all-features -- -D warnings(passed locally)cd crates && cargo test --lib— 53common+ 173brightstaff(passed locally)cd cli && uv run pytest -v— 123 tests (passed locally)digitalocean/anthropic-claude-opus-4.7+requesting-code-reviewskill: orchestrator picks the skill, brightstaff logsinjecting activated Agent Skills into upstream system prompt, and the model returns a structured code review. Verified locally with a 30+ KB streaminggit diffpayload (stream: true+curl -N).no route determined; activating skills against default modeland the originally-requested model is used.skills:lists, prompt cross-route, confirmwarn!(skills not on this route's allow-list)appears.planoai skills add openai/skillsfollowed byplanoai upfrom outside the plano repo — config templates resolve, brightstaff loads the catalog, skills appear in the orchestrator<skills>block.