fix(skills): honor skills-only orchestrator decisions, dedupe runtime helpers, warn on dropped picks

Addresses the code-review findings on 7f5bf641:

- 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 /.

cli/planoai/skills.py

@@ -102,24 +102,55 @@ class Skill:
         }
 
 
-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
 

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::<Vec<_>>(),
+                        "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`).
 ///
-/// 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.
+/// # Behavior contract
+///
+/// * **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<Message>) -> 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("<skill_content name=\"pdf\""));
+        assert!(txt.contains("process pdfs"));
+    }
+
+    #[test]
+    fn augments_existing_system_message_in_place_preserving_user_message() {
+        let mut req = req_with_messages(vec![system_msg("you are helpful"), user_msg("hi")]);
+        inject_activated_skills_into_request(&mut req, &[skill("pdf", "process pdfs")]);
+        let messages = req.get_messages();
+        assert_eq!(messages.len(), 2);
+        let txt = first_system_text(&req);
+        assert!(txt.starts_with("you are helpful"));
+        assert!(txt.contains("<skill_content name=\"pdf\""));
+        assert_eq!(messages[1].role, Role::User);
+    }
+
+    #[test]
+    fn noop_when_no_skills_activated() {
+        let mut req = req_with_messages(vec![system_msg("base"), user_msg("hi")]);
+        let before = req.get_messages();
+        inject_activated_skills_into_request(&mut req, &[]);
+        let after = req.get_messages();
+        assert_eq!(after.len(), before.len());
+        assert_eq!(first_system_text(&req), "base");
+    }
+
+    #[test]
+    fn augments_only_the_first_system_message() {
+        // Two system messages (rare in practice). Only the first one is
+        // augmented; the trailing one is left untouched. Documented contract.
+        let mut req = req_with_messages(vec![
+            system_msg("primary"),
+            system_msg("secondary"),
+            user_msg("hi"),
+        ]);
+        inject_activated_skills_into_request(&mut req, &[skill("pdf", "process pdfs")]);
+        let messages = req.get_messages();
+        assert!(messages[0]
+            .content
+            .as_ref()
+            .unwrap()
+            .extract_text()
+            .contains("primary"));
+        assert!(messages[0]
+            .content
+            .as_ref()
+            .unwrap()
+            .extract_text()
+            .contains("<skill_content"));
+        assert_eq!(
+            messages[1].content.as_ref().unwrap().extract_text(),
+            "secondary"
+        );
+    }
+
+    #[test]
+    fn flattens_parts_system_content() {
+        // Documented behavior: `MessageContent::Parts` system content is
+        // extracted to plain text via ExtractText and re-emitted as
+        // `MessageContent::Text`. Non-text parts (e.g. images) are dropped
+        // — no production provider ships images in a system message.
+        let parts_system = Message {
+            role: Role::System,
+            content: Some(MessageContent::Parts(vec![
+                ContentPart::Text {
+                    text: "be brief".to_string(),
+                },
+                ContentPart::Text {
+                    text: " and polite".to_string(),
+                },
+            ])),
+            name: None,
+            tool_calls: None,
+            tool_call_id: None,
+        };
+        let mut req = req_with_messages(vec![parts_system, user_msg("hi")]);
+        inject_activated_skills_into_request(&mut req, &[skill("pdf", "body")]);
+        let messages = req.get_messages();
+        let system = &messages[0];
+        match system.content.as_ref().unwrap() {
+            MessageContent::Text(t) => {
+                assert!(t.contains("be brief"));
+                assert!(t.contains(" and polite"));
+                assert!(t.contains("<skill_content name=\"pdf\""));
+            }
+            MessageContent::Parts(_) => 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"
+        );
+    }
+}

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<String>,
 }
 
-/// 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[<route>].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[<route>].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<String>,
     pub models: Vec<String>,
     pub activated_skills: Vec<SkillRef>,
 }
@@ -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<SkillRef> =
+                        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<SkillRef> =
+                    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<String, TopLevelRoutingPreference>,
-) -> Vec<SkillRef> {
-    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());
-            }
-        }
+/// 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)"
+        );
     }
-
-    let mut out: Vec<SkillRef> = Vec::new();
-    let mut seen: std::collections::HashSet<String> = std::collections::HashSet::new();
-    for skill in catalog {
-        if referenced.contains(skill.name.as_str()) && seen.insert(skill.name.clone()) {
-            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
-}
-
-/// 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<SkillRef> {
-    let allowed: std::collections::HashSet<&str> =
-        route_allowlist.iter().map(String::as_str).collect();
-    let mut out: Vec<SkillRef> = 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)]
@@ -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);

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<String>,
+    /// Names that ARE allow-listed for the route but are missing from the
+    /// runtime catalog (skill removed / never installed / hallucinated).
+    pub dropped_unknown: Vec<String>,
+}
+
+/// 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
+/// `<skills>` 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<String, TopLevelRoutingPreference>,
+) -> Vec<SkillRef> {
+    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<SkillRef> = Vec::new();
+    let mut seen: HashSet<String> = 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<String> = Vec::new();
+    let mut dropped_unknown: Vec<String> = 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
-/// `<skill_content name="...">` tags so a future context-management pass can
-/// recognize and recompact them.
+/// `<skill_content name="..." [base_dir="..."]>…</skill_content>` 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.
+/// Behavior contract (relied on by `brightstaff::handlers::llm::model_selection`):
+///
+/// * 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<String>,
     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!("<skill_content name=\"{}\"", skill.name));
+        buf.push_str(&format!(
+            "<skill_content name=\"{}\"",
+            xml_attr_escape(&skill.name)
+        ));
         if let Some(base_dir) = skill.base_dir.as_deref() {
-            buf.push_str(&format!(" base_dir=\"{}\"", base_dir));
+            buf.push_str(&format!(" base_dir=\"{}\"", xml_attr_escape(base_dir)));
         }
         buf.push_str(">\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("</skill_content>\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("&amp;"),
+            '<' => out.push_str("&lt;"),
+            '>' => out.push_str("&gt;"),
+            '"' => out.push_str("&quot;"),
+            _ => 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<TopLevelRoutingPreference>,
+    ) -> HashMap<String, TopLevelRoutingPreference> {
+        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 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 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 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("<skill_content name=\"code-review\""));
         assert!(augmented.contains("look at diffs"));
     }
+
+    #[test]
+    fn augment_xml_escapes_skill_name_and_base_dir() {
+        let mut s = skill("safe-name", "body");
+        s.name = "bad\"name".to_string();
+        s.base_dir = Some("/path/with\"quote".to_string());
+        let augmented = augment_system_prompt_with_skills(None, &[&s]).expect("augmented");
+        // Raw double-quote must NOT appear inside the attribute value — only
+        // its escaped form. Otherwise it would close the attribute and let a
+        // skill name inject arbitrary attributes / break out of the wrapper.
+        assert!(augmented.contains("name=\"bad&quot;name\""));
+        assert!(augmented.contains("base_dir=\"/path/with&quot;quote\""));
+    }
+
+    #[test]
+    fn augment_truncates_oversized_skill_body() {
+        let big_body: String = "a".repeat(MAX_SKILL_BODY_BYTES * 2);
+        let s = skill("huge", &big_body);
+        let augmented = augment_system_prompt_with_skills(None, &[&s]).expect("augmented");
+        // Truncation marker is present, so the body did NOT pass through verbatim.
+        assert!(augmented.contains("[skill body truncated]"));
+        // And the body slice cannot be longer than MAX_SKILL_BODY_BYTES + a
+        // little wrapper overhead — definitely not 2× the cap.
+        let body_section_end = augmented.find("</skill_content>").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);
+    }
 }