From f9069038809f17c6c63a7aa2d81b114f44182e17 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Sat, 12 Sep 2026 21:08:23 +0300 Subject: [PATCH 1/9] docs: add design for invoking a skill from a message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A skill reaches a QuickApp agent only if the app author attached it and the model decides to call read_skill — the user has no deterministic way to say "use this skill for this message", and no way in at all for skills from their own catalog. This design specifies how QuickApps consumes the custom_content.skills wire contract agreed with DIAL Core (epam/ai-dial-core#1956): resolve each picked skill through DialSkillResolver, register it under a collision-free user:: id, and inject a synthetic read_skill call and result after the user message so the manifest is in context before the model's turn. Phase 1 covers the user's own skills; picking the agent's own skills is deferred to #550. Claude-Session: https://claude.ai/code/session_0184dFYXFuKAjivbVvNxL2fa --- docs/designs/skill_invocation.md | 533 +++++++++++++++++++++++++++++++ 1 file changed, 533 insertions(+) create mode 100644 docs/designs/skill_invocation.md diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md new file mode 100644 index 00000000..9455812e --- /dev/null +++ b/docs/designs/skill_invocation.md @@ -0,0 +1,533 @@ +# Design: Invoking a Skill from a Message + +- **Status:** Approved +- **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) +- **Dependencies:** + - [`skills_as_dial_resource.md`](skills_as_dial_resource.md) — `DialSkillResolver`, `DialSkillReader`, the + `` inventory. Branch `feat/418-skills-as-dial-resource`, not yet on `development`. + - `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. + +Bringing the user's skills in must not create a third problem: **two skills with the same name**. A skill is +identified only by its name, which is unique within an agent today. A user's `code-review` next to the agent's +`code-review` would leave the model unable to say which one it means. + +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. A picked skill stays usable for the rest of the conversation: the model sees it in ``, and its + bundled files stay readable through `read_skill`. +4. **Every name in `` stays unique**, so the model always gets exactly the skill it names. A picked + skill never takes an agent skill's name, and there is no precedence rule between the user's skills and the + agent's. +5. The model is never shown the user's bucket id. +6. A chip on the current message that fails is visible to both the user and the model. The request is still served. +7. The skills contract doesn't change: `` keeps `name` and `description`, and `read_skill` keeps + its parameters. Requests that don't carry the field behave exactly as today, system prompt included. +8. The change is as small as possible: the existing skills merge, XML generator and `read_skill` tool stay as they + are. + +## Phasing + +- **Phase 1 — this design.** The client knows only the user's skills. It lists them in the `/` palette and sends the + one the user picks as a URL reference (a chip). The agent's own skills, declared or predefined, stay exactly as + today: only the agent invokes them. A user who wants one asks for it in words ("use the release-notes skill"), and + the model reads it from `` as it would anyway. +- **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`, lists it in +`` as `user:sql-style:`, and inserts a synthetic `read_skill` call and result for that name +after the user message. +**Outcome:** The model starts its turn with the skill's manifest already in context. The response shows a +"Reading Skill: sql-style (user skill)" stage. From this turn on, the skill is listed and its bundled files are +readable. + +### UC-2: 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: the model normally reads the skill it was asked for, but that isn't guaranteed. Guaranteed +invocation of the agent's own skills is phase 2. + +### UC-3: Reading a bundled file three turns later + +**Trigger:** On turn 4 the model calls `read_skill("user:sql-style:", "references/naming.md")` for a skill picked +on turn 1, using the name it sees in `` and in the turn-1 synthetic call. +**Behavior:** The turn-1 user message still carries the reference, so Core shares the skill again and QuickApps +resolves and lists 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. + +### 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 listed. +**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 and the agent's skill share a name + +**Trigger:** The agent has `code-review`. The user picks their own `skills//code-review` from the +palette. +**Behavior:** Both are listed: the agent's as `code-review`, the user's as `user:code-review:3f9a2c`, each with its +own description. The synthetic read uses the user's name, and the injected content itself starts with that name and +how to read the skill's files. Later, `read_skill("code-review")` returns the agent's skill, because that is what +`code-review` means in the list; the user's is read by its own name. When the user asks for "my code-review" in words, +the model sees which entry is the user's. +**Outcome:** Both skills stay usable for the whole conversation, and the model always gets exactly the one it named. +Nothing is shadowed, and nothing depends on which skill "wins". + +--- + +## 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 chips from all user messages and resolve them + QA->>QA: user skills provider - list each one as user:name:hash + QA->>QA: injector - chips of the last user msg become synthetic read_skill pairs + 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) +``` + +The design has four concerns. + +### 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. + - `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 it 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 of the skill would then be denied. + - 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 (UC-3). + - `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 **newest to oldest** and collect the `url`s of their `custom_content.skills`, each + canonicalised by stripping a trailing `/`. The canonical URL is what the listed name is derived from, so it must + be the same on every turn. Identical chips on one message count once. On each message only the first + `SKILL_INVOCATION_MAX_PER_MESSAGE` distinct chips count; the rest are never resolved or listed, and the injector + gives them an error result (concern 4). + 2. Deduplicate, keeping each URL's **latest** occurrence: that is what decides its position. The chips of the last + message are therefore always the newest picks and are never dropped by the cap in step 4. A skill picked again + later moves to the end of the user skills in ``, which changes the system prompt once, like any + new pick. + 3. Reject a URL that isn't under `skills/`. Core already answers such a request with `400`, so this is only a + defensive check. + 4. Keep the newest `SKILL_INVOCATION_MAX_SKILLS` distinct URLs. The cap drops the oldest picks. A dropped pick's + manifest is still in history, but it is no longer listed or readable. + 5. Wrap each URL in a `DialSkillConfig` and hand the list to the existing `DialSkillResolver`, with its + duplicate-name check **off** (`unique_names=False`). Two picked skills with the same `name` (the user's and an + organisation's, say) are both legitimate: they are listed under different names (concern 3). Manifest parsing, + ``, byte caps, per-URL failure isolation and warning severities all come for free. + 6. Store the resolved skills by position, oldest first, and a failure reason per URL for the ones that didn't + resolve. +- **No dedup against the app config.** A picked URL that the app also declares is resolved again and listed as a + user skill next to the agent's entry. It is the same content under two names. That costs one extra fetch in a rare + case 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`. +- **Change.** A new initializer. `DialSkillResolver.resolve` gains `unique_names: bool = True`; the existing caller + keeps the default. + +### 3. Registration — listed as `user::` + +- **What.** + - `_InvokedSkillsContext` is an ordinary `SkillsProvider`, like `_DialSkillsContext`: `order = 30`, + `display_name = "user skills"`. + - Its `resolved_skills` are trimmed copies of the resolved skills, listed under a **listed name**: + + ``` + user:: + ``` + + `` is the last segment of the canonical URL, and `` is the first six hex characters of the SHA-256 of + the canonical URL. Both come from the URL alone, so the listed name is known before the skill resolves, and is the + same whether it resolves or not. + - The copy differs from the resolved skill in three ways; `url`, `files` and the file reader stay as resolved: + - `metadata` keeps only `name` (the listed name) and `description`, cut hard at 1024 characters. `license`, + `compatibility`, `metadata` and `allowed-tools` are dropped, so they never reach the system prompt. + - `content` starts with one line that names the skill the way the model must address it: + ``Skill `user:code-review:3f9a2c`, selected by the user. Read its files with + `read_skill("user:code-review:3f9a2c", )`.`` The manifest itself still says `name: code-review`, and without + this line a model could read a bundled file by that bare name and get the agent's file instead. + - Two small helpers in `quickapp/skills/` own the format. `make_user_skill_name(url)` builds the listed name. + `parse_user_skill_name(listed) -> str | None` strips the `user:` prefix and the trailing `:<6 hex>`, so a `:` + inside the name survives, and returns the name or `None`. The provider and the injector use the first; the stage + title (Secondary Fixes) uses the second. +- **Owner.** `quickapp/skills/` for the name format; `quickapp/skill_invocation/` for the provider. +- **Semantics.** + - Everything downstream works as it does today, because the listed name is just a name: + - the `SkillsRegistry` merge sees one more provider; + - `generate_skills_xml` renders `user:code-review:3f9a2c` and the description, as for any skill; + - `read_skill("user:code-review:3f9a2c", …)` is the same dictionary lookup as for any skill; + - bundled files are read by `skill.url` (`DialSkillReader.read_bundled_file`), which the copy keeps, and + file-level errors name the skill by its listed name. + - **Unique by construction.** A valid skill name is `[a-z0-9-]` and never contains `:`, so a listed user name can't + equal a valid agent skill's name. Two picked skills from different buckets get different hashes, even when their + paths end the same. A six-character hash collision among at most ten picks is practically impossible; if one + happens, the existing merge keeps the older entry and reports the other as a collision, as it does for any + duplicate name. + - **Stable across turns.** The listed name depends only on the canonical URL, so it is the same on every turn, with + no stored state. It doesn't change when the user edits the skill's `name` in `SKILL.md`, and it doesn't shift when + an older pick drops out of the cap. + - **No bucket id.** The model sees the skill's path segment and a hash. The URL, and the bucket in it, never reach + the model. + - **The `user:` prefix helps the model.** When the user asks for "my code-review" in words, the model can tell which + entry is the user's. +- **Why list the user's skills in the system prompt.** The model can only choose between two same-named skills if it + can see both, with their descriptions, in one place. The cost: `` grows on the turn a skill is + first picked, so that request misses the prompt cache, and later turns are stable until the next pick. The client + never sees the system prompt; QuickApps rebuilds it on every request (`_AddSystemPromptTransformer`), as it already + does when a declared skill's description changes. +- **Change.** The two helpers and the trimmed copy. + +### 4. Injection — `_SkillInvocationInjector` + +- **What.** A `MessagesTransformer` in `quickapp/skill_invocation/`. It inserts one synthetic `read_skill` call and + result per distinct chip on the last user message. +- **Owner.** `quickapp/skill_invocation/`. +- **Semantics.** + - **When.** It acts only when the **last** message is a user message that carries chips. Earlier invocations are + already in history: the pair was inserted after the user message of its turn, and `Orchestrator` persists + everything after the last user message into `state.tool_execution_history`. `_MessagesSetup.extract_tool_calls` + restores it on the next turn. Acting on every historical chip would duplicate them. + - **A pair already in history is not injected again.** Identical chips on one message count once (concern 2), and a + chip is skipped when the conversation already holds a pair for the same tool and arguments. The check matches the + call-id prefix that `_make_call_id_prefix` builds from the tool name and the arguments, which for a picked skill + derive from the canonical URL alone. It needs no content, so `read_skill` is not run for a re-pick at all: the + user sees no "Reading Skill" stage for a pair that would be thrown away, and a request never carries two pairs + sharing a `tool_call_id`, nor two pairs with the same arguments and different content. + + The manifest the model reads therefore stays the one from the turn that first picked the skill, even if the user + edits the skill afterwards. That is how every other tool result in the history behaves, and it keeps the history + consistent with what the model has already seen. + - **Where.** It inserts directly after that user message, in chip order. The explicit index keeps the pairs ahead + of any other transformer that appends to the end (`_TimestampInjectionTransformer`, + `_AttachmentNotificationInjector`), whatever the module order. + - **What.** For each chip: + + | Case | Tool call | Tool result | + |---|---|---| + | The skill resolved | `read_skill({"skill_name": "user::"})` | The result of actually running `read_skill`: the one-line header, the manifest and `` | + | Failed to resolve, rejected, or over a cap | `read_skill({"skill_name": "user::"})` | ``Error: the user's skill `user::` could not be loaded. The reason is shown to the user.`` | + + The listed name comes from the URL (concern 3), so a failed chip is named exactly as it would have been listed. + The failure result is a fixed sentence on purpose. The resolver's reason can contain the skill URL, for example + `Skill validation failed for 'skills//…'` from `parse_frontmatter`, so it goes to the "Initialization + issues" stage and the logs, where the user seeing their own bucket is fine, and never to the model. + + - **How.** For a resolved chip, the injector looks up the `read_skill` `StagedBaseTool` by its function name, the + same way `StagedToolSyntheticInjector` does, and runs it through `arun`. The result is then byte-identical to what + a model-initiated call returns, and the user sees the tool's normal stage at the app's `stage_display_level`, not + DEBUG, because the invocation is something the user did. For a failed chip, the injector writes the pair itself: + there is no "Reading Skill" stage, only the "Initialization issues" one. + - **Scrubbing.** The same transformer removes `custom_content.skills` from every message in the working copy. The + orchestrator LLM and any DIAL deployment tool that forwards the conversation don't need it. Forwarding it would + make Core share the user's skill folders with those deployments too. +- **Change.** A new transformer. `_build_pair`, `make_call_id` and the call-id prefix helper in + `common/synthetic_injection/` become public module-level helpers, so a multi-call injector can reuse them without + subclassing `SyntheticToolCallInjector`. That base class assumes one tool call per transformer. + +--- + +## Secondary Fixes + +### Stage title shows the user skill's short name + +`_SkillReaderStageWrapper._get_stage_title_from_params` titles the stage with the raw `skill_name` argument, which +would show `Reading Skill: user:code-review:3f9a2c`. When `parse_user_skill_name` recognises the argument, the title +uses the `` part and marks it: `Reading Skill: code-review (user skill)`, or +`Reading Skill: code-review/references/checklist.md (user skill)` for a bundled file. It is a string parse, with no +registry access, and it applies to model-initiated reads of user skills as well as the synthetic one. An agent skill +whose name happens to have the same shape, which only an invalid name can, would be titled the same way. + +### Reporting + +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 chips of the **last** user message (failed resolution, +over a cap) are recorded as `SkillInitializationException` on `_InvokedSkillsContext` and shown in the +"Initialization issues" stage, with the full reason; problems with older picks are logged. The model learns about a +failed chip from its fixed error result (concern 4), and about an older pick that is no longer available from the +list itself. **No path fails the request.** + +--- + +## Out of Scope + +- **Phase 2: letting the user pick the agent's own skills.** It needs a way for the client to list them. It gets its + own design. 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 per-app switch to disable invocation.** A picked skill never replaces one of the agent's skills, and access is + capped by the user's own reach. It does put the skill's name and description into the system prompt, the channel + the app author owns, which pasting text into a message can't. That is limited to one description of at most 1024 + characters per skill, and to `SKILL_INVOCATION_MAX_SKILLS` skills. 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 story (Core resource + dependencies) 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 this team reviews PRs + user:code-review:3f9a2cHow I want my code reviewed + … +user /code-review the diff in the attachment, focus on auth +assistant tool_calls: read_skill {"skill_name": "user:code-review:3f9a2c"} +tool Skill `user:code-review:3f9a2c`, selected by the user. Read its files with `read_skill("user:code-review:3f9a2c", )`. + ---\nname: code-review\n… \n\nreferences/checklist.md\n +``` + +The user sees the stage "Reading Skill: code-review (user skill)". + +### 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 lists the picked skill again under the same +name so its files stay readable, and injects nothing new unless the new message carries its own chips. + +### Limits + +| Variable | Default | Purpose | +|---|---|---| +| `SKILL_INVOCATION_MAX_SKILLS` | `10` | Distinct picked skills resolved and listed per request across the conversation, newest first. Each listed skill adds a `` block to the system prompt on every turn, so this is also a prompt budget. | +| `SKILL_INVOCATION_MAX_PER_MESSAGE` | `5` | Chips honoured on one message; the rest get an error result and are never listed. | +| `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` | Reused unchanged for the injected manifest. | + +### 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, listed or injected, and neither the user nor the model is told. `custom_content.skills` also keeps its place on the messages, so it reaches the orchestrator deployment and Core shares those skills with it. Accepted while the feature is preview-only; registering the scrub in `SkillsModule`, which is never gated, would close both halves | +| 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 listed 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 listed | +| Picked skill has the same `name` as an agent skill | Both listed, under `code-review` and `user:code-review:` (UC-5) | +| Two picked skills share a `name` | Both listed, with different hashes | +| The same skill picked again later | Moves to the end of the user skills in the list; never dropped by the cap on the message that picks it; no new pair is injected, and no "Reading Skill" stage is shown | +| The user edits a picked skill's content 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 | +| The user edits the skill's `name` in `SKILL.md` mid-conversation | The listed name doesn't change: it comes from the URL | +| Identical chips on one message | Counted once: one pair, one entry | +| Picked URL the app also declares | Listed twice: under the agent's name and as a user skill; either name reads the same content | +| Over `SKILL_INVOCATION_MAX_PER_MESSAGE` on one message | The extra chips get an error result and are not listed | +| Over `SKILL_INVOCATION_MAX_SKILLS` in the conversation | The oldest picks are no longer listed; their manifests stay in history; their names return "not found" | +| DIAL Core outage | Every picked skill gets an error result or drops out of the list; the agent's own skills are unaffected; request served | + +--- + +## Known Gaps + +### 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. + +--- + +## Alternatives Considered + +| Alternative | Why not | +|---|---| +| **A precedence rule between same-named skills** (the user's pick wins the name, or the agent's does) | Whichever skill loses becomes unreachable by name, and the model silently gets a different skill than it may have meant. If the user's pick wins, it can also replace predefined skills, and the mapping flips on its own when a pick is deleted or drops out of the cap. | +| **Add `` and `` to every skill, with an ambiguity error for shared names** | Solves collisions, but changes the skills contract and the system prompt of every app, and needs a new lookup order in `SkillsRegistry`. A derived name gives the same guarantee with no contract change. | +| **The URL as the listed name** | The simplest unique name, but it puts the user's bucket id in front of the model and in the stage title. | +| **`` from the skill's `SKILL.md`** | Readable, but the listed name would change when the user edits the manifest mid-conversation, and a chip that fails to resolve has no manifest name at all. The URL's last segment gives one stable name per pick. | +| **`user:` without a hash** | Two picked skills with the same name (the user's and an organisation's) can't both be listed, and the name would switch skills when one of them is picked, deleted or dropped. | +| **`user:-` numbered by pick order** | The numbering shifts when an older pick drops out of the cap, and a name then points to a different skill. | +| **Address picked skills by URL but keep them out of ``** | Keeps the system prompt stable, but the model can't see that a second `code-review` exists, and a bare-name call silently goes to the agent's. | +| **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. | +| **Match a leading `/name` against the agent's own skills** | Considered for phase 1 and dropped. Without a palette the user can't discover the names, and it would be a second invocation path that phase 2 replaces anyway. Asking in words covers the need until then. | +| **Only list the picked skill, without injecting it** | Not deterministic (goal 1): the model decides whether to read it. | +| **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, since Core already shares any attachment URL. But when the user switches models mid-conversation, every other deployment would see a `skills/` URL as a file attachment. QuickApps would also have to filter it out of its own attachment pipeline. | +| **Inline the manifest into the user message every turn** | Re-fetches and re-inlines every historical invocation on every turn. If the skill is edited, earlier turns silently change, which makes history differ from what the model actually saw. The synthetic pair freezes the manifest as it was when invoked. | + +--- + +## Migration + +### Breaking changes + +None. + +### 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` — `SkillReference(url)`; parses `custom_content.model_extra["skills"]` from user messages and + canonicalises the URL. +- `_invoked_skills_context.py` — `_InvokedSkillsContext(SkillsProvider)`, `order = 30`, `display_name = "user skills"`. + Lists trimmed copies of the resolved skills under `user::` (name and a capped description only, content + with the one-line header), by position; holds a failure reason per URL and the exceptions for the last user + message's chips. +- `_skill_invocation_initializer.py` — `_SkillInvocationInitializer(CompletionInitializer)`: collect newest first, + per-message dedup and cap, canonicalise, keep the latest occurrence, validate, conversation cap, delegate to + `DialSkillResolver`. +- `_skill_invocation_injector.py` — `_SkillInvocationInjector(MessagesTransformer)`: synthetic `read_skill` pairs for + the distinct chips of the last user message, a fixed error result for failed ones, and `custom_content.skills` + scrubbing. +- `_settings.py` — `SkillInvocationSettings` (`SKILL_INVOCATION_MAX_SKILLS`, `SKILL_INVOCATION_MAX_PER_MESSAGE`). +- `skill_invocation_module.py` — `@preview_module`; multiproviders for `CompletionInitializer`, `SkillsProvider`, + `MessagesTransformer`, `InitializationException`. Registered in `app_factory.py` after `DialSkillsModule`. + +### `quickapp/skills/` + +- `_user_skill_names.py` — `make_user_skill_name(url)`, `parse_user_skill_name(listed)`. +- `_skill_reader_stage_wrapper.py` — the stage title for a user skill shows its short name, marked "(user skill)". +- `__init__.py` — export `SKILL_READER_TOOL_NAME` and the two helpers. + +### `quickapp/dial_skills/` + +- `_dial_skill_resolver.py` — `resolve(..., unique_names: bool = True)`. The invoked-skills initializer passes + `False`; the existing caller is unchanged. + +### `quickapp/common/` + +- `_di_types.py` — `REQUEST_MESSAGES`. +- `synthetic_injection/synthetic_tool_call_injector.py` — `build_synthetic_pair` and `make_synthetic_call_id` as + public module-level helpers; the class keeps using them. + +### `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`. + +### 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, the `user::` listing, and the limits once + implemented. From 53dc0eeb66a00ee14f729b92a65645feb1c63262 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 12:00:55 +0300 Subject: [PATCH 2/9] docs: rewrite the skill invocation design as phase 1a #549 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrows the design to what the feature actually needs, and states the assumptions it now rests on instead of engineering around them. A picked skill is registered as an ordinary SkillsProvider entry under its own manifest name (order = -10), so it is listed, injected as a synthetic read_skill pair, and its bundled files stay readable on later turns through the existing registry — no new lookup path and no new collision machinery. Name clashes go through the merge SkillsRegistry already performs; the design adds no rule of its own and instead states the assumption that picked skills have non-clashing names. Shadowing of predefined and declared skills is recorded as a known gap, closed by 1b. Drops the user:: scheme, the trimmed metadata copy, the stage-title parse and the per-message cap. Moves the custom_content.skills scrub into the never-gated SkillsModule so the field cannot reach the orchestrator deployment with preview off. Adds phase 1b as a follow-up section covering collision-free names. Refs #549 --- docs/designs/skill_invocation.md | 534 ++++++++++++++++--------------- 1 file changed, 272 insertions(+), 262 deletions(-) diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md index 9455812e..bb2f50a2 100644 --- a/docs/designs/skill_invocation.md +++ b/docs/designs/skill_invocation.md @@ -1,9 +1,12 @@ # Design: Invoking a Skill from a Message -- **Status:** Approved +- **Status:** Draft - **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. Branch `feat/418-skills-as-dial-resource`, not yet on `development`. @@ -29,10 +32,6 @@ That leaves two gaps: `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. -Bringing the user's skills in must not create a third problem: **two skills with the same name**. A skill is -identified only by its name, which is unique within an agent today. A user's `code-review` next to the agent's -`code-review` would leave the model unable to say which one it means. - 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. @@ -42,24 +41,25 @@ does for `custom_content.attachments[*]`. This design specifies how QuickApps co 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. A picked skill stays usable for the rest of the conversation: the model sees it in ``, and its - bundled files stay readable through `read_skill`. -4. **Every name in `` stays unique**, so the model always gets exactly the skill it names. A picked - skill never takes an agent skill's name, and there is no precedence rule between the user's skills and the - agent's. -5. The model is never shown the user's bucket id. -6. A chip on the current message that fails is visible to both the user and the model. The request is still served. -7. The skills contract doesn't change: `` keeps `name` and `description`, and `read_skill` keeps - its parameters. Requests that don't carry the field behave exactly as today, system prompt included. -8. The change is as small as possible: the existing skills merge, XML generator and `read_skill` tool stay as they - are. +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 1 — this design.** The client knows only the user's skills. It lists them in the `/` palette and sends the - one the user picks as a URL reference (a chip). The agent's own skills, declared or predefined, stay exactly as - today: only the agent invokes them. A user who wants one asks for it in words ("use the release-notes skill"), and - the model reads it from `` as it would anyway. +- **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. @@ -71,50 +71,45 @@ does for `custom_content.attachments[*]`. This design specifies how QuickApps co **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`, lists it in -`` as `user:sql-style:`, and inserts a synthetic `read_skill` call and result for that name -after the user message. -**Outcome:** The model starts its turn with the skill's manifest already in context. The response shows a -"Reading Skill: sql-style (user skill)" stage. From this turn on, the skill is listed and its bundled files are -readable. +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 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-2: Asking for one of the agent's own skills +### 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: the model normally reads the skill it was asked for, but that isn't guaranteed. Guaranteed -invocation of the agent's own skills is phase 2. - -### UC-3: Reading a bundled file three turns later - -**Trigger:** On turn 4 the model calls `read_skill("user:sql-style:", "references/naming.md")` for a skill picked -on turn 1, using the name it sees in `` and in the turn-1 synthetic call. -**Behavior:** The turn-1 user message still carries the reference, so Core shares the skill again and QuickApps -resolves and lists 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. +**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 listed. +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 and the agent's skill share a name +### UC-5: The user's skill has the same name as one of the agent's -**Trigger:** The agent has `code-review`. The user picks their own `skills//code-review` from the -palette. -**Behavior:** Both are listed: the agent's as `code-review`, the user's as `user:code-review:3f9a2c`, each with its -own description. The synthetic read uses the user's name, and the injected content itself starts with that name and -how to read the skill's files. Later, `read_skill("code-review")` returns the agent's skill, because that is what -`code-review` means in the list; the user's is read by its own name. When the user asks for "my code-review" in words, -the model sees which entry is the user's. -**Outcome:** Both skills stay usable for the whole conversation, and the model always gets exactly the one it named. -Nothing is shadowed, and nothing depends on which skill "wins". +**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). --- @@ -131,15 +126,16 @@ sequenceDiagram Core->>Core: validate and auto-share every skills[].url to the per-request key Core->>QA: chat/completions QA->>QA: initializer: collect chips from all user messages and resolve them - QA->>QA: user skills provider - list each one as user:name:hash - QA->>QA: injector - chips of the last user msg become synthetic read_skill pairs + QA->>QA: user skills provider (order -10) joins the SkillsRegistry merge + QA->>QA: injector: chips of the last user msg become synthetic read_skill pairs 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) ``` -The design has four concerns. +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` @@ -169,11 +165,13 @@ The design has four concerns. - a skill the user can't read → `403` for the whole request. - `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 it 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 of the skill would then be denied. + - 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 (UC-3). + 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. @@ -187,84 +185,67 @@ The design has four concerns. - **Owner.** `quickapp/skill_invocation/`. - **Semantics.** 1. Walk the user messages **newest to oldest** and collect the `url`s of their `custom_content.skills`, each - canonicalised by stripping a trailing `/`. The canonical URL is what the listed name is derived from, so it must - be the same on every turn. Identical chips on one message count once. On each message only the first - `SKILL_INVOCATION_MAX_PER_MESSAGE` distinct chips count; the rest are never resolved or listed, and the injector - gives them an error result (concern 4). + canonicalised by stripping a trailing `/`. Identical chips on one message count once. 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. 2. Deduplicate, keeping each URL's **latest** occurrence: that is what decides its position. The chips of the last - message are therefore always the newest picks and are never dropped by the cap in step 4. A skill picked again - later moves to the end of the user skills in ``, which changes the system prompt once, like any - new pick. - 3. Reject a URL that isn't under `skills/`. Core already answers such a request with `400`, so this is only a - defensive check. - 4. Keep the newest `SKILL_INVOCATION_MAX_SKILLS` distinct URLs. The cap drops the oldest picks. A dropped pick's - manifest is still in history, but it is no longer listed or readable. - 5. Wrap each URL in a `DialSkillConfig` and hand the list to the existing `DialSkillResolver`, with its - duplicate-name check **off** (`unique_names=False`). Two picked skills with the same `name` (the user's and an - organisation's, say) are both legitimate: they are listed under different names (concern 3). Manifest parsing, - ``, byte caps, per-URL failure isolation and warning severities all come for free. - 6. Store the resolved skills by position, oldest first, and a failure reason per URL for the ones that didn't - resolve. -- **No dedup against the app config.** A picked URL that the app also declares is resolved again and listed as a - user skill next to the agent's entry. It is the same content under two names. That costs one extra fetch in a rare - case and needs no merging logic. + message are therefore the newest picks and are never dropped by the cap in step 3. + 3. Keep the newest `SKILL_INVOCATION_MAX_SKILLS` distinct URLs, walking newest-first so the cap drops the **oldest** + picks. 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 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 by position, oldest first, the set of URLs that resolved, and **the chips of the + message being answered**. The injector reads all three back: the first two to choose between a real result and + the error sentence, the third to know which chips belong to this turn. Recording "this turn's chips" 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`. -- **Change.** A new initializer. `DialSkillResolver.resolve` gains `unique_names: bool = True`; the existing caller - keeps the default. - -### 3. Registration — listed as `user::` - -- **What.** - - `_InvokedSkillsContext` is an ordinary `SkillsProvider`, like `_DialSkillsContext`: `order = 30`, - `display_name = "user skills"`. - - Its `resolved_skills` are trimmed copies of the resolved skills, listed under a **listed name**: - - ``` - user:: - ``` - - `` is the last segment of the canonical URL, and `` is the first six hex characters of the SHA-256 of - the canonical URL. Both come from the URL alone, so the listed name is known before the skill resolves, and is the - same whether it resolves or not. - - The copy differs from the resolved skill in three ways; `url`, `files` and the file reader stay as resolved: - - `metadata` keeps only `name` (the listed name) and `description`, cut hard at 1024 characters. `license`, - `compatibility`, `metadata` and `allowed-tools` are dropped, so they never reach the system prompt. - - `content` starts with one line that names the skill the way the model must address it: - ``Skill `user:code-review:3f9a2c`, selected by the user. Read its files with - `read_skill("user:code-review:3f9a2c", )`.`` The manifest itself still says `name: code-review`, and without - this line a model could read a bundled file by that bare name and get the agent's file instead. - - Two small helpers in `quickapp/skills/` own the format. `make_user_skill_name(url)` builds the listed name. - `parse_user_skill_name(listed) -> str | None` strips the `user:` prefix and the trailing `:<6 hex>`, so a `:` - inside the name survives, and returns the name or `None`. The provider and the injector use the first; the stage - title (Secondary Fixes) uses the second. -- **Owner.** `quickapp/skills/` for the name format; `quickapp/skill_invocation/` for the provider. + 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.** - - Everything downstream works as it does today, because the listed name is just a name: + - 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. Every clash — with an agent's skill or between two picks — goes through the merge that + `SkillsRegistry` already performs: the first entry it meets wins, the rest are reported as collisions. This + design adds no rule of its own about which one that should be, and none of it is new code. + + **The assumption this rests on: picked skills have names that don't clash.** It is the user's to keep, and 1b + removes the need for it. + - 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 `user:code-review:3f9a2c` and the description, as for any skill; - - `read_skill("user:code-review:3f9a2c", …)` is the same dictionary lookup as for any skill; - - bundled files are read by `skill.url` (`DialSkillReader.read_bundled_file`), which the copy keeps, and - file-level errors name the skill by its listed name. - - **Unique by construction.** A valid skill name is `[a-z0-9-]` and never contains `:`, so a listed user name can't - equal a valid agent skill's name. Two picked skills from different buckets get different hashes, even when their - paths end the same. A six-character hash collision among at most ten picks is practically impossible; if one - happens, the existing merge keeps the older entry and reports the other as a collision, as it does for any - duplicate name. - - **Stable across turns.** The listed name depends only on the canonical URL, so it is the same on every turn, with - no stored state. It doesn't change when the user edits the skill's `name` in `SKILL.md`, and it doesn't shift when - an older pick drops out of the cap. - - **No bucket id.** The model sees the skill's path segment and a hash. The URL, and the bucket in it, never reach - the model. - - **The `user:` prefix helps the model.** When the user asks for "my code-review" in words, the model can tell which - entry is the user's. -- **Why list the user's skills in the system prompt.** The model can only choose between two same-named skills if it - can see both, with their descriptions, in one place. The cost: `` grows on the turn a skill is - first picked, so that request misses the prompt cache, and later turns are stable until the next pick. The client - never sees the system prompt; QuickApps rebuilds it on every request (`_AddSystemPromptTransformer`), as it already - does when a declared skill's description changes. -- **Change.** The two helpers and the trimmed copy. + - `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` @@ -272,91 +253,79 @@ The design has four concerns. result per distinct chip on the last user message. - **Owner.** `quickapp/skill_invocation/`. - **Semantics.** - - **When.** It acts only when the **last** message is a user message that carries chips. Earlier invocations are - already in history: the pair was inserted after the user message of its turn, and `Orchestrator` persists - everything after the last user message into `state.tool_execution_history`. `_MessagesSetup.extract_tool_calls` - restores it on the next turn. Acting on every historical chip would duplicate them. - - **A pair already in history is not injected again.** Identical chips on one message count once (concern 2), and a - chip is skipped when the conversation already holds a pair for the same tool and arguments. The check matches the - call-id prefix that `_make_call_id_prefix` builds from the tool name and the arguments, which for a picked skill - derive from the canonical URL alone. It needs no content, so `read_skill` is not run for a re-pick at all: the - user sees no "Reading Skill" stage for a pair that would be thrown away, and a request never carries two pairs - sharing a `tool_call_id`, nor two pairs with the same arguments and different content. + - **When.** Only when the **last** message is a user message carrying chips. Earlier invocations are already in + history: the pair was inserted after the user message of its turn, `Orchestrator` persists everything after the + last user message into `state.tool_execution_history`, and `_MessagesSetup.extract_tool_calls` restores it. + Acting on every historical chip would duplicate them. + - **A pair already in history is not injected again.** A chip is skipped when the conversation already holds a pair + for the same tool and arguments, matched on the call-id prefix that `make_synthetic_call_id_prefix` builds from + the tool name and arguments. It needs no content, so `read_skill` is not run for a re-pick at all: no stage is + shown for a result that would be thrown away, and a request never carries two pairs sharing a `tool_call_id`. The manifest the model reads therefore stays the one from the turn that first picked the skill, even if the user - edits the skill afterwards. That is how every other tool result in the history behaves, and it keeps the history - consistent with what the model has already seen. - - **Where.** It inserts directly after that user message, in chip order. The explicit index keeps the pairs ahead - of any other transformer that appends to the end (`_TimestampInjectionTransformer`, - `_AttachmentNotificationInjector`), whatever the module order. + 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). + - **Where.** Directly after that user message, in chip order. The explicit index keeps the pairs ahead of any + transformer that appends to the end (`_TimestampInjectionTransformer`, `_AttachmentNotificationInjector`), + whatever the module order. - **What.** For each chip: | Case | Tool call | Tool result | |---|---|---| - | The skill resolved | `read_skill({"skill_name": "user::"})` | The result of actually running `read_skill`: the one-line header, the manifest and `` | - | Failed to resolve, rejected, or over a cap | `read_skill({"skill_name": "user::"})` | ``Error: the user's skill `user::` could not be loaded. The reason is shown to the user.`` | - - The listed name comes from the URL (concern 3), so a failed chip is named exactly as it would have been listed. - The failure result is a fixed sentence on purpose. The resolver's reason can contain the skill URL, for example - `Skill validation failed for 'skills//…'` from `parse_frontmatter`, so it goes to the "Initialization - issues" stage and the logs, where the user seeing their own bucket is fine, and never to the model. - - - **How.** For a resolved chip, the injector looks up the `read_skill` `StagedBaseTool` by its function name, the - same way `StagedToolSyntheticInjector` does, and runs it through `arun`. The result is then byte-identical to what - a model-initiated call returns, and the user sees the tool's normal stage at the app's `stage_display_level`, not - DEBUG, because the invocation is something the user did. For a failed chip, the injector writes the pair itself: - there is no "Reading Skill" stage, only the "Initialization issues" one. - - **Scrubbing.** The same transformer removes `custom_content.skills` from every message in the working copy. The - orchestrator LLM and any DIAL deployment tool that forwards the conversation don't need it. Forwarding it would - make Core share the user's skill folders with those deployments too. -- **Change.** A new transformer. `_build_pair`, `make_call_id` and the call-id prefix helper in - `common/synthetic_injection/` become public module-level helpers, so a multi-call injector can reuse them without - subclassing `SyntheticToolCallInjector`. That base class assumes one tool call per transformer. - ---- - -## Secondary Fixes - -### Stage title shows the user skill's short name - -`_SkillReaderStageWrapper._get_stage_title_from_params` titles the stage with the raw `skill_name` argument, which -would show `Reading Skill: user:code-review:3f9a2c`. When `parse_user_skill_name` recognises the argument, the title -uses the `` part and marks it: `Reading Skill: code-review (user skill)`, or -`Reading Skill: code-review/references/checklist.md (user skill)` for a bundled file. It is a string parse, with no -registry access, and it applies to model-initiated reads of user skills as well as the synthetic one. An agent skill -whose name happens to have the same shape, which only an invalid name can, would be titled the same way. - -### Reporting - -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 chips of the **last** user message (failed resolution, -over a cap) are recorded as `SkillInitializationException` on `_InvokedSkillsContext` and shown in the -"Initialization issues" stage, with the full reason; problems with older picks are logged. The model learns about a -failed chip from its fixed error result (concern 4), and about an older pick that is no longer available from the -list itself. **No path fails the request.** + | 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 chip `` 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.** For a resolved chip the injector looks up the `read_skill` `StagedBaseTool` by its function name, the + same way `StagedToolSyntheticInjector` does, and runs it through `arun`. Because the skill is registered + (concern 3), the result is byte-identical to a model-initiated call, `` included, and the user sees + the tool's normal stage at the app's `stage_display_level` — the invocation is something the user did, so its + stage belongs in the response. For a failed chip the injector writes the pair itself, and there is 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 chips from the context + (concern 2, step 5) rather than re-reading the messages. + - **Reporting.** 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 chips of the **last** user message are + recorded as `SkillInitializationException` and shown in "Initialization issues"; problems with older picks are + logged. The model learns about a failed chip from its error result. **No path fails the request.** +- **Change.** A new transformer. `build_synthetic_pair`, `make_synthetic_call_id` and `make_synthetic_call_id_prefix` + in `common/synthetic_injection/` become public module-level helpers, so a multi-call injector can reuse them without + subclassing `SyntheticToolCallInjector` — that base class assumes one tool call per transformer. --- ## Out of Scope -- **Phase 2: letting the user pick the agent's own skills.** It needs a way for the client to list them. It gets its - own design. 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. +- **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 per-app switch to disable invocation.** A picked skill never replaces one of the agent's skills, and access is - capped by the user's own reach. It does put the skill's name and description into the system prompt, the channel - the app author owns, which pasting text into a message can't. That is limited to one description of at most 1024 - characters per skill, and to `SKILL_INVOCATION_MAX_SKILLS` skills. 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 story (Core resource - dependencies) 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. +- **A per-message cap.** One cap is enough: the conversation cap already bounds how many skills are fetched and + listed, and it is spent newest-first, so the current turn's chips are 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. + bounded by the cap. Resolving only on a `read_skill` miss would remove that cost but needs an async path through the + registry merge. --- @@ -383,56 +352,77 @@ What the orchestrator LLM receives, when the agent also has its own `code-review ```text system … - code-reviewHow this team reviews PRs - user:code-review:3f9a2cHow I want my code reviewed - … + 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": "user:code-review:3f9a2c"} -tool Skill `user:code-review:3f9a2c`, selected by the user. Read its files with `read_skill("user:code-review:3f9a2c", )`. +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 (user skill)". +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 lists the picked skill again under the same +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 carries its own chips. ### Limits | Variable | Default | Purpose | |---|---|---| -| `SKILL_INVOCATION_MAX_SKILLS` | `10` | Distinct picked skills resolved and listed per request across the conversation, newest first. Each listed skill adds a `` block to the system prompt on every turn, so this is also a prompt budget. | -| `SKILL_INVOCATION_MAX_PER_MESSAGE` | `5` | Chips honoured on one message; the rest get an error result and are never listed. | -| `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` | Reused unchanged for the injected manifest. | +| `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, listed or injected, and neither the user nor the model is told. `custom_content.skills` also keeps its place on the messages, so it reaches the orchestrator deployment and Core shares those skills with it. Accepted while the feature is preview-only; registering the scrub in `SkillsModule`, which is never gated, would close both halves | +| `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 listed and its name returns "not found" | +| 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 listed | -| Picked skill has the same `name` as an agent skill | Both listed, under `code-review` and `user:code-review:` (UC-5) | -| Two picked skills share a `name` | Both listed, with different hashes | -| The same skill picked again later | Moves to the end of the user skills in the list; never dropped by the cap on the message that picks it; no new pair is injected, and no "Reading Skill" stage is shown | -| The user edits a picked skill's content 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 | -| The user edits the skill's `name` in `SKILL.md` mid-conversation | The listed name doesn't change: it comes from the URL | +| 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 the existing `SkillsRegistry` merge — the first entry it meets wins, the other is reported as a collision. This design adds no rule of its own | +| Picked URL the app also declares | Same content, one entry; the user's copy wins by order | +| The same skill picked again later | Moves to the end of the user skills in the list; never dropped by the cap on the message that picks it; no new pair is injected and no stage is shown | +| 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 | | Identical chips on one message | Counted once: one pair, one entry | -| Picked URL the app also declares | Listed twice: under the agent's name and as a user skill; either name reads the same content | -| Over `SKILL_INVOCATION_MAX_PER_MESSAGE` on one message | The extra chips get an error result and are not listed | -| Over `SKILL_INVOCATION_MAX_SKILLS` in the conversation | The oldest picks are no longer listed; their manifests stay in history; their names return "not found" | -| DIAL Core outage | Every picked skill gets an error result or drops out of the list; the agent's own skills are unaffected; request served | +| 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 | --- ## Known Gaps +### 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 @@ -443,8 +433,8 @@ Core doesn't check public skills. A deleted skill of either kind passes Core and 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. +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. --- @@ -452,19 +442,37 @@ Until then the user's only way out is to start a new conversation. | Alternative | Why not | |---|---| -| **A precedence rule between same-named skills** (the user's pick wins the name, or the agent's does) | Whichever skill loses becomes unreachable by name, and the model silently gets a different skill than it may have meant. If the user's pick wins, it can also replace predefined skills, and the mapping flips on its own when a pick is deleted or drops out of the cap. | -| **Add `` and `` to every skill, with an ambiguity error for shared names** | Solves collisions, but changes the skills contract and the system prompt of every app, and needs a new lookup order in `SkillsRegistry`. A derived name gives the same guarantee with no contract change. | -| **The URL as the listed name** | The simplest unique name, but it puts the user's bucket id in front of the model and in the stage title. | -| **`` from the skill's `SKILL.md`** | Readable, but the listed name would change when the user edits the manifest mid-conversation, and a chip that fails to resolve has no manifest name at all. The URL's last segment gives one stable name per pick. | -| **`user:` without a hash** | Two picked skills with the same name (the user's and an organisation's) can't both be listed, and the name would switch skills when one of them is picked, deleted or dropped. | -| **`user:-` numbered by pick order** | The numbering shifts when an older pick drops out of the cap, and a name then points to a different skill. | -| **Address picked skills by URL but keep them out of ``** | Keeps the system prompt stable, but the model can't see that a second `code-review` exists, and a bare-name call silently goes to the agent's. | -| **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. | -| **Match a leading `/name` against the agent's own skills** | Considered for phase 1 and dropped. Without a palette the user can't discover the names, and it would be a second invocation path that phase 2 replaces anyway. Asking in words covers the need until then. | -| **Only list the picked skill, without injecting it** | Not deterministic (goal 1): the model decides whether to read it. | -| **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, since Core already shares any attachment URL. But when the user switches models mid-conversation, every other deployment would see a `skills/` URL as a file attachment. QuickApps would also have to filter it out of its own attachment pipeline. | -| **Inline the manifest into the user message every turn** | Re-fetches and re-inlines every historical invocation on every turn. If the skill is edited, earlier turns silently change, which makes history differ from what the model actually saw. The synthetic pair freezes the manifest as it was when invoked. | +| **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 | +| **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. --- @@ -472,7 +480,10 @@ Until then the user's only way out is to start a new conversation. ### Breaking changes -None. +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 @@ -484,44 +495,44 @@ None. ### `quickapp/skill_invocation/` (new) -- `_skill_reference.py` — `SkillReference(url)`; parses `custom_content.model_extra["skills"]` from user messages and - canonicalises the URL. -- `_invoked_skills_context.py` — `_InvokedSkillsContext(SkillsProvider)`, `order = 30`, `display_name = "user skills"`. - Lists trimmed copies of the resolved skills under `user::` (name and a capped description only, content - with the one-line header), by position; holds a failure reason per URL and the exceptions for the last user - message's chips. +- `_skill_reference.py` — `SkillReference(url)`; parses `custom_content.model_extra["skills"]` from user messages, + canonicalises the URL, dedupes keeping the latest occurrence, applies the conversation cap. +- `_invoked_skills_context.py` — `_InvokedSkillsContext(SkillsProvider)`, `order = -10`, + `display_name = "user skills"`. Holds the resolved skills as resolved, with one header line prepended to `content`, + the set of URLs that resolved, this turn's chips, and the exceptions for them. - `_skill_invocation_initializer.py` — `_SkillInvocationInitializer(CompletionInitializer)`: collect newest first, - per-message dedup and cap, canonicalise, keep the latest occurrence, validate, conversation cap, delegate to - `DialSkillResolver`. + dedupe, cap, delegate to `DialSkillResolver`. - `_skill_invocation_injector.py` — `_SkillInvocationInjector(MessagesTransformer)`: synthetic `read_skill` pairs for - the distinct chips of the last user message, a fixed error result for failed ones, and `custom_content.skills` - scrubbing. -- `_settings.py` — `SkillInvocationSettings` (`SKILL_INVOCATION_MAX_SKILLS`, `SKILL_INVOCATION_MAX_PER_MESSAGE`). + this turn's chips via the real tool, a fixed error result for failed ones, failure reporting. No scrubbing. +- `_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/skills/` - -- `_user_skill_names.py` — `make_user_skill_name(url)`, `parse_user_skill_name(listed)`. -- `_skill_reader_stage_wrapper.py` — the stage title for a user skill shows its short name, marked "(user skill)". -- `__init__.py` — export `SKILL_READER_TOOL_NAME` and the two helpers. - -### `quickapp/dial_skills/` - -- `_dial_skill_resolver.py` — `resolve(..., unique_names: bool = True)`. The invoked-skills initializer passes - `False`; the existing caller is unchanged. - ### `quickapp/common/` - `_di_types.py` — `REQUEST_MESSAGES`. -- `synthetic_injection/synthetic_tool_call_injector.py` — `build_synthetic_pair` and `make_synthetic_call_id` as - public module-level helpers; the class keeps using them. +- `synthetic_injection/synthetic_tool_call_injector.py` — `build_synthetic_pair`, `make_synthetic_call_id` and + `make_synthetic_call_id_prefix` as public module-level helpers; the class keeps using them. ### `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 @@ -529,5 +540,4 @@ None. - `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, the `user::` listing, and the limits once - implemented. +- `docs/skills.md` — document invocation, the wire field and the limits once implemented. From adf9be77b0ff94844906224e866d9954ae37b64d Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 12:17:55 +0300 Subject: [PATCH 3/9] docs: approve the phase 1a skill invocation design #549 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corrects the one claim an implementer would have built against: clashes between two picks are settled by DialSkillResolver's name dedup, not by SkillsRegistry. Two picks live in the same provider, so the registry never sees the loser. Documents the real mechanism — oldest pick wins, reported with the resolver's duplicate-name wording every turn — and accepts the three consequences, all unreachable while picked skill names do not clash. Also removes two traps that would have produced silently broken code: only the tool lookup follows StagedToolSyntheticInjector, since its get_content forces stage_level=DEBUG and would suppress the "Reading Skill" stage; and the injector's trigger is stated once in terms of the context, because the SkillsModule scrub runs first and the messages no longer carry chips by then. Adds failure-modes rows for the two-URL same-name case, and records why reporting a duplicate distinctly and unique_names=False were rejected. Refs #549 --- docs/designs/skill_invocation.md | 65 ++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md index bb2f50a2..d9757cc6 100644 --- a/docs/designs/skill_invocation.md +++ b/docs/designs/skill_invocation.md @@ -1,6 +1,7 @@ # Design: Invoking a Skill from a Message -- **Status:** Draft +- **Status:** Approved +- **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) @@ -233,12 +234,32 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). 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. Every clash — with an agent's skill or between two picks — goes through the merge that - `SkillsRegistry` already performs: the first entry it meets wins, the rest are reported as collisions. This - design adds no rule of its own about which one that should be, and none of it is new code. + like every other skill, and the two kinds of clash are settled in two different places that both already exist: - **The assumption this rests on: picked skills have names that don't clash.** It is the user's to keep, and 1b - removes the need for it. + | 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 5 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 produces **nothing at all** — no pair, no stage, no error. Its chip derives the same + `{"skill_name": ""}` arguments as the first pick, so the skip check in concern 4 matches the earlier + pair and skips it before anything is reported. + + **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; @@ -253,10 +274,14 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). result per distinct chip on the last user message. - **Owner.** `quickapp/skill_invocation/`. - **Semantics.** - - **When.** Only when the **last** message is a user message carrying chips. Earlier invocations are already in - history: the pair was inserted after the user message of its turn, `Orchestrator` persists everything after the - last user message into `state.tool_execution_history`, and `_MessagesSetup.extract_tool_calls` restores it. - Acting on every historical chip would duplicate them. + - **When.** When `_InvokedSkillsContext` recorded chips for this turn (concern 2, step 5) — **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. The + insertion point is still taken from the messages: the index after the last `USER` message. + + Earlier invocations need nothing: the pair was inserted after the user message of its turn, `Orchestrator` + persists everything after the last user message into `state.tool_execution_history`, and + `_MessagesSetup.extract_tool_calls` restores it. Acting on every historical chip would duplicate them. - **A pair already in history is not injected again.** A chip is skipped when the conversation already holds a pair for the same tool and arguments, matched on the call-id prefix that `make_synthetic_call_id_prefix` builds from the tool name and arguments. It needs no content, so `read_skill` is not run for a re-pick at all: no stage is @@ -283,11 +308,16 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). `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.** For a resolved chip the injector looks up the `read_skill` `StagedBaseTool` by its function name, the - same way `StagedToolSyntheticInjector` does, and runs it through `arun`. Because the skill is registered - (concern 3), the result is byte-identical to a model-initiated call, `` included, and the user sees - the tool's normal stage at the app's `stage_display_level` — the invocation is something the user did, so its - stage belongs in the response. For a failed chip the injector writes the pair itself, and there is no stage. + - **How.** For a resolved chip the injector looks up the `read_skill` `StagedBaseTool` by its function name — the + **lookup only** follows `StagedToolSyntheticInjector` — and runs it through `arun`. Because the skill is + registered (concern 3), the result is byte-identical to a model-initiated call, `` included. + + > `StagedToolSyntheticInjector.get_content` forces `stage_level=StageDisplayLevel.DEBUG`, which suppresses the + > stage for a normal app. Copying that call verbatim would make UC-1's "Reading Skill" stage silently never + > appear. `arun` must be called **without** `stage_level`, so it defaults to `INFO` as it does in + > `tool_executor.py` — the invocation is something the user did, so its stage belongs in the response. + + For a failed chip the injector writes the pair itself, and there is 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 @@ -387,7 +417,8 @@ name so its files stay readable, and injects nothing new unless the new message | 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 the existing `SkillsRegistry` merge — the first entry it meets wins, the other is reported as a collision. This design adds no rule of its own | +| 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. The newer chip derives identical `read_skill` arguments, so the skip check matches the older pair and the re-pick produces no pair, no stage and no error. 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 skill picked again later | Moves to the end of the user skills in the list; never dropped by the cap on the message that picks it; no new pair is injected and no stage is shown | | 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 | @@ -447,6 +478,8 @@ then the user's only way out is to start a new conversation. | **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") | Does not actually surface it: both picks derive the same `{"skill_name": ""}` arguments, so the skip check still matches the earlier pair. Bypassing the skip would write a "could not be loaded" pair directly after one that loaded the same name fine — contradictory history for the model, 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 | From 3cd3da3b0a19bcab7efb51abf090a471ef8e9d45 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 12:42:05 +0300 Subject: [PATCH 4/9] feat(skills): invoke a user's own skill from a message #549 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds quickapp/skill_invocation/, a preview-gated module that reads custom_content.skills[*] off the user messages, resolves each pick through the existing DialSkillResolver, and registers it as an ordinary SkillsProvider entry (order -10) under its own manifest name — so the merge, generate_skills_xml, read_skill and bundled-file reads all work unchanged. A synthetic read_skill pair is injected right after the message that picked the skill, so the model always starts that turn with the manifest and its inventory in context; later turns restore the pair from state.tool_execution_history, which keeps the manifest stable and the skill's files readable. A chip that fails to resolve gets a fixed error result and the reason reaches the user in the initialization issues stage. Scrubbing custom_content.skills off the working messages lives in the never-preview-gated SkillsModule, so the field never reaches the orchestrator or a DIAL deployment tool regardless of the preview flag. quickapp/dial_skills/ is untouched. Phase 1a of the approved design, which accepts that a pick shadows an agent skill of the same name. --- CLAUDE.md | 3 + README.md | 1 + docs/skills.md | 67 ++++++++ src/quickapp/app_factory.py | 2 + src/quickapp/common/__init__.py | 1 + src/quickapp/common/_di_types.py | 5 +- .../synthetic_tool_call_injector.py | 64 ++++--- src/quickapp/common/tool_message_utils.py | 8 + .../core/application/_request_context.py | 14 ++ .../application/_request_context_setup.py | 1 + src/quickapp/core/application/app_module.py | 5 + src/quickapp/skill_invocation/__init__.py | 3 + .../_invoked_skills_context.py | 64 +++++++ src/quickapp/skill_invocation/_settings.py | 19 +++ .../_skill_invocation_initializer.py | 87 ++++++++++ .../_skill_invocation_injector.py | 112 ++++++++++++ .../skill_invocation/_skill_reference.py | 71 ++++++++ .../skill_invocation_module.py | 57 +++++++ src/quickapp/skills/__init__.py | 2 + .../skills/_scrub_skill_chips_transformer.py | 42 +++++ src/quickapp/skills/skills_module.py | 9 +- .../skill_invocation_tests/__init__.py | 0 .../test_invoked_skills_context.py | 51 ++++++ .../test_skill_invocation_initializer.py | 130 ++++++++++++++ .../test_skill_invocation_injector.py | 159 ++++++++++++++++++ .../test_skill_invocation_wiring.py | 77 +++++++++ .../test_skill_reference.py | 93 ++++++++++ .../test_scrub_skill_chips_transformer.py | 48 ++++++ 28 files changed, 1172 insertions(+), 23 deletions(-) create mode 100644 src/quickapp/skill_invocation/__init__.py create mode 100644 src/quickapp/skill_invocation/_invoked_skills_context.py create mode 100644 src/quickapp/skill_invocation/_settings.py create mode 100644 src/quickapp/skill_invocation/_skill_invocation_initializer.py create mode 100644 src/quickapp/skill_invocation/_skill_invocation_injector.py create mode 100644 src/quickapp/skill_invocation/_skill_reference.py create mode 100644 src/quickapp/skill_invocation/skill_invocation_module.py create mode 100644 src/quickapp/skills/_scrub_skill_chips_transformer.py create mode 100644 src/tests/unit_tests/skill_invocation_tests/__init__.py create mode 100644 src/tests/unit_tests/skill_invocation_tests/test_invoked_skills_context.py create mode 100644 src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_initializer.py create mode 100644 src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_injector.py create mode 100644 src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_wiring.py create mode 100644 src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py create mode 100644 src/tests/unit_tests/skills_tests/test_scrub_skill_chips_transformer.py 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/skills.md b/docs/skills.md index 434563cc..e54bb8dd 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -294,6 +294,73 @@ 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. +- `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 directly after the message that picked + the skill, and the user sees the normal "Reading Skill: ``" stage. +- On later turns the pair comes back from the assistant state like any other tool result: the model + 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/synthetic_tool_call_injector.py b/src/quickapp/common/synthetic_injection/synthetic_tool_call_injector.py index 90ef3df1..a4e95572 100644 --- a/src/quickapp/common/synthetic_injection/synthetic_tool_call_injector.py +++ b/src/quickapp/common/synthetic_injection/synthetic_tool_call_injector.py @@ -18,9 +18,11 @@ logger = logging.getLogger(__name__) +SYNTHETIC_CALL_ID_PREFIX = "synth_" + class SyntheticToolCallInjector(MessagesTransformer, ABC): - call_id_prefix: str = "synth_" + call_id_prefix: str = SYNTHETIC_CALL_ID_PREFIX def __init__( self, @@ -50,9 +52,7 @@ async def get_content(self, messages: list[Message]) -> str | None: def _make_call_id_prefix(self, tool_name: str, arguments: dict) -> str: """Return the stable prefix for a tool+args pair (no content, no TTL).""" - tool_hash = _hash6(tool_name) - args_hash = _hash6(json.dumps(arguments, sort_keys=True)) - return f"{self.call_id_prefix}t_{tool_hash}_a_{args_hash}_" + return make_synthetic_call_id_prefix(tool_name, arguments, self.call_id_prefix) def make_call_id( self, @@ -61,18 +61,10 @@ def make_call_id( content: str, ttl_expiry_seconds: int | None = None, ) -> str: - """Build a structured call_id for the given tool+args+content. - - Format: {prefix}t_{tool_hash6}_a_{args_hash6}_c_{content_hash6}[_ttl_{expiry:08x}] - Maximum length: prefix + 2+6+3+6+3+6+5+8 = prefix + 39 chars (≤ 64 for any prefix ≤ 25). - """ - tool_hash = _hash6(tool_name) - args_hash = _hash6(json.dumps(arguments, sort_keys=True)) - content_hash = _hash6(content) - base = f"{self.call_id_prefix}t_{tool_hash}_a_{args_hash}_c_{content_hash}" - if ttl_expiry_seconds is not None: - return f"{base}_ttl_{ttl_expiry_seconds:08x}" - return base + """Build a structured call_id for the given tool+args+content.""" + return make_synthetic_call_id( + tool_name, arguments, content, ttl_expiry_seconds, self.call_id_prefix + ) @staticmethod def _parse_call_id_ttl_expiry(call_id: str) -> int | None: @@ -127,10 +119,10 @@ async def _inject_append_if_changed( pair_idx, _ = pair # Replace in place (re-stamps TTL when expiry changed, no-op when identical) state = self._enrich_state(call_id, content) - new_pair = _build_pair(tool_name, call_id, arguments, content, state) + new_pair = build_synthetic_pair(tool_name, call_id, arguments, content, state) return messages[:pair_idx] + list(new_pair) + messages[pair_idx + 2 :] - has_prior_args = _has_any_pair_with_prefix(messages, args_prefix) + has_prior_args = has_synthetic_pair_with_prefix(messages, args_prefix) idx = len(messages) if has_prior_args else after_first_user_idx(messages) return self._inject_at(messages, idx, tool_name, call_id, arguments, content) @@ -144,7 +136,7 @@ def _inject_at( content: str, ) -> list[Message]: state = self._enrich_state(call_id, content) - pair = _build_pair(tool_name, call_id, arguments, content, state) + pair = build_synthetic_pair(tool_name, call_id, arguments, content, state) return messages[:idx] + list(pair) + messages[idx:] def _enrich_state(self, call_id: str, content: str) -> dict[str, Any] | None: @@ -172,7 +164,37 @@ def _hash6(value: str) -> str: return hashlib.sha256(value.encode()).hexdigest()[:6] -def _has_any_pair_with_prefix(messages: list[Message], id_prefix: str) -> bool: +def make_synthetic_call_id_prefix( + tool_name: str, + arguments: dict, + call_id_prefix: str = SYNTHETIC_CALL_ID_PREFIX, +) -> str: + """Return the stable call-id prefix for a tool+args pair (no content, no TTL).""" + tool_hash = _hash6(tool_name) + args_hash = _hash6(json.dumps(arguments, sort_keys=True)) + return f"{call_id_prefix}t_{tool_hash}_a_{args_hash}_" + + +def make_synthetic_call_id( + tool_name: str, + arguments: dict, + content: str, + ttl_expiry_seconds: int | None = None, + call_id_prefix: str = SYNTHETIC_CALL_ID_PREFIX, +) -> str: + """Build a structured call_id for the given tool+args+content. + + Format: {prefix}t_{tool_hash6}_a_{args_hash6}_c_{content_hash6}[_ttl_{expiry:08x}] + Maximum length: prefix + 2+6+3+6+3+6+5+8 = prefix + 39 chars (≤ 64 for any prefix ≤ 25). + """ + content_hash = _hash6(content) + base = f"{make_synthetic_call_id_prefix(tool_name, arguments, call_id_prefix)}c_{content_hash}" + if ttl_expiry_seconds is not None: + return f"{base}_ttl_{ttl_expiry_seconds:08x}" + return base + + +def has_synthetic_pair_with_prefix(messages: list[Message], id_prefix: str) -> bool: return any( m.role == Role.TOOL and m.tool_call_id is not None and m.tool_call_id.startswith(id_prefix) for m in messages @@ -193,7 +215,7 @@ def _find_pair_with_args_and_content( return None -def _build_pair( +def build_synthetic_pair( tool_name: str, call_id: str, arguments: dict, diff --git a/src/quickapp/common/tool_message_utils.py b/src/quickapp/common/tool_message_utils.py index fc9f587a..fc88c955 100644 --- a/src/quickapp/common/tool_message_utils.py +++ b/src/quickapp/common/tool_message_utils.py @@ -9,6 +9,14 @@ def after_first_user_idx(messages: list[Message]) -> int: ) +def after_last_user_idx(messages: list[Message]) -> int | None: + """Return the index immediately after the last USER message, or None if there is none.""" + return next( + (i + 1 for i in range(len(messages) - 1, -1, -1) if messages[i].role == Role.USER), + None, + ) + + def tool_function_name_for_tool_message(messages: list[Message], index: int) -> str | None: """Resolve OpenAI function name for a TOOL message from the preceding assistant tool_calls.""" if index < 0 or index >= len(messages): 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..690dfaa9 --- /dev/null +++ b/src/quickapp/skill_invocation/_invoked_skills_context.py @@ -0,0 +1,64 @@ +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._skills_by_url: dict[str, ResolvedSkill] = {} + self._exceptions: list[InitializationException] = [] + self._current_turn_urls: list[str] = [] + + @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_turn_urls(self) -> list[str]: + """Canonical URLs of the chips on the message being answered, in chip order. + + 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_turn_urls + + def set_current_turn_urls(self, urls: list[str]) -> None: + self._current_turn_urls = urls + + def set_resolved_skills(self, skills: list[ResolvedSkill]) -> None: + 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: + 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..b89e6495 --- /dev/null +++ b/src/quickapp/skill_invocation/_skill_invocation_initializer.py @@ -0,0 +1,87 @@ +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_skill_urls, message_skill_urls + +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 historical pick is re-resolved on every turn. That is what keeps a skill + picked on turn 1 registered on turn 4, so its bundled files stay 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: + self._context.set_current_turn_urls(self.__current_turn_urls()) + + urls = collect_skill_urls(self._messages, self._settings.max_skills) + if not urls: + return + + 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 __current_turn_urls(self) -> list[str]: + """Chips of the message being answered — the only ones the injector acts on.""" + for message in reversed(self._messages): + if message.role == Role.USER: + return message_skill_urls(message) + return [] + + def __report(self, exceptions: list[SkillInitializationException]) -> None: + """Surface only what went wrong with this turn's chips. + + Every historical 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 chip from its error result. + """ + current = set(self._context.current_turn_urls) + for exception in exceptions: + if exception.url is None or exception.url in current: + self._context.append_exception(exception) + else: + logger.info("A skill picked on an earlier turn could not be loaded") 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..30718068 --- /dev/null +++ b/src/quickapp/skill_invocation/_skill_invocation_injector.py @@ -0,0 +1,112 @@ +import logging + +from aidial_sdk.chat_completion import Message +from injector import ProviderOf, inject + +from quickapp.common.abstract.base_transformer import MessagesTransformer +from quickapp.common.staged_base_tool import StagedBaseTool +from quickapp.common.synthetic_injection.synthetic_tool_call_injector import ( + build_synthetic_pair, + has_synthetic_pair_with_prefix, + make_synthetic_call_id, + make_synthetic_call_id_prefix, +) +from quickapp.common.tool_message_utils import after_last_user_idx +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._skill_reference import skill_name_from_url +from quickapp.skills._tool_configs import SKILL_READER_TOOL_NAME + +logger = logging.getLogger(__name__) + +_ARUN_SYNTHETIC_CALL_ID = "skill_invocation" + +# 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(MessagesTransformer): + """Inserts one synthetic ``read_skill`` call/result pair per distinct chip on the + message being answered, directly after that message. + + The explicit index keeps the pairs ahead of any transformer that appends to the + end of the list, whatever the module order. + + Earlier turns need nothing: their pair was inserted after the user message of + their own turn, ``Orchestrator`` persisted it into + ``state.tool_execution_history`` and ``_MessagesSetup`` restored it. + """ + + @inject + def __init__( + self, + context: _InvokedSkillsContext, + tools_provider: ProviderOf[list[StagedBaseTool]], + ) -> None: + self._context = context + self._tools_provider = tools_provider + + async def transform(self, messages: list[Message]) -> list[Message]: + urls = self._context.current_turn_urls + if not urls: + return messages + + index = after_last_user_idx(messages) + if index is None: + return messages + + pairs: list[Message] = [] + injected_prefixes: set[str] = set() + for url in urls: + skill = self._context.find_skill(url) + name = skill.metadata.name if skill is not None else skill_name_from_url(url) + arguments = {"skill_name": name} + prefix = make_synthetic_call_id_prefix(SKILL_READER_TOOL_NAME, arguments) + # A pair for the same tool and arguments is already there: the model keeps + # the manifest from the turn that first picked the skill, and a request + # never carries two pairs sharing a tool_call_id. + if prefix in injected_prefixes or has_synthetic_pair_with_prefix(messages, prefix): + continue + + if skill is None: + content: str | None = _LOAD_FAILED.format(name=name) + else: + content = await self.__read_skill(name) + if content is None: + continue + + call_id = make_synthetic_call_id(SKILL_READER_TOOL_NAME, arguments, content) + pairs.extend(build_synthetic_pair(SKILL_READER_TOOL_NAME, call_id, arguments, content)) + injected_prefixes.add(prefix) + + if not pairs: + return messages + return messages[:index] + pairs + messages[index:] + + async def __read_skill(self, skill_name: str) -> str | None: + """Run the real ``read_skill`` tool, so the result is byte-identical to a + model-initiated call, ```` included. + + ``arun`` is called *without* ``stage_level`` so it defaults to ``INFO``, as it + does in ``tool_executor``: the invocation is something the user did, so its + stage belongs in the response. + """ + tool = self.__find_skill_reader() + if tool is None: + logger.warning("Skill reader tool is unavailable; skipping a skill invocation") + return None + result = await tool.arun(_ARUN_SYNTHETIC_CALL_ID, skill_name=skill_name) + return result.content + + def __find_skill_reader(self) -> StagedBaseTool | None: + return next( + ( + tool + for tool in self._tools_provider.get() + if tool.tool_config.open_ai_tool.function.name == SKILL_READER_TOOL_NAME + ), + 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..b861ef9a --- /dev/null +++ b/src/quickapp/skill_invocation/_skill_reference.py @@ -0,0 +1,71 @@ +import logging +from typing import Any + +from aidial_sdk.chat_completion import Message, Role + +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 + + +def collect_skill_urls(messages: list[Message], max_skills: int) -> list[str]: + """Every skill URL picked in the conversation, oldest first, capped. + + A URL keeps the position of its *latest* occurrence, so the cap drops the + oldest picks and the chips of the message being answered survive it. + """ + ordered: list[str] = [] + for message in messages: + for url in message_skill_urls(message): + if url in ordered: + ordered.remove(url) + ordered.append(url) + return ordered[-max_skills:] + + +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/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..b0d18bc2 --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_invoked_skills_context.py @@ -0,0 +1,51 @@ +"""``_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 TestResolvedSkills: + + def test_starts_empty(self): + assert _InvokedSkillsContext().resolved_skills == [] + assert _InvokedSkillsContext().current_turn_urls == [] + + 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")] + ) + + entry = context.resolved_skills[0] + assert entry.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..26e36752 --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_initializer.py @@ -0,0 +1,130 @@ +"""``_SkillInvocationInitializer`` — collect chips, 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 + + +class TestInitialize: + + @pytest.mark.asyncio + async def test_no_chips_skips_resolution_entirely(self): + initializer, resolver, context = _make([Message(role=Role.USER, content="hi")]) + + await initializer.initialize() + + resolver.resolve.assert_not_awaited() + assert context.resolved_skills == [] + assert context.current_turn_urls == [] + + @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() + + configs = resolver.resolve.await_args.args[0] + assert [cfg.url for cfg in configs] == ["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_records_the_chips_of_the_message_being_answered(self): + initializer, _, context = _make( + [_user("skills/b/a"), Message(role=Role.ASSISTANT, content="ok"), _user("skills/b/z")] + ) + + await initializer.initialize() + + assert context.current_turn_urls == ["skills/b/z"] + + @pytest.mark.asyncio + async def test_registers_what_resolved(self): + skill = _skill("skills/b/a", "a") + initializer, _, context = _make( + [_user("skills/b/a")], + DialSkillResolverOutput(resolved=[skill], 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 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 [exc.reason for exc in context.exceptions] == ["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 [exc.reason for exc in context.exceptions] == ["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..3fd668e4 --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_injector.py @@ -0,0 +1,159 @@ +"""``_SkillInvocationInjector`` — synthetic ``read_skill`` pairs for this turn's chips.""" + +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.synthetic_injection.synthetic_tool_call_injector import ( + build_synthetic_pair, + make_synthetic_call_id, +) +from quickapp.skill_invocation._invoked_skills_context import _InvokedSkillsContext +from quickapp.skill_invocation._skill_invocation_injector import _SkillInvocationInjector +from quickapp.skills._tool_configs import SKILL_READER_TOOL_NAME +from tests.unit_tests.common.common import make_resolved_skill as _skill + + +def _skill_reader_tool(content: str = "manifest") -> MagicMock: + tool = MagicMock(spec=StagedBaseTool) + tool.tool_config = MagicMock() + tool.tool_config.open_ai_tool.function.name = SKILL_READER_TOOL_NAME + tool.arun = AsyncMock( + return_value=ToolCallResult(content=content, content_type="text/markdown") + ) + return tool + + +def _make(context: _InvokedSkillsContext, tool: MagicMock | None = None): + tools_provider = MagicMock() + tools_provider.get.return_value = [tool] if tool is not None else [] + return _SkillInvocationInjector(context, tools_provider) + + +def _context(urls: list[str], resolved: list = ()) -> _InvokedSkillsContext: + context = _InvokedSkillsContext() + context.set_current_turn_urls(urls) + context.set_resolved_skills(list(resolved)) + return context + + +class TestInjection: + + @pytest.mark.asyncio + async def test_no_chips_this_turn_leaves_the_messages_alone(self): + messages = [Message(role=Role.USER, content="hi")] + + assert await _make(_context([])).transform(messages) == messages + + @pytest.mark.asyncio + async def test_a_resolved_chip_becomes_a_pair_running_the_real_tool(self): + tool = _skill_reader_tool("the manifest") + context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + + result = await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + + assert [m.role for m in result] == [Role.USER, Role.ASSISTANT, Role.TOOL] + call = result[1].tool_calls[0] + assert call.function.name == SKILL_READER_TOOL_NAME + assert json.loads(call.function.arguments) == {"skill_name": "sql-style"} + assert result[2].content == "the manifest" + assert result[2].tool_call_id == call.id + + @pytest.mark.asyncio + async def test_the_stage_level_is_left_at_its_default(self): + tool = _skill_reader_tool() + context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + + await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + + assert "stage_level" not in tool.arun.await_args.kwargs + + @pytest.mark.asyncio + async def test_pairs_go_right_after_the_last_user_message(self): + tool = _skill_reader_tool() + context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + messages = [ + Message(role=Role.USER, content="first"), + Message(role=Role.ASSISTANT, content="ok"), + Message(role=Role.USER, content="second"), + ] + + result = await _make(context, tool).transform(messages) + + assert [m.role for m in result] == [ + Role.USER, + Role.ASSISTANT, + Role.USER, + Role.ASSISTANT, + Role.TOOL, + ] + + @pytest.mark.asyncio + async def test_chips_keep_their_order(self): + tool = _skill_reader_tool() + context = _context( + ["skills/b/a", "skills/b/z"], + [_skill("skills/b/a", "a"), _skill("skills/b/z", "z")], + ) + + result = await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + + names = [ + json.loads(m.tool_calls[0].function.arguments)["skill_name"] + for m in result + if m.role == Role.ASSISTANT + ] + assert names == ["a", "z"] + + +class TestFailedChips: + + @pytest.mark.asyncio + async def test_an_unresolved_chip_gets_a_fixed_error_result_named_after_its_url(self): + context = _context(["skills/b/sql-style"]) + + result = await _make(context, _skill_reader_tool()).transform( + [Message(role=Role.USER, content="hi")] + ) + + assert json.loads(result[1].tool_calls[0].function.arguments) == {"skill_name": "sql-style"} + assert result[2].content == ( + "Error: the user's skill `sql-style` could not be loaded." + " The reason is shown to the user." + ) + + @pytest.mark.asyncio + async def test_a_missing_skill_reader_tool_injects_nothing(self): + context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + messages = [Message(role=Role.USER, content="hi")] + + assert await _make(context).transform(messages) == messages + + +class TestSkipping: + + @pytest.mark.asyncio + async def test_a_pair_already_in_history_is_not_injected_again(self): + tool = _skill_reader_tool() + arguments = {"skill_name": "sql-style"} + call_id = make_synthetic_call_id(SKILL_READER_TOOL_NAME, arguments, "old manifest") + existing = build_synthetic_pair(SKILL_READER_TOOL_NAME, call_id, arguments, "old manifest") + messages = [Message(role=Role.USER, content="hi"), *existing] + context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + + result = await _make(context, tool).transform(messages) + + assert result == messages + tool.arun.assert_not_awaited() + + @pytest.mark.asyncio + async def test_two_chips_deriving_the_same_name_produce_one_pair(self): + tool = _skill_reader_tool() + context = _context(["skills/b/dup", "skills/other/dup"]) + + result = await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + + assert [m.role for m in result] == [Role.USER, Role.ASSISTANT, Role.TOOL] 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..ccb97cc9 --- /dev/null +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py @@ -0,0 +1,93 @@ +"""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_skill_urls, + 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 TestCollectSkillUrls: + + def test_returns_conversation_picks_oldest_first(self): + messages = [ + _user("skills/b/a"), + Message(role=Role.ASSISTANT, content="ok"), + _user("skills/b/z"), + ] + assert collect_skill_urls(messages, 10) == ["skills/b/a", "skills/b/z"] + + def test_a_repicked_url_moves_to_the_end(self): + messages = [_user("skills/b/a"), _user("skills/b/z"), _user("skills/b/a")] + assert collect_skill_urls(messages, 10) == ["skills/b/z", "skills/b/a"] + + def test_the_cap_drops_the_oldest_picks(self): + messages = [_user("skills/b/a"), _user("skills/b/z"), _user("skills/b/k")] + assert collect_skill_urls(messages, 2) == ["skills/b/z", "skills/b/k"] + + def test_no_chips_anywhere_yields_nothing(self): + assert collect_skill_urls([Message(role=Role.USER, content="hi")], 10) == [] + + +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] From 2cb6012dacb7ee2ef8bea09f43cd2ce7d84ec8b8 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 15:03:19 +0300 Subject: [PATCH 5/9] refactor(skills): reuse StagedToolSyntheticInjector for skill invocation #549 Replaces the bespoke injector with a StagedToolSyntheticInjector subclass: the tool lookup by function name, the not-found branch and the arun call all come from the mixin, leaving five small methods and one override. APPEND_IF_CHANGED puts the pair after the first user message, the slot the built-in file-transfer skill already uses, and should_inject limits the transformer to the turn the pick is made on. Later turns restore the pair from state.tool_execution_history like any other tool result, so read_skill is never re-run and the model keeps the manifest it first saw. StagedToolSyntheticInjector.stage_level becomes an overridable class attribute, defaulting to the DEBUG it hardcoded before, so every existing subclass is unaffected. Skill invocation raises it to INFO: the user asked for the skill, so the ordinary "Reading Skill" stage belongs in the response. get_content is overridden only to return a fixed error sentence for a skill that never reached the registry, rather than running the tool for the sake of its own "not found". Only one skill may be invoked per message. Extra chips on the message being answered are ignored and reported as a warning-severity initialization issue naming them, rather than dropped silently; extras on earlier turns are logged, so the stage does not repeat an issue the user can no longer act on. --- docs/skills.md | 16 +- .../staged_tool_synthetic_injector.py | 9 +- .../synthetic_tool_call_injector.py | 64 +++---- src/quickapp/common/tool_message_utils.py | 8 - .../_invoked_skills_context.py | 18 +- .../_skill_invocation_initializer.py | 61 ++++--- .../_skill_invocation_injector.py | 139 +++++++-------- .../skill_invocation/_skill_reference.py | 63 +++++-- .../test_staged_tool_synthetic_injector.py | 12 ++ .../test_invoked_skills_context.py | 11 +- .../test_skill_invocation_initializer.py | 77 +++++++-- .../test_skill_invocation_injector.py | 163 ++++++++---------- .../test_skill_reference.py | 33 ++-- 13 files changed, 369 insertions(+), 305 deletions(-) diff --git a/docs/skills.md b/docs/skills.md index e54bb8dd..540216bc 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -317,6 +317,8 @@ the rest of the conversation. The model does not get to choose — a picked skil - 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 @@ -326,11 +328,15 @@ the rest of the conversation. The model does not get to choose — a picked skil - 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 directly after the message that picked - the skill, and the user sees the normal "Reading Skill: ``" stage. -- On later turns the pair comes back from the assistant state like any other tool result: the model - 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 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. On later turns the pair comes back from + the assistant state like any other tool result, so `read_skill` is never re-run for it. +- 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. 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/common/synthetic_injection/synthetic_tool_call_injector.py b/src/quickapp/common/synthetic_injection/synthetic_tool_call_injector.py index a4e95572..90ef3df1 100644 --- a/src/quickapp/common/synthetic_injection/synthetic_tool_call_injector.py +++ b/src/quickapp/common/synthetic_injection/synthetic_tool_call_injector.py @@ -18,11 +18,9 @@ logger = logging.getLogger(__name__) -SYNTHETIC_CALL_ID_PREFIX = "synth_" - class SyntheticToolCallInjector(MessagesTransformer, ABC): - call_id_prefix: str = SYNTHETIC_CALL_ID_PREFIX + call_id_prefix: str = "synth_" def __init__( self, @@ -52,7 +50,9 @@ async def get_content(self, messages: list[Message]) -> str | None: def _make_call_id_prefix(self, tool_name: str, arguments: dict) -> str: """Return the stable prefix for a tool+args pair (no content, no TTL).""" - return make_synthetic_call_id_prefix(tool_name, arguments, self.call_id_prefix) + tool_hash = _hash6(tool_name) + args_hash = _hash6(json.dumps(arguments, sort_keys=True)) + return f"{self.call_id_prefix}t_{tool_hash}_a_{args_hash}_" def make_call_id( self, @@ -61,10 +61,18 @@ def make_call_id( content: str, ttl_expiry_seconds: int | None = None, ) -> str: - """Build a structured call_id for the given tool+args+content.""" - return make_synthetic_call_id( - tool_name, arguments, content, ttl_expiry_seconds, self.call_id_prefix - ) + """Build a structured call_id for the given tool+args+content. + + Format: {prefix}t_{tool_hash6}_a_{args_hash6}_c_{content_hash6}[_ttl_{expiry:08x}] + Maximum length: prefix + 2+6+3+6+3+6+5+8 = prefix + 39 chars (≤ 64 for any prefix ≤ 25). + """ + tool_hash = _hash6(tool_name) + args_hash = _hash6(json.dumps(arguments, sort_keys=True)) + content_hash = _hash6(content) + base = f"{self.call_id_prefix}t_{tool_hash}_a_{args_hash}_c_{content_hash}" + if ttl_expiry_seconds is not None: + return f"{base}_ttl_{ttl_expiry_seconds:08x}" + return base @staticmethod def _parse_call_id_ttl_expiry(call_id: str) -> int | None: @@ -119,10 +127,10 @@ async def _inject_append_if_changed( pair_idx, _ = pair # Replace in place (re-stamps TTL when expiry changed, no-op when identical) state = self._enrich_state(call_id, content) - new_pair = build_synthetic_pair(tool_name, call_id, arguments, content, state) + new_pair = _build_pair(tool_name, call_id, arguments, content, state) return messages[:pair_idx] + list(new_pair) + messages[pair_idx + 2 :] - has_prior_args = has_synthetic_pair_with_prefix(messages, args_prefix) + has_prior_args = _has_any_pair_with_prefix(messages, args_prefix) idx = len(messages) if has_prior_args else after_first_user_idx(messages) return self._inject_at(messages, idx, tool_name, call_id, arguments, content) @@ -136,7 +144,7 @@ def _inject_at( content: str, ) -> list[Message]: state = self._enrich_state(call_id, content) - pair = build_synthetic_pair(tool_name, call_id, arguments, content, state) + pair = _build_pair(tool_name, call_id, arguments, content, state) return messages[:idx] + list(pair) + messages[idx:] def _enrich_state(self, call_id: str, content: str) -> dict[str, Any] | None: @@ -164,37 +172,7 @@ def _hash6(value: str) -> str: return hashlib.sha256(value.encode()).hexdigest()[:6] -def make_synthetic_call_id_prefix( - tool_name: str, - arguments: dict, - call_id_prefix: str = SYNTHETIC_CALL_ID_PREFIX, -) -> str: - """Return the stable call-id prefix for a tool+args pair (no content, no TTL).""" - tool_hash = _hash6(tool_name) - args_hash = _hash6(json.dumps(arguments, sort_keys=True)) - return f"{call_id_prefix}t_{tool_hash}_a_{args_hash}_" - - -def make_synthetic_call_id( - tool_name: str, - arguments: dict, - content: str, - ttl_expiry_seconds: int | None = None, - call_id_prefix: str = SYNTHETIC_CALL_ID_PREFIX, -) -> str: - """Build a structured call_id for the given tool+args+content. - - Format: {prefix}t_{tool_hash6}_a_{args_hash6}_c_{content_hash6}[_ttl_{expiry:08x}] - Maximum length: prefix + 2+6+3+6+3+6+5+8 = prefix + 39 chars (≤ 64 for any prefix ≤ 25). - """ - content_hash = _hash6(content) - base = f"{make_synthetic_call_id_prefix(tool_name, arguments, call_id_prefix)}c_{content_hash}" - if ttl_expiry_seconds is not None: - return f"{base}_ttl_{ttl_expiry_seconds:08x}" - return base - - -def has_synthetic_pair_with_prefix(messages: list[Message], id_prefix: str) -> bool: +def _has_any_pair_with_prefix(messages: list[Message], id_prefix: str) -> bool: return any( m.role == Role.TOOL and m.tool_call_id is not None and m.tool_call_id.startswith(id_prefix) for m in messages @@ -215,7 +193,7 @@ def _find_pair_with_args_and_content( return None -def build_synthetic_pair( +def _build_pair( tool_name: str, call_id: str, arguments: dict, diff --git a/src/quickapp/common/tool_message_utils.py b/src/quickapp/common/tool_message_utils.py index fc88c955..fc9f587a 100644 --- a/src/quickapp/common/tool_message_utils.py +++ b/src/quickapp/common/tool_message_utils.py @@ -9,14 +9,6 @@ def after_first_user_idx(messages: list[Message]) -> int: ) -def after_last_user_idx(messages: list[Message]) -> int | None: - """Return the index immediately after the last USER message, or None if there is none.""" - return next( - (i + 1 for i in range(len(messages) - 1, -1, -1) if messages[i].role == Role.USER), - None, - ) - - def tool_function_name_for_tool_message(messages: list[Message], index: int) -> str | None: """Resolve OpenAI function name for a TOOL message from the preceding assistant tool_calls.""" if index < 0 or index >= len(messages): diff --git a/src/quickapp/skill_invocation/_invoked_skills_context.py b/src/quickapp/skill_invocation/_invoked_skills_context.py index 690dfaa9..5657b629 100644 --- a/src/quickapp/skill_invocation/_invoked_skills_context.py +++ b/src/quickapp/skill_invocation/_invoked_skills_context.py @@ -20,18 +20,18 @@ class _InvokedSkillsContext(SkillsProvider): 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`` 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._current_turn_urls: list[str] = [] @property def resolved_skills(self) -> list[ResolvedSkill]: @@ -42,16 +42,16 @@ def exceptions(self) -> list[InitializationException]: return self._exceptions @property - def current_turn_urls(self) -> list[str]: - """Canonical URLs of the chips on the message being answered, in chip order. + 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_turn_urls + return self._current_pick_url - def set_current_turn_urls(self, urls: list[str]) -> None: - self._current_turn_urls = urls + def set_current_pick_url(self, url: str | None) -> None: + self._current_pick_url = url def set_resolved_skills(self, skills: list[ResolvedSkill]) -> None: self._skills_by_url = {skill.url: _mark_user_selected(skill) for skill in skills} diff --git a/src/quickapp/skill_invocation/_skill_invocation_initializer.py b/src/quickapp/skill_invocation/_skill_invocation_initializer.py index b89e6495..4747c405 100644 --- a/src/quickapp/skill_invocation/_skill_invocation_initializer.py +++ b/src/quickapp/skill_invocation/_skill_invocation_initializer.py @@ -13,7 +13,7 @@ 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_skill_urls, message_skill_urls +from quickapp.skill_invocation._skill_reference import collect_picks, skill_name_from_url logger = logging.getLogger(__name__) @@ -26,10 +26,10 @@ class _SkillInvocationInitializer(CompletionInitializer): An initializer rather than a transformer: ``SkillsRegistry`` caches its merge on first call, and that call comes from ``_AddSystemPromptTransformer``. - Every historical pick is re-resolved on every turn. That is what keeps a skill - picked on turn 1 registered on turn 4, so its bundled files stay readable. The - cost is one Core fetch per picked skill per turn, bounded by the cap and run in - parallel by ``DialSkillResolver``. + 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__( @@ -45,12 +45,19 @@ def __init__( self._settings = settings async def initialize(self) -> None: - self._context.set_current_turn_urls(self.__current_turn_urls()) + 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)) - urls = collect_skill_urls(self._messages, self._settings.max_skills) - if not urls: + 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: @@ -65,23 +72,37 @@ async def initialize(self) -> None: self._context.set_resolved_skills(output.resolved) self.__report(output.exceptions) - def __current_turn_urls(self) -> list[str]: - """Chips of the message being answered — the only ones the injector acts on.""" - for message in reversed(self._messages): - if message.role == Role.USER: - return message_skill_urls(message) - return [] + 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 chips. + """Surface only what went wrong with this turn's pick. - Every historical 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 chip from its error result. + 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 = set(self._context.current_turn_urls) + current = self._context.current_pick_url for exception in exceptions: - if exception.url is None or exception.url in current: + if exception.url is None or exception.url == current: self._context.append_exception(exception) else: logger.info("A skill picked on an earlier turn could not be loaded") diff --git a/src/quickapp/skill_invocation/_skill_invocation_injector.py b/src/quickapp/skill_invocation/_skill_invocation_injector.py index 30718068..1190d040 100644 --- a/src/quickapp/skill_invocation/_skill_invocation_injector.py +++ b/src/quickapp/skill_invocation/_skill_invocation_injector.py @@ -3,23 +3,20 @@ from aidial_sdk.chat_completion import Message from injector import ProviderOf, inject -from quickapp.common.abstract.base_transformer import MessagesTransformer +from quickapp.common.abstract.tool_call_result_enricher import ToolCallResultEnricher from quickapp.common.staged_base_tool import StagedBaseTool -from quickapp.common.synthetic_injection.synthetic_tool_call_injector import ( - build_synthetic_pair, - has_synthetic_pair_with_prefix, - make_synthetic_call_id, - make_synthetic_call_id_prefix, +from quickapp.common.synthetic_injection.injection_enums import InjectionFrequency +from quickapp.common.synthetic_injection.staged_tool_synthetic_injector import ( + StagedToolSyntheticInjector, ) -from quickapp.common.tool_message_utils import after_last_user_idx +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 from quickapp.skills._tool_configs import SKILL_READER_TOOL_NAME logger = logging.getLogger(__name__) -_ARUN_SYNTHETIC_CALL_ID = "skill_invocation" - # 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. @@ -28,85 +25,65 @@ ) -class _SkillInvocationInjector(MessagesTransformer): - """Inserts one synthetic ``read_skill`` call/result pair per distinct chip on the - message being answered, directly after that message. +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. - The explicit index keeps the pairs ahead of any transformer that appends to the - end of the list, whatever the module order. + 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. - Earlier turns need nothing: their pair was inserted after the user message of - their own turn, ``Orchestrator`` persisted it into - ``state.tool_execution_history`` and ``_MessagesSetup`` restored it. + ``APPEND_IF_CHANGED`` puts the pair after the first user message, the same slot the + built-in file-transfer skill uses. It runs only on the turn the pick is made + (``should_inject``); afterwards the pair is restored from + ``state.tool_execution_history`` like any other tool result, so the model keeps the + manifest it first saw and ``read_skill`` is never re-run for it. """ + stage_level = StageDisplayLevel.INFO + @inject def __init__( self, context: _InvokedSkillsContext, - tools_provider: ProviderOf[list[StagedBaseTool]], + tools: list[StagedBaseTool], + enrichers_provider: ProviderOf[list[ToolCallResultEnricher]], ) -> None: - self._context = context - self._tools_provider = tools_provider - - async def transform(self, messages: list[Message]) -> list[Message]: - urls = self._context.current_turn_urls - if not urls: - return messages - - index = after_last_user_idx(messages) - if index is None: - return messages - - pairs: list[Message] = [] - injected_prefixes: set[str] = set() - for url in urls: - skill = self._context.find_skill(url) - name = skill.metadata.name if skill is not None else skill_name_from_url(url) - arguments = {"skill_name": name} - prefix = make_synthetic_call_id_prefix(SKILL_READER_TOOL_NAME, arguments) - # A pair for the same tool and arguments is already there: the model keeps - # the manifest from the turn that first picked the skill, and a request - # never carries two pairs sharing a tool_call_id. - if prefix in injected_prefixes or has_synthetic_pair_with_prefix(messages, prefix): - continue - - if skill is None: - content: str | None = _LOAD_FAILED.format(name=name) - else: - content = await self.__read_skill(name) - if content is None: - continue - - call_id = make_synthetic_call_id(SKILL_READER_TOOL_NAME, arguments, content) - pairs.extend(build_synthetic_pair(SKILL_READER_TOOL_NAME, call_id, arguments, content)) - injected_prefixes.add(prefix) - - if not pairs: - return messages - return messages[:index] + pairs + messages[index:] - - async def __read_skill(self, skill_name: str) -> str | None: - """Run the real ``read_skill`` tool, so the result is byte-identical to a - model-initiated call, ```` included. - - ``arun`` is called *without* ``stage_level`` so it defaults to ``INFO``, as it - does in ``tool_executor``: the invocation is something the user did, so its - stage belongs in the response. + 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 SKILL_READER_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. """ - tool = self.__find_skill_reader() - if tool is None: - logger.warning("Skill reader tool is unavailable; skipping a skill invocation") - return None - result = await tool.arun(_ARUN_SYNTHETIC_CALL_ID, skill_name=skill_name) - return result.content - - def __find_skill_reader(self) -> StagedBaseTool | None: - return next( - ( - tool - for tool in self._tools_provider.get() - if tool.tool_config.open_ai_tool.function.name == SKILL_READER_TOOL_NAME - ), - None, - ) + 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 index b861ef9a..027dedd5 100644 --- a/src/quickapp/skill_invocation/_skill_reference.py +++ b/src/quickapp/skill_invocation/_skill_reference.py @@ -2,6 +2,7 @@ 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 @@ -11,10 +12,9 @@ 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. + ``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 [] @@ -35,19 +35,54 @@ def message_skill_urls(message: Message) -> list[str]: return urls -def collect_skill_urls(messages: list[Message], max_skills: int) -> list[str]: - """Every skill URL picked in the conversation, oldest first, capped. +class ConversationPicks(BaseModel): + """What the chips of a whole conversation add up to.""" - A URL keeps the position of its *latest* occurrence, so the cap drops the - oldest picks and the chips of the message being answered survive it. + 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. """ - ordered: list[str] = [] + by_ordinal: dict[int, str] = {} + ignored_by_ordinal: dict[int, list[str]] = {} + seen: set[str] = set() + ordinal = -1 + for message in messages: - for url in message_skill_urls(message): - if url in ordered: - ordered.remove(url) - ordered.append(url) - return ordered[-max_skills:] + 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:] + logger.warning( + "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: 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/test_invoked_skills_context.py b/src/tests/unit_tests/skill_invocation_tests/test_invoked_skills_context.py index b0d18bc2..c20b21cf 100644 --- 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 @@ -18,11 +18,13 @@ def test_display_name_is_human_readable(self): assert _InvokedSkillsContext.display_name == "user skills" -class TestResolvedSkills: +class TestState: def test_starts_empty(self): - assert _InvokedSkillsContext().resolved_skills == [] - assert _InvokedSkillsContext().current_turn_urls == [] + 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() @@ -30,8 +32,7 @@ def test_content_is_prefixed_with_a_user_selected_header(self): [_skill("skills/b/sql-style", "sql-style", content="---\nbody")] ) - entry = context.resolved_skills[0] - assert entry.content == ( + assert context.resolved_skills[0].content == ( "Skill `sql-style`, selected by the user for this conversation.\n---\nbody" ) 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 index 26e36752..91da790f 100644 --- 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 @@ -1,4 +1,4 @@ -"""``_SkillInvocationInitializer`` — collect chips, cap, delegate, report this turn.""" +"""``_SkillInvocationInitializer`` — collect picks, cap, delegate, report this turn.""" from unittest.mock import AsyncMock, MagicMock @@ -24,7 +24,9 @@ def _user(*urls: str) -> Message: def _make( - messages: list[Message], output: DialSkillResolverOutput | None = None, max_skills: int = 10 + messages: list[Message], + output: DialSkillResolverOutput | None = None, + max_skills: int = 10, ): resolver = MagicMock(spec=DialSkillResolver) resolver.resolve = AsyncMock( @@ -35,17 +37,20 @@ def _make( 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_chips_skips_resolution_entirely(self): + 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.resolved_skills == [] - assert context.current_turn_urls == [] + assert context.current_pick_url is None @pytest.mark.asyncio async def test_resolves_every_pick_in_the_conversation_oldest_first(self): @@ -53,8 +58,10 @@ async def test_resolves_every_pick_in_the_conversation_oldest_first(self): await initializer.initialize() - configs = resolver.resolve.await_args.args[0] - assert [cfg.url for cfg in configs] == ["skills/b/a", "skills/b/z"] + 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): @@ -65,21 +72,26 @@ async def test_the_cap_drops_the_oldest_picks(self): assert [cfg.url for cfg in resolver.resolve.await_args.args[0]] == ["skills/b/z"] @pytest.mark.asyncio - async def test_records_the_chips_of_the_message_being_answered(self): - initializer, _, context = _make( - [_user("skills/b/a"), Message(role=Role.ASSISTANT, content="ok"), _user("skills/b/z")] - ) + 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_turn_urls == ["skills/b/z"] + assert context.current_pick_url is None @pytest.mark.asyncio async def test_registers_what_resolved(self): - skill = _skill("skills/b/a", "a") initializer, _, context = _make( [_user("skills/b/a")], - DialSkillResolverOutput(resolved=[skill], exceptions=[]), + DialSkillResolverOutput(resolved=[_skill("skills/b/a", "a")], exceptions=[]), ) await initializer.initialize() @@ -97,6 +109,39 @@ async def test_a_resolver_crash_is_reported_and_does_not_raise(self): 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 @@ -114,7 +159,7 @@ async def test_only_this_turns_problems_reach_initialization_issues(self): await initializer.initialize() - assert [exc.reason for exc in context.exceptions] == ["fresh"] + assert _reasons(context) == ["fresh"] @pytest.mark.asyncio async def test_an_exception_without_a_url_is_always_reported(self): @@ -127,4 +172,4 @@ async def test_an_exception_without_a_url_is_always_reported(self): await initializer.initialize() - assert [exc.reason for exc in context.exceptions] == ["global"] + 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 index 3fd668e4..48c7e4fa 100644 --- 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 @@ -1,4 +1,5 @@ -"""``_SkillInvocationInjector`` — synthetic ``read_skill`` pairs for this turn's chips.""" +"""``_SkillInvocationInjector`` — a ``StagedToolSyntheticInjector`` on +``InjectionFrequency.APPEND_IF_CHANGED``.""" import json from unittest.mock import AsyncMock, MagicMock @@ -7,17 +8,16 @@ from aidial_sdk.chat_completion import Message, Role from quickapp.common import StagedBaseTool, ToolCallResult -from quickapp.common.synthetic_injection.synthetic_tool_call_injector import ( - build_synthetic_pair, - make_synthetic_call_id, -) +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 quickapp.skills._tool_configs import SKILL_READER_TOOL_NAME from tests.unit_tests.common.common import make_resolved_skill as _skill +_URL = "skills/b/code-review" -def _skill_reader_tool(content: str = "manifest") -> MagicMock: + +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 = SKILL_READER_TOOL_NAME @@ -27,133 +27,116 @@ def _skill_reader_tool(content: str = "manifest") -> MagicMock: return tool -def _make(context: _InvokedSkillsContext, tool: MagicMock | None = None): - tools_provider = MagicMock() - tools_provider.get.return_value = [tool] if tool is not None else [] - return _SkillInvocationInjector(context, tools_provider) - - -def _context(urls: list[str], resolved: list = ()) -> _InvokedSkillsContext: +def _make(current: str | None, resolved: list = (), tool: MagicMock | None = None): context = _InvokedSkillsContext() - context.set_current_turn_urls(urls) + context.set_current_pick_url(current) context.set_resolved_skills(list(resolved)) - return context + enrichers_provider = MagicMock() + enrichers_provider.get.return_value = [] -class TestInjection: - - @pytest.mark.asyncio - async def test_no_chips_this_turn_leaves_the_messages_alone(self): - messages = [Message(role=Role.USER, content="hi")] + injector = _SkillInvocationInjector( + context, [tool] if tool is not None else [], enrichers_provider + ) + return injector, tool - assert await _make(_context([])).transform(messages) == messages - @pytest.mark.asyncio - async def test_a_resolved_chip_becomes_a_pair_running_the_real_tool(self): - tool = _skill_reader_tool("the manifest") - context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) +def _args(message: Message) -> dict: + return json.loads(message.tool_calls[0].function.arguments) - result = await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) - assert [m.role for m in result] == [Role.USER, Role.ASSISTANT, Role.TOOL] - call = result[1].tool_calls[0] - assert call.function.name == SKILL_READER_TOOL_NAME - assert json.loads(call.function.arguments) == {"skill_name": "sql-style"} - assert result[2].content == "the manifest" - assert result[2].tool_call_id == call.id +class TestInjection: @pytest.mark.asyncio - async def test_the_stage_level_is_left_at_its_default(self): - tool = _skill_reader_tool() - context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) - - await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + 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 "stage_level" not in tool.arun.await_args.kwargs + assert await injector.transform(messages) == messages + tool.arun.assert_not_awaited() @pytest.mark.asyncio - async def test_pairs_go_right_after_the_last_user_message(self): - tool = _skill_reader_tool() - context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + 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="first"), + Message(role=Role.USER, content="/code-review …"), Message(role=Role.ASSISTANT, content="ok"), - Message(role=Role.USER, content="second"), + Message(role=Role.USER, content="and also …"), ] - result = await _make(context, tool).transform(messages) + result = await injector.transform(messages) assert [m.role for m in result] == [ - Role.USER, - Role.ASSISTANT, 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_chips_keep_their_order(self): - tool = _skill_reader_tool() - context = _context( - ["skills/b/a", "skills/b/z"], - [_skill("skills/b/a", "a"), _skill("skills/b/z", "z")], - ) + 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()) - result = await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + await injector.transform([Message(role=Role.USER, content="hi")]) - names = [ - json.loads(m.tool_calls[0].function.arguments)["skill_name"] - for m in result - if m.role == Role.ASSISTANT - ] - assert names == ["a", "z"] + 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 TestFailedChips: +class TestLaterTurns: @pytest.mark.asyncio - async def test_an_unresolved_chip_gets_a_fixed_error_result_named_after_its_url(self): - context = _context(["skills/b/sql-style"]) + 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")]) - result = await _make(context, _skill_reader_tool()).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 json.loads(result[1].tool_calls[0].function.arguments) == {"skill_name": "sql-style"} - assert result[2].content == ( - "Error: the user's skill `sql-style` could not be loaded." - " The reason is shown to the user." - ) + assert await injector.transform(messages) == messages + tool.arun.assert_not_awaited() @pytest.mark.asyncio - async def test_a_missing_skill_reader_tool_injects_nothing(self): - context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) - messages = [Message(role=Role.USER, content="hi")] + 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 await _make(context).transform(messages) == messages + assert [m.role for m in second] == [m.role for m in first] -class TestSkipping: +class TestFailedPick: @pytest.mark.asyncio - async def test_a_pair_already_in_history_is_not_injected_again(self): - tool = _skill_reader_tool() - arguments = {"skill_name": "sql-style"} - call_id = make_synthetic_call_id(SKILL_READER_TOOL_NAME, arguments, "old manifest") - existing = build_synthetic_pair(SKILL_READER_TOOL_NAME, call_id, arguments, "old manifest") - messages = [Message(role=Role.USER, content="hi"), *existing] - context = _context(["skills/b/sql-style"], [_skill("skills/b/sql-style", "sql-style")]) + 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 _make(context, tool).transform(messages) + result = await injector.transform([Message(role=Role.USER, content="hi")]) - assert result == messages + 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_two_chips_deriving_the_same_name_produce_one_pair(self): - tool = _skill_reader_tool() - context = _context(["skills/b/dup", "skills/other/dup"]) - - result = await _make(context, tool).transform([Message(role=Role.USER, content="hi")]) + 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 [m.role for m in result] == [Role.USER, Role.ASSISTANT, Role.TOOL] + assert await injector.transform(messages) == messages 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 index ccb97cc9..fa7bb5fe 100644 --- a/src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py +++ b/src/tests/unit_tests/skill_invocation_tests/test_skill_reference.py @@ -3,7 +3,7 @@ from aidial_sdk.chat_completion import Message, Role from quickapp.skill_invocation._skill_reference import ( - collect_skill_urls, + collect_picks, message_skill_urls, skill_name_from_url, ) @@ -65,26 +65,37 @@ def test_other_chip_fields_are_ignored(self): assert message_skill_urls(message) == ["skills/b/a"] -class TestCollectSkillUrls: +class TestCollectPicks: - def test_returns_conversation_picks_oldest_first(self): + 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_skill_urls(messages, 10) == ["skills/b/a", "skills/b/z"] + assert collect_picks(messages).by_ordinal == {0: "skills/b/a", 2: "skills/b/z"} - def test_a_repicked_url_moves_to_the_end(self): - messages = [_user("skills/b/a"), _user("skills/b/z"), _user("skills/b/a")] - assert collect_skill_urls(messages, 10) == ["skills/b/z", "skills/b/a"] + def test_only_the_first_chip_of_a_message_is_loaded(self): + collected = collect_picks([_user("skills/b/a", "skills/b/z")]) - def test_the_cap_drops_the_oldest_picks(self): - messages = [_user("skills/b/a"), _user("skills/b/z"), _user("skills/b/k")] - assert collect_skill_urls(messages, 2) == ["skills/b/z", "skills/b/k"] + 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_skill_urls([Message(role=Role.USER, content="hi")], 10) == [] + assert collect_picks([Message(role=Role.USER, content="hi")]).by_ordinal == {} class TestSkillNameFromUrl: From 3306b69144a1735a6deb58990b588e8807b87db5 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 16:12:39 +0300 Subject: [PATCH 6/9] docs: align the skill invocation design with the implementation #549 The design predates the simplifications made while building it, so the approved doc no longer described what shipped. A message invokes at most one skill: only the first entry is loaded, and extras are ignored - reported as a warning on the message being answered, logged on earlier turns. Picks are keyed by user-message ordinal and deduped keeping the first occurrence, so a re-pick neither refreshes the skill's position under the cap nor injects a second pair. The context holds the resolved skills by URL and a single current-pick URL, not a list of this turn's chips. The injector is a StagedToolSyntheticInjector subclass on APPEND_IF_CHANGED, which puts the pair after the first user message rather than the last, gated by should_inject instead of a call-id prefix skip check. stage_level became an overridable class attribute, so the planned public helpers in synthetic_tool_call_injector.py were never needed and that file is untouched. Three failure-mode rows were wrong as a consequence and are corrected, together with the Alternatives row that rested on the removed skip check. Claude-Session: https://claude.ai/code/session_017gdYmTNUda8ZpR81aeahCB --- docs/designs/skill_invocation.md | 189 ++++++++++++++++++------------- 1 file changed, 108 insertions(+), 81 deletions(-) diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md index d9757cc6..38cf5e3e 100644 --- a/docs/designs/skill_invocation.md +++ b/docs/designs/skill_invocation.md @@ -73,7 +73,7 @@ does for `custom_content.attachments[*]`. This design specifies how QuickApps co **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 user message. +`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. @@ -126,9 +126,9 @@ sequenceDiagram 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 chips from all user messages and resolve them + 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: chips of the last user msg become synthetic read_skill pairs + 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 @@ -164,6 +164,10 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). - 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 @@ -185,22 +189,29 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). `_InvokedSkillsContext` (concern 3). - **Owner.** `quickapp/skill_invocation/`. - **Semantics.** - 1. Walk the user messages **newest to oldest** and collect the `url`s of their `custom_content.skills`, each - canonicalised by stripping a trailing `/`. Identical chips on one message count once. 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. - 2. Deduplicate, keeping each URL's **latest** occurrence: that is what decides its position. The chips of the last - message are therefore the newest picks and are never dropped by the cap in step 3. - 3. Keep the newest `SKILL_INVOCATION_MAX_SKILLS` distinct URLs, walking newest-first so the cap drops the **oldest** - picks. 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 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 by position, oldest first, the set of URLs that resolved, and **the chips of the - message being answered**. The injector reads all three back: the first two to choose between a real result and - the error sentence, the third to know which chips belong to this turn. Recording "this turn's chips" here rather - than re-parsing the messages later means the injector does not care whether the scrub has already run. + 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 @@ -239,7 +250,7 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). | 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 5 hands it the list oldest-first | + | 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 @@ -253,9 +264,10 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). 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 produces **nothing at all** — no pair, no stage, no error. Its chip derives the same - `{"skill_name": ""}` arguments as the first pick, so the skip check in concern 4 matches the earlier - pair and skips it before anything is reported. + - 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 @@ -270,37 +282,39 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). ### 4. Injection — `_SkillInvocationInjector` -- **What.** A `MessagesTransformer` in `quickapp/skill_invocation/`. It inserts one synthetic `read_skill` call and - result per distinct chip on the last user message. +- **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.** When `_InvokedSkillsContext` recorded chips for this turn (concern 2, step 5) — **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. The - insertion point is still taken from the messages: the index after the last `USER` message. - - Earlier invocations need nothing: the pair was inserted after the user message of its turn, `Orchestrator` - persists everything after the last user message into `state.tool_execution_history`, and - `_MessagesSetup.extract_tool_calls` restores it. Acting on every historical chip would duplicate them. - - **A pair already in history is not injected again.** A chip is skipped when the conversation already holds a pair - for the same tool and arguments, matched on the call-id prefix that `make_synthetic_call_id_prefix` builds from - the tool name and arguments. It needs no content, so `read_skill` is not run for a re-pick at all: no stage is - shown for a result that would be thrown away, and a request never carries two pairs sharing a `tool_call_id`. - - The manifest the model reads therefore stays the one from the turn that first 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). - - **Where.** Directly after that user message, in chip order. The explicit index keeps the pairs ahead of any - transformer that appends to the end (`_TimestampInjectionTransformer`, `_AttachmentNotificationInjector`), - whatever the module order. - - **What.** For each chip: + - **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 chip `` is the manifest's name. For a failed one the skill has no manifest, so the URL's last + 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. @@ -308,16 +322,21 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). `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.** For a resolved chip the injector looks up the `read_skill` `StagedBaseTool` by its function name — the - **lookup only** follows `StagedToolSyntheticInjector` — and runs it through `arun`. Because the skill is - registered (concern 3), the result is byte-identical to a model-initiated call, `` included. - - > `StagedToolSyntheticInjector.get_content` forces `stage_level=StageDisplayLevel.DEBUG`, which suppresses the - > stage for a normal app. Copying that call verbatim would make UC-1's "Reading Skill" stage silently never - > appear. `arun` must be called **without** `stage_level`, so it defaults to `INFO` as it does in - > `tool_executor.py` — the invocation is something the user did, so its stage belongs in the response. - - For a failed chip the injector writes the pair itself, and there is no stage. + - **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 @@ -325,15 +344,16 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). 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 chips from the context + 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.** 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 chips of the **last** user message are - recorded as `SkillInitializationException` and shown in "Initialization issues"; problems with older picks are - logged. The model learns about a failed chip from its error result. **No path fails the request.** -- **Change.** A new transformer. `build_synthetic_pair`, `make_synthetic_call_id` and `make_synthetic_call_id_prefix` - in `common/synthetic_injection/` become public module-level helpers, so a multi-call injector can reuse them without - subclassing `SyntheticToolCallInjector` — that base class assumes one tool call per transformer. + - **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. --- @@ -345,8 +365,9 @@ which is what makes bundled files and later turns work for free (goals 4 and 7). 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 per-message cap.** One cap is enough: the conversation cap already bounds how many skills are fetched and - listed, and it is spent newest-first, so the current turn's chips are never dropped in favour of older ones. +- **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. @@ -397,7 +418,9 @@ The user sees the stage "Reading Skill: code-review", and an "Initialization iss 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 carries its own chips. +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. ### Limits @@ -418,11 +441,11 @@ name so its files stay readable, and injects nothing new unless the new message | 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. The newer chip derives identical `read_skill` arguments, so the skip check matches the older pair and the re-pick produces no pair, no stage and no error. Accepted: unreachable under the non-clashing-names assumption (concern 3) | +| 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 skill picked again later | Moves to the end of the user skills in the list; never dropped by the cap on the message that picks it; no new pair is injected and no stage is shown | +| 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 | -| Identical chips on one message | Counted once: one pair, one entry | +| 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 | @@ -478,7 +501,7 @@ then the user's only way out is to start a new conversation. | **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") | Does not actually surface it: both picks derive the same `{"skill_name": ""}` arguments, so the skip check still matches the earlier pair. Bypassing the skip would write a "could not be loaded" pair directly after one that loaded the same name fine — contradictory history for the model, for a case the non-clashing-names assumption rules out | +| **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 | @@ -528,15 +551,18 @@ preview-gated. ### `quickapp/skill_invocation/` (new) -- `_skill_reference.py` — `SkillReference(url)`; parses `custom_content.model_extra["skills"]` from user messages, - canonicalises the URL, dedupes keeping the latest occurrence, applies the conversation cap. +- `_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, with one header line prepended to `content`, - the set of URLs that resolved, this turn's chips, and the exceptions for them. -- `_skill_invocation_initializer.py` — `_SkillInvocationInitializer(CompletionInitializer)`: collect newest first, - dedupe, cap, delegate to `DialSkillResolver`. -- `_skill_invocation_injector.py` — `_SkillInvocationInjector(MessagesTransformer)`: synthetic `read_skill` pairs for - this turn's chips via the real tool, a fixed error result for failed ones, failure reporting. No scrubbing. + `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`. @@ -544,8 +570,9 @@ preview-gated. ### `quickapp/common/` - `_di_types.py` — `REQUEST_MESSAGES`. -- `synthetic_injection/synthetic_tool_call_injector.py` — `build_synthetic_pair`, `make_synthetic_call_id` and - `make_synthetic_call_id_prefix` as public module-level helpers; the class keeps using them. +- `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/` From 09f2175e32bfc3bb0158eb7bf4dd87132e7bc225 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 17:33:16 +0300 Subject: [PATCH 7/9] fix(skills): report skill issues that belong to a message, not a URL #549 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SkillInitializationException built without a `url` fell through the renderer's unhandled branch after the stage had already opened, so invoking more than one skill on a message produced an empty "Initialization issues" stage with no explanation. The exception class already formats for `url=None`; only the renderer did not. The same guard silently dropped the url-less resolver exceptions that `_SkillInvocationInitializer.__report` deliberately forwards. Alongside it, three conventions fixes and a correction to the docs: - Import the skill-reader tool name from `common.tool_names` rather than the private `skills._tool_configs`; it is a literal alias, and this was the only cross-package private import in the app tree. - Drop the per-turn skip log to DEBUG and name the skill, and drop the multi-chip log to DEBUG — both re-fired on every turn for the life of a conversation because each turn re-parses the whole message list. - Guard `_InvokedSkillsContext` mutators with a lock, as both sibling skill contexts already do. - State the real persistence rule in the injector docstring and docs/skills.md: only a pick made on the first user message is stored in `state.tool_execution_history`, because the orchestrator persists just what follows the last user message. A pick made later is in context for its own turn only; the skill stays listed and readable either way. Recorded as a known gap for phase 1a. The design doc gains the acknowledged stale-manifest gap: the shared `SyntheticToolCallInjector` appends rather than replaces when a pair's content changed, so editing a skill between turns can leave two manifests under one name. --- docs/designs/skill_invocation.md | 17 +++++++++++ docs/skills.md | 10 +++++-- .../_initialization_error_handler.py | 6 ++-- .../_invoked_skills_context.py | 9 ++++-- .../_skill_invocation_initializer.py | 5 +++- .../_skill_invocation_injector.py | 21 ++++++++++---- .../skill_invocation/_skill_reference.py | 4 ++- .../test_initialization_error_handler.py | 29 +++++++++++++++++++ .../test_skill_invocation_injector.py | 4 +-- 9 files changed, 89 insertions(+), 16 deletions(-) diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md index 38cf5e3e..77e128b8 100644 --- a/docs/designs/skill_invocation.md +++ b/docs/designs/skill_invocation.md @@ -490,6 +490,23 @@ QuickApps can't work around this, because the request never reaches it. The fix 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 diff --git a/docs/skills.md b/docs/skills.md index 540216bc..ef2f2d24 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -333,8 +333,14 @@ the rest of the conversation. The model does not get to choose — a picked skil 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. On later turns the pair comes back from - the assistant state like any other tool result, so `read_skill` is never re-run for it. +- 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 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/skill_invocation/_invoked_skills_context.py b/src/quickapp/skill_invocation/_invoked_skills_context.py index 5657b629..b48636e4 100644 --- a/src/quickapp/skill_invocation/_invoked_skills_context.py +++ b/src/quickapp/skill_invocation/_invoked_skills_context.py @@ -1,3 +1,5 @@ +import threading + from quickapp.common.exceptions import InitializationException, SkillInitializationException from quickapp.skills import ResolvedSkill, SkillsProvider @@ -32,6 +34,7 @@ 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]: @@ -54,11 +57,13 @@ def set_current_pick_url(self, url: str | None) -> None: self._current_pick_url = url def set_resolved_skills(self, skills: list[ResolvedSkill]) -> None: - self._skills_by_url = {skill.url: _mark_user_selected(skill) for skill in skills} + 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: - self._exceptions.append(exception) + with self._lock: + self._exceptions.append(exception) diff --git a/src/quickapp/skill_invocation/_skill_invocation_initializer.py b/src/quickapp/skill_invocation/_skill_invocation_initializer.py index 4747c405..97ff31cb 100644 --- a/src/quickapp/skill_invocation/_skill_invocation_initializer.py +++ b/src/quickapp/skill_invocation/_skill_invocation_initializer.py @@ -105,4 +105,7 @@ def __report(self, exceptions: list[SkillInitializationException]) -> None: if exception.url is None or exception.url == current: self._context.append_exception(exception) else: - logger.info("A skill picked on an earlier turn could not be loaded") + 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 index 1190d040..598c069c 100644 --- a/src/quickapp/skill_invocation/_skill_invocation_injector.py +++ b/src/quickapp/skill_invocation/_skill_invocation_injector.py @@ -9,11 +9,11 @@ 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 -from quickapp.skills._tool_configs import SKILL_READER_TOOL_NAME logger = logging.getLogger(__name__) @@ -35,10 +35,19 @@ class _SkillInvocationInjector(StagedToolSyntheticInjector): "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. It runs only on the turn the pick is made - (``should_inject``); afterwards the pair is restored from - ``state.tool_execution_history`` like any other tool result, so the model keeps the - manifest it first saw and ``read_skill`` is never re-run for it. + 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 @@ -57,7 +66,7 @@ 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 SKILL_READER_TOOL_NAME + return INTERNAL_SKILLS_READ_SKILL_TOOL_NAME async def get_frequency(self, messages: list[Message]) -> InjectionFrequency: return InjectionFrequency.APPEND_IF_CHANGED diff --git a/src/quickapp/skill_invocation/_skill_reference.py b/src/quickapp/skill_invocation/_skill_reference.py index 027dedd5..d49c7d0b 100644 --- a/src/quickapp/skill_invocation/_skill_reference.py +++ b/src/quickapp/skill_invocation/_skill_reference.py @@ -75,7 +75,9 @@ def collect_picks(messages: list[Message]) -> ConversationPicks: continue if len(urls) > 1: ignored_by_ordinal[ordinal] = urls[1:] - logger.warning( + # 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: 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/skill_invocation_tests/test_skill_invocation_injector.py b/src/tests/unit_tests/skill_invocation_tests/test_skill_invocation_injector.py index 48c7e4fa..f344cb22 100644 --- 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 @@ -8,10 +8,10 @@ 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 quickapp.skills._tool_configs import SKILL_READER_TOOL_NAME from tests.unit_tests.common.common import make_resolved_skill as _skill _URL = "skills/b/code-review" @@ -20,7 +20,7 @@ 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 = SKILL_READER_TOOL_NAME + 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") ) From 34f5fed055da8a84cccc6dc2a59aa030b9e041bf Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 17:38:02 +0300 Subject: [PATCH 8/9] docs: mark the skill invocation design as implemented #549 --- docs/designs/skill_invocation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md index 77e128b8..6076b112 100644 --- a/docs/designs/skill_invocation.md +++ b/docs/designs/skill_invocation.md @@ -1,6 +1,6 @@ # Design: Invoking a Skill from a Message -- **Status:** Approved +- **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 From ee0165d6dc025a60cbdd36016ad1ee4b11e85317 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 14 Sep 2026 17:52:03 +0300 Subject: [PATCH 9/9] docs: reconcile skill invocation design with implementation Update docs/designs/skill_invocation.md to match the current repo state: - skills_as_dial_resource.md's dependency (feat/418) is merged to development (#524), not pending - document the known gap where a skill picked on a non-first user message doesn't survive the next turn, matching what docs/skills.md and the injector's docstring already say #549 --- docs/designs/skill_invocation.md | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md index 6076b112..c0348ecb 100644 --- a/docs/designs/skill_invocation.md +++ b/docs/designs/skill_invocation.md @@ -10,7 +10,7 @@ ([Follow-up](#follow-up-phase-1b--collision-free-names)). - **Dependencies:** - [`skills_as_dial_resource.md`](skills_as_dial_resource.md) — `DialSkillResolver`, `DialSkillReader`, the - `` inventory. Branch `feat/418-skills-as-dial-resource`, not yet on `development`. + `` 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 @@ -422,6 +422,9 @@ name so its files stay readable, and injects nothing new unless the new message 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 | @@ -449,11 +452,31 @@ the turn-1 pair — so the manifests sit together at the head of the conversatio | 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