diff --git a/CLAUDE.md b/CLAUDE.md index 1b00156c..48b244f1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -86,6 +86,9 @@ Skills are reusable instruction modules. Three sources: predefined skills loaded and DIAL skill resources (`dial_skills/`) fetched per request from Core's `/v2/skills` API — a folder with `SKILL.md` plus bundled text files the agent reads on demand via `read_skill(skill_name, file_path)`. `SkillsRegistry` merges all three per request and owns precedence (predefined > dial-prompt > dial-skill). +A user can also invoke one of their own skills from a message (`skill_invocation/`, preview): the +`custom_content.skills[*]` chips are resolved per request, registered ahead of every agent source, and +injected as a synthetic `read_skill` pair. See [`docs/skills.md`](docs/skills.md). ### Configuration Model diff --git a/README.md b/README.md index d1c78db9..c267594c 100644 --- a/README.md +++ b/README.md @@ -172,6 +172,7 @@ Controls which tool-execution stages are surfaced in the DIAL UI for each app. S | `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` | No | Cap on a single file read from a DIAL skill resource, `SKILL.md` included. Must exceed the largest manifest you expect: an over-cap manifest drops the skill. See [docs/skills.md](docs/skills.md). | | `DIAL_SKILLS_MAX_FILES` | `200` | No | Maximum bundled files advertised to the agent per DIAL skill resource; beyond it the listing is truncated | | `DIAL_SKILLS_LISTING_MAX_PAGES` | `10` | No | Maximum file-listing pages followed per DIAL skill resource, bounding a server-supplied cursor | +| `SKILL_INVOCATION_MAX_SKILLS` | `10` | No | Maximum distinct skills a user may have invoked from the messages of one conversation (`custom_content.skills`), counted newest first. Each one adds a `` block to the system prompt and one DIAL Core fetch per turn; beyond the cap the oldest picks stop being registered. Preview-gated. See [docs/skills.md](docs/skills.md). | | **Feature Gating** | | | | | `ENABLE_PREVIEW_FEATURES` | `false` | No | Enable preview features across the deployment (schema visibility + runtime activation) | | **Templates** | | | | diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md new file mode 100644 index 00000000..c0348ecb --- /dev/null +++ b/docs/designs/skill_invocation.md @@ -0,0 +1,643 @@ +# Design: Invoking a Skill from a Message + +- **Status:** Implemented +- **Approved:** 2026-09-14 +- **Issue:** [epam/ai-dial-quickapps-backend#549](https://github.com/epam/ai-dial-quickapps-backend/issues/549), a + sub-issue of [#421](https://github.com/epam/ai-dial-quickapps-backend/issues/421) ([EPIC] Advanced Agent Skills + support) +- **Scope:** Phase 1a — a picked skill is loaded, listed and readable, under its **own** name. Making a picked skill + unable to collide with an agent's skill is phase 1b + ([Follow-up](#follow-up-phase-1b--collision-free-names)). +- **Dependencies:** + - [`skills_as_dial_resource.md`](skills_as_dial_resource.md) — `DialSkillResolver`, `DialSkillReader`, the + `` inventory. Landed on `development` in #524 (`cb67eb25`). + - `ai-dial-core` — `CollectRequestSkillsFn` collects `messages[*].custom_content.skills[*]` and auto-shares each skill + to the per-request key; the field is in the OpenAPI message schemas as `RequestSkill`. **Done:** + [epam/ai-dial-core#1956](https://github.com/epam/ai-dial-core/pull/1956) (issue #1955), on `development`. It + rejects a URL the user can't read with `403`, which leaves one gap (see [Known Gaps](#known-gaps)). + - `ai-dial-chat` — `chat-api` accepts the field (`MessageCustomContentDto`), and the composer emits it from a `/` + palette of the user's own skills. **Agreed, not started.** + +## Problem Statement + +A skill reaches a QuickApp agent in exactly one way: the app author lists it in `ApplicationConfig.skills`, QuickApps +advertises it in ``, and the **model** decides whether to call `read_skill`. The user has no say. +They can't tell the agent "use this skill for this message", and they can't use a skill the author never attached. + +That leaves two gaps: + +1. **No deterministic invocation.** Even when the skill is attached, "please use the code-review skill" is a hint the + model may or may not act on. Other agent products solve this with a `/skill-name` command whose effect is + guaranteed: the skill is loaded, every time. +2. **No way in for the user's own skills.** DIAL Chat has a full skill catalog and editor. A user who wrote + `code-review` for themselves cannot bring it into a conversation with an agent they don't own. Under a per-request + key, the app cannot read `skills//…` at all unless something shares it. + +DIAL Core and DIAL Chat have agreed on a wire contract that closes the access half: a user message may carry +`custom_content.skills[*]`, and Core auto-shares each referenced skill to the app's per-request key, as it already +does for `custom_content.attachments[*]`. This design specifies how QuickApps consumes that field. + +## Design Goals + +1. A skill the user picks is **always** loaded into the model's context on that turn. The model doesn't choose. +2. The user can pick any skill they can read from their own catalog: their own, one shared with them, or a published + one. It doesn't need to be in the app config. +3. The loaded manifest **stays** in the model's context for the rest of the conversation, without being re-injected + and without changing after the turn that loaded it. +4. The skill's **bundled files stay readable** on any later turn, through `read_skill(name, file_path)`, exactly as + for a declared skill. +5. A chip on the current message that fails is visible to both the user and the model. The request is still served. +6. The **shape** of the skills contract doesn't change: `` keeps its fields, `read_skill` keeps its + parameters, and a request without the field behaves exactly as today. What does change is *which* skills an agent + ends up with — a picked skill can displace an agent's same-named one. That is a deliberate relaxation, not a + property this design preserves; see [Known Gaps](#known-gaps). +7. The change is as small as possible. In particular, a picked skill is an ordinary `SkillsProvider` entry — no new + lookup path, no second read tool, no new collision machinery, and no special case in `SkillsRegistry`. + +## Phasing + +- **Phase 1a — this design.** A picked skill is resolved, injected as a synthetic `read_skill` pair, and registered + as an ordinary skill under **its own manifest name**. It wins any name collision with the agent's skills + ([UC-5](#uc-5-the-users-skill-has-the-same-name-as-one-of-the-agents)). +- **Phase 1b — [follow-up](#follow-up-phase-1b--collision-free-names).** A picked skill gets a name it cannot collide + on, so it stops shadowing the agent's skills and both stay reachable. +- **Phase 2 — to be designed** ([#550](https://github.com/epam/ai-dial-quickapps-backend/issues/550)). The user can + pick the agent's own skills too. How the client learns about them is left for that design. + +--- + +## Use Cases + +### UC-1: Picking a personal skill from the palette + +**Trigger:** The user picks their own `skills//sql-style` from the palette. +**Behavior:** Chat sends `content: "/sql-style …"` with `custom_content.skills: [{url: "skills//sql-style"}]`. +Core auto-shares the skill to the per-request key. QuickApps resolves it through `DialSkillResolver`, registers it as +`sql-style`, and inserts a synthetic `read_skill` call and result for that name after the first user message. +**Outcome:** The model starts its turn with the skill's manifest and its `` inventory already in context. +The response shows the normal "Reading Skill: sql-style" stage. + +### UC-2: Reading a bundled file three turns later + +**Trigger:** On turn 4 the model calls `read_skill("sql-style", "references/naming.md")` for a skill picked on turn 1. +**Behavior:** The turn-1 user message still carries the chip, so Core shares the skill again and QuickApps resolves +and registers it again under the same name. The manifest itself is not re-injected: it comes back from the turn-1 +assistant state like any other tool result. +**Outcome:** The file is returned. The conversation history is byte-identical to what the model saw on turn 1, even if +the user edited the skill in the meantime. + +### UC-3: Asking for one of the agent's own skills + +**Trigger:** The agent has an `acme-release-notes` skill. The palette doesn't offer it, so the user writes "use the +release notes skill for 0.9.0". +**Behavior:** Nothing new. The message carries no `custom_content.skills`, and QuickApps doesn't parse the text. The +skill is in `` as today, and the model decides whether to call `read_skill`. +**Outcome:** Same as today. Guaranteed invocation of the agent's own skills is phase 2. + +### UC-4: The picked skill can't be loaded + +**Trigger:** The skill was deleted, or its manifest is invalid. +**Behavior:** The synthetic pair is still inserted, with an error result that names the skill and says it could not +be loaded. The reason is reported to the user in the "Initialization issues" stage. The skill is not registered. +**Outcome:** The model knows the user asked for a skill it doesn't have, and says so. The rest of the answer is +produced normally. + +### UC-5: The user's skill has the same name as one of the agent's + +**Trigger:** The agent has a predefined `code-review`. The user picks their own `skills//code-review`. +**Behavior:** The user's skill wins. It is listed as `code-review`, `read_skill("code-review")` returns it, and the +agent's `code-review` is dropped from the merged set and reported in "Initialization issues" by the existing +`SkillsRegistry` collision path — no new mechanism. +**Outcome:** For that conversation the agent's same-named skill is unavailable. **This is accepted for 1a**, on the +assumption that a user does not pick skills whose names clash with the agent's. Phase 1b removes the possibility; +see [Known Gaps](#known-gaps). + +--- + +## Proposed Design + +```mermaid +sequenceDiagram + participant Chat as DIAL Chat + participant Core as DIAL Core + participant QA as QuickApps + participant LLM as Orchestrator LLM + + Chat->>Core: user msg with content and optional custom_content.skills[].url + Core->>Core: validate and auto-share every skills[].url to the per-request key + Core->>QA: chat/completions + QA->>QA: initializer: collect one pick per user message, dedupe, cap, resolve + QA->>QA: user skills provider (order -10) joins the SkillsRegistry merge + QA->>QA: injector: this turn's pick becomes one synthetic read_skill pair, after the first user msg + QA->>QA: strip custom_content.skills from the working copy + QA->>LLM: system prompt with available_skills + history + synthetic pairs + LLM-->>QA: answer + QA-->>Chat: answer + state.tool_execution_history (includes the pair) +``` + +Four concerns. Nothing below adds a lookup path: a picked skill becomes an ordinary entry in the existing registry, +which is what makes bundled files and later turns work for free (goals 4 and 7). + +### 1. Wire contract — `custom_content.skills` + +- **What.** An array on a **user** message's `custom_content`: + + ```jsonc + { + "role": "user", + "content": "/code-review focus on auth", + "custom_content": { + "skills": [ + { "url": "skills//" } + ] + } + } + ``` + +- **Owner.** The client writes it, Core validates and shares what it references, and QuickApps interprets it. +- **Semantics.** + - In phase 1 the array only ever carries the user's skills, picked from the palette. The agent's own skills never + go in it. + - What Core does with each entry (`CollectRequestSkillsFn`), before QuickApps sees the request: + - an entry that isn't an object, has no `url`, or whose `url` is absolute or not a `skills/` resource → `400` for + the whole request; + - a public skill → nothing to share, since any key can read it; + - a skill the user can read → shared read-only with the per-request key; + - a skill the user can't read → `403` for the whole request. + - **One skill per message.** The field is an array, but QuickApps loads only its **first** entry. Any others are + ignored: on the message being answered they are reported as a warning-severity initialization issue naming them, + and on earlier turns they are only logged, so the stage does not repeat an issue the user can no longer act on. + Identical entries on one message count once. + - `url` is the only field QuickApps reads. Anything else (`title`, say) passes Core and is ignored, so the client + may carry display data. + - The URL has **no trailing slash**: `skills//`. Core shares exactly that URL and authorises every + read of the skill (`SKILL.md` and each bundled file) against it, so one share covers the whole skill. A URL ending + in `/` would be shared under a different key and every read would then be denied. QuickApps strips a trailing `/` + before use, so the dedup key and the share key stay aligned. + - The field is **per message**, not per request. An invocation belongs to the turn it was made on. Because it stays + on that message, Core re-shares the skill on every later turn while the client keeps resending the history, which + is what keeps bundled files readable (UC-2). + - `custom_content.skills` on a non-user message is ignored. + - `content` is opaque. QuickApps never parses it, and the chip is the only invocation signal. The client sends it + as the user typed it, `/name` token included, so a message with a chip is never empty. +- **Change.** Nothing to parse in the SDK: `CustomContent` is an `ExtraAllowModel`, so the field arrives in + `model_extra`. A typed `skills` field in `aidial-sdk` is welcome but not required. + +### 2. Resolution — `_SkillInvocationInitializer` + +- **What.** A `CompletionInitializer` in a new package `quickapp/skill_invocation/`. It fills a request-scoped + `_InvokedSkillsContext` (concern 3). +- **Owner.** `quickapp/skill_invocation/`. +- **Semantics.** + 1. Walk the user messages in order and collect **one** `url` per message — the first entry of its + `custom_content.skills`, canonicalised by stripping a trailing `/` — keyed by the **ordinal** of that user + message (`0` for the first, `1` for the second, …). Extra entries are recorded as ignored (concern 1). A + malformed entry is dropped with a debug log — Core answers such a request with `400`, so reaching here means + something upstream changed, and refusing the turn over it would be worse. + + The ordinal is the anchor rather than a list index because the injector sees a different list from the one + parsed here: `extract_tool_calls` has expanded the stored tool history and the scrub transformer has copied the + messages that carried the field. User messages survive both, in order, so counting them is stable. + 2. Deduplicate, keeping each URL's **first** occurrence: a skill is loaded once, ahead of the message that first + asked for it, and picking it again later changes nothing. A re-pick therefore neither refreshes the skill's + position under the cap nor injects a second pair. + 3. Keep the picks of the newest `SKILL_INVOCATION_MAX_SKILLS` ordinals, so the cap drops the **oldest** picks and + never the pick made on the message being answered. A dropped pick's manifest is still in history, but it is no + longer registered, so its name stops resolving and its files stop being readable. + 4. Wrap each URL in a `DialSkillConfig` and hand the list, **oldest first**, to the existing `DialSkillResolver`, + unchanged. Manifest parsing, the `` inventory, byte caps, per-URL failure isolation and warning + severities all come for free. + 5. Store the resolved skills **keyed by URL**, and separately **the URL picked on the message being answered** + (`None` when this turn picked nothing — a re-pick of an already-loaded URL included). The injector reads both + back: the lookup to choose between a real result and the error sentence, the current pick to know whether this + turn injects at all. Recording "this turn's pick" here rather than re-parsing the messages later means the + injector does not care whether the scrub has already run. +- **Every historical pick is re-resolved on every turn.** That is what keeps a turn-1 skill registered on turn 4 so + its files stay readable (goal 4, UC-2). The cost is one Core fetch per picked skill per turn, bounded by the cap. + Resolving lazily on a `read_skill` miss would remove it but needs an async path through the registry merge; see + [Out of Scope](#out-of-scope). +- **No dedup against the app config.** A picked URL the app also declares is resolved again and wins its own name by + order. It is the same content from two sources, and needs no merging logic. +- **Where it gets the messages.** Initializers run **before** `setup_messages`, so `MessagesMixin.messages` is still + empty. `_RequestContextSetup.setup_context` will also store the raw `request.messages`, and `AppModule` will expose + them under a new DI alias, `REQUEST_MESSAGES`, next to `DIAL_API_KEY` and `TOOL_CHOICE` in `common/_di_types.py`. + An initializer rather than a transformer is deliberate: `SkillsRegistry` caches its merge on first call, and that + call comes from `_AddSystemPromptTransformer`, so populating the provider from another transformer would depend on + transformer ordering. +- **Change.** A new initializer and the `REQUEST_MESSAGES` alias. `DialSkillResolver` is used as it is. + +### 3. Registration — an ordinary skill, under its own name + +- **What.** `_InvokedSkillsContext` is an ordinary `SkillsProvider`, like `_DialSkillsContext`, with + `display_name = "user skills"` and: + + ```python + order = -10 + ``` + +- **Owner.** `quickapp/skill_invocation/`. +- **Semantics.** + - The skills it returns are the resolved skills **as resolved**: the manifest's own `name` and `description`, the + real `url`, `files` and reader. The only change is one line prepended to `content`, naming the skill as + user-selected so the model can tell it apart from the agent's own when the user refers to it in words. + - `order = -10` puts it ahead of every agent source (agent/predefined `0`, dial-prompt `10`, dial-skill `20`), so a + picked skill **wins** a name collision: it is the one listed, and the one `read_skill` returns. The agent's + same-named skill is dropped and reported by the existing `SkillsRegistry` collision path. See + [UC-5](#uc-5-the-users-skill-has-the-same-name-as-one-of-the-agents) and [Known Gaps](#known-gaps). + - **There is no uniqueness scheme in 1a, and no new collision logic.** A picked skill is named by its own manifest, + like every other skill, and the two kinds of clash are settled in two different places that both already exist: + + | Clash | Settled by | Winner | + |---|---|---| + | A pick vs. an agent's skill | `SkillsRegistry._get_merged`, by provider `order` | The pick (`order = -10`) | + | A pick vs. another pick | `DialSkillResolver.resolve`'s name dedup, **before** the provider is populated | The **oldest** pick — the resolver keeps the first occurrence, and step 4 hands it the list oldest-first | + + The second row is the one worth reading twice: two picks live in the *same* provider, so the registry never sees + the loser. `DialSkillResolver` drops it and emits `Duplicate skill name ''; keeping first occurrence`, which + reaches "Initialization issues" with the resolver's wording rather than the registry's, and does so on **every** + turn — the *Reporting* rule below cannot suppress it, because the exception is produced inside the resolver and + flows straight into the context's exception list. + + Three further consequences follow, all **accepted** for 1a: + + - The dropped pick's URL is not among the URLs that resolved, so the injector's table classifies it as *failed to + resolve* and the model is told it could not be loaded. That is misleading about the cause but correct about the + outcome: the skill genuinely is not available to the model. + - A user who re-picks a same-named skill from a different URL keeps the **older** one. + - That re-pick still produces a pair, and it is an **error** pair: the loser's URL never reaches the context, so + the injector classifies it as failed and writes the fixed "could not be loaded" sentence, naming its URL's last + segment. The model is told the second pick failed while the first one, carrying the same manifest name, sits in + the same conversation. + + **All three are unreachable under the assumption this phase rests on: picked skills have names that don't clash.** + It is the user's to keep, nothing enforces it, and 1b removes the need for it. Reporting the duplicate distinctly + was considered and rejected — see [Alternatives](#alternatives-considered). + - Everything downstream then works unchanged, which is the point of registering rather than special-casing: + - the `SkillsRegistry` merge sees one more provider; + - `generate_skills_xml` renders the skill like any other; + - `read_skill("sql-style", …)` is the same dictionary lookup; + - bundled files are read by `skill.url` through `DialSkillReader.read_bundled_file`, which the entry keeps. +- **Change.** The provider class. No change to `SkillsRegistry`, `generate_skills_xml`, `read_skill` or + `_SkillReaderStageWrapper`. + +### 4. Injection — `_SkillInvocationInjector` + +- **What.** A `StagedToolSyntheticInjector` in `quickapp/skill_invocation/`. It inserts one synthetic `read_skill` + call and result for the skill picked on the message being answered. +- **Owner.** `quickapp/skill_invocation/`. +- **Semantics.** + - **When.** Only when `_InvokedSkillsContext` recorded a pick for this turn (concern 2, step 5) — `should_inject` + returns `False` otherwise. It is read from the context, **not** by re-reading the messages: the scrub transformer + belongs to `SkillsModule`, which `app_factory` registers ahead of this module, so by the time the injector runs + the messages no longer carry `custom_content.skills` at all. + + Earlier invocations need nothing: `Orchestrator` persists everything after the last user message into + `state.tool_execution_history`, and `_MessagesSetup.extract_tool_calls` restores it. Acting on every historical + pick would duplicate them. + - **Where.** `InjectionFrequency.APPEND_IF_CHANGED`, which puts the pair at `after_first_user_idx` — directly after + the **first** user message, the same slot the built-in file-transfer skill uses. The instructions therefore read + ahead of the turns they apply to, and the explicit index keeps the pair ahead of any transformer that appends to + the end (`_TimestampInjectionTransformer`, `_AttachmentNotificationInjector`), whatever the module order. + + The frequency carries its own dedup as well: a pair already in the conversation for the same tool, arguments and + content is replaced in place rather than added, so a request can never carry two pairs sharing a `tool_call_id`. + With `should_inject` already limited to the turn the pick is made, that is a second line of defence rather than + the mechanism. + + The manifest the model reads therefore stays the one from the turn that picked the skill, even if the user edits + the skill afterwards. That is how every other tool result behaves, and it keeps history consistent with what the + model has already seen (goal 3). + - **What.** + + | Case | Tool call | Tool result | + |---|---|---| + | The skill resolved | `read_skill({"skill_name": ""})` | The result of actually running `read_skill`: the header line, the manifest and `` | + | Failed to resolve, or over the cap | `read_skill({"skill_name": ""})` | ``Error: the user's skill `` could not be loaded. The reason is shown to the user.`` | + + For a resolved pick `` is the manifest's name. For a failed one the skill has no manifest, so the URL's last + segment is used — it is what the name would almost certainly have been, and it gives the model something to name + in its apology. + + The failure result is a fixed sentence on purpose. The resolver's reason is operator detail — for example + `Skill validation failed for 'skills//…'` from `parse_frontmatter` — which belongs in the "Initialization + issues" stage and the logs, where the user can act on it. Feeding a varying, internals-shaped string to the model + gives it something to improvise on; a fixed sentence keeps its reaction predictable. + - **How.** `StagedToolSyntheticInjector` already looks the `read_skill` `StagedBaseTool` up by its function name and + runs it through `arun`, so the subclass supplies little more than `should_inject`, `get_tool_name`, + `get_frequency` and `get_arguments`. Because the skill is registered (concern 3), the result is byte-identical to + a model-initiated call, `` included. + + Two things do change: + + - `StagedToolSyntheticInjector.stage_level` becomes an **overridable class attribute**, defaulting to the + `StageDisplayLevel.DEBUG` it hardcoded before — so every existing subclass keeps its hidden stage — and this + injector raises it to `INFO`. The invocation is something the user did explicitly, so the ordinary + "Reading Skill: ``" stage belongs in the response, and forcing `DEBUG` would make UC-1's stage silently + never appear. + - `get_content` is overridden for the one case the base class handles badly: a pick that never reached the + registry. Running `read_skill` for it would only produce the tool's own "not found", so the fixed sentence is + returned directly instead — and because the tool never runs, a failed pick shows no stage. + - **Scrubbing lives elsewhere.** Removing `custom_content.skills` from the working copy is **not** this + transformer's job: it belongs to a small transformer in `SkillsModule`, which is never preview-gated. The + orchestrator LLM and any DIAL deployment tool that forwards the conversation don't need the field, and forwarding + it makes Core share the user's skill folders with those deployments too — a leak that must not depend on whether + the preview flag is on. Messages carrying the field are copied, not edited: the working list shares objects with + the request's own messages. + + The two transformers need no ordering between them, because the injector takes this turn's pick from the context + (concern 2, step 5) rather than re-reading the messages. + - **Reporting** (done by the initializer, concern 2). Every historical pick is resolved again on every turn, so + reporting everything every turn would repeat the same issues until the conversation ends. Only problems with the + **message being answered** — its pick failing to resolve, or extra entries dropped by the one-per-message rule — + are recorded as `SkillInitializationException` and shown in "Initialization issues"; problems with older picks + are logged. The model learns about a failed pick from its error result. **No path fails the request.** +- **Change.** A new transformer, plus the `stage_level` class attribute on `StagedToolSyntheticInjector`. + `synthetic_tool_call_injector.py` is untouched: one pair per turn is exactly what `SyntheticToolCallInjector` + already assumes. + +--- + +## Out of Scope + +- **Phase 1b: collision-free names.** [Below](#follow-up-phase-1b--collision-free-names). +- **Phase 2: letting the user pick the agent's own skills.** It needs a way for the client to list them. One + constraint is already known: Core rejects a `custom_content.skills` entry without a `url` with `400`, so a name-only + reference can't go in that array. +- **Picking `prompts/` URLs.** Core accepts only `skills/` resources in the field. Declared `prompts/` skills are used + by the model as today. +- **A configurable per-message limit.** A message invokes at most one skill, fixed. Only the conversation cap is + tunable: it already bounds how many skills are fetched and listed, and it is spent newest-first, so the current + turn's pick is never dropped in favour of older ones. +- **A per-app switch to disable invocation.** Access is capped by the user's own reach. A `features.skill_invocation` + toggle can be added if an app author asks for one. +- **Autoloading the user's skills.** Implicit, every-turn skill loading has its own access and prompt-budget story. +- **Argument templating** (`$ARGUMENTS` in the manifest). The message text is the argument. Templating would rewrite + skill content per invocation and break the "history is what the model saw" property. +- **Enforcing `allowed-tools`.** Unchanged: still advertised in the XML and never enforced. +- **Lazy resolution of older invocations.** Today every picked skill in the conversation is resolved every turn, + bounded by the cap. Resolving only on a `read_skill` miss would remove that cost but needs an async path through the + registry merge. + +--- + +## Configuration / Usage Examples + +### Picking a personal skill — turn 1 + +```jsonc +{ + "messages": [ + { + "role": "user", + "content": "/code-review the diff in the attachment, focus on auth", + "custom_content": { + "attachments": [{ "type": "text/x-diff", "url": "files//pr-812.diff" }], + "skills": [{ "url": "skills//code-review" }] + } + } + ] +} +``` + +What the orchestrator LLM receives, when the agent also has its own `code-review`: + +```text +system … + code-reviewHow I want my code reviewed + … ← the user's; the agent's was dropped (UC-5) +user /code-review the diff in the attachment, focus on auth +assistant tool_calls: read_skill {"skill_name": "code-review"} +tool Skill `code-review`, selected by the user for this conversation. + ---\nname: code-review\n… \n\nreferences/checklist.md\n +``` + +The user sees the stage "Reading Skill: code-review", and an "Initialization issues" note that the agent's +`code-review` was superseded. + +### Turn 2 + +The client resends turn 1, with `custom_content.skills` still on the first user message, then the new user message. +QuickApps restores the turn-1 pair from assistant state, resolves and registers the picked skill again under the same +name so its files stay readable, and injects nothing new unless the new message picks a URL it has not seen before. +When it does, that pair is inserted right after the **first** user message too — ahead of turn 1's answer and ahead of +the turn-1 pair — so the manifests sit together at the head of the conversation, most recent pick first. + +This walkthrough is the case where the pick is on the **first** user message. A pick made on any later message behaves +differently — see [Known Gaps](#known-gaps). + +### Limits + +| Variable | Default | Purpose | +|---|---|---| +| `SKILL_INVOCATION_MAX_SKILLS` | `10` | Distinct picked skills resolved and listed per request across the conversation, newest first. Each one adds a `` block to the system prompt on every turn and one Core fetch per turn. The fetches are parallel, so this is a prompt budget and a Core-load budget more than a latency one | +| `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` | Reused unchanged for the manifest and each bundled file | + +### Failure modes + +| Condition | Result | +|---|---| +| No chips anywhere | Exactly as today, system prompt included; no extra Core calls | +| `ENABLE_PREVIEW_FEATURES=false` | The module is unregistered, so a chip is not resolved, registered or injected, and neither the user nor the model is told. The scrub still runs, because it lives in the never-gated `SkillsModule`, so the field does not reach the orchestrator deployment either way | +| Malformed entry, absolute URL, or not a `skills/` resource | Core rejects the request with `400`; QuickApps never sees it | +| Picked skill deleted (user's own, or public) | Error result on the invoking turn; on later turns it isn't registered and its name returns "not found" | +| Picked skill shared with the user, share later revoked | Core rejects the request with `403`, on every later turn of that conversation (Known Gaps) | +| Invalid or over-cap `SKILL.md` | Fixed error result for the model; the reason in "Initialization issues"; not registered | +| Picked skill has the same `name` as an agent skill | The user's wins and the agent's is dropped and reported (UC-5) | +| Two picked skills share a `name` | Decided by `DialSkillResolver`'s name dedup, not the registry — the **oldest** pick wins, the other is dropped with `Duplicate skill name …`, reported every turn | +| The same manifest `name` picked from two different URLs | The older pick stays registered. The newer is dropped by the resolver's name dedup, so it never reaches the context and gets an error pair naming its URL's last segment — the model is told the second pick failed while the first, under the same name, is present. Accepted: unreachable under the non-clashing-names assumption (concern 3) | +| Picked URL the app also declares | Same content, one entry; the user's copy wins by order | +| The same URL picked again later | The first pick stands: no new pair, no stage, and its position under the cap is **not** refreshed, so a re-picked old skill can still age out | +| The user edits a picked skill mid-conversation | The model keeps the manifest from the turn that first picked it; a later `read_skill` of a bundled file returns the current file | +| More than one entry on one message | Only the first is loaded. The rest are ignored — reported as a warning on the message being answered, logged on earlier turns. Identical entries count once | +| Over `SKILL_INVOCATION_MAX_SKILLS` in the conversation | The oldest picks stop being registered; their manifests stay in history; their names return "not found" | +| A dropped pick's files are read later | Its turn-1 manifest is still in history advertising ``, but its name no longer resolves, so `read_skill(name, path)` answers "not found" with no explanation of why. A rough edge of the cap, not engineered around | +| DIAL Core outage | Every picked skill gets an error result or drops out; the agent's own skills are unaffected; request served | +| Picked on a user message that isn't the first | The pair is still inserted at `after_first_user_idx`, ahead of the conversation's first turn — but `Orchestrator` only persists messages after the **last** user message into `state.tool_execution_history`. On the next turn the pair sits before that boundary, so it isn't restored and nothing re-injects it (`should_inject` is `False` — no new pick). The manifest silently drops out of context even though the skill stays registered and its files stay readable via `read_skill`. See [Known Gaps](#known-gaps) | + +--- + +## Known Gaps + +### A pick on a later message doesn't survive the next turn + +The injector always places its synthetic pair at `after_first_user_idx` — the slot after the conversation's **first** +user message, regardless of which message the pick was made on (concern 4, *Where*). `Orchestrator` persists into +`state.tool_execution_history` only the messages after the **last** user message. Those two boundaries coincide only +when the pick was made on the first user message. + +For a pick made on any later message, the pair sits before the persistence boundary on the very turn it is injected, +so the next turn does not restore it, and `should_inject` finds no new pick to inject in its place. The manifest is +gone from context from the following turn on, even though the skill is still registered and `read_skill` still serves +its bundled files. + +**Accepted for 1a.** The Turn-2 walkthrough above shows the case that works — a pick on the first message — because +that is also the case every use case in this design happens to use. Fixing it means either persisting from +`after_first_user_idx` instead of the last user message (a change to `Orchestrator`, shared with every other injector) +or moving the pair's insertion point to track the picking message instead of the first one (a change to +`InjectionFrequency.APPEND_IF_CHANGED`'s fixed slot). Both are out of scope for a phase whose goal is one new provider +and one new injector, not changes to shared orchestration. `docs/skills.md` documents this as a known gap for phase 1a. + +### A picked skill shadows the agent's same-named skill + +`order = -10` means a user's pick replaces an agent skill of the same name for the whole conversation — including a +**predefined** skill the app author considers part of the product. The collision is reported in "Initialization +issues", so it is visible, but the author cannot prevent it and the user may not realise what they displaced. + +**Accepted for 1a**, and it rests on one assumption stated plainly: **picked skills must have names that do not +clash with the agent's.** Nothing enforces it in this phase. A user who breaks it silently loses the agent's skill of +that name for the conversation. + +The alternative orders are worse. At `order = 30` the agent wins instead, and the synthetic pair would inject the +agent's skill while the user picked their own — silently doing the wrong thing on the one action the user took +explicitly. An order that spares predefined skills but not declared ones (`order = 5`) was considered and rejected as +a half-measure: it splits the rule in two without removing the clash. Phase 1b removes the need for the assumption +altogether by giving picked skills names that cannot collide. + +A second consequence of registering the skill as resolved: its `description`, `license`, `compatibility`, +`allowed-tools` and arbitrary `metadata` are rendered into `` by `generate_skills_xml`, so a picked +skill puts user-controlled text into the system prompt, the channel the app author owns — the `description` +unbounded. Accepted as it stands: the manifest body already reaches the model as a tool result either way, and the +number of picked skills is capped. Nothing is trimmed in 1a; 1b revisits it, where a picked skill stops being listed +under its own identity at all. + +### A revoked share breaks the conversation + +Core rejects a request with `403` when any `custom_content.skills` URL in it is unreadable by the user, the same +fail-closed rule it applies to attachments. The client resends the whole history every turn, so a skill that was +shared with the user and then unshared makes **every later turn** of that conversation fail, not just the turn that +picked it. The user's own skills and public skills are not affected: the user can always read their own bucket, and +Core doesn't check public skills. A deleted skill of either kind passes Core and becomes an ordinary QuickApps error +result. + +QuickApps can't work around this, because the request never reaches it. The fix belongs in Core: skip an unreadable +skill reference instead of failing the request, and let QuickApps report it like any other skill it can't load. Until +then the user's only way out is to start a new conversation. + +### An edited skill can leave two manifests in context + +`SyntheticToolCallInjector._inject_append_if_changed` matches an existing pair on tool, arguments **and** content. +When a pair for the same skill is already in the history but its content has changed — the user edited the skill in +Core between turns — the match fails and the injector appends a second pair instead of replacing the first. The +conversation then carries two manifests under one skill name: the frozen one from the turn that first picked it, and +the current one. The model sees both, with no marker saying which is current. + +The flaw is in the shared injector rather than in this feature. `_InjectFileTransferInstructionTransformer` re-injects +on every turn and has the same exposure, but only a redeploy changes the built-in skill's content mid-conversation. +What skill invocation adds is an ordinary user action that reaches it: edit your own skill, then invoke it again. + +**Accepted for 1a.** The blast radius is one skill and two versions of it, and the later pair wins in practice by +sitting closer to the end of the context. Both fixes — replacing a prior pair on an arguments match regardless of +content, or dropping the stale pair outright — change `SyntheticToolCallInjector` for every injector that uses it, +and reopening the shared injector is the one thing this phase set out not to do (goal 7). + +--- + +## Alternatives Considered + +| Alternative | Why not | +|---|---| +| **A collision-free derived name now** (`user::`) | It is phase 1b. It needs a listed-name format, a trimmed metadata copy, a stage-title parser so the user doesn't read a hash, and the resolver's name-dedup turned off. None of it is needed to make `/skill-name` work, and the clash it prevents is one we are willing to assume away for now | +| **The agent's skill wins instead** (`order = 30`) | The synthetic pair addresses the skill by name, so it would inject the agent's skill while the user picked their own — silently wrong on the one action the user took explicitly | +| **Register the skill for reads but keep it out of ``** | Keeps user-controlled text out of the system prompt, but `SkillsRegistry` builds the XML and the lookup table in one pass, so it needs a new `listed` flag on `SkillsProvider` — a special case for one provider, against goal 7. The model also can't see that it has the skill | +| **A separate `read_user_skill` tool** | Sidesteps naming entirely and allows lazy resolution, but adds a second skill-reading tool to every preview app's tool list and two mental models for "read a skill" | +| **Only list the picked skill, without injecting it** | Not deterministic (goal 1): the model decides whether to read it | +| **Report a duplicate-name pick distinctly** (a field separating "dropped as a duplicate" from "failed to load") | The user is already told: the resolver's `Duplicate skill name …` reaches "Initialization issues" verbatim. Splitting the model-facing result too would replace one fixed sentence with two, for a case the non-clashing-names assumption rules out | +| **`unique_names=False` on the resolver, letting the registry settle pick-vs-pick** | Would make clash behaviour coherent and is only ~9 lines, but reopens `dial_skills/` — the one thing that keeps this phase to "add a provider". Deferred to 1b, which needs the flag anyway | +| **Only inject, without registering** | The manifest reaches the model, but bundled files are unreadable and the model can't see it has the skill (goals 4 and 7) | +| **Persist name/description/files in choice state and skip the per-turn fetch** | Would remove the re-resolution, but `ResolvedSkill.content` is an eager `str`, so `get_skill_content` would need a lazy path — a change to the skills contract, against goal 7. The fetches are already parallel (`asyncio.gather` in `DialSkillResolver`), so the saving is Core load, not latency | +| **Inline the manifest into the user message every turn** | Re-fetches and re-inlines on every turn. If the skill is edited, earlier turns silently change, so history stops matching what the model saw. The synthetic pair freezes the manifest as it was when invoked | +| **Parse `/name` from the text** | A name carries no access, so a personal skill still couldn't be read, and it is ambiguous among the user's same-named skills | +| **Request-level `custom_fields.skills.invoked`** | An invocation belongs to a turn. A request-level field only describes the latest turn and is gone from history once the next message is sent, so Core can't re-share the skill and its files become unreadable | +| **Send the skill as a `custom_content.attachments` entry** | Works with no Core change, but when the user switches models mid-conversation every other deployment would see a `skills/` URL as a file attachment, and QuickApps would have to filter it out of its own attachment pipeline | + +--- + +## Follow-up: phase 1b — collision-free names + +Tracked separately; sketched here so 1a's boundary is legible. 1b stops a picked skill from shadowing the agent's, so +both stay listed and reachable, and the app author's product is never displaced by a user's pick. + +What it adds, none of which 1a needs: + +| Addition | Why | +|---|---| +| A derived listed name (`user::` or similar) | Unique by construction: a valid skill name is `[a-z0-9-]` and never contains `:`, so a picked skill can never take an agent skill's name, and two picks from different buckets differ by hash | +| A trimmed metadata copy | Once the entry is listed under a name of QuickApps' making, `license`, `compatibility`, `allowed-tools` and `metadata` should not ride along into the author's system prompt, and the description wants a hard cap | +| `DialSkillResolver.resolve(unique_names=False)` | Two picked skills may share a manifest `name`; both are legitimate once each is listed under a derived name | +| A stage-title parse | So the user reads `code-review (user skill)`, not `code-review:3f9a2c` | +| `order` becomes irrelevant | With no possible collision there is nothing for precedence to decide | + +1b is additive: the wire contract, the resolution walk, the injection, the scrub and the failure handling all stay, +and the injector's `skill_name` argument switches from the manifest name to the derived one. + +--- + +## Migration + +### Breaking changes + +None for existing apps. Within the feature, 1b changes the name a picked skill is listed under, which changes the +system prompt and the synthetic call arguments for conversations in flight; a conversation started under 1a keeps its +turn-1 pair under the old name, so its bundled files stop resolving after the upgrade. Acceptable while the feature is +preview-gated. + +### Non-breaking changes + +- Requests without `custom_content.skills` behave exactly as today, system prompt included: no new Core calls, no new + messages, no change to ``. +- The package is gated by `@preview_module`, matching `DialSkillsModule`, whose `DialSkillResolver` it reuses. + +## Summary of Changes + +### `quickapp/skill_invocation/` (new) + +- `_skill_reference.py` — `message_skill_urls`, `collect_picks` and the `ConversationPicks` it returns: parse + `custom_content.model_extra["skills"]` on user messages, canonicalise each URL, keep one pick per message keyed by + user-message ordinal, dedupe across the conversation keeping the **first** occurrence, and record the entries the + one-per-message rule ignored. Plus `skill_name_from_url`, for a pick with no manifest to ask. +- `_invoked_skills_context.py` — `_InvokedSkillsContext(SkillsProvider)`, `order = -10`, + `display_name = "user skills"`. Holds the resolved skills as resolved, keyed by URL, with one header line prepended + to `content`; the URL picked on the message being answered; and the exceptions to report. +- `_skill_invocation_initializer.py` — `_SkillInvocationInitializer(CompletionInitializer)`: collect, dedupe, cap + newest-first, delegate to `DialSkillResolver`, report this turn's problems. +- `_skill_invocation_injector.py` — `_SkillInvocationInjector(StagedToolSyntheticInjector)`: one synthetic + `read_skill` pair for this turn's pick via the real tool, `stage_level = INFO`, and a fixed error result for a pick + that never reached the registry. No scrubbing, no reporting. +- `_settings.py` — `SkillInvocationSettings` (`SKILL_INVOCATION_MAX_SKILLS`). +- `skill_invocation_module.py` — `@preview_module`; multiproviders for `CompletionInitializer`, `SkillsProvider`, + `MessagesTransformer`, `InitializationException`. Registered in `app_factory.py` after `DialSkillsModule`. + +### `quickapp/common/` + +- `_di_types.py` — `REQUEST_MESSAGES`. +- `synthetic_injection/staged_tool_synthetic_injector.py` — `stage_level` becomes an overridable class attribute, + defaulting to the `DEBUG` it hardcoded before, so existing subclasses are unaffected. +- `synthetic_injection/synthetic_tool_call_injector.py` — unchanged. + +### `quickapp/core/application/` + +- `_request_context.py`, `_request_context_setup.py`, `app_module.py` — store the raw request messages in + `setup_context` and provide them as `REQUEST_MESSAGES`. + +### `quickapp/skills/` + +- `_scrub_skill_chips_transformer.py` (new) — a `MessagesTransformer` that removes `custom_content.skills` from the + working copy, registered by `SkillsModule`. It lives here, not in `skill_invocation/`, because `SkillsModule` is + never preview-gated and the field must not reach the orchestrator deployment even with the feature off. +- `skills_module.py` — register that transformer. + +Nothing else here changes: no name helpers, no listed-name format, no stage-title change. + +### Unchanged + +`quickapp/dial_skills/` is not touched — `DialSkillResolver` keeps its signature and its name-dedup. That is goal 7, +and it is what 1b will start changing. + +### Outside this repo + +- `ai-dial-core` — done in #1956 for chat completions. Follow-up request: skip an unreadable skill reference instead + of rejecting the request (Known Gaps). +- `ai-dial-chat` — `skills?: SkillRefDto[]` on `MessageCustomContentDto` (`forbidNonWhitelisted` rejects it today), + the `/` palette of the user's own skills, and the skill chip. `content` is sent as typed, `/name` token included. +- `aidial-sdk` (optional) — a typed `CustomContent.skills`. +- `docs/skills.md` — document invocation, the wire field and the limits once implemented. diff --git a/docs/skills.md b/docs/skills.md index 434563cc..ef2f2d24 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -294,6 +294,85 @@ For design details, see [the design doc](designs/skills_as_dial_resource.md). --- +## Invoking a Skill from a Message (preview) + +A user can bring **their own** skill into a conversation with an agent they do not own. The client +puts the picked skill on the user message; QuickApps loads it for that turn and keeps it readable for +the rest of the conversation. The model does not get to choose — a picked skill is always loaded. + +### Wire Contract + +```jsonc +{ + "role": "user", + "content": "/code-review focus on auth", + "custom_content": { + "skills": [{ "url": "skills//code-review" }] + } +} +``` + +- `url` is the only field QuickApps reads; anything else the client adds (a title, say) is ignored. +- The URL carries **no trailing slash** — DIAL Core shares exactly the URL it is given. +- The field is **per message**, not per request: it stays on the turn it was picked on, which is what + lets Core re-share the skill on every later turn and keeps the bundled files readable. +- `content` is opaque. QuickApps never parses it, so the `/name` token is just text. +- **One skill per message.** The field is an array, but only the first entry is loaded; any others + are ignored and reported as a warning in the initialization issues stage. +- `custom_content.skills` on a non-user message is ignored. +- DIAL Core auto-shares each referenced skill to the application's per-request key, and rejects the + whole request with `400` for a malformed entry or a non-`skills/` URL, and with `403` for a skill + the user cannot read. + +### Behavior + +- Each picked skill is resolved like a `dial-skill` and registered under **its own manifest name**, + so ``, `read_skill` and bundled-file reads all work unchanged. +- A synthetic `read_skill` call and result pair is inserted at the head of the conversation, right + after the first user message — the same place the built-in file-transfer skill goes — so the + instructions read ahead of the turns they apply to. The user sees the normal + "Reading Skill: ``" stage — the invocation is something they did explicitly, so unlike + other synthetic injections it is not hidden. +- The injection runs only on the turn the pick is made. A pick made on the **first** user message + of a conversation is persisted with the rest of that turn's tool history, so on later turns the + pair comes back from the assistant state like any other tool result and `read_skill` is never + re-run for it. +- A pick made on a **later** message is not persisted: only what follows the last user message is + stored, and the pair is inserted ahead of that point. The manifest is in context for that turn + only. The skill stays listed in `` and its files stay readable, but the model + has to call `read_skill` itself to see the manifest again. Known gap for phase 1a. +- The model therefore keeps the manifest it saw when the skill was picked, even if the user edits + the skill afterwards. A later read of a **bundled file** returns the current file. +- A picked skill **wins** a name collision with one of the agent's skills, which is then dropped and + reported in the initialization issues stage. Picked skills are expected to have names that do not + clash with the agent's; nothing enforces it yet. +- A chip that cannot be loaded gets an error result naming the skill, so the model can say so, and + the reason is reported to the user. The request is still served. +- The field is stripped from the working messages before they reach the orchestrator or any DIAL + deployment tool, whether or not preview features are enabled. + +### Limits + +| Variable | Default | Purpose | +|---|---|---| +| `SKILL_INVOCATION_MAX_SKILLS` | `10` | Distinct picked skills resolved and listed per request across the conversation, newest first. Each one adds a `` block to the system prompt and one DIAL Core fetch per turn. | +| `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` | Reused unchanged for the manifest and each bundled file. | + +Beyond the cap the **oldest** picks stop being registered: their manifests stay in the history, but +their names no longer resolve. + +### Limitations + +- Preview-gated: with `ENABLE_PREVIEW_FEATURES=false` a chip is neither resolved nor injected. +- The user can only pick skills from their own catalog — the agent's own skills are not offered. +- Only `skills/` resources can be picked; a declared `prompts/` skill is used by the model as today. +- A skill that was shared with the user and later unshared makes every later turn of that + conversation fail with `403` in DIAL Core, before the request reaches QuickApps. + +For design details, see [the design doc](designs/skill_invocation.md). + +--- + ## Migrating from Agent Instructions The `config/predefined/instructions/` directory convention and `AgentInstructionsProvider` have been removed. The skills diff --git a/src/quickapp/app_factory.py b/src/quickapp/app_factory.py index 052f1a81..b3660057 100644 --- a/src/quickapp/app_factory.py +++ b/src/quickapp/app_factory.py @@ -29,6 +29,7 @@ ) from quickapp.rest_api_tooling import RestApiToolingModule from quickapp.shared import shared_module +from quickapp.skill_invocation import SkillInvocationModule from quickapp.skills.skills_module import SkillsModule from quickapp.starters.starters_module import StartersModule from quickapp.timestamp_tooling.timestamp_module import TimestampModule @@ -60,6 +61,7 @@ def build_di_modules() -> list[Module]: SkillsModule(), DialPromptSkillsModule(), DialSkillsModule(), + SkillInvocationModule(), TimestampModule(), AgentHooksModule(), DialFilesToolingModule(), diff --git a/src/quickapp/common/__init__.py b/src/quickapp/common/__init__.py index cb8d7715..bd5f10d2 100644 --- a/src/quickapp/common/__init__.py +++ b/src/quickapp/common/__init__.py @@ -8,6 +8,7 @@ DIAL_BEARER, EXTERNAL_TOOL_NAMES, ORCHESTRATOR_AZURE_CLIENT, + REQUEST_MESSAGES, RESPONSE_FORMAT, TOOL_CHOICE, ACCEPT_LANGUAGE, diff --git a/src/quickapp/common/_di_types.py b/src/quickapp/common/_di_types.py index faae894c..d3cbf084 100644 --- a/src/quickapp/common/_di_types.py +++ b/src/quickapp/common/_di_types.py @@ -1,6 +1,6 @@ from typing import Annotated -from aidial_sdk.chat_completion import ResponseFormat +from aidial_sdk.chat_completion import Message, ResponseFormat from aidial_sdk.chat_completion.request import ToolChoice from openai.lib.azure import AsyncAzureOpenAI from pydantic import SecretStr @@ -16,3 +16,6 @@ ORCHESTRATOR_AZURE_CLIENT = Annotated[AsyncAzureOpenAI, "ORCHESTRATOR_AZURE_CLIENT"] DEPLOYMENT_AZURE_CLIENT = Annotated[AsyncAzureOpenAI, "DEPLOYMENT_AZURE_CLIENT"] ACCEPT_LANGUAGE = Annotated[str | None, "ACCEPT_LANGUAGE"] +# Raw request messages, as they arrived. Available to initializers, which run +# before `_RequestContextSetup.setup_messages` populates `context.messages`. +REQUEST_MESSAGES = Annotated[list[Message], "REQUEST_MESSAGES"] diff --git a/src/quickapp/common/synthetic_injection/staged_tool_synthetic_injector.py b/src/quickapp/common/synthetic_injection/staged_tool_synthetic_injector.py index 3c960ce0..608c3cbf 100644 --- a/src/quickapp/common/synthetic_injection/staged_tool_synthetic_injector.py +++ b/src/quickapp/common/synthetic_injection/staged_tool_synthetic_injector.py @@ -20,6 +20,11 @@ class StagedToolSyntheticInjector(SyntheticToolCallInjector, ABC): """Provides `get_content` by locating a `StagedBaseTool` by its sanitized OpenAI function name and calling `tool.arun()` with the declared arguments.""" + stage_level: StageDisplayLevel = StageDisplayLevel.DEBUG + """How visible the injected call's stage is. Defaults to DEBUG, which hides it: + an injection the user did not ask for should not look like work they requested. + A subclass acting on an explicit user action overrides it with INFO.""" + @inject def __init__( self, @@ -41,7 +46,5 @@ async def get_content(self, messages: list[Message]) -> str | None: ) return None arguments = await self.get_arguments() - result = await tool.arun( - _ARUN_SYNTHETIC_CALL_ID, stage_level=StageDisplayLevel.DEBUG, **arguments - ) + result = await tool.arun(_ARUN_SYNTHETIC_CALL_ID, stage_level=self.stage_level, **arguments) return result.content diff --git a/src/quickapp/core/application/_initialization_error_handler.py b/src/quickapp/core/application/_initialization_error_handler.py index 9e5fd181..ca34f9e0 100644 --- a/src/quickapp/core/application/_initialization_error_handler.py +++ b/src/quickapp/core/application/_initialization_error_handler.py @@ -74,8 +74,10 @@ def handle_initialization_issues(self) -> None: tool_lines.append(fenced_code_block(exc.details)) elif isinstance(exc, SkillCatastrophicInitializationException): catastrophic_lines.append(f"- {exc.reason}") - elif isinstance(exc, SkillInitializationException) and exc.url is not None: - line = f"- **{exc.url}**: {exc.reason}" + elif isinstance(exc, SkillInitializationException): + # A skill issue that belongs to the message rather than to one URL + # (e.g. more skills invoked than a message may carry) has no url. + line = f"- {exc.reason}" if exc.url is None else f"- **{exc.url}**: {exc.reason}" if exc.severity == "warning": per_url_warning_lines.append(line) else: diff --git a/src/quickapp/core/application/_request_context.py b/src/quickapp/core/application/_request_context.py index f2b9796e..3c534913 100644 --- a/src/quickapp/core/application/_request_context.py +++ b/src/quickapp/core/application/_request_context.py @@ -7,6 +7,7 @@ CLIENT_CHANNEL_ID, DIAL_API_KEY, DIAL_BEARER, + REQUEST_MESSAGES, TOOL_CHOICE, ForwardedHeaders, ) @@ -57,6 +58,19 @@ class _RequestContext(MessagesMixin): _tool_choice: TOOL_CHOICE = None _extra_tools: list[Tool] | None = None _accept_language: ACCEPT_LANGUAGE = None + _request_messages: REQUEST_MESSAGES | None = None + + @property + def request_messages(self) -> REQUEST_MESSAGES: + """Raw request messages, readable by initializers before + ``setup_messages`` populates the transformed ``messages``.""" + return self._request_messages if self._request_messages is not None else [] + + @request_messages.setter + def request_messages(self, value: REQUEST_MESSAGES) -> None: + if self._request_messages is not None: + raise RuntimeError("Request messages are already set") + self._request_messages = value @property def bearer(self) -> DIAL_BEARER: diff --git a/src/quickapp/core/application/_request_context_setup.py b/src/quickapp/core/application/_request_context_setup.py index c6e9c742..819b4aef 100644 --- a/src/quickapp/core/application/_request_context_setup.py +++ b/src/quickapp/core/application/_request_context_setup.py @@ -64,6 +64,7 @@ async def setup_context( ) log_customised_catch_all_strategies(context.application_config) if isinstance(request, Request): + context.request_messages = request.messages context.forwarded_headers = extract_x_headers_from_request(request) context.client_channel_id = _extract_client_channel_id(context.forwarded_headers) context.accept_language = request.headers.get(self.__proxy_settings.language_header) diff --git a/src/quickapp/core/application/app_module.py b/src/quickapp/core/application/app_module.py index d817a889..fcf555dd 100644 --- a/src/quickapp/core/application/app_module.py +++ b/src/quickapp/core/application/app_module.py @@ -9,6 +9,7 @@ CLIENT_CHANNEL_ID, DIAL_API_KEY, DIAL_BEARER, + REQUEST_MESSAGES, RESPONSE_FORMAT, TOOL_CHOICE, ForwardedHeaders, @@ -83,6 +84,10 @@ def __provide_choice(self, context: _RequestContext) -> Choice: def __provide_response_format(self, context: _RequestContext) -> RESPONSE_FORMAT: return context.response_format + @multiprovider + def __provide_request_messages(self, context: _RequestContext) -> REQUEST_MESSAGES: + return context.request_messages + @provider def __provide_tool_choice(self, context: _RequestContext) -> TOOL_CHOICE: return context.tool_choice diff --git a/src/quickapp/skill_invocation/__init__.py b/src/quickapp/skill_invocation/__init__.py new file mode 100644 index 00000000..376482b9 --- /dev/null +++ b/src/quickapp/skill_invocation/__init__.py @@ -0,0 +1,3 @@ +from quickapp.skill_invocation.skill_invocation_module import SkillInvocationModule + +__all__ = ["SkillInvocationModule"] diff --git a/src/quickapp/skill_invocation/_invoked_skills_context.py b/src/quickapp/skill_invocation/_invoked_skills_context.py new file mode 100644 index 00000000..b48636e4 --- /dev/null +++ b/src/quickapp/skill_invocation/_invoked_skills_context.py @@ -0,0 +1,69 @@ +import threading + +from quickapp.common.exceptions import InitializationException, SkillInitializationException +from quickapp.skills import ResolvedSkill, SkillsProvider + +_USER_SELECTED_HEADER = "Skill `{name}`, selected by the user for this conversation." + + +def _mark_user_selected(skill: ResolvedSkill) -> ResolvedSkill: + """Prepend one line naming the skill as user-selected, so the model can tell it + apart from the agent's own when the user refers to it in words.""" + header = _USER_SELECTED_HEADER.format(name=skill.metadata.name) + return skill.model_copy(update={"content": f"{header}\n{skill.content}"}) + + +class _InvokedSkillsContext(SkillsProvider): + """Request-scoped bag of the skills picked on this conversation's messages, + populated by ``_SkillInvocationInitializer``, and the ``SkillsProvider`` + ``SkillsRegistry`` consumes for them. + + The entries are the skills as resolved — own name, own description, real URL, + files and reader — so everything downstream (the merge, ``generate_skills_xml``, + ``read_skill``, bundled files) works unchanged. + + ``order`` runs ahead of every agent source (agent/predefined ``0``, dial-prompt + ``10``, dial-skill ``20``), so a picked skill wins a name collision and the + agent's same-named skill is dropped by the registry's collision path. + """ + + order = -10 + display_name = "user skills" + + def __init__(self) -> None: + self._current_pick_url: str | None = None + self._skills_by_url: dict[str, ResolvedSkill] = {} + self._exceptions: list[InitializationException] = [] + self._lock = threading.Lock() + + @property + def resolved_skills(self) -> list[ResolvedSkill]: + return list(self._skills_by_url.values()) + + @property + def exceptions(self) -> list[InitializationException]: + return self._exceptions + + @property + def current_pick_url(self) -> str | None: + """The pick on the message being answered, if this turn made one. + + Recorded here rather than re-parsed from the messages later, so the injector + does not care whether the scrub transformer has already run. + """ + return self._current_pick_url + + def set_current_pick_url(self, url: str | None) -> None: + self._current_pick_url = url + + def set_resolved_skills(self, skills: list[ResolvedSkill]) -> None: + with self._lock: + self._skills_by_url = {skill.url: _mark_user_selected(skill) for skill in skills} + + def find_skill(self, url: str) -> ResolvedSkill | None: + """The skill resolved for *url*, or ``None`` if it failed or was over the cap.""" + return self._skills_by_url.get(url) + + def append_exception(self, exception: SkillInitializationException) -> None: + with self._lock: + self._exceptions.append(exception) diff --git a/src/quickapp/skill_invocation/_settings.py b/src/quickapp/skill_invocation/_settings.py new file mode 100644 index 00000000..c68bd777 --- /dev/null +++ b/src/quickapp/skill_invocation/_settings.py @@ -0,0 +1,19 @@ +from pydantic import Field +from pydantic_settings import BaseSettings, SettingsConfigDict + + +class SkillInvocationSettings(BaseSettings): + """Operator-level limits for skills the user invokes from a message.""" + + model_config = SettingsConfigDict() + + max_skills: int = Field( + default=10, + gt=0, + description=( + "Maximum number of distinct picked skills resolved and listed per request " + "across the conversation, newest first. Each one adds a block to the " + "system prompt and one Core fetch on every turn." + ), + alias="SKILL_INVOCATION_MAX_SKILLS", + ) diff --git a/src/quickapp/skill_invocation/_skill_invocation_initializer.py b/src/quickapp/skill_invocation/_skill_invocation_initializer.py new file mode 100644 index 00000000..97ff31cb --- /dev/null +++ b/src/quickapp/skill_invocation/_skill_invocation_initializer.py @@ -0,0 +1,111 @@ +import logging + +from aidial_sdk.chat_completion import Role +from injector import inject + +from quickapp.common import REQUEST_MESSAGES +from quickapp.common.base_initializer import CompletionInitializer +from quickapp.common.exceptions import ( + SkillCatastrophicInitializationException, + SkillInitializationException, +) +from quickapp.config.skill import DialSkillConfig +from quickapp.dial_skills import DialSkillResolver +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._settings import SkillInvocationSettings +from quickapp.skill_invocation._skill_reference import collect_picks, skill_name_from_url + +logger = logging.getLogger(__name__) + + +@inject +class _SkillInvocationInitializer(CompletionInitializer): + """Resolves every skill picked anywhere in the conversation, so the merged skill + set is available to ``_AddSystemPromptTransformer``. + + An initializer rather than a transformer: ``SkillsRegistry`` caches its merge on + first call, and that call comes from ``_AddSystemPromptTransformer``. + + Every pick is re-resolved on every turn. That is what keeps a skill picked on + turn 1 listed in ```` on turn 4 and its bundled files readable. + The cost is one Core fetch per picked skill per turn, bounded by the cap and run + in parallel by ``DialSkillResolver``. + """ + + def __init__( + self, + messages: REQUEST_MESSAGES, + resolver: DialSkillResolver, + context: _InvokedSkillsContext, + settings: SkillInvocationSettings, + ) -> None: + self._messages = messages + self._resolver = resolver + self._context = context + self._settings = settings + + async def initialize(self) -> None: + collected = collect_picks(self._messages) + picks = collected.by_ordinal + last_ordinal = sum(1 for message in self._messages if message.role == Role.USER) - 1 + self._context.set_current_pick_url(picks.get(last_ordinal)) + + self.__report_ignored(collected.ignored_by_ordinal, last_ordinal) + if not picks: + return + + # The cap is spent newest-first, so the pick made on the message being + # answered is never the one dropped. + urls = [picks[ordinal] for ordinal in sorted(picks)][-self._settings.max_skills :] + + try: + output = await self._resolver.resolve([DialSkillConfig(url=url) for url in urls]) + except Exception as exc: + logger.exception("User skill resolution failed") + self._context.append_exception( + SkillCatastrophicInitializationException( + reason=f"Failed to resolve user skills: {exc}" + ) + ) + return + + self._context.set_resolved_skills(output.resolved) + self.__report(output.exceptions) + + def __report_ignored(self, ignored_by_ordinal: dict[int, list[str]], last_ordinal: int) -> None: + """Tell the user when the message being answered invoked more than one skill. + + Only one skill may be invoked per message. Older turns are left to the log, + like every other historical problem, so the stage does not repeat an issue + the user can no longer act on. + """ + ignored = ignored_by_ordinal.get(last_ordinal) + if not ignored: + return + names = ", ".join(skill_name_from_url(url) for url in ignored) + self._context.append_exception( + SkillInitializationException( + reason=( + "Only one skill can be invoked per message;" + f" the first one was loaded and these were ignored: {names}" + ), + severity="warning", + ) + ) + + def __report(self, exceptions: list[SkillInitializationException]) -> None: + """Surface only what went wrong with this turn's pick. + + Every pick is resolved again on every turn, so reporting all of them would + repeat the same issues until the conversation ends. The model still learns + about a failed pick from its error result. + """ + current = self._context.current_pick_url + for exception in exceptions: + if exception.url is None or exception.url == current: + self._context.append_exception(exception) + else: + logger.debug( + "Skipping a skill picked on an earlier turn that could not be loaded: %s", + skill_name_from_url(exception.url), + ) diff --git a/src/quickapp/skill_invocation/_skill_invocation_injector.py b/src/quickapp/skill_invocation/_skill_invocation_injector.py new file mode 100644 index 00000000..598c069c --- /dev/null +++ b/src/quickapp/skill_invocation/_skill_invocation_injector.py @@ -0,0 +1,98 @@ +import logging + +from aidial_sdk.chat_completion import Message +from injector import ProviderOf, inject + +from quickapp.common.abstract.tool_call_result_enricher import ToolCallResultEnricher +from quickapp.common.staged_base_tool import StagedBaseTool +from quickapp.common.synthetic_injection.injection_enums import InjectionFrequency +from quickapp.common.synthetic_injection.staged_tool_synthetic_injector import ( + StagedToolSyntheticInjector, +) +from quickapp.common.tool_names import INTERNAL_SKILLS_READ_SKILL_TOOL_NAME +from quickapp.config.application import StageDisplayLevel +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._skill_reference import skill_name_from_url +from quickapp.skills import ResolvedSkill + +logger = logging.getLogger(__name__) + +# Fixed on purpose: the resolver's reason is operator detail that belongs in the +# "Initialization issues" stage and the logs. A varying, internals-shaped string +# gives the model something to improvise on. +_LOAD_FAILED = ( + "Error: the user's skill `{name}` could not be loaded. The reason is shown to the user." +) + + +class _SkillInvocationInjector(StagedToolSyntheticInjector): + """Turns the skill the user invoked on this message into a synthetic ``read_skill`` + call and result, so the model always starts the turn with the manifest in context. + + Everything but the arguments comes from ``StagedToolSyntheticInjector``: it looks + the tool up by its function name and runs it. The stage is raised to ``INFO`` — + the invocation is something the user did explicitly, so the ordinary + "Reading Skill: " stage belongs in the response. + + ``APPEND_IF_CHANGED`` puts the pair after the first user message, the same slot the + built-in file-transfer skill uses, and it runs only on the turn the pick is made + (``should_inject``). + + How long the pair survives depends on *which* turn made the pick. + ``Orchestrator._build_tool_execution_history`` persists only what follows the + **last** user message, so a pick made on the first user message is stored and comes + back from ``state.tool_execution_history`` on every later turn, like any other tool + result. A pick made on any later message lands ahead of that boundary, is never + persisted, and is therefore in context for its own turn only — the skill stays + listed in ```` and readable through ``read_skill``, but the model + is no longer handed the manifest unprompted. Accepted for phase 1a; the file-transfer + injector does not hit this because it re-injects on every turn instead of relying on + persistence. + """ + + stage_level = StageDisplayLevel.INFO + + @inject + def __init__( + self, + context: _InvokedSkillsContext, + tools: list[StagedBaseTool], + enrichers_provider: ProviderOf[list[ToolCallResultEnricher]], + ) -> None: + super().__init__(tools, enrichers_provider) + self.__context = context + + async def should_inject(self, messages: list[Message]) -> bool: + return self.__context.current_pick_url is not None + + async def get_tool_name(self) -> str: + return INTERNAL_SKILLS_READ_SKILL_TOOL_NAME + + async def get_frequency(self, messages: list[Message]) -> InjectionFrequency: + return InjectionFrequency.APPEND_IF_CHANGED + + async def get_arguments(self) -> dict: + return {"skill_name": self.__skill_name()} + + async def get_content(self, messages: list[Message]) -> str | None: + """Delegate to the tool, except for a skill that never made it into the registry. + + Running ``read_skill`` for one would only produce the tool's own "not found", + so the fixed sentence is returned directly instead. + """ + if self.__resolved_skill() is None: + return _LOAD_FAILED.format(name=self.__skill_name()) + return await super().get_content(messages) + + def __skill_name(self) -> str: + """The picked skill's own name, or the URL's last segment when it failed to + resolve — which is what the name would almost certainly have been, and gives + the model something to name in its apology.""" + skill = self.__resolved_skill() + if skill is not None: + return skill.metadata.name + return skill_name_from_url(self.__context.current_pick_url or "") + + def __resolved_skill(self) -> ResolvedSkill | None: + url = self.__context.current_pick_url + return self.__context.find_skill(url) if url is not None else None diff --git a/src/quickapp/skill_invocation/_skill_reference.py b/src/quickapp/skill_invocation/_skill_reference.py new file mode 100644 index 00000000..d49c7d0b --- /dev/null +++ b/src/quickapp/skill_invocation/_skill_reference.py @@ -0,0 +1,108 @@ +import logging +from typing import Any + +from aidial_sdk.chat_completion import Message, Role +from pydantic import BaseModel, ConfigDict, Field + +from quickapp.skills import SKILL_CHIPS_FIELD + +logger = logging.getLogger(__name__) + + +def message_skill_urls(message: Message) -> list[str]: + """Canonical skill URLs picked on one user message, in chip order, deduplicated. + + ``custom_content.skills`` on a non-user message is ignored. A malformed entry is + dropped with a debug log — Core answers such a request with ``400``, so reaching + here means something upstream changed, and refusing the turn over it would be worse. + """ + if message.role != Role.USER or message.custom_content is None: + return [] + + entries = (message.custom_content.model_extra or {}).get(SKILL_CHIPS_FIELD) + if not isinstance(entries, list): + if entries is not None: + logger.debug("Ignoring a malformed custom_content.%s field", SKILL_CHIPS_FIELD) + return [] + + urls: list[str] = [] + for entry in entries: + url = _canonical_url(entry) + if url is None: + logger.debug("Ignoring a malformed skill reference") + elif url not in urls: + urls.append(url) + return urls + + +class ConversationPicks(BaseModel): + """What the chips of a whole conversation add up to.""" + + model_config = ConfigDict(frozen=True) + + by_ordinal: dict[int, str] = Field(default_factory=dict) + """Picked URL, keyed by the ordinal of the user message that picked it.""" + + ignored_by_ordinal: dict[int, list[str]] = Field(default_factory=dict) + """Extra URLs dropped because only one skill may be invoked per message.""" + + +def collect_picks(messages: list[Message]) -> ConversationPicks: + """Every skill pick in the conversation, keyed by the **ordinal** of the user + message that made it (0 for the first user message, 1 for the second, ...). + + The ordinal is the anchor rather than a list index because the injector sees a + different list from the one parsed here: ``extract_tool_calls`` has expanded the + stored tool history and the scrub transformer has copied the chipped messages. + User messages survive both, in order, so counting them is stable. + + One skill per turn: a message carrying more than one chip keeps the first and + reports the rest, rather than dropping them silently. A URL picked again on a + later turn keeps its first pick, so the skill is loaded once, ahead of the + message that first asked for it. + """ + by_ordinal: dict[int, str] = {} + ignored_by_ordinal: dict[int, list[str]] = {} + seen: set[str] = set() + ordinal = -1 + + for message in messages: + if message.role != Role.USER: + continue + ordinal += 1 + urls = message_skill_urls(message) + if not urls: + continue + if len(urls) > 1: + ignored_by_ordinal[ordinal] = urls[1:] + # Debug, not warning: every turn re-parses the whole conversation, so a + # warning here would repeat for every historical message that carried extras. + logger.debug( + "A user message carries %d skill chips; only the first is loaded", len(urls) + ) + if urls[0] not in seen: + seen.add(urls[0]) + by_ordinal[ordinal] = urls[0] + + return ConversationPicks(by_ordinal=by_ordinal, ignored_by_ordinal=ignored_by_ordinal) + + +def skill_name_from_url(url: str) -> str: + """Last segment of a skill URL — what the manifest name would almost certainly + have been, for a skill that failed to load and has no manifest to ask.""" + return url.rsplit("/", 1)[-1] + + +def _canonical_url(entry: Any) -> str | None: + """``url`` of one chip without its trailing slash, or ``None`` if malformed. + + Core shares ``skills//`` exactly as written and authorises every + read of the skill against it, so the trailing slash has to go: a URL ending in + ``/`` is shared under a different key and every read of it is then denied. + """ + if not isinstance(entry, dict): + return None + url = entry.get("url") + if not isinstance(url, str): + return None + return url.strip().rstrip("/") or None diff --git a/src/quickapp/skill_invocation/skill_invocation_module.py b/src/quickapp/skill_invocation/skill_invocation_module.py new file mode 100644 index 00000000..f24bd1de --- /dev/null +++ b/src/quickapp/skill_invocation/skill_invocation_module.py @@ -0,0 +1,57 @@ +import logging + +from fastapi_injector import request_scope +from injector import Binder, Module, ProviderOf, multiprovider, singleton + +from quickapp.common.abstract.base_transformer import MessagesTransformer +from quickapp.common.base_initializer import CompletionInitializer +from quickapp.common.exceptions import InitializationException +from quickapp.common.preview import preview_module +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._settings import SkillInvocationSettings +from quickapp.skill_invocation._skill_invocation_initializer import _SkillInvocationInitializer +from quickapp.skill_invocation._skill_invocation_injector import _SkillInvocationInjector +from quickapp.skills import SkillsProvider + +logger = logging.getLogger(__name__) + + +@preview_module +class SkillInvocationModule(Module): + """Wires skills a user invokes from a message, via ``custom_content.skills``. + + Preview-gated, matching ``DialSkillsModule``, whose ``DialSkillResolver`` it + reuses. Scrubbing the field off the working messages deliberately lives in the + never-gated ``SkillsModule`` instead. + """ + + def configure(self, binder: Binder) -> None: + binder.bind(SkillInvocationSettings, to=SkillInvocationSettings, scope=singleton) + binder.bind(_InvokedSkillsContext, to=_InvokedSkillsContext, scope=request_scope) + binder.bind( + _SkillInvocationInitializer, to=_SkillInvocationInitializer, scope=request_scope + ) + binder.bind(_SkillInvocationInjector, to=_SkillInvocationInjector, scope=request_scope) + logger.debug("SkillInvocationModule configuration completed") + + @multiprovider + def __provide_initializers( + self, initializer_provider: ProviderOf[_SkillInvocationInitializer] + ) -> list[CompletionInitializer]: + return [initializer_provider.get()] + + @multiprovider + def __provide_initialization_exceptions( + self, context: _InvokedSkillsContext + ) -> list[InitializationException]: + return context.exceptions + + @multiprovider + def __provide_skill_providers(self, context: _InvokedSkillsContext) -> list[SkillsProvider]: + return [context] + + @multiprovider + def __provide_message_transformers( + self, injector: _SkillInvocationInjector + ) -> list[MessagesTransformer]: + return [injector] diff --git a/src/quickapp/skills/__init__.py b/src/quickapp/skills/__init__.py index cf4df51d..25e9e395 100644 --- a/src/quickapp/skills/__init__.py +++ b/src/quickapp/skills/__init__.py @@ -1,9 +1,11 @@ from quickapp.skills._exceptions import SkillFileNotFoundError from quickapp.skills._frontmatter import parse_frontmatter +from quickapp.skills._scrub_skill_chips_transformer import SKILL_CHIPS_FIELD from quickapp.skills._skill_metadata import SkillMetadata from quickapp.skills.skills_provider import ResolvedSkill, SkillFileReader, SkillsProvider __all__ = [ + "SKILL_CHIPS_FIELD", "ResolvedSkill", "SkillFileNotFoundError", "SkillFileReader", diff --git a/src/quickapp/skills/_scrub_skill_chips_transformer.py b/src/quickapp/skills/_scrub_skill_chips_transformer.py new file mode 100644 index 00000000..bc4fd911 --- /dev/null +++ b/src/quickapp/skills/_scrub_skill_chips_transformer.py @@ -0,0 +1,42 @@ +import logging + +from aidial_sdk.chat_completion import Message + +from quickapp.common.abstract.base_transformer import MessagesTransformer + +logger = logging.getLogger(__name__) + +SKILL_CHIPS_FIELD = "skills" +"""Name of the ``custom_content`` field a client uses to invoke a skill.""" + + +class _ScrubSkillChipsTransformer(MessagesTransformer): + """Removes ``custom_content.skills`` from the working message copy. + + Registered by the never-preview-gated ``SkillsModule``: neither the + orchestrator LLM nor a DIAL deployment tool that forwards the conversation + needs the field, and forwarding it makes Core share the user's skill folders + with those deployments too — a leak that must not depend on the preview flag. + + Messages carrying the field are copied, not edited: the working list shares + objects with the request's own messages. + """ + + async def transform(self, messages: list[Message]) -> list[Message]: + result: list[Message] = [] + scrubbed = 0 + for message in messages: + custom_content = message.custom_content + if custom_content is None or SKILL_CHIPS_FIELD not in ( + custom_content.model_extra or {} + ): + result.append(message) + continue + clean_custom_content = custom_content.model_copy() + delattr(clean_custom_content, SKILL_CHIPS_FIELD) + result.append(message.model_copy(update={"custom_content": clean_custom_content})) + scrubbed += 1 + + if scrubbed: + logger.debug("Removed skill chips from %d message(s)", scrubbed) + return result diff --git a/src/quickapp/skills/skills_module.py b/src/quickapp/skills/skills_module.py index f9f4eb8c..87394e5b 100644 --- a/src/quickapp/skills/skills_module.py +++ b/src/quickapp/skills/skills_module.py @@ -10,6 +10,7 @@ from quickapp.skills._inject_file_transfer_instruction_transformer import ( _InjectFileTransferInstructionTransformer, ) +from quickapp.skills._scrub_skill_chips_transformer import _ScrubSkillChipsTransformer from quickapp.skills._skill_reader_tool import _SkillReaderTool from quickapp.skills._skills_registry import SkillsRegistry from quickapp.skills._tool_configs import SKILL_READER_TOOL_CONFIG, SKILL_READER_TOOL_NAME @@ -30,6 +31,11 @@ def configure(self, binder: Binder) -> None: to=_InjectFileTransferInstructionTransformer, scope=request_scope, ) + binder.bind( + _ScrubSkillChipsTransformer, + to=_ScrubSkillChipsTransformer, + scope=request_scope, + ) @multiprovider def _provide_internal_tools( @@ -65,5 +71,6 @@ def _provide_initialization_exceptions( def _provide_message_transformers( self, file_transfer_transformer: _InjectFileTransferInstructionTransformer, + scrub_skill_chips_transformer: _ScrubSkillChipsTransformer, ) -> list[MessagesTransformer]: - return [file_transfer_transformer] + return [file_transfer_transformer, scrub_skill_chips_transformer] diff --git a/src/tests/unit_tests/application_tests/test_initialization_error_handler.py b/src/tests/unit_tests/application_tests/test_initialization_error_handler.py index 4d1953ac..f9d39555 100644 --- a/src/tests/unit_tests/application_tests/test_initialization_error_handler.py +++ b/src/tests/unit_tests/application_tests/test_initialization_error_handler.py @@ -122,6 +122,35 @@ def test_warning_renders_under_warnings_subheader_and_keeps_completed(self): assert "Errors:" not in rendered stage.close.assert_called_once_with(Status.COMPLETED) + def test_a_url_less_warning_is_rendered_rather_than_leaving_an_empty_stage(self): + """A skill issue that belongs to the message, not to one URL — e.g. more skills + invoked than a message may carry — used to fall through to the unhandled branch, + opening the stage with no content in it.""" + stage = MagicMock(spec=Stage) + warning = SkillInitializationException( + reason="Only one skill can be invoked per message; these were ignored: b, c", + severity="warning", + ) + handler = _make_handler(stage, [warning]) + + handler.handle_initialization_issues() + + rendered = _stage_content(stage) + assert "Warnings:" in rendered + assert "Only one skill can be invoked per message" in rendered + assert rendered.strip() + stage.close.assert_called_once_with(Status.COMPLETED) + + def test_a_url_less_error_is_rendered_under_errors(self): + stage = MagicMock(spec=Stage) + handler = _make_handler(stage, [SkillInitializationException(reason="boom")]) + + handler.handle_initialization_issues() + + rendered = _stage_content(stage) + assert "Errors:" in rendered + assert "boom" in rendered + def test_error_and_warning_render_in_separate_subheaders(self): stage = MagicMock(spec=Stage) error = SkillInitializationException(reason="boom", url="prompts/bucket/broken") diff --git a/src/tests/unit_tests/common/test_staged_tool_synthetic_injector.py b/src/tests/unit_tests/common/test_staged_tool_synthetic_injector.py index 8ff87382..0c6bc1ae 100644 --- a/src/tests/unit_tests/common/test_staged_tool_synthetic_injector.py +++ b/src/tests/unit_tests/common/test_staged_tool_synthetic_injector.py @@ -89,6 +89,18 @@ async def test_passes_stage_level_system(self): _, kwargs = tool.arun.call_args assert kwargs.get("stage_level") == StageDisplayLevel.DEBUG + @pytest.mark.asyncio + async def test_a_subclass_can_raise_the_stage_level(self): + """A subclass acting on an explicit user action shows its stage.""" + tool = _make_staged_tool("my_tool", "result") + injector = _ConcreteInjector([tool], "my_tool") + injector.stage_level = StageDisplayLevel.INFO + + await injector.transform([Message(role=Role.USER, content="hi")]) + + _, kwargs = tool.arun.call_args + assert kwargs.get("stage_level") == StageDisplayLevel.INFO + @pytest.mark.asyncio async def test_multiple_tools_correct_one_selected(self): tool_a = _make_staged_tool("tool_a", "from a") diff --git a/src/tests/unit_tests/skill_invocation_tests/__init__.py b/src/tests/unit_tests/skill_invocation_tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/tests/unit_tests/skill_invocation_tests/test_invoked_skills_context.py b/src/tests/unit_tests/skill_invocation_tests/test_invoked_skills_context.py new file mode 100644 index 00000000..c20b21cf --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_invoked_skills_context.py @@ -0,0 +1,52 @@ +"""``_InvokedSkillsContext`` — an ordinary ``SkillsProvider`` holding the picks.""" + +from quickapp.dial_prompt_skills import _DialPromptSkillsContext +from quickapp.dial_skills import _DialSkillsContext +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skills.agent_skills_provider import AgentSkillsProvider +from tests.unit_tests.common.common import make_resolved_skill as _skill + + +class TestOrdering: + + def test_a_pick_wins_over_every_agent_source(self): + assert _InvokedSkillsContext.order < AgentSkillsProvider.order + assert _InvokedSkillsContext.order < _DialPromptSkillsContext.order + assert _InvokedSkillsContext.order < _DialSkillsContext.order + + def test_display_name_is_human_readable(self): + assert _InvokedSkillsContext.display_name == "user skills" + + +class TestState: + + def test_starts_empty(self): + context = _InvokedSkillsContext() + + assert context.resolved_skills == [] + assert context.current_pick_url is None + + def test_content_is_prefixed_with_a_user_selected_header(self): + context = _InvokedSkillsContext() + context.set_resolved_skills( + [_skill("skills/b/sql-style", "sql-style", content="---\nbody")] + ) + + assert context.resolved_skills[0].content == ( + "Skill `sql-style`, selected by the user for this conversation.\n---\nbody" + ) + + def test_everything_else_is_kept_as_resolved(self): + skill = _skill("skills/b/sql-style", "sql-style", files=("a.md",)) + context = _InvokedSkillsContext() + context.set_resolved_skills([skill]) + + entry = context.resolved_skills[0] + assert (entry.url, entry.metadata, entry.files) == (skill.url, skill.metadata, skill.files) + + def test_find_skill_looks_up_by_url(self): + context = _InvokedSkillsContext() + context.set_resolved_skills([_skill("skills/b/sql-style", "sql-style")]) + + assert context.find_skill("skills/b/sql-style") is not None + assert context.find_skill("skills/b/other") is None diff --git a/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_initializer.py b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_initializer.py new file mode 100644 index 00000000..91da790f --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_initializer.py @@ -0,0 +1,175 @@ +"""``_SkillInvocationInitializer`` — collect picks, cap, delegate, report this turn.""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from aidial_sdk.chat_completion import Message, Role + +from quickapp.common.exceptions import SkillInitializationException +from quickapp.dial_skills._dial_skill_resolver import DialSkillResolver, DialSkillResolverOutput +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._settings import SkillInvocationSettings +from quickapp.skill_invocation._skill_invocation_initializer import _SkillInvocationInitializer +from tests.unit_tests.common.common import make_resolved_skill as _skill + + +def _user(*urls: str) -> Message: + return Message.model_validate( + { + "role": "user", + "content": "hi", + "custom_content": {"skills": [{"url": url} for url in urls]}, + } + ) + + +def _make( + messages: list[Message], + output: DialSkillResolverOutput | None = None, + max_skills: int = 10, +): + resolver = MagicMock(spec=DialSkillResolver) + resolver.resolve = AsyncMock( + return_value=output or DialSkillResolverOutput(resolved=[], exceptions=[]) + ) + context = _InvokedSkillsContext() + settings = SkillInvocationSettings(SKILL_INVOCATION_MAX_SKILLS=max_skills) + return _SkillInvocationInitializer(messages, resolver, context, settings), resolver, context + + +def _reasons(context: _InvokedSkillsContext) -> list[str]: + return [getattr(exc, "reason", str(exc)) for exc in context.exceptions] + + +class TestInitialize: + + @pytest.mark.asyncio + async def test_no_picks_skips_resolution_entirely(self): + initializer, resolver, context = _make([Message(role=Role.USER, content="hi")]) + + await initializer.initialize() + + resolver.resolve.assert_not_awaited() + assert context.current_pick_url is None + + @pytest.mark.asyncio + async def test_resolves_every_pick_in_the_conversation_oldest_first(self): + initializer, resolver, _ = _make([_user("skills/b/a"), _user("skills/b/z")]) + + await initializer.initialize() + + assert [cfg.url for cfg in resolver.resolve.await_args.args[0]] == [ + "skills/b/a", + "skills/b/z", + ] + + @pytest.mark.asyncio + async def test_the_cap_drops_the_oldest_picks(self): + initializer, resolver, _ = _make([_user("skills/b/a"), _user("skills/b/z")], max_skills=1) + + await initializer.initialize() + + assert [cfg.url for cfg in resolver.resolve.await_args.args[0]] == ["skills/b/z"] + + @pytest.mark.asyncio + async def test_the_pick_of_the_message_being_answered_is_singled_out(self): + initializer, _, context = _make([_user("skills/b/a"), _user("skills/b/z")]) + + await initializer.initialize() + + assert context.current_pick_url == "skills/b/z" + + @pytest.mark.asyncio + async def test_no_current_pick_when_the_last_message_carries_no_chip(self): + initializer, _, context = _make([_user("skills/b/a"), Message(role=Role.USER, content="?")]) + + await initializer.initialize() + + assert context.current_pick_url is None + + @pytest.mark.asyncio + async def test_registers_what_resolved(self): + initializer, _, context = _make( + [_user("skills/b/a")], + DialSkillResolverOutput(resolved=[_skill("skills/b/a", "a")], exceptions=[]), + ) + + await initializer.initialize() + + assert context.find_skill("skills/b/a") is not None + + @pytest.mark.asyncio + async def test_a_resolver_crash_is_reported_and_does_not_raise(self): + initializer, resolver, context = _make([_user("skills/b/a")]) + resolver.resolve = AsyncMock(side_effect=RuntimeError("boom")) + + await initializer.initialize() + + assert len(context.exceptions) == 1 + assert "Failed to resolve user skills" in str(context.exceptions[0]) + + +class TestOneSkillPerMessage: + + @pytest.mark.asyncio + async def test_extra_chips_on_this_turn_are_reported_as_a_warning(self): + initializer, _, context = _make([_user("skills/b/a", "skills/b/z", "skills/b/k")]) + + await initializer.initialize() + + assert context.exceptions[0].severity == "warning" + assert _reasons(context) == [ + "Only one skill can be invoked per message;" + " the first one was loaded and these were ignored: z, k" + ] + + @pytest.mark.asyncio + async def test_only_the_first_chip_is_resolved(self): + initializer, resolver, _ = _make([_user("skills/b/a", "skills/b/z")]) + + await initializer.initialize() + + assert [cfg.url for cfg in resolver.resolve.await_args.args[0]] == ["skills/b/a"] + + @pytest.mark.asyncio + async def test_extra_chips_on_an_earlier_turn_are_not_repeated_to_the_user(self): + initializer, _, context = _make( + [_user("skills/b/a", "skills/b/z"), Message(role=Role.USER, content="?")] + ) + + await initializer.initialize() + + assert context.exceptions == [] + + +class TestReporting: + + @pytest.mark.asyncio + async def test_only_this_turns_problems_reach_initialization_issues(self): + initializer, _, context = _make( + [_user("skills/b/old"), _user("skills/b/new")], + DialSkillResolverOutput( + resolved=[], + exceptions=[ + SkillInitializationException(url="skills/b/old", reason="stale"), + SkillInitializationException(url="skills/b/new", reason="fresh"), + ], + ), + ) + + await initializer.initialize() + + assert _reasons(context) == ["fresh"] + + @pytest.mark.asyncio + async def test_an_exception_without_a_url_is_always_reported(self): + initializer, _, context = _make( + [_user("skills/b/a")], + DialSkillResolverOutput( + resolved=[], exceptions=[SkillInitializationException(reason="global")] + ), + ) + + await initializer.initialize() + + assert _reasons(context) == ["global"] diff --git a/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_injector.py b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_injector.py new file mode 100644 index 00000000..f344cb22 --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_injector.py @@ -0,0 +1,142 @@ +"""``_SkillInvocationInjector`` — a ``StagedToolSyntheticInjector`` on +``InjectionFrequency.APPEND_IF_CHANGED``.""" + +import json +from unittest.mock import AsyncMock, MagicMock + +import pytest +from aidial_sdk.chat_completion import Message, Role + +from quickapp.common import StagedBaseTool, ToolCallResult +from quickapp.common.tool_names import INTERNAL_SKILLS_READ_SKILL_TOOL_NAME +from quickapp.config.application import StageDisplayLevel +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._skill_invocation_injector import _SkillInvocationInjector +from tests.unit_tests.common.common import make_resolved_skill as _skill + +_URL = "skills/b/code-review" + + +def _skill_reader_tool(content: str = "the manifest") -> MagicMock: + tool = MagicMock(spec=StagedBaseTool) + tool.tool_config = MagicMock() + tool.tool_config.open_ai_tool.function.name = INTERNAL_SKILLS_READ_SKILL_TOOL_NAME + tool.arun = AsyncMock( + return_value=ToolCallResult(content=content, content_type="text/markdown") + ) + return tool + + +def _make(current: str | None, resolved: list = (), tool: MagicMock | None = None): + context = _InvokedSkillsContext() + context.set_current_pick_url(current) + context.set_resolved_skills(list(resolved)) + + enrichers_provider = MagicMock() + enrichers_provider.get.return_value = [] + + injector = _SkillInvocationInjector( + context, [tool] if tool is not None else [], enrichers_provider + ) + return injector, tool + + +def _args(message: Message) -> dict: + return json.loads(message.tool_calls[0].function.arguments) + + +class TestInjection: + + @pytest.mark.asyncio + async def test_no_pick_this_turn_leaves_the_messages_alone(self): + injector, tool = _make(None, tool=_skill_reader_tool()) + messages = [Message(role=Role.USER, content="hi")] + + assert await injector.transform(messages) == messages + tool.arun.assert_not_awaited() + + @pytest.mark.asyncio + async def test_the_pair_goes_after_the_first_user_message(self): + injector, _ = _make(_URL, [_skill(_URL, "code-review")], _skill_reader_tool()) + messages = [ + Message(role=Role.USER, content="/code-review …"), + Message(role=Role.ASSISTANT, content="ok"), + Message(role=Role.USER, content="and also …"), + ] + + result = await injector.transform(messages) + + assert [m.role for m in result] == [ + Role.USER, + Role.ASSISTANT, + Role.TOOL, + Role.ASSISTANT, + Role.USER, + ] + assert _args(result[1]) == {"skill_name": "code-review"} + assert result[2].content == "the manifest" + assert result[2].tool_call_id == result[1].tool_calls[0].id + + @pytest.mark.asyncio + async def test_the_skill_is_read_at_info_so_the_stage_is_shown(self): + injector, tool = _make(_URL, [_skill(_URL, "code-review")], _skill_reader_tool()) + + await injector.transform([Message(role=Role.USER, content="hi")]) + + tool.arun.assert_awaited_once() + assert tool.arun.await_args.kwargs["stage_level"] == StageDisplayLevel.INFO + + @pytest.mark.asyncio + async def test_the_skill_name_is_passed_to_the_tool(self): + injector, tool = _make(_URL, [_skill(_URL, "code-review")], _skill_reader_tool()) + + await injector.transform([Message(role=Role.USER, content="hi")]) + + assert tool.arun.await_args.kwargs["skill_name"] == "code-review" + + +class TestLaterTurns: + + @pytest.mark.asyncio + async def test_nothing_is_injected_once_the_pick_is_no_longer_this_turns(self): + injector, tool = _make(_URL, [_skill(_URL, "code-review")], _skill_reader_tool()) + first = await injector.transform([Message(role=Role.USER, content="hi")]) + + # Next turn the chip is on an earlier message, so no pick is current. + injector, tool = _make(None, [_skill(_URL, "code-review")], _skill_reader_tool()) + messages = [*first, Message(role=Role.USER, content="clarify?")] + + assert await injector.transform(messages) == messages + tool.arun.assert_not_awaited() + + @pytest.mark.asyncio + async def test_re_running_the_same_turn_does_not_duplicate_the_pair(self): + injector, _ = _make(_URL, [_skill(_URL, "code-review")], _skill_reader_tool()) + first = await injector.transform([Message(role=Role.USER, content="hi")]) + + second = await injector.transform(first) + + assert [m.role for m in second] == [m.role for m in first] + + +class TestFailedPick: + + @pytest.mark.asyncio + async def test_an_unresolved_pick_gets_a_fixed_error_result_named_after_its_url(self): + injector, tool = _make(_URL, tool=_skill_reader_tool()) + + result = await injector.transform([Message(role=Role.USER, content="hi")]) + + assert _args(result[1]) == {"skill_name": "code-review"} + assert result[2].content == ( + "Error: the user's skill `code-review` could not be loaded." + " The reason is shown to the user." + ) + tool.arun.assert_not_awaited() + + @pytest.mark.asyncio + async def test_a_missing_skill_reader_tool_injects_nothing(self): + injector, _ = _make(_URL, [_skill(_URL, "code-review")]) + messages = [Message(role=Role.USER, content="hi")] + + assert await injector.transform(messages) == messages diff --git a/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_wiring.py b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_wiring.py new file mode 100644 index 00000000..768090d7 --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_wiring.py @@ -0,0 +1,77 @@ +"""DI wiring for ``SkillInvocationModule`` — an ordinary provider, initializer and +transformer, registered in ``app_factory`` behind the preview flag.""" + +from unittest.mock import MagicMock, patch + +from fastapi_injector import Injected, request_scope +from injector import Binder, Module, multiprovider +from starlette.testclient import TestClient + +from quickapp.app_factory import AppFactory +from quickapp.common import REQUEST_MESSAGES, StagedBaseTool +from quickapp.common.abstract.base_transformer import MessagesTransformer +from quickapp.common.base_initializer import CompletionInitializer +from quickapp.dial_skills._dial_skill_resolver import DialSkillResolver +from quickapp.skill_invocation import SkillInvocationModule +from quickapp.skills.skills_provider import SkillsProvider +from tests.unit_tests.common.common import create_test_app + + +class _StubDependenciesModule(Module): + """Binds what the module takes from the rest of the app.""" + + def configure(self, binder: Binder) -> None: + binder.bind( + DialSkillResolver, to=lambda: MagicMock(spec=DialSkillResolver), scope=request_scope + ) + + @multiprovider + def _provide_request_messages(self) -> REQUEST_MESSAGES: + return [] + + @multiprovider + def _provide_tools(self) -> list[StagedBaseTool]: + return [] + + +def _make_client() -> TestClient: + app = create_test_app([_StubDependenciesModule(), SkillInvocationModule()]) + + @app.get("/types") + async def types( + providers: list[SkillsProvider] = Injected(list[SkillsProvider]), + initializers: list[CompletionInitializer] = Injected(list[CompletionInitializer]), + transformers: list[MessagesTransformer] = Injected(list[MessagesTransformer]), + ) -> dict[str, list[str]]: + return { + "providers": [type(p).__name__ for p in providers], + "initializers": [type(i).__name__ for i in initializers], + "transformers": [type(t).__name__ for t in transformers], + } + + return TestClient(app) + + +class TestWiring: + + def test_the_module_contributes_a_provider_an_initializer_and_a_transformer(self): + body = _make_client().get("/types").json() + + assert body["providers"] == ["_InvokedSkillsContext"] + assert body["initializers"] == ["_SkillInvocationInitializer"] + assert body["transformers"] == ["_SkillInvocationInjector"] + + +class TestRegistration: + + def test_registered_when_preview_features_are_on(self): + with patch.dict("os.environ", {"ENABLE_PREVIEW_FEATURES": "true"}): + names = [type(m).__name__ for m in AppFactory.build_di_modules()] + assert "SkillInvocationModule" in names + + def test_dropped_when_preview_features_are_off(self): + with patch.dict("os.environ", {"ENABLE_PREVIEW_FEATURES": "false"}): + names = [type(m).__name__ for m in AppFactory.build_di_modules()] + assert "SkillInvocationModule" not in names + # The scrub still runs: it lives in the never-gated SkillsModule. + assert "SkillsModule" in names diff --git a/src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py b/src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py new file mode 100644 index 00000000..fa7bb5fe --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py @@ -0,0 +1,104 @@ +"""Parsing ``custom_content.skills`` chips off the request messages.""" + +from aidial_sdk.chat_completion import Message, Role + +from quickapp.skill_invocation._skill_reference import ( + collect_picks, + message_skill_urls, + skill_name_from_url, +) + + +def _user(*urls: str, **extra) -> Message: + custom_content = {"skills": [{"url": url} for url in urls], **extra} + return Message.model_validate( + {"role": "user", "content": "hi", "custom_content": custom_content} + ) + + +class TestMessageSkillUrls: + + def test_reads_urls_in_chip_order(self): + assert message_skill_urls(_user("skills/b/a", "skills/b/z")) == [ + "skills/b/a", + "skills/b/z", + ] + + def test_strips_a_trailing_slash(self): + assert message_skill_urls(_user("skills/b/a/")) == ["skills/b/a"] + + def test_identical_chips_count_once(self): + assert message_skill_urls(_user("skills/b/a", "skills/b/a/")) == ["skills/b/a"] + + def test_a_message_without_chips_yields_nothing(self): + assert message_skill_urls(Message(role=Role.USER, content="hi")) == [] + + def test_chips_on_a_non_user_message_are_ignored(self): + message = _user("skills/b/a") + message.role = Role.ASSISTANT + assert message_skill_urls(message) == [] + + def test_malformed_entries_are_dropped(self): + message = Message.model_validate( + { + "role": "user", + "content": "hi", + "custom_content": {"skills": ["skills/b/a", {}, {"url": 7}, {"url": "skills/b/k"}]}, + } + ) + assert message_skill_urls(message) == ["skills/b/k"] + + def test_a_non_list_field_is_ignored(self): + message = Message.model_validate( + {"role": "user", "content": "hi", "custom_content": {"skills": "skills/b/a"}} + ) + assert message_skill_urls(message) == [] + + def test_other_chip_fields_are_ignored(self): + message = Message.model_validate( + { + "role": "user", + "content": "hi", + "custom_content": {"skills": [{"url": "skills/b/a", "title": "A"}]}, + } + ) + assert message_skill_urls(message) == ["skills/b/a"] + + +class TestCollectPicks: + + def test_keys_picks_by_the_ordinal_of_the_user_message(self): + messages = [ + _user("skills/b/a"), + Message(role=Role.ASSISTANT, content="ok"), + Message(role=Role.USER, content="plain"), + _user("skills/b/z"), + ] + assert collect_picks(messages).by_ordinal == {0: "skills/b/a", 2: "skills/b/z"} + + def test_only_the_first_chip_of_a_message_is_loaded(self): + collected = collect_picks([_user("skills/b/a", "skills/b/z")]) + + assert collected.by_ordinal == {0: "skills/b/a"} + + def test_the_extra_chips_of_a_message_are_reported_not_dropped_silently(self): + collected = collect_picks([_user("skills/b/a", "skills/b/z", "skills/b/k")]) + + assert collected.ignored_by_ordinal == {0: ["skills/b/z", "skills/b/k"]} + + def test_a_single_chip_reports_nothing(self): + assert collect_picks([_user("skills/b/a")]).ignored_by_ordinal == {} + + def test_a_repicked_url_keeps_its_first_pick(self): + collected = collect_picks([_user("skills/b/a"), _user("skills/b/a")]) + + assert collected.by_ordinal == {0: "skills/b/a"} + + def test_no_chips_anywhere_yields_nothing(self): + assert collect_picks([Message(role=Role.USER, content="hi")]).by_ordinal == {} + + +class TestSkillNameFromUrl: + + def test_uses_the_last_segment(self): + assert skill_name_from_url("skills/bucket/nested/code-review") == "code-review" diff --git a/src/tests/unit_tests/skills_tests/test_scrub_skill_chips_transformer.py b/src/tests/unit_tests/skills_tests/test_scrub_skill_chips_transformer.py new file mode 100644 index 00000000..97461cdd --- /dev/null +++ b/src/tests/unit_tests/skills_tests/test_scrub_skill_chips_transformer.py @@ -0,0 +1,48 @@ +"""``_ScrubSkillChipsTransformer`` — the field never leaves QuickApps.""" + +import pytest +from aidial_sdk.chat_completion import Message, Role + +from quickapp.skills._scrub_skill_chips_transformer import _ScrubSkillChipsTransformer + + +def _user_with_chips() -> Message: + return Message.model_validate( + { + "role": "user", + "content": "hi", + "custom_content": {"skills": [{"url": "skills/b/a"}], "state": {"k": 1}}, + } + ) + + +class TestScrub: + + @pytest.mark.asyncio + async def test_removes_the_field_and_keeps_the_rest_of_custom_content(self): + message = _user_with_chips() + + result = await _ScrubSkillChipsTransformer().transform([message]) + + assert result[0].custom_content.model_extra == {} + assert result[0].custom_content.state == {"k": 1} + assert result[0].content == "hi" + + @pytest.mark.asyncio + async def test_the_request_message_is_left_untouched(self): + message = _user_with_chips() + + await _ScrubSkillChipsTransformer().transform([message]) + + assert message.custom_content.model_extra == {"skills": [{"url": "skills/b/a"}]} + + @pytest.mark.asyncio + async def test_messages_without_the_field_are_passed_through_by_reference(self): + messages = [ + Message(role=Role.USER, content="hi"), + Message(role=Role.ASSISTANT, content="ok"), + ] + + result = await _ScrubSkillChipsTransformer().transform(messages) + + assert result[0] is messages[0] and result[1] is messages[1]