diff --git a/CLAUDE.md b/CLAUDE.md index 3d8c4c7f..1b00156c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,9 +81,11 @@ per-app `features.external_url_fetch.enabled` field. The deployment-attachment p ### Skills -Skills are reusable instruction modules. Predefined skills are loaded at startup from `config/predefined/skills/`. -DIAL prompt skills (`dial_prompt_skills/`) are fetched at request time from DIAL Core's prompts API. -`SkillsRegistry` merges both sources per request. +Skills are reusable instruction modules. Three sources: predefined skills loaded at startup from +`config/predefined/skills/`; DIAL prompt skills (`dial_prompt_skills/`) fetched per request from Core's prompts API; +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). ### Configuration Model diff --git a/README.md b/README.md index d72c23a8..d1c78db9 100644 --- a/README.md +++ b/README.md @@ -168,6 +168,10 @@ Controls which tool-execution stages are surfaced in the DIAL UI for each app. S | `EXTERNAL_URL_FETCH_HOST_ALLOWLIST` | — | No | Comma-separated allowlist of host patterns for external URL fetches. Unset (default) means no admin-level host restriction. Patterns: exact host (`example.com`) or `*.example.com` for any subdomain. Re-checked on every redirect hop. Per-app `features.external_url_fetch.host_allowlist` narrows further (intersection) but never expands. | | `EXTERNAL_URL_FETCH_MAX_REDIRECTS` | `5` | No | Maximum HTTP redirects on external URL fetches. Each hop is SSRF-checked. Hard ceiling 10. | | `EXTERNAL_URL_FETCH_CONNECT_TIMEOUT_SECONDS` | `5.0` | No | TCP connect timeout (seconds) for external URL fetches. Read/write/pool timeouts use the resolved tool timeout. | +| **Skills** | | | | +| `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 | | **Feature Gating** | | | | | `ENABLE_PREVIEW_FEATURES` | `false` | No | Enable preview features across the deployment (schema visibility + runtime activation) | | **Templates** | | | | diff --git a/docs/designs/skill_invocation.md b/docs/designs/skill_invocation.md new file mode 100644 index 00000000..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. diff --git a/docs/designs/skills_as_dial_resource.md b/docs/designs/skills_as_dial_resource.md new file mode 100644 index 00000000..8be07951 --- /dev/null +++ b/docs/designs/skills_as_dial_resource.md @@ -0,0 +1,494 @@ +# Design: Skills as DIAL Resource (Phase 1) + +- **Status:** Approved +- **Approved:** 2026-08-31 +- **Issue:** [epam/ai-dial-quickapps-backend#418](https://github.com/epam/ai-dial-quickapps-backend/issues/418) +- **Dependencies:** + - [epam/ai-dial-core#1633](https://github.com/epam/ai-dial-core/issues/1633) — folder-as-resource / `/v2/skills`. All + child issues are closed; the read path this design uses is live. + - `ai-dial-client-python` `feat/skills-read` — the `client.skills` resource. **Written, not merged, not released.** + - Prior art: [`dial_prompts_as_skills.md`](dial_prompts_as_skills.md), [`skills_and_file_transfer.md`](skills_and_file_transfer.md) + +## Problem Statement + +DIAL Core now stores skills as folder-shaped resources: a mandatory `SKILL.md` manifest plus an arbitrary file +hierarchy (`references/`, `scripts/`, `assets/` — the [Agent Skills spec](https://agentskills.io/specification)), +served through `/v2/skills`. + +QuickApps cannot consume them at all. The only user-configurable skill source is `dial-prompt`, a single DIAL prompt +blob (`prompts//`), and `read_skill` returns exactly one string per skill. Consequences today: + +- A skill authored in DIAL Chat as a skill resource **cannot be referenced from a QuickApp config**. There is no + config type for it. +- Progressive disclosure — the mechanism the spec is built around, where `SKILL.md` stays small and points at + bundled reference files the agent opens on demand — **does not exist**. A `dial-prompt` skill that says + "see `references/api-schema.md`" silently degrades: the agent has no way to open it. +- `docs/skills.md` documents both of these as "Not supported". + +## Design Goals + +1. An app config can reference a DIAL skill resource by URL (`skills//`) and the agent sees it in + `` exactly like any other skill. +2. The agent can read the manifest **and** any bundled text file of that skill, on demand, one round-trip per file. +3. The agent discovers which files exist without guessing — the file list is part of what it gets back when it + reads the manifest. +4. Nothing about predefined skills or `dial-prompt` skills changes. No behavior regression, no refactoring of + either. +5. A broken, oversized, or inaccessible skill degrades to a reported initialization issue; the request is still + served. + +--- + +## Phasing + +The issue asks for three things: load the whole folder, progressive disclosure, and metadata-listing/etag-driven +caching. This document designs and commits to **Phase 1 only**; the rest is named here so the seams are deliberate, +not so it is promised. + +### Phase 1 — this design + +| In | Out | +|---|---| +| New `dial-skill` config type over `skills//` | Predefined skills — untouched, still flat `SKILL.md` | +| Manifest + **text-file inventory** resolved per request | `dial-prompt` — untouched, not deprecated yet | +| `read_skill(skill_name, file_path?)` for progressive disclosure | Binary / non-text bundled files | +| Docs + schema regeneration | Cross-request caching, etag probes | +| | Any unification of the three skill sources into one model | +| | A QuickApps-side validator for `dial-skill` — Core validates on write | + +Deliberate non-goal: **no refactoring.** `dial_skills/` is a new package that mirrors the existing +`dial_prompt_skills/` shape. `SkillsRegistry` gains one more source and one async method. Nothing else is rewritten. + +### Phase 2 — follow-ups (separate issues) + +- **Binary/asset files.** Today a non-text file is neither advertised nor readable. Serving one means returning an + attachment rather than a string — a different contract for `read_skill`, and it needs the file-transfer path. +- **Caching.** Resolution is per-request, same as `dial-prompt` today. A cheap "did this skill change" probe is + blocked on Core (see [C-2](#c-2--no-cheap-aggregate-etag-probe)). +- **`read_skill` and offload.** A large file read can be swallowed by `tool_call_result_offload` and handed back to + the model as a pointer. Add `internal_skills_read_skill` to the mandatory offload exclusions and cap the result. +- **Skill browsing for the editor.** `GET /skills` lists predefined skills only. Listing a user's DIAL skills is + blocked on Core (see [C-3](#c-3--children-listing-carries-no-skill-metadata)). + +### Later + +Predefined skills as folder skills (one `Skill` model across all three sources), `dial-prompt` deprecation, +`scripts/` execution, `allowed-tools` enforcement. + +--- + +## Use Cases + +### UC-1: Reference a DIAL skill resource from an app config + +**Trigger:** A builder adds `{"type": "dial-skill", "url": "skills//refund-policy"}` to `skills`. +**Behavior:** At request initialization QuickApps reads the skill's `SKILL.md` and lists its text files. +**Outcome:** `refund-policy` appears in `` with its name and description, indistinguishable from a +predefined skill. + +### UC-2: Agent reads the manifest and sees what else is there + +**Trigger:** The model calls `read_skill(skill_name="refund-policy")`. +**Behavior:** QuickApps returns the manifest body, followed by a `` block listing the skill's readable +files by path. +**Outcome:** The model knows `references/eu-rules.md` exists without the manifest having to spell out a URL scheme. + +### UC-3: Agent opens a bundled file + +**Trigger:** The model calls `read_skill(skill_name="refund-policy", file_path="references/eu-rules.md")`. +**Behavior:** One `GET /v2/skills/{bucket}/{path}/files/references/eu-rules.md` against Core. +**Outcome:** The file's text, in the tool result. Nothing was fetched that the model did not ask for. + +### UC-4: Broken or inaccessible skill + +**Trigger:** The configured URL 403s (see [C-1](#c-1--config-declared-skills-are-not-auto-shared-blocking-for-the-headline-use-case)), or its +`SKILL.md` has no frontmatter. +**Behavior:** The skill is dropped; a `SkillInitializationException` is recorded. +**Outcome:** The "Initialization issues" stage names the URL and the reason. Every other skill and the request itself +are unaffected. + +--- + +## Proposed Design + +```mermaid +sequenceDiagram + participant I as _DialSkillInitializer + participant R as _DialSkillResolver + participant C as AsyncDial.skills + participant Reg as SkillsRegistry + participant T as read_skill tool + + Note over I,C: initialization phase — once per request, per configured skill + I->>R: resolve([DialSkillConfig]) + R->>C: get_file(url, "SKILL.md") + C-->>R: manifest bytes + R->>C: list_files(url, recursive=True) + C-->>R: file items (paged) + R-->>I: ResolvedDialSkill(metadata, manifest, inventory) + + Note over Reg: prompt-building phase — pure in-memory merge + Reg->>Reg: predefined + dial-prompt + dial-skill → available_skills XML + + Note over T,C: orchestrator loop — on demand + T->>Reg: read_skill_file(name, "references/eu-rules.md") + Reg->>C: get_file(url, "references/eu-rules.md") + C-->>T: text +``` + +### 1. Config: a new `dial-skill` union member + +**What.** `DialSkillConfig` joins `SkillConfig`'s discriminated union in `quickapp/config/skill.py`: + +```python +class DialSkillConfig(BaseModel): + type: Literal["dial-skill"] = "dial-skill" + url: Annotated[str, DialResourceConfigField( + description="Relative skill resource URL in DIAL (e.g. skills//)" + )] + +SkillConfig = Annotated[DialPromptSkillConfig | DialSkillConfig, Field(discriminator="type")] +``` + +**Semantics.** `DialResourceConfigField` tags the URL with `dial:resource` so Core's collector sees it — the same +annotation `dial-prompt` carries. See [C-1](#c-1--config-declared-skills-are-not-auto-shared-blocking-for-the-headline-use-case) for what Core +does (and does not) do with it today. + +**Change.** `SkillConfig` goes from a one-member `Annotated[...]` to a real union. `make dump_app_schema` +regenerates `docs/generated-app-schema.json` and `docs/generated-config-support-openapi.json`. + +**Not preview-gated.** `dial-prompt` is not, and gating one *member* of a union is not expressible with the +field-level `x-preview` marker the schema generator understands — it would take either a schema post-process or a +second union, both of which cost more than the feature. The code path is dormant unless a config declares a +`dial-skill`. If we later decide the Core gap makes it too sharp an edge, `@preview_module` on `DialSkillsModule` is +a one-line change that stops resolution (at the cost of ignoring such entries silently). + +### 2. `quickapp/dial_skills/` — a new package mirroring `dial_prompt_skills/` + +Four components, each the direct analogue of its `dial_prompt_skills/` counterpart. This is copy-shaped on purpose: +the lifecycle is identical, and sharing it would mean refactoring the prompt path. + +| Component | Responsibility | +|---|---| +| `_DialSkillResolver` (request-scoped) | Dedup by URL → fetch in parallel → parse frontmatter → list files → dedup by name. Per-URL failures become `SkillInitializationException`. | +| `_DialSkillsContext` (request-scoped) | Holds `resolved_skills` and `exceptions`. Lock-guarded, like `_DialPromptSkillsContext`. | +| `_DialSkillInitializer` (`CompletionInitializer`) | Filters `ApplicationConfig.skills` to `DialSkillConfig`, calls the resolver, pushes results into the context. Resolver-level blowups become `SkillCatastrophicInitializationException`. | +| `DialSkillsModule` | Binds the three; contributes the initializer and the context's exceptions to the existing multiproviders. | + +`ResolvedDialSkill` is a frozen model: `url`, `metadata: SkillMetadata`, `manifest: str`, `files: tuple[str, ...]`, +`warnings: list[str]`. + +**Per-skill cost at initialization:** one `get_file(url, "SKILL.md")` plus one `list_files(url, recursive=True)` +(plus continuation pages). Both run concurrently across skills via `asyncio.gather(return_exceptions=True)`, exactly +as the prompt resolver does. + +### 3. The inventory: what a skill advertises + +`list_files(url, recursive=True)` returns items whose `url` is `skills///files/`; folders are +distinguished by a trailing `/` (**not** by `node_type` — Core reports every entry as `ITEM`; the client's +`SkillFileItem` docstring records this). Relative paths come from stripping the `/files/` prefix and +percent-decoding. + +An entry is advertised only if **all** of these hold: + +1. It is not a folder (no trailing `/`). +2. No path segment starts with `.` — hidden files and the `.dial-resource` marker never reach the model. +3. Its extension is in the text allowlist: `.md .markdown .txt .json .yaml .yml .csv .tsv .xml .html .toml .ini .sql .py .sh .js .ts`. +4. It is not `SKILL.md` — that is what `read_skill(skill_name)` already returns. + +**One rule, two places.** The same allowlist decides what is *listed* and what is *readable*, and readability is +enforced by inventory membership: `read_skill` serves a `file_path` **only if it is in that skill's inventory**. +That single check subsumes path traversal, encoded-separator smuggling, and dotfile leakage without a bespoke +validator — the model can only ask for what we told it exists. (The client also rejects `.`/`..` and encoded +separators before a request is built, so this is belt and braces.) + +Non-text files are invisible in Phase 1. The manifest may still mention them in prose; a `read_skill` for one comes +back as "not available", with the inventory repeated as a hint. + +### 4. `SkillsRegistry`: one more source, one new method + +**Change (merge).** The registry takes an optional `_DialSkillsContext` alongside the existing optional +`_DialPromptSkillsContext`, and its merge loop grows a third pass. Precedence is unchanged in spirit and made +explicit: **predefined > dial-prompt > dial-skill**, and within a source, first-configured wins. A losing skill is +reported as a `SkillInitializationException` on its own context, the way prompt collisions already are. + +**Change (routing).** The registry gains: + +```python +async def read_skill_file(self, skill_name: str, file_path: str) -> str +``` + +It looks up a per-skill *file reader* registered by the dial-skill source. Predefined and `dial-prompt` skills +register none — for them the method raises the "this skill has no bundled files" error. `get_skill_content` stays +synchronous and unchanged; the manifest string it returns for a dial-skill has the `` block appended at +resolve time. + +Reads are memoized in the request-scoped context, so a model that opens the same reference twice in one +conversation pays one round-trip. + +### 5. `read_skill(skill_name, file_path?)` + +**What.** One optional parameter added to `SKILL_READER_TOOL_CONFIG`: + +> `file_path` — *(optional)* Path of a bundled file to read, relative to the skill root, exactly as listed in the +> skill's `` block (e.g. `references/eu-rules.md`). Omit to read the skill's instructions. + +**Semantics.** + +| Call | Result | +|---|---| +| `read_skill(name)` | Manifest. For a dial-skill, followed by `` when the skill has any. | +| `read_skill(name, "references/x.md")` | That file's text. | +| `read_skill(name, "SKILL.md")` | The manifest — accepted rather than treated as an error. | +| `file_path` not in the inventory | `Error: ... is not available in skill 'name'.` plus the inventory, so the model can correct itself in the next turn. | +| Skill has no bundled files | `Error: skill 'name' has no bundled files.` | + +**Why one tool and not two.** A separate `read_skill_file` tool costs a second tool slot in every request's tool +list for a strictly narrower capability, and splits "reading a skill" across two names the model has to choose +between. Extending the existing tool keeps the prompt surface flat. + +**Stage display.** `_SkillReaderStageWrapper` shows the manifest today. It gets the file path in the stage title +when one is present — a two-line change, no new wrapper. + +### 6. Limits + +`DialSkillsSettings` (`pydantic-settings`, module-local, per `CODESTYLE.md`): + +| Env var | Default | Purpose | +|---|---|---| +| `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` (256 KiB) | Cap on any single fetched file, manifest included. Enforced **after** the response arrives — Core's listing carries no size ([C-4](#c-4--file-listing-carries-no-size)). An over-cap manifest drops the skill with a reported reason; an over-cap file read returns an error to the model. | +| `DIAL_SKILLS_MAX_FILES` | `200` | Inventory ceiling per skill. Beyond it the listing stops and the block ends with a truncation note. | +| `DIAL_SKILLS_LISTING_MAX_PAGES` | `10` | Pagination ceiling. Follow `next_token` at most this many times, and stop on a repeated token — a stuck cursor must not hang initialization. | + +Decoding is strict UTF-8; a decode failure is reported as "not a text file" rather than mangled into the context. + +### 7. `/skills/validate` stays `dial-prompt`-only + +**No QuickApps-side validator for `dial-skill`.** A DIAL skill resource is created and validated by Core: the +`/v2/skills` write path enforces a mandatory `SKILL.md` with parseable frontmatter server-side +([#1633](https://github.com/epam/ai-dial-core/issues/1633), requirement 6). A stored skill is already valid, so +re-checking it at config time would duplicate Core's rule and risk drifting from it. `dial-prompt` keeps its +validator because a prompt is an arbitrary text blob that Core knows nothing about. + +**But the endpoint's request type must be narrowed**, or widening the union ships a bug. The handler is annotated +`config: SkillConfig`, so widening the union regenerates the config-support OpenAPI to advertise `dial-skill` on +`/skills/validate` — a request the handler's `else` branch answers `400 Unsupported skill type`. Pinning the +annotation to `DialPromptSkillConfig` keeps the schema honest about what the endpoint accepts, and lets the now +unreachable `isinstance` check and its `400` branch go with it. A `dial-skill` payload then fails the `type` +literal and gets a `422` from FastAPI, matching the published schema. + +The editor should not offer Validate for a `dial-skill` at all — that is part of the `ai-dial-chat` work in +[C-5](#c-5--dial-chat-editor). + +--- + +## Alternatives Considered + +**A-1 — Download the whole skill as a ZIP once per request.** `client.skills.download()` is a single round-trip and +removes pagination, per-file fetches, and the inventory listing. Rejected: it pays the full byte cost of every +bundled asset on every request even when the model opens nothing, with no way to check size before committing to the +download, and it is the opposite of progressive disclosure. + +**A-2 — Expose bundled files through the existing `file:` reference scheme.** Would reuse `FileLoaderService` and +need no tool change. Rejected: skill-relative paths are not DIAL file URLs, and the resolution would have to be +scoped per skill to stay contained — more machinery than a `file_path` parameter. + +**A-3 — Populate the registry from Core's metadata listing instead of reading each manifest.** Would remove one +round-trip per skill. Blocked: the listing carries no name/description ([C-3](#c-3--children-listing-carries-no-skill-metadata)). + +**A-4 — Decode-sniff instead of an extension allowlist.** Attempt UTF-8, treat failure as binary. Rejected for the +*listing* (it would require fetching every file to decide what to advertise); kept as a secondary guard on read. + +--- + +## Dependencies and Known Gaps + +### D-1 — `ai-dial-client-python` `client.skills` is unreleased (**blocking**) + +The `feat/skills-read` branch adds `AsyncSkills` with `get_metadata`, `list_files`, `get_file`, `stream_file`, and +`download`, plus `my_skills_home()` and the `SkillMetadata` / `SkillFileMetadata` types. Everything Phase 1 needs is +there. It must be merged and released before this can ship; `pyproject.toml` then moves from +`aidial-client (>=0.16.0,<0.17.0)` to the release that carries it. **No vendored copy and no git dependency** — a +pinned git ref in `pyproject.toml` is not something we want in a release build. + +### C-1 — Config-declared skills are not auto-shared (**blocking for the headline use case**) + +Verified against `ai-dial-core@development`: `ApplicationSchemaService` exposes `getFiles` / `getPrompts` / +`getDeployments` and no `getSkills`; `BaseRequestFunction` has no `shareApplicationSkills`; `ApiKeyData` has no +`attachedSkills`. A `skills/...` URL tagged `dial:resource` is collected and then dropped, so the app's per-request +key gets `403`. + +Until Core closes this, a `dial-skill` works only when the **caller's own key already has access**: a skill in the +user's own bucket, a skill shared with them, or a published skill. That covers "I authored a skill in Chat and want +my QuickApp to use it" — the primary Phase 1 scenario — and does not cover "the app ships with a skill from the +builder's bucket". + +**Action:** file this against `ai-dial-core` (needs `getSkills`, `shareApplicationSkills`, `ApiKeyData.attachedSkills` ++ its `AccessService.getAutoSharedAccess` branch, and a marker-aware existence check, since a skill URL has no blob +of its own). Nothing on the QuickApps side changes when it lands. + +### C-2 — No cheap aggregate-etag probe + +The `.dial-resource` marker carries an aggregate etag bumped on every mutation, but no read path returns it cheaply: +`listChildren` omits it deliberately, the single-file GET returns the individual blob's etag, and only the ZIP +download exposes the aggregate. A `HEAD /v2/skills/{bucket}/{path}` would make Phase 2 caching trivial. + +### C-3 — Children listing carries no skill metadata + +`nodeMetadata` sets node type, timestamps, and author — not the marker's cached name/description/version. Browsing +skills therefore costs one manifest read each. Does not affect Phase 1 (configs address skills by URL); gates any +"pick a skill from a list" editor UX. + +### C-4 — File listing carries no size + +`ResourceItemMetadata` has no size field, so `DIAL_SKILLS_FILE_MAX_BYTES` can only be enforced after the response +arrives, and the inventory cannot tell the model which references are cheap to open. + +### C-5 — DIAL Chat editor + +Skill authoring in Chat is live enough to have its own bug reports (e.g. `epam/ai-dial-core#1861`), so Phase 1 has a +real authoring surface to consume. Referencing a skill from a QuickApp config still needs editor support for the new +`dial-skill` type — a separate `ai-dial-chat` issue. + +--- + +## Out of Scope + +| Deferred | Why | What it would need | +|---|---|---| +| Predefined skills as folders | Phase 1 must not touch them; today they are flat `SKILL.md` files read at startup | A lazy disk-backed reader and one `Skill` model across sources | +| Binary / asset files | Different result contract (attachment, not string) | File-transfer integration in `read_skill` | +| Cross-request caching | Per-request resolution matches `dial-prompt` today | [C-2](#c-2--no-cheap-aggregate-etag-probe) | +| `dial-prompt` deprecation | Nothing forces it yet; both can coexist | A migration note once `dial-skill` has soaked | +| `scripts/` execution, `allowed-tools` enforcement | Neither is supported for any skill source today | Separate design | +| A `dial-skill` branch in `/skills/validate` | Core validates `SKILL.md` on write; duplicating that rule invites drift | Nothing — the editor should not offer Validate for this type | +| Listing a user's DIAL skills in the editor | Not needed to reference one by URL | [C-3](#c-3--children-listing-carries-no-skill-metadata) | + +--- + +## Configuration / Usage Examples + +### Config + +```json +{ + "skills": [ + { "type": "dial-prompt", "url": "prompts/my-bucket/skills/tone-of-voice" }, + { "type": "dial-skill", "url": "skills/my-bucket/refund-policy" } + ] +} +``` + +### Skill layout in DIAL + +``` +skills/my-bucket/refund-policy/ +├── SKILL.md ← manifest, always read +├── references/ +│ ├── eu-rules.md ← advertised, readable +│ └── us-rules.md ← advertised, readable +├── assets/logo.png ← not advertised in Phase 1 +└── .dial-resource ← Core's marker, never advertised +``` + +### What the agent sees + +System prompt (``) — unchanged shape: + +```xml + + refund-policy + How to handle refund requests, by region. + +``` + +`read_skill(skill_name="refund-policy")`: + +```markdown +--- +name: refund-policy +description: How to handle refund requests, by region. +--- + +# Refund Policy + +Determine the customer's region, then read the matching reference file. + + +references/eu-rules.md +references/us-rules.md + +``` + +`read_skill(skill_name="refund-policy", file_path="references/eu-rules.md")` → that file's text. + +### Failure modes + +| Situation | Where it surfaces | Effect | +|---|---|---| +| URL 403/404 | Initialization issues stage, with the URL | Skill dropped, request served | +| `SKILL.md` missing or unparseable frontmatter | Initialization issues stage | Skill dropped | +| Manifest over `DIAL_SKILLS_FILE_MAX_BYTES` | Initialization issues stage | Skill dropped | +| Name collides with a predefined skill | Initialization issues stage | Predefined wins | +| `file_path` not in the inventory | Tool result | Error + inventory reprinted | +| File over cap, or not valid UTF-8 | Tool result | Error, other files still readable | +| Core unreachable during initialization | Initialization issues stage (catastrophic) | All dial-skills dropped, request served | + +--- + +## Migration + +### Breaking changes + +None. `dial-skill` is additive; `dial-prompt` and predefined skills behave exactly as before, and the `read_skill` +signature is backward compatible (`file_path` optional). + +### Non-breaking changes + +- `SkillConfig` becomes a two-member union — a config with only `dial-prompt` entries validates identically. +- `read_skill`'s description and parameters change, so the model sees a slightly different tool schema. +- `/skills/validate` publishes a narrower request schema (`DialPromptSkillConfig` instead of `SkillConfig`), which + is what it has always actually accepted. A `dial-skill` payload now fails as `422` rather than `400`; no caller + could have been sending one before, since the type did not exist. + +--- + +## Summary of Changes + +### `quickapp/config/` + +| Change | Detail | +|---|---| +| Add `DialSkillConfig` | `type: "dial-skill"`, `url` tagged `DialResourceConfigField` | +| Widen `SkillConfig` | `DialPromptSkillConfig \| DialSkillConfig`, discriminated on `type` | + +### `quickapp/dial_skills/` (new) + +| File | Contents | +|---|---| +| `_dial_skill_resolver.py` | `_DialSkillResolver`, `ResolvedDialSkill`, resolver output model | +| `_dial_skills_context.py` | `_DialSkillsContext` — resolved skills, exceptions, per-request file-read memo | +| `_dial_skill_initializer.py` | `_DialSkillInitializer(CompletionInitializer)` | +| `_dial_skills_client.py` | Thin wrapper over `client.skills`: manifest fetch, paged inventory, single-file read, limits | +| `_settings.py` | `DialSkillsSettings` — `DIAL_SKILLS_FILE_MAX_BYTES`, `DIAL_SKILLS_MAX_FILES`, `DIAL_SKILLS_LISTING_MAX_PAGES` | +| `dial_skills_module.py` | `DialSkillsModule` — bindings, initializer and exception multiproviders | + +### `quickapp/skills/` (minimal deltas, no refactoring) + +| Component | Change | +|---|---| +| `SkillsRegistry` | Optional `_DialSkillsContext`; third merge pass with explicit precedence; new `async read_skill_file` | +| `_SkillReaderTool` | Optional `file_path` argument, routed to `read_skill_file` | +| `_tool_configs.py` | `file_path` parameter added to the tool schema; description updated | +| `_skill_reader_stage_wrapper.py` | Show `file_path` in the stage title when present | + +### Cross-cutting + +| Item | Change | +|---|---| +| `app_factory.py` | Register `DialSkillsModule` | +| `configuration_support/_controller.py` | Narrow `validate_skill` to `DialPromptSkillConfig`; drop the dead `isinstance` check and its `400` branch | +| `pyproject.toml` | Bump `aidial-client` to the release carrying `client.skills` | +| `docs/generated-*.json` | Regenerated via `make dump_app_schema` | +| `docs/skills.md` | New "DIAL Skill Resources" section; flip the two "Not supported" rows for the dial-skill source | +| `CLAUDE.md` | Name the third skill source | diff --git a/docs/generated-app-schema.json b/docs/generated-app-schema.json index 9e2b66c7..147d45d8 100644 --- a/docs/generated-app-schema.json +++ b/docs/generated-app-schema.json @@ -1329,6 +1329,29 @@ "title": "DialPromptSkillConfig", "type": "object" }, + "DialSkillConfig": { + "properties": { + "type": { + "const": "dial-skill", + "default": "dial-skill", + "description": "Skill sourced from a DIAL skill resource (folder with SKILL.md).", + "title": "Type", + "type": "string" + }, + "url": { + "description": "Relative skill resource URL in DIAL (e.g. skills//)", + "dial:resource": true, + "title": "Url", + "type": "string" + } + }, + "required": [ + "url" + ], + "title": "DialSkillConfig", + "type": "object", + "x-preview": true + }, "DialSystemPromptConfig": { "properties": { "type": { @@ -3743,13 +3766,17 @@ "items": { "discriminator": { "mapping": { - "dial-prompt": "#/$defs/DialPromptSkillConfig" + "dial-prompt": "#/$defs/DialPromptSkillConfig", + "dial-skill": "#/$defs/DialSkillConfig" }, "propertyName": "type" }, "oneOf": [ { "$ref": "#/$defs/DialPromptSkillConfig" + }, + { + "$ref": "#/$defs/DialSkillConfig" } ] }, diff --git a/docs/generated-config-support-openapi.json b/docs/generated-config-support-openapi.json index cb957174..86714160 100644 --- a/docs/generated-config-support-openapi.json +++ b/docs/generated-config-support-openapi.json @@ -71,18 +71,7 @@ "content": { "application/json": { "schema": { - "oneOf": [ - { - "$ref": "#/components/schemas/DialPromptSkillConfig" - } - ], - "title": "Config", - "discriminator": { - "propertyName": "type", - "mapping": { - "dial-prompt": "#/components/schemas/DialPromptSkillConfig" - } - } + "$ref": "#/components/schemas/DialPromptSkillConfig" } } }, diff --git a/docs/generated-internal-tools.json b/docs/generated-internal-tools.json index 2535d566..383c187c 100644 --- a/docs/generated-internal-tools.json +++ b/docs/generated-internal-tools.json @@ -357,6 +357,18 @@ }, "type": "string", "description": "The name of the skill to read. This should match the name from the available_skills list." + }, + "file_path": { + "display": { + "stage": { + "ignore": true, + "ignore_parameter_name": false, + "show_value_in_stage_title": false, + "order": 0 + } + }, + "type": "string", + "description": "Optional. Path of a file bundled with the skill, relative to the skill root, written exactly as listed in that skill's block (e.g. references/api-schema.md). Omit it to read the skill's instructions." } }, "required": [ diff --git a/docs/skills.md b/docs/skills.md index feebf9ae..434563cc 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -10,6 +10,14 @@ named directories. They are automatically loaded and made available to the agent ## How Skills Work +Skills come from three sources, merged per request: + +| Source | Origin | Bundled files | +|---|---|---| +| Predefined | `config/predefined/skills/`, loaded at startup | No | +| [DIAL prompt](#dial-prompt-skills) | `prompts//`, per request | No | +| [DIAL skill resource](#dial-skill-resources) | `skills//`, per request | Yes, text files | + - Skills are loaded from `config/predefined/skills/` at startup. Each skill lives in its own subdirectory (e.g. `skills/my-skill/SKILL.md`). - Each skill is presented to the agent as XML metadata in the system prompt. @@ -114,9 +122,9 @@ optional features. The table below summarises what is and isn't supported. | `name` validation (length, charset, consecutive hyphens) | Supported | | | `description`, `license`, `compatibility`, `metadata` | Supported | | | `allowed-tools` | Partial | Exposed in XML metadata but **not enforced** at runtime. | -| Optional subdirectories (`scripts/`, `references/`, `assets/`) | Not supported | Only `SKILL.md` is read; other directory contents are ignored. | -| Progressive disclosure (on-demand file references) | Not supported | The agent can read `SKILL.md` content via `read_skill` but cannot access referenced files within the skill directory. | -| Dynamic skill registration | Not supported | Skills are loaded once at startup; adding or modifying skills requires a restart. | +| Optional subdirectories (`scripts/`, `references/`, `assets/`) | Partial | Supported for [DIAL skill resources](#dial-skill-resources) (text files only). Predefined and DIAL-prompt skills read `SKILL.md` alone. | +| Progressive disclosure (on-demand file references) | Partial | Supported for [DIAL skill resources](#dial-skill-resources) via `read_skill(skill_name, file_path)`. Not available for the other two sources. | +| Dynamic skill registration | Partial | DIAL-prompt and DIAL-skill sources are resolved fresh per request. Predefined skills are loaded once at startup; adding or modifying them requires a restart. | For the full specification, see [agentskills.io/specification](https://agentskills.io/specification). For design rationale and known limitations, see [the design doc](designs/skills_and_file_transfer.md). @@ -177,12 +185,115 @@ skill takes precedence**. The DIAL prompt skill is skipped and a warning is logg - DIAL prompts are single text documents — they cannot contain `scripts/`, `references/`, or `assets/` subdirectories. - DIAL prompts are fetched fresh on each request (no cross-request caching). -- The `skills` config field is a **preview feature** — it requires `ENABLE_PREVIEW_FEATURES=true`. +- A prompt cannot bundle files. Use a [DIAL skill resource](#dial-skill-resources) when the skill + needs reference material the agent can open on demand. For design details, see [the design doc](designs/dial_prompts_as_skills.md). --- +## DIAL Skill Resources + +DIAL Core stores skills as **folder-shaped resources**: a mandatory `SKILL.md` plus an arbitrary file +hierarchy, served through the `/v2/skills` API. Unlike a DIAL prompt, such a skill can bundle the +reference material its manifest points at, and the agent reads those files **on demand**. + +### Configuration + +```json +{ + "skills": [ + { + "type": "dial-skill", + "url": "skills//" + } + ] +} +``` + +The `url` is a relative path including the `skills/` resource type prefix +(e.g. `skills/my-bucket/refund-policy`), following the same convention as `dial-prompt` and file +context URLs. + +### Progressive Disclosure + +At request time QuickApps reads the skill's `SKILL.md` and lists its bundled files. The file list is +appended to the manifest as a `` block, so the agent sees it the moment it calls +`read_skill`: + +```markdown +--- +name: refund-policy +description: How to handle refund requests, by region. +--- + +# Refund Policy + +Determine the customer's region, then read the matching reference file. + + +references/eu-rules.md +references/us-rules.md + +``` + +The agent then opens one with `read_skill(skill_name="refund-policy", file_path="references/eu-rules.md")` +— one request to DIAL Core per file, and only for files it actually asks for. Repeat reads within a +request are served from memory. + +A path is readable **only if it appears in that skill's `` block**. Anything else — a +traversal attempt, a hidden file, a path the model invented — is refused, and the error hands the +inventory back so the agent can correct itself. + +### Which Files Are Advertised + +A bundled file is listed and readable when it is a regular file whose extension is one of +`.md`, `.markdown`, `.txt`, `.json`, `.yaml`, `.yml`, `.csv`, `.tsv`, `.xml`, `.html`, `.toml`, +`.ini`, `.sql`, `.py`, `.sh`, `.js`, `.ts`. + +Excluded: subfolders, hidden entries at any depth (including Core's own `.dial-resource` marker), +`SKILL.md` itself (already returned by `read_skill` without a `file_path`), and everything binary — +images, PDFs and other assets are **not** available in this release. + +### Limits + +| Variable | Default | Purpose | +|---|---|---| +| `DIAL_SKILLS_FILE_MAX_BYTES` | `262144` | Cap on a single file read, `SKILL.md` included. | +| `DIAL_SKILLS_MAX_FILES` | `200` | Maximum files advertised per skill. | +| `DIAL_SKILLS_LISTING_MAX_PAGES` | `10` | Maximum listing pages followed per skill. | + +An over-cap `SKILL.md` drops the skill; an over-cap bundled file fails that one read. Files must be +valid UTF-8. + +### Name Collision + +Precedence is **predefined > dial-prompt > dial-skill**, and first configured wins within a source. +A skill that loses a collision is skipped and reported in the initialization issues stage. + +### Error Handling + +- **Inaccessible skill** (403, 404): skipped with a reported reason; other skills stay available. +- **Invalid `SKILL.md`**: skipped, same as a DIAL prompt skill. +- **File listing fails**: the skill is still loaded, without its bundled files, and a warning is + reported. +- **DIAL Core outage**: all DIAL skills are dropped and the request is served with the remaining + sources. + +### Limitations + +- **Access**: DIAL Core does not yet auto-share config-declared skills to the application's + per-request key. A `dial-skill` therefore resolves only when the caller's own key already has + access — a skill in the user's own bucket, one shared with them, or a published one. +- Skills are fetched fresh on each request (no cross-request caching). +- Binary and asset files are not readable. +- Validation of a `dial-skill` URL is not offered by `/skills/validate`: DIAL Core validates + `SKILL.md` when the skill is written, so a stored skill is already valid. + +For design details, see [the design doc](designs/skills_as_dial_resource.md). + +--- + ## Migrating from Agent Instructions The `config/predefined/instructions/` directory convention and `AgentInstructionsProvider` have been removed. The skills diff --git a/poetry.lock b/poetry.lock index 74aaf7ee..cb4d197b 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,15 +1,15 @@ -# This file is automatically @generated by Poetry 2.3.1 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.3.2 and should not be changed by hand. [[package]] name = "aidial-client" -version = "0.16.1" +version = "0.17.0" description = "A Python client library for the AI DIAL API" optional = false python-versions = "<3.14,>=3.10" groups = ["main"] files = [ - {file = "aidial_client-0.16.1-py3-none-any.whl", hash = "sha256:8cbc25ca28d54c19559c6f06b46ef5fa7cf4f4205bf91d6769b5fb951b51f00b"}, - {file = "aidial_client-0.16.1.tar.gz", hash = "sha256:6dce4b6d8254727b5eb69c5343c6117dc7bb43acbae4dbaf9087571100c35d69"}, + {file = "aidial_client-0.17.0-py3-none-any.whl", hash = "sha256:78755eeb64bc40b25336b602c26fe859e73194e7c7072bae0eca93aba8ae7bac"}, + {file = "aidial_client-0.17.0.tar.gz", hash = "sha256:10aa1a615a64914c43d0ae0c4b075d3a5fb5cdee448552dba80c47740708d553"}, ] [package.dependencies] @@ -5850,4 +5850,4 @@ type = ["pytest-mypy"] [metadata] lock-version = "2.1" python-versions = ">=3.13,<3.14" -content-hash = "55085968cd5fa3a6dae3f97408d6218b66d545218995ed57ecfb3591213b1709" +content-hash = "8c1732230ed3fda3e289db350086904a73320d0060a214843ee78f31f3ac500f" diff --git a/pyproject.toml b/pyproject.toml index 7037c694..e6daa9ce 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,7 @@ authors = [ ] dependencies = [ # Core framework & DI - "aidial-client (>=0.16.0,<0.17.0)", # DIAL API client + "aidial-client (>=0.17.0,<0.18.0)", # DIAL API client "aidial-sdk[telemetry]>=0.38.0,<0.39.0", # DIAL integration SDK "pydantic>=2.12.4,<3.0.0", # Data validation "pydantic-settings>=2.14.2,<3.0.0", # Settings management diff --git a/src/quickapp/app_factory.py b/src/quickapp/app_factory.py index 5b2651a4..052f1a81 100644 --- a/src/quickapp/app_factory.py +++ b/src/quickapp/app_factory.py @@ -16,6 +16,7 @@ from quickapp.dial_deployment_tooling import DialDeploymentToolingModule from quickapp.dial_files_tooling.dial_files_tooling_module import DialFilesToolingModule from quickapp.dial_prompt_skills.dial_prompt_skills_module import DialPromptSkillsModule +from quickapp.dial_skills.dial_skills_module import DialSkillsModule from quickapp.file_transfer import FileTransferModule from quickapp.internal_tooling.internal_tooling_module import InternalToolModule from quickapp.mcp_tooling import MCPToolingModule @@ -58,6 +59,7 @@ def build_di_modules() -> list[Module]: LazyOnDemandStrategyModule(), SkillsModule(), DialPromptSkillsModule(), + DialSkillsModule(), TimestampModule(), AgentHooksModule(), DialFilesToolingModule(), diff --git a/src/quickapp/config/skill.py b/src/quickapp/config/skill.py index 4d195b7f..2df77f33 100644 --- a/src/quickapp/config/skill.py +++ b/src/quickapp/config/skill.py @@ -2,7 +2,7 @@ from pydantic import BaseModel, Field -from quickapp.common.base_config import DialResourceConfigField +from quickapp.common.base_config import DialResourceConfigField, preview_model class DialPromptSkillConfig(BaseModel): @@ -18,7 +18,21 @@ class DialPromptSkillConfig(BaseModel): ] +@preview_model +class DialSkillConfig(BaseModel): + type: Literal["dial-skill"] = Field( + default="dial-skill", + description="Skill sourced from a DIAL skill resource (folder with SKILL.md).", + ) + url: Annotated[ + str, + DialResourceConfigField( + description="Relative skill resource URL in DIAL (e.g. skills//)" + ), + ] + + SkillConfig = Annotated[ - DialPromptSkillConfig, + DialPromptSkillConfig | DialSkillConfig, Field(discriminator="type"), ] diff --git a/src/quickapp/configuration_support/_controller.py b/src/quickapp/configuration_support/_controller.py index 59775f0a..2651a1d0 100644 --- a/src/quickapp/configuration_support/_controller.py +++ b/src/quickapp/configuration_support/_controller.py @@ -7,7 +7,7 @@ from quickapp.common.dial_settings import DialSettings from quickapp.config.application import ApplicationConfig -from quickapp.config.skill import DialPromptSkillConfig, SkillConfig +from quickapp.config.skill import DialPromptSkillConfig from quickapp.dial_prompt_skills._dial_prompt_skill_resolver import ( fetch_and_validate_dial_prompt_skill, ) @@ -47,14 +47,14 @@ async def get_default_configuration() -> dict[str, Any]: async def get_skills() -> list[SkillMetadata]: return self.__skills_provider.get_all_skills() + # Deliberately typed to DialPromptSkillConfig rather than the SkillConfig + # union: a DIAL skill resource is created and validated by Core (a stored + # skill already has a valid SKILL.md), so there is nothing for this + # endpoint to add for `dial-skill`. Keeping the union here would publish + # an OpenAPI that advertises a request this handler cannot serve. @app.post(CONFIG_SUPPORT_URI + "/skills/validate", response_model=SkillMetadata) - async def validate_skill(config: SkillConfig, request: Request) -> SkillMetadata: - if isinstance(config, DialPromptSkillConfig): - return await self._validate_dial_prompt_skill(config, request) - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=f"Unsupported skill type: {config.type}", - ) + async def validate_skill(config: DialPromptSkillConfig, request: Request) -> SkillMetadata: + return await self._validate_dial_prompt_skill(config, request) async def _validate_dial_prompt_skill( self, diff --git a/src/quickapp/dial_files_tooling/dial_files_tooling_module.py b/src/quickapp/dial_files_tooling/dial_files_tooling_module.py index 8bdcbfc7..1496849e 100644 --- a/src/quickapp/dial_files_tooling/dial_files_tooling_module.py +++ b/src/quickapp/dial_files_tooling/dial_files_tooling_module.py @@ -12,6 +12,7 @@ INTERNAL_FILE_READ_LINES_TOOL_NAME, INTERNAL_FILE_SEARCH_TOOL_NAME, INTERNAL_FILE_TOOL_NAME_PREFIX, + INTERNAL_SKILLS_READ_SKILL_TOOL_NAME, ) from quickapp.config.application import ApplicationConfig from quickapp.config.dial_files import DialFilesConfig @@ -75,8 +76,17 @@ def configure(self, binder: Binder) -> None: # from offload, regardless of config: a large read-back slice must never be # re-offloaded (infinite recursion). This guard cannot be removed via the # per-app / env-var `excluded_tools`, which is additive on top of it. + # + # `read_skill` joins them for a different reason: its result can carry the + # manifest's `` inventory, which progressive disclosure depends + # on. Offloading it into a DIAL-file pointer would strip that inventory from + # the tool result the model actually sees. _MANDATORY_EXCLUDED_TOOLS = frozenset( - {INTERNAL_FILE_READ_LINES_TOOL_NAME, INTERNAL_FILE_SEARCH_TOOL_NAME} + { + INTERNAL_FILE_READ_LINES_TOOL_NAME, + INTERNAL_FILE_SEARCH_TOOL_NAME, + INTERNAL_SKILLS_READ_SKILL_TOOL_NAME, + } ) @request_scope diff --git a/src/quickapp/dial_prompt_skills/__init__.py b/src/quickapp/dial_prompt_skills/__init__.py index 1aa5305e..4cb0bbdf 100644 --- a/src/quickapp/dial_prompt_skills/__init__.py +++ b/src/quickapp/dial_prompt_skills/__init__.py @@ -1,7 +1,4 @@ -from quickapp.dial_prompt_skills._dial_prompt_skill_resolver import ( - DialPromptSkillResolver, - ResolvedDialPromptSkill, -) +from quickapp.dial_prompt_skills._dial_prompt_skill_resolver import DialPromptSkillResolver from quickapp.dial_prompt_skills._dial_prompt_skills_context import _DialPromptSkillsContext -__all__ = ["DialPromptSkillResolver", "ResolvedDialPromptSkill", "_DialPromptSkillsContext"] +__all__ = ["DialPromptSkillResolver", "_DialPromptSkillsContext"] diff --git a/src/quickapp/dial_prompt_skills/_dial_prompt_skill_resolver.py b/src/quickapp/dial_prompt_skills/_dial_prompt_skill_resolver.py index a1a1b129..3bc5c96e 100644 --- a/src/quickapp/dial_prompt_skills/_dial_prompt_skill_resolver.py +++ b/src/quickapp/dial_prompt_skills/_dial_prompt_skill_resolver.py @@ -2,24 +2,14 @@ from aidial_client import AsyncDial from injector import inject -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict from quickapp.common.exceptions import SkillInitializationException from quickapp.config.skill import DialPromptSkillConfig from quickapp.skills._exceptions import SkillValidationError from quickapp.skills._frontmatter import parse_frontmatter -from quickapp.skills._skill_metadata import ParsedSkill, SkillMetadata - - -class ResolvedDialPromptSkill(BaseModel): - """A successfully fetched DIAL prompt skill, including its source URL.""" - - model_config = ConfigDict(frozen=True) - - url: str - metadata: SkillMetadata - content: str - warnings: list[str] = Field(default_factory=list) +from quickapp.skills._skill_metadata import ParsedSkill +from quickapp.skills.skills_provider import ResolvedSkill class DialPromptSkillResolverOutput(BaseModel): @@ -27,7 +17,7 @@ class DialPromptSkillResolverOutput(BaseModel): model_config = ConfigDict(arbitrary_types_allowed=True, frozen=True) - resolved: list[ResolvedDialPromptSkill] + resolved: list[ResolvedSkill] exceptions: list[SkillInitializationException] @@ -57,7 +47,7 @@ async def resolve( self, skill_configs: list[DialPromptSkillConfig], ) -> DialPromptSkillResolverOutput: - """Resolve skill configs into validated ``ResolvedDialPromptSkill`` entries. + """Resolve skill configs into validated ``ResolvedSkill`` entries. - Deduplicates by URL before fetching. - Fetches in parallel with ``asyncio.gather(return_exceptions=True)``. @@ -82,7 +72,7 @@ async def resolve( return_exceptions=True, ) - resolved: list[ResolvedDialPromptSkill] = [] + resolved: list[ResolvedSkill] = [] exceptions: list[SkillInitializationException] = [] seen_names: set[str] = set() @@ -117,9 +107,9 @@ async def resolve( async def _fetch_one( self, config: DialPromptSkillConfig, - ) -> ResolvedDialPromptSkill: + ) -> ResolvedSkill: parsed, content = await fetch_and_validate_dial_prompt_skill(self._dial_client, config.url) - return ResolvedDialPromptSkill( + return ResolvedSkill( url=config.url, metadata=parsed.metadata, content=content, diff --git a/src/quickapp/dial_prompt_skills/_dial_prompt_skills_context.py b/src/quickapp/dial_prompt_skills/_dial_prompt_skills_context.py index dc0000e4..140036f6 100644 --- a/src/quickapp/dial_prompt_skills/_dial_prompt_skills_context.py +++ b/src/quickapp/dial_prompt_skills/_dial_prompt_skills_context.py @@ -1,32 +1,35 @@ import threading from quickapp.common.exceptions import InitializationException, SkillInitializationException +from quickapp.skills import ResolvedSkill, SkillsProvider -from ._dial_prompt_skill_resolver import ResolvedDialPromptSkill - -class _DialPromptSkillsContext: +class _DialPromptSkillsContext(SkillsProvider): """Request-scoped bag of state populated by ``_DialPromptSkillInitializer`` - and consumed by ``SkillsRegistry`` during message finalization. + and the ``SkillsProvider`` ``SkillsRegistry`` consumes for it. Mirrors ``_MCPToolingContext`` in spirit but is a standalone class — the ``ToolingContextBase._tools`` field name does not fit the skills domain. + Prompt skills are single-document, so their skills carry no ``reader``. """ + order = 10 + display_name = "DIAL prompt skills" + def __init__(self) -> None: - self._resolved_skills: list[ResolvedDialPromptSkill] = [] + self._resolved_skills: list[ResolvedSkill] = [] self._exceptions: list[InitializationException] = [] self._lock = threading.Lock() @property - def resolved_skills(self) -> list[ResolvedDialPromptSkill]: + def resolved_skills(self) -> list[ResolvedSkill]: return self._resolved_skills @property def exceptions(self) -> list[InitializationException]: return self._exceptions - def extend_resolved_skills(self, skills: list[ResolvedDialPromptSkill]) -> None: + def extend_resolved_skills(self, skills: list[ResolvedSkill]) -> None: with self._lock: self._resolved_skills.extend(skills) diff --git a/src/quickapp/dial_prompt_skills/dial_prompt_skills_module.py b/src/quickapp/dial_prompt_skills/dial_prompt_skills_module.py index 99e9c52f..c1c79bcc 100644 --- a/src/quickapp/dial_prompt_skills/dial_prompt_skills_module.py +++ b/src/quickapp/dial_prompt_skills/dial_prompt_skills_module.py @@ -8,6 +8,7 @@ from quickapp.dial_prompt_skills._dial_prompt_skill_initializer import _DialPromptSkillInitializer from quickapp.dial_prompt_skills._dial_prompt_skill_resolver import DialPromptSkillResolver from quickapp.dial_prompt_skills._dial_prompt_skills_context import _DialPromptSkillsContext +from quickapp.skills import SkillsProvider logger = logging.getLogger(__name__) @@ -33,3 +34,7 @@ def __provide_initialization_exceptions( self, context: _DialPromptSkillsContext ) -> list[InitializationException]: return context.exceptions + + @multiprovider + def __provide_skill_providers(self, context: _DialPromptSkillsContext) -> list[SkillsProvider]: + return [context] diff --git a/src/quickapp/dial_skills/__init__.py b/src/quickapp/dial_skills/__init__.py new file mode 100644 index 00000000..1c2c605e --- /dev/null +++ b/src/quickapp/dial_skills/__init__.py @@ -0,0 +1,5 @@ +from quickapp.dial_skills._dial_skill_reader import DialSkillReader +from quickapp.dial_skills._dial_skill_resolver import DialSkillResolver +from quickapp.dial_skills._dial_skills_context import _DialSkillsContext + +__all__ = ["DialSkillReader", "DialSkillResolver", "_DialSkillsContext"] diff --git a/src/quickapp/dial_skills/_dial_skill_initializer.py b/src/quickapp/dial_skills/_dial_skill_initializer.py new file mode 100644 index 00000000..67183a54 --- /dev/null +++ b/src/quickapp/dial_skills/_dial_skill_initializer.py @@ -0,0 +1,53 @@ +import logging + +from injector import ProviderOf, inject + +from quickapp.common.base_initializer import CompletionInitializer +from quickapp.common.exceptions import SkillCatastrophicInitializationException +from quickapp.config.application import ApplicationConfig +from quickapp.config.skill import DialSkillConfig +from quickapp.dial_skills._dial_skill_resolver import DialSkillResolver +from quickapp.dial_skills._dial_skills_context import _DialSkillsContext + +logger = logging.getLogger(__name__) + + +@inject +class _DialSkillInitializer(CompletionInitializer): + """Eagerly resolves DIAL skill resources during the initialization phase so + the merged skill set is available to ``_AddSystemPromptTransformer``. + + The direct analogue of ``_DialPromptSkillInitializer``: reads + ``ApplicationConfig.skills`` via ``ProviderOf``, delegates to + ``DialSkillResolver``, and pushes the output into ``_DialSkillsContext``. + """ + + def __init__( + self, + config_provider: ProviderOf[ApplicationConfig], + resolver: DialSkillResolver, + context: _DialSkillsContext, + ) -> None: + self._config_provider = config_provider + self._resolver = resolver + self._context = context + + async def initialize(self) -> None: + skill_configs = self._config_provider.get().skills or [] + dial_skill_configs = [cfg for cfg in skill_configs if isinstance(cfg, DialSkillConfig)] + if not dial_skill_configs: + return + + try: + output = await self._resolver.resolve(dial_skill_configs) + except Exception as exc: + logger.exception("DIAL skill resolution failed") + self._context.append_exception( + SkillCatastrophicInitializationException( + reason=f"Failed to resolve DIAL skills: {exc}" + ) + ) + return + + self._context.extend_resolved_skills(output.resolved) + self._context.extend_exceptions(output.exceptions) diff --git a/src/quickapp/dial_skills/_dial_skill_reader.py b/src/quickapp/dial_skills/_dial_skill_reader.py new file mode 100644 index 00000000..d71a52dc --- /dev/null +++ b/src/quickapp/dial_skills/_dial_skill_reader.py @@ -0,0 +1,49 @@ +import threading + +from injector import inject + +from quickapp.dial_skills._dial_skills_client import MANIFEST_NAME, _DialSkillsClient +from quickapp.skills import ResolvedSkill, SkillFileNotFoundError, SkillFileReader + + +def _normalize_file_path(file_path: str) -> str: + """Strip the decorations a model tends to add around a listed path.""" + return file_path.strip().lstrip("/").removeprefix("./") + + +@inject +class DialSkillReader(SkillFileReader): + """Reads one file bundled with a resolved DIAL skill. + + Request-scoped; its own memoization cache lives as long as one request. + """ + + def __init__(self, client: _DialSkillsClient) -> None: + self._client = client + self._file_cache: dict[tuple[str, str], str] = {} + self._lock = threading.Lock() + + async def read_bundled_file(self, skill: ResolvedSkill, file_path: str) -> str: + """Read one file bundled with *skill*, honoring its inventory. + + Readability is inventory membership — covers traversal, encoded + separators and hidden files too, since the model can only ask for + what it was told exists. + """ + normalized = _normalize_file_path(file_path) + if normalized == MANIFEST_NAME: + # The manifest is not in the inventory; serve what read_skill would. + return skill.content + if normalized not in skill.files: + raise SkillFileNotFoundError(skill.metadata.name, file_path, skill.files) + + key = (skill.url, normalized) + with self._lock: + cached = self._file_cache.get(key) + if cached is not None: + return cached + + content = await self._client.read_text_file(skill.url, normalized) + with self._lock: + self._file_cache[key] = content + return content diff --git a/src/quickapp/dial_skills/_dial_skill_resolver.py b/src/quickapp/dial_skills/_dial_skill_resolver.py new file mode 100644 index 00000000..b2c7c4a4 --- /dev/null +++ b/src/quickapp/dial_skills/_dial_skill_resolver.py @@ -0,0 +1,149 @@ +import asyncio +import logging + +from injector import inject +from pydantic import BaseModel, ConfigDict + +from quickapp.common.exceptions import SkillInitializationException +from quickapp.config.skill import DialSkillConfig +from quickapp.dial_skills._dial_skill_reader import DialSkillReader +from quickapp.dial_skills._dial_skills_client import SkillInventory, _DialSkillsClient +from quickapp.dial_skills._exceptions import describe_exception +from quickapp.skills import ResolvedSkill, parse_frontmatter + +logger = logging.getLogger(__name__) + + +class DialSkillResolverOutput(BaseModel): + """Return shape of ``DialSkillResolver.resolve``.""" + + model_config = ConfigDict(arbitrary_types_allowed=True, frozen=True) + + resolved: list[ResolvedSkill] + exceptions: list[SkillInitializationException] + + +def _build_skill_files_block(inventory: SkillInventory, max_files: int) -> str: + """Render the ```` inventory appended to a skill's manifest. + + Paths are emitted verbatim — deliberately not XML-escaped. Escaping would + advertise `references/user's-guide.md` as `user's-guide.md`, a name no + lookup resolves. + """ + if not inventory.files: + return "" + block = "\n".join(("", *inventory.files, "")) + if inventory.truncated: + block += f"\nNote: file listing truncated at {max_files} entries." + return block + + +@inject +class DialSkillResolver: + """Request-scoped resolver that fetches DIAL skill resources and validates them.""" + + def __init__(self, client: _DialSkillsClient, reader: DialSkillReader) -> None: + self._client = client + self._reader = reader + + async def resolve( + self, + skill_configs: list[DialSkillConfig], + ) -> DialSkillResolverOutput: + """Resolve skill configs into validated ``ResolvedSkill`` entries. + + Mirrors ``DialPromptSkillResolver.resolve``: dedup by URL, fetch in + parallel, dedup by name (first configured wins), and turn both per-URL + failures and non-fatal warnings into ``SkillInitializationException`` + entries distinguished by ``severity``. + """ + seen_urls: set[str] = set() + unique_configs: list[DialSkillConfig] = [] + for cfg in skill_configs: + if cfg.url not in seen_urls: + seen_urls.add(cfg.url) + unique_configs.append(cfg) + + if not unique_configs: + return DialSkillResolverOutput(resolved=[], exceptions=[]) + + results = await asyncio.gather(*(self._fetch_labeled(cfg) for cfg in unique_configs)) + + resolved: list[ResolvedSkill] = [] + exceptions: list[SkillInitializationException] = [] + seen_names: set[str] = set() + + for url, result in results: + if isinstance(result, BaseException): + exceptions.append( + SkillInitializationException(url=url, reason=describe_exception(result)) + ) + continue + + for warning in result.warnings: + exceptions.append( + SkillInitializationException(url=url, reason=warning, severity="warning") + ) + + if result.metadata.name in seen_names: + exceptions.append( + SkillInitializationException( + url=url, + reason=( + f"Duplicate skill name '{result.metadata.name}';" + " keeping first occurrence" + ), + ) + ) + continue + + seen_names.add(result.metadata.name) + resolved.append(result) + + return DialSkillResolverOutput(resolved=resolved, exceptions=exceptions) + + async def _fetch_labeled( + self, config: DialSkillConfig + ) -> tuple[str, ResolvedSkill | BaseException]: + """Fetch one skill, pairing the result with its own URL. + + Pairing here (rather than correlating results back to + ``unique_configs`` by position) keeps the association obvious at the + call site instead of resting on ``asyncio.gather`` preserving order. + """ + try: + return config.url, await self._fetch_one(config) + except BaseException as exc: + return config.url, exc + + async def _fetch_one(self, config: DialSkillConfig) -> ResolvedSkill: + manifest = await self._client.read_manifest(config.url) + parsed = parse_frontmatter(manifest, config.url) + + inventory, warnings = await self._list_files(config.url) + block = _build_skill_files_block(inventory, self._client.max_files) + content = f"{manifest.rstrip()}\n\n{block}\n" if block else manifest + + return ResolvedSkill( + reader=self._reader, + url=config.url, + metadata=parsed.metadata, + content=content, + files=inventory.files, + warnings=[*parsed.warnings, *warnings], + ) + + async def _list_files(self, url: str) -> tuple[SkillInventory, list[str]]: + """List a skill's files, degrading to "no bundled files" on failure. + + A skill whose manifest reads fine is still useful without its inventory, + so a listing failure downgrades the skill rather than dropping it. + """ + try: + return await self._client.list_text_files(url), [] + except Exception as exc: + logger.warning("Failed to list files of a DIAL skill: %s", describe_exception(exc)) + return SkillInventory(), [ + f"Could not list bundled files: {describe_exception(exc)};" + " the skill is available without them" + ] diff --git a/src/quickapp/dial_skills/_dial_skills_client.py b/src/quickapp/dial_skills/_dial_skills_client.py new file mode 100644 index 00000000..60bc2917 --- /dev/null +++ b/src/quickapp/dial_skills/_dial_skills_client.py @@ -0,0 +1,170 @@ +import logging +from pathlib import PurePosixPath +from typing import Any +from urllib.parse import unquote + +from aidial_client import AsyncDial +from injector import inject +from pydantic import BaseModel, ConfigDict + +from quickapp.dial_skills._exceptions import ( + DialSkillFileNotTextError, + DialSkillFileReadError, + DialSkillFileTooLargeError, + describe_exception, +) +from quickapp.dial_skills._settings import DialSkillsSettings + +logger = logging.getLogger(__name__) + +MANIFEST_NAME = "SKILL.md" + +# Extensions a skill may publish to the model. The same allowlist decides what is +# advertised in and what `read_skill` will serve, so the model can +# only ask for what it was told exists. Binary and asset files are deferred. +_TEXT_EXTENSIONS = frozenset( + { + ".md", + ".markdown", + ".txt", + ".json", + ".yaml", + ".yml", + ".csv", + ".tsv", + ".xml", + ".html", + ".toml", + ".ini", + ".sql", + ".py", + ".sh", + ".js", + ".ts", + } +) + + +class SkillInventory(BaseModel): + """The readable files of one skill, as advertised to the model.""" + + model_config = ConfigDict(frozen=True) + + files: tuple[str, ...] = () + truncated: bool = False + + +@inject +class _DialSkillsClient: + """Read-side wrapper over DIAL Core's ``/v2/skills`` API. + + Owns the three things the resolver and the reader tool should not have to + care about: which files a skill is allowed to publish, the byte cap on any + single read, and the bounds on a paged listing. + """ + + def __init__(self, dial_client: AsyncDial, settings: DialSkillsSettings) -> None: + self._dial_client = dial_client + self._settings = settings + + @property + def max_files(self) -> int: + """The inventory ceiling, so callers can name it in a truncation note.""" + return self._settings.max_files + + @property + def _skills(self) -> Any: + """The client's ``/v2/skills`` resource.""" + return self._dial_client.skills # type: ignore[attr-defined] + + async def read_manifest(self, url: str) -> str: + """Read a skill's ``SKILL.md``.""" + return await self.read_text_file(url, MANIFEST_NAME) + + async def read_text_file(self, url: str, file_path: str) -> str: + """Read one file of a skill as UTF-8 text. + + Raises ``DialSkillFileReadError`` (or a subclass) for every failure mode + — transport, over-cap, or undecodable — so callers have one thing to + catch and a message that always names a reason. + """ + try: + response = await self._skills(url=url).files(path=file_path).read() + content = await response.aget_content() + except Exception as exc: + raise DialSkillFileReadError(file_path, describe_exception(exc)) from exc + + limit = self._settings.file_max_bytes + if len(content) > limit: + raise DialSkillFileTooLargeError(file_path, len(content), limit) + + try: + return content.decode("utf-8") + except UnicodeDecodeError as exc: + raise DialSkillFileNotTextError(file_path) from exc + + async def list_text_files(self, url: str) -> SkillInventory: + """List the skill's readable files, relative to the skill root. + + Follows Core's continuation token, bounded by ``listing_max_pages`` and + by a repeated-token guard: nothing above this imposes a deadline, so a + stuck cursor must not be able to hang initialization. + """ + # Built once: the url is parsed here rather than on every page. + files = self._skills(url=url).files + + paths: dict[str, None] = {} + seen_tokens: set[str] = set() + token: str | None = None + truncated = False + + for _ in range(self._settings.listing_max_pages): + page = await files.list(token=token, recursive=True) + prefix = page.url if page.url.endswith("/") else f"{page.url}/" + + for item in page.items or []: + if len(paths) >= self._settings.max_files: + truncated = True + break + relative = self._relative_path(item.url, prefix) + if relative is not None and self._is_advertisable(relative): + paths[relative] = None + + token = page.next_token + if truncated or not token or token in seen_tokens: + # A replayed token means the server is not advancing; stop rather + # than spend the page budget re-reading the same entries. + break + seen_tokens.add(token) + else: + # Ran out of the page budget with a token still pending. + truncated = truncated or bool(token) + + return SkillInventory(files=tuple(paths), truncated=truncated) + + @staticmethod + def _relative_path(item_url: str, prefix: str) -> str | None: + """Convert a listing entry's url into a path relative to the skill root. + + Returns ``None`` for an entry outside the listed folder. ``removeprefix`` + is a silent no-op on a mismatch, which would advertise a full + ``skills//...`` url as a path inside the skill. + """ + if not item_url.startswith(prefix): + logger.warning("Skipping skill file listing entry outside the listed folder") + return None + return unquote(item_url[len(prefix) :]) + + @staticmethod + def _is_advertisable(relative_path: str) -> bool: + """Whether *relative_path* may be shown to, and read by, the model.""" + if not relative_path or relative_path.endswith("/"): + return False + if relative_path == MANIFEST_NAME: + # Already served by read_skill without a file_path. + return False + segments = relative_path.split("/") + if any(not segment or segment.startswith(".") for segment in segments): + # Hidden entries at any depth, and Core's own .dial-resource marker. + return False + return PurePosixPath(relative_path).suffix.lower() in _TEXT_EXTENSIONS diff --git a/src/quickapp/dial_skills/_dial_skills_context.py b/src/quickapp/dial_skills/_dial_skills_context.py new file mode 100644 index 00000000..d3f56fe0 --- /dev/null +++ b/src/quickapp/dial_skills/_dial_skills_context.py @@ -0,0 +1,41 @@ +import threading + +from quickapp.common.exceptions import InitializationException, SkillInitializationException +from quickapp.skills import ResolvedSkill, SkillsProvider + + +class _DialSkillsContext(SkillsProvider): + """Request-scoped bag of state populated by ``_DialSkillInitializer`` and + the ``SkillsProvider`` ``SkillsRegistry`` consumes for it. + + Mirrors ``_DialPromptSkillsContext`` in shape. It does no I/O — each + skill already carries the reader it needs. + """ + + order = 20 + display_name = "DIAL skill resources" + + def __init__(self) -> None: + self._resolved_skills: list[ResolvedSkill] = [] + self._exceptions: list[InitializationException] = [] + self._lock = threading.Lock() + + @property + def resolved_skills(self) -> list[ResolvedSkill]: + return self._resolved_skills + + @property + def exceptions(self) -> list[InitializationException]: + return self._exceptions + + def extend_resolved_skills(self, skills: list[ResolvedSkill]) -> None: + with self._lock: + self._resolved_skills.extend(skills) + + def append_exception(self, exception: SkillInitializationException) -> None: + with self._lock: + self._exceptions.append(exception) + + def extend_exceptions(self, exceptions: list[SkillInitializationException]) -> None: + with self._lock: + self._exceptions.extend(exceptions) diff --git a/src/quickapp/dial_skills/_exceptions.py b/src/quickapp/dial_skills/_exceptions.py new file mode 100644 index 00000000..acf6a996 --- /dev/null +++ b/src/quickapp/dial_skills/_exceptions.py @@ -0,0 +1,36 @@ +class DialSkillFileReadError(Exception): + """Raised when a file of a DIAL skill resource cannot be read. + + The message is rendered verbatim to the model (via the ``read_skill`` tool) + or to the user (via the initialization-issues stage), so it must always + carry a reason — several httpx transport errors and ``TimeoutError`` have an + empty ``str()``, which is what ``describe_exception`` guards against. + """ + + def __init__(self, file_path: str, reason: str) -> None: + self.file_path = file_path + self.reason = reason + super().__init__(f"Failed to read '{file_path}' from DIAL: {reason}") + + +class DialSkillFileTooLargeError(DialSkillFileReadError): + """Raised when a fetched file exceeds ``DIAL_SKILLS_FILE_MAX_BYTES``.""" + + def __init__(self, file_path: str, size: int, limit: int) -> None: + super().__init__(file_path, f"file is {size} bytes, over the {limit} byte limit") + + +class DialSkillFileNotTextError(DialSkillFileReadError): + """Raised when a fetched file is not valid UTF-8 text.""" + + def __init__(self, file_path: str) -> None: + super().__init__(file_path, "file is not UTF-8 text") + + +def describe_exception(exc: BaseException) -> str: + """Return a non-empty description of *exc*. + + ``str()`` is empty for ``TimeoutError`` and several httpx transport errors, + which would otherwise produce a reason-less "Failed to read ...: " message. + """ + return str(exc) or type(exc).__name__ diff --git a/src/quickapp/dial_skills/_settings.py b/src/quickapp/dial_skills/_settings.py new file mode 100644 index 00000000..4aefd576 --- /dev/null +++ b/src/quickapp/dial_skills/_settings.py @@ -0,0 +1,38 @@ +from pydantic import Field +from pydantic_settings import BaseSettings, SettingsConfigDict + + +class DialSkillsSettings(BaseSettings): + """Operator-level limits for reading DIAL skill resources.""" + + model_config = SettingsConfigDict() + + file_max_bytes: int = Field( + default=262144, + gt=0, + description=( + "Maximum size of a single file read from a DIAL skill, manifest included. " + "Enforced after the response arrives: Core's file listing carries no size, " + "so there is nothing to check beforehand. An over-cap SKILL.md drops the " + "skill; an over-cap bundled file fails that read only." + ), + alias="DIAL_SKILLS_FILE_MAX_BYTES", + ) + max_files: int = Field( + default=200, + gt=0, + description=( + "Maximum number of bundled files advertised per skill. Beyond it the " + "inventory is truncated and says so." + ), + alias="DIAL_SKILLS_MAX_FILES", + ) + listing_max_pages: int = Field( + default=10, + gt=0, + description=( + "Maximum number of pages followed when listing a skill's files. Bounds a " + "server-supplied cursor so a stuck listing cannot hang initialization." + ), + alias="DIAL_SKILLS_LISTING_MAX_PAGES", + ) diff --git a/src/quickapp/dial_skills/dial_skills_module.py b/src/quickapp/dial_skills/dial_skills_module.py new file mode 100644 index 00000000..a3aaeb75 --- /dev/null +++ b/src/quickapp/dial_skills/dial_skills_module.py @@ -0,0 +1,46 @@ +import logging + +from fastapi_injector import request_scope +from injector import Binder, Module, ProviderOf, multiprovider, singleton + +from quickapp.common.base_initializer import CompletionInitializer +from quickapp.common.exceptions import InitializationException +from quickapp.common.preview import preview_module +from quickapp.dial_skills._dial_skill_initializer import _DialSkillInitializer +from quickapp.dial_skills._dial_skill_reader import DialSkillReader +from quickapp.dial_skills._dial_skill_resolver import DialSkillResolver +from quickapp.dial_skills._dial_skills_client import _DialSkillsClient +from quickapp.dial_skills._dial_skills_context import _DialSkillsContext +from quickapp.dial_skills._settings import DialSkillsSettings +from quickapp.skills import SkillsProvider + +logger = logging.getLogger(__name__) + + +@preview_module +class DialSkillsModule(Module): + + def configure(self, binder: Binder) -> None: + binder.bind(DialSkillsSettings, to=DialSkillsSettings, scope=singleton) + binder.bind(_DialSkillsClient, to=_DialSkillsClient, scope=request_scope) + binder.bind(DialSkillResolver, to=DialSkillResolver, scope=request_scope) + binder.bind(_DialSkillsContext, to=_DialSkillsContext, scope=request_scope) + binder.bind(DialSkillReader, to=DialSkillReader, scope=request_scope) + binder.bind(_DialSkillInitializer, to=_DialSkillInitializer, scope=request_scope) + logger.debug("DialSkillsModule configuration completed") + + @multiprovider + def __provide_initializers( + self, initializer_provider: ProviderOf[_DialSkillInitializer] + ) -> list[CompletionInitializer]: + return [initializer_provider.get()] + + @multiprovider + def __provide_initialization_exceptions( + self, context: _DialSkillsContext + ) -> list[InitializationException]: + return context.exceptions + + @multiprovider + def __provide_skill_providers(self, context: _DialSkillsContext) -> list[SkillsProvider]: + return [context] diff --git a/src/quickapp/skills/__init__.py b/src/quickapp/skills/__init__.py index e69de29b..cf4df51d 100644 --- a/src/quickapp/skills/__init__.py +++ b/src/quickapp/skills/__init__.py @@ -0,0 +1,13 @@ +from quickapp.skills._exceptions import SkillFileNotFoundError +from quickapp.skills._frontmatter import parse_frontmatter +from quickapp.skills._skill_metadata import SkillMetadata +from quickapp.skills.skills_provider import ResolvedSkill, SkillFileReader, SkillsProvider + +__all__ = [ + "ResolvedSkill", + "SkillFileNotFoundError", + "SkillFileReader", + "SkillMetadata", + "SkillsProvider", + "parse_frontmatter", +] diff --git a/src/quickapp/skills/_exceptions.py b/src/quickapp/skills/_exceptions.py index 35888ec2..7f9f8bc7 100644 --- a/src/quickapp/skills/_exceptions.py +++ b/src/quickapp/skills/_exceptions.py @@ -1,3 +1,6 @@ +from collections.abc import Sequence + + class SkillValidationError(Exception): """Raised by ``parse_frontmatter`` when skill content is invalid.""" @@ -5,3 +8,34 @@ def __init__(self, source_id: str, reason: str) -> None: self.source_id = source_id self.reason = reason super().__init__(f"Skill validation failed for '{source_id}': {reason}") + + +class SkillFilesNotSupportedError(FileNotFoundError): + """Raised when a ``file_path`` is asked of a skill source that has no files. + + Predefined and DIAL-prompt skills are single documents. Subclasses + ``FileNotFoundError`` so the reader tool's existing handler renders the + message to the model unchanged. + """ + + def __init__(self, skill_name: str) -> None: + super().__init__( + f"Skill '{skill_name}' has no bundled files." + " Call read_skill without file_path to read its instructions." + ) + + +class SkillFileNotFoundError(FileNotFoundError): + """Raised when a ``file_path`` is not among a skill's advertised files. + + Carries the inventory so the model can correct itself on the next turn + instead of guessing again. + """ + + def __init__(self, skill_name: str, file_path: str, available: Sequence[str]) -> None: + message = f"File '{file_path}' is not available in skill '{skill_name}'." + if available: + message += " Available files: " + ", ".join(available) + else: + message += " This skill has no readable bundled files." + super().__init__(message) diff --git a/src/quickapp/skills/_skill_reader_stage_wrapper.py b/src/quickapp/skills/_skill_reader_stage_wrapper.py index 150f092e..eb635623 100644 --- a/src/quickapp/skills/_skill_reader_stage_wrapper.py +++ b/src/quickapp/skills/_skill_reader_stage_wrapper.py @@ -12,6 +12,17 @@ class _SkillReaderStageWrapper(TimedStageWrapper): def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: return "" + def _get_stage_title_from_params(self, parameters: dict[str, Any]) -> str: + """Name the skill, and the bundled file when one was requested. + + The base implementation stops at the first parameter marked + ``show_value_in_stage_title``, which would show whichever of the two the + model happened to serialize first. + """ + skill_name = parameters.get("skill_name") or "" + file_path = parameters.get("file_path") + return f"{skill_name}/{file_path}" if file_path else str(skill_name) + def _build_debug_info_from_exception(self, exception: Exception) -> str: return f"Error:\n{fenced_code_block(str(exception))}\n" diff --git a/src/quickapp/skills/_skill_reader_tool.py b/src/quickapp/skills/_skill_reader_tool.py index 391971f5..1e40c9bc 100644 --- a/src/quickapp/skills/_skill_reader_tool.py +++ b/src/quickapp/skills/_skill_reader_tool.py @@ -44,6 +44,7 @@ async def _run_in_stage_async( stage_wrapper: BaseStageWrapper | None = None, tool_call_id: str | None = None, skill_name: str | None = None, + file_path: str | None = None, *args: Any, **kwargs: Any, ) -> ToolCallResult: @@ -57,7 +58,10 @@ async def _run_in_stage_async( return result try: - content = self.__skills_registry.get_skill_content(skill_name) + if file_path: + content = await self.__skills_registry.read_skill_file(skill_name, file_path) + else: + content = self.__skills_registry.get_skill_content(skill_name) result = ToolCallResult(content=content, content_type="text/markdown") if stage_wrapper: stage_wrapper.add_result(result) diff --git a/src/quickapp/skills/_skills_registry.py b/src/quickapp/skills/_skills_registry.py index 33277d38..cdaa03c0 100644 --- a/src/quickapp/skills/_skills_registry.py +++ b/src/quickapp/skills/_skills_registry.py @@ -2,92 +2,93 @@ from pydantic import BaseModel, ConfigDict from quickapp.common.abstract.base_prompt_provider import PromptPartProvider -from quickapp.common.exceptions import SkillInitializationException -from quickapp.dial_prompt_skills._dial_prompt_skills_context import _DialPromptSkillsContext +from quickapp.common.exceptions import InitializationException, SkillInitializationException +from quickapp.skills._skill_metadata import SkillMetadata from quickapp.skills._xml import generate_skills_xml -from quickapp.skills.agent_skills_provider import AgentSkillsProvider +from quickapp.skills.skills_provider import ResolvedSkill, SkillsProvider class _MergedSkills(BaseModel): - model_config = ConfigDict(frozen=True) + model_config = ConfigDict(arbitrary_types_allowed=True, frozen=True) xml: str - contents: dict[str, str] + entries: dict[str, ResolvedSkill] @inject class SkillsRegistry(PromptPartProvider): - """Request-scoped registry that merges predefined and DIAL-prompt skills - into a single ```` XML block. + """Request-scoped registry that merges every ``SkillsProvider`` into a single + ```` XML block. - By contract the DIAL-prompt skill context is populated by - ``_DialPromptSkillInitializer`` during the initialization phase, so this - class does **no I/O**: the first ``get_prompt_part()`` call runs a pure - in-memory merge and caches the result for the rest of the request. + No I/O during merge — providers are populated by their own initializers + beforehand; ``get_prompt_part()`` runs a pure in-memory merge and caches + it. Reading a bundled file is the lazy exception, via ``read_skill_file``. - Predefined-vs-external name collisions are reported back to the context as - ``SkillInitializationException`` entries, so they surface in the unified - "Initialization issues" stage alongside per-URL and catastrophic failures. + Precedence is fixed by ``SkillsProvider.order`` (lower wins), not by DI + module registration order. A name collision is recorded on + ``collision_exceptions``, which ``SkillsModule`` contributes to the + aggregated "Initialization issues" stage. """ - def __init__( - self, - predefined_provider: AgentSkillsProvider, - dial_prompt_skills_context: _DialPromptSkillsContext | None = None, - ) -> None: - self._predefined_provider = predefined_provider - self._context = dial_prompt_skills_context + def __init__(self, providers: list[SkillsProvider]) -> None: + self._providers = sorted(providers, key=lambda p: p.order) + self._collisions: list[InitializationException] = [] self._merged: _MergedSkills | None = None + @property + def collision_exceptions(self) -> list[InitializationException]: + """Name collisions found during the merge, reported as initialization issues.""" + return self._collisions + def _get_merged(self) -> _MergedSkills: if self._merged is not None: return self._merged - predefined_skills = list(self._predefined_provider.get_all_skills()) - contents = dict(self._predefined_provider.get_all_skill_contents()) - predefined_names = {s.name for s in predefined_skills} - merged_skills = list(predefined_skills) + taken: dict[str, SkillsProvider] = {} + merged_skills: list[SkillMetadata] = [] + entries: dict[str, ResolvedSkill] = {} - if self._context is not None: - collisions: list[SkillInitializationException] = [] - for skill in self._context.resolved_skills: - if skill.metadata.name in predefined_names: - collisions.append( + for provider in self._providers: + for skill in provider.resolved_skills: + name = skill.metadata.name + winner = taken.get(name) + if winner is not None: + self._collisions.append( SkillInitializationException( url=skill.url, reason=( - "Has the same name as a predefined skill;" - " predefined takes precedence" + f"Skill '{name}' is already provided by" + f" {winner.display_name}; this definition is ignored." ), ) ) continue + taken[name] = provider merged_skills.append(skill.metadata) - contents[skill.metadata.name] = skill.content - if collisions: - self._context.extend_exceptions(collisions) + entries[name] = skill self._merged = _MergedSkills( xml=generate_skills_xml(merged_skills), - contents=contents, + entries=entries, ) return self._merged async def get_prompt_part(self) -> str: - """Return merged skills XML for inclusion in the system prompt. - - ``async`` to match ``PromptPartProvider``; body does no ``await`` — - the skill data is already loaded. - """ + """Merged skills XML for the system prompt.""" return self._get_merged().xml def get_skill_content(self, skill_name: str) -> str: - """Return the full content of a skill by name. - - Synchronous pure dict lookup. Raises ``FileNotFoundError`` if the skill - is not in the merged set. - """ + """Full content of a skill by name. Raises ``FileNotFoundError`` if unknown.""" try: - return self._get_merged().contents[skill_name] + return self._get_merged().entries[skill_name].content except KeyError: raise FileNotFoundError(f"Skill not found: {skill_name}") + + async def read_skill_file(self, skill_name: str, file_path: str) -> str: + """Content of a file bundled with *skill_name*, delegated to the skill.""" + merged = self._get_merged() + skill = merged.entries.get(skill_name) + if skill is None: + raise FileNotFoundError(f"Skill not found: {skill_name}") + + return await skill.read_file(file_path) diff --git a/src/quickapp/skills/_tool_configs.py b/src/quickapp/skills/_tool_configs.py index 78af0416..9b670860 100644 --- a/src/quickapp/skills/_tool_configs.py +++ b/src/quickapp/skills/_tool_configs.py @@ -38,7 +38,17 @@ ignore=True, ) ), - ) + ), + "file_path": ConfigurableSchemaSimpleType( + type=JsonTypeEnum.string, + description=( + "Optional. Path of a file bundled with the skill, relative to the" + " skill root, written exactly as listed in that skill's" + " block (e.g. references/api-schema.md). Omit it to" + " read the skill's instructions." + ), + display=ParameterDisplayConfig(stage=FormattedParameterConfig(ignore=True)), + ), }, required=["skill_name"], ), diff --git a/src/quickapp/skills/agent_skills_provider.py b/src/quickapp/skills/agent_skills_provider.py index d82f8d4b..486122ac 100644 --- a/src/quickapp/skills/agent_skills_provider.py +++ b/src/quickapp/skills/agent_skills_provider.py @@ -6,21 +6,25 @@ from quickapp.skills._exceptions import SkillValidationError from quickapp.skills._frontmatter import parse_frontmatter from quickapp.skills._skill_metadata import SkillMetadata +from quickapp.skills.skills_provider import ResolvedSkill, SkillsProvider logger = logging.getLogger(__name__) @inject -class AgentSkillsProvider: - """Pure data store for predefined skills. +class AgentSkillsProvider(SkillsProvider): + """Data store for predefined skills, and the ``SkillsProvider`` for them. - Loads skills at startup, parses frontmatter, and exposes metadata and content. - Does not generate XML — that is the responsibility of ``SkillsRegistry``. + Loads skills at startup and parses frontmatter. Predefined skills are + single-document (no ``reader``) and always win a name collision + (``order = 0``). """ + order = 0 + display_name = "predefined skills" + def __init__(self, provider: PredefinedContentProvider) -> None: - self._skills: list[SkillMetadata] = [] - self._contents: dict[str, str] = {} + self._by_name: dict[str, ResolvedSkill] = {} self._provider = provider self._load_skills() @@ -31,8 +35,7 @@ def _load_skills(self) -> None: logger.debug("No skills found in predefined content") return - skills: list[SkillMetadata] = [] - contents: dict[str, str] = {} + skills: dict[str, ResolvedSkill] = {} for file_stem in skill_names: try: logger.debug(f"Loading skill `{file_stem}`") @@ -55,25 +58,25 @@ def _load_skills(self) -> None: metadata.name, file_stem, ) - skills.append(metadata) - contents[metadata.name] = content + skills[metadata.name] = ResolvedSkill( + url=f"predefined:{metadata.name}", metadata=metadata, content=content + ) - self._skills = skills - self._contents = contents + self._by_name = skills # DEBUG: superseded at INFO by the request-initialized lifecycle event. logger.debug("Loaded %d skill(s)", len(skills)) + @property + def resolved_skills(self) -> list[ResolvedSkill]: + return list(self._by_name.values()) + def get_all_skills(self) -> list[SkillMetadata]: """Return the cached list of predefined skill metadata.""" - return self._skills - - def get_all_skill_contents(self) -> dict[str, str]: - """Return ``{name: full_content}`` for all predefined skills.""" - return self._contents + return [skill.metadata for skill in self._by_name.values()] def get_skill_content(self, skill_name: str) -> str: """Return the full content of a skill file. Raises FileNotFoundError if not found.""" try: - return self._contents[skill_name] + return self._by_name[skill_name].content except KeyError: raise FileNotFoundError(f"Skill not found: {skill_name}") diff --git a/src/quickapp/skills/skills_module.py b/src/quickapp/skills/skills_module.py index da1b58f8..f9f4eb8c 100644 --- a/src/quickapp/skills/skills_module.py +++ b/src/quickapp/skills/skills_module.py @@ -6,6 +6,7 @@ from quickapp.common import StagedBaseTool from quickapp.common.abstract.base_prompt_provider import PromptPartProvider from quickapp.common.abstract.base_transformer import MessagesTransformer +from quickapp.common.exceptions import InitializationException from quickapp.skills._inject_file_transfer_instruction_transformer import ( _InjectFileTransferInstructionTransformer, ) @@ -13,6 +14,7 @@ from quickapp.skills._skills_registry import SkillsRegistry from quickapp.skills._tool_configs import SKILL_READER_TOOL_CONFIG, SKILL_READER_TOOL_NAME from quickapp.skills.agent_skills_provider import AgentSkillsProvider +from quickapp.skills.skills_provider import SkillsProvider logger = logging.getLogger(__name__) @@ -49,6 +51,16 @@ def _provide_prompt_parts( ) -> list[PromptPartProvider]: return [skills_registry] + @multiprovider + def _provide_skill_providers(self, provider: AgentSkillsProvider) -> list[SkillsProvider]: + return [provider] + + @multiprovider + def _provide_initialization_exceptions( + self, registry: SkillsRegistry + ) -> list[InitializationException]: + return registry.collision_exceptions + @multiprovider def _provide_message_transformers( self, diff --git a/src/quickapp/skills/skills_provider.py b/src/quickapp/skills/skills_provider.py new file mode 100644 index 00000000..f1ad6780 --- /dev/null +++ b/src/quickapp/skills/skills_provider.py @@ -0,0 +1,67 @@ +from abc import ABC, abstractmethod + +from pydantic import BaseModel, ConfigDict, Field + +from quickapp.skills._exceptions import SkillFilesNotSupportedError +from quickapp.skills._skill_metadata import SkillMetadata + + +class SkillFileReader(ABC): + """Reads one bundled file of a ``ResolvedSkill``. + + Implemented by sources with bundled-file capability (today, only + ``DialSkillReader``) and stored on each skill they resolve. + """ + + @abstractmethod + async def read_bundled_file(self, skill: "ResolvedSkill", file_path: str) -> str: ... + + +class ResolvedSkill(BaseModel): + """One resolved skill, whatever produced it. + + ``reader`` is ``None`` for sources with no bundled-file capability; + ``read_file`` raises ``SkillFilesNotSupportedError`` in that case. + """ + + model_config = ConfigDict(arbitrary_types_allowed=True, frozen=True) + + url: str + metadata: SkillMetadata + content: str + files: tuple[str, ...] = () + warnings: list[str] = Field(default_factory=list) + reader: SkillFileReader | None = None + + async def read_file(self, file_path: str) -> str: + if self.reader is None: + raise SkillFilesNotSupportedError(self.metadata.name) + return await self.reader.read_bundled_file(self, file_path) + + +class SkillsProvider(ABC): + """Contributes already-resolved skills to the merged ```` set. + + No I/O here — resolution already ran during the initializer phase. + ``SkillsRegistry`` sorts providers by ``order`` and merges their + ``resolved_skills``. + """ + + @property + @abstractmethod + def order(self) -> int: + """Lower wins a name collision. A real ``@property`` so a subclass + that forgets to set it can't be instantiated; a plain class + attribute (``order = 0``) still satisfies it.""" + ... + + @property + @abstractmethod + def display_name(self) -> str: + """Human-readable label naming this provider as the winner in a + collision message — e.g. ``"predefined skills"``.""" + ... + + @property + @abstractmethod + def resolved_skills(self) -> list[ResolvedSkill]: ... diff --git a/src/tests/unit_tests/common/common.py b/src/tests/unit_tests/common/common.py index 3ce68819..ef9a8a7b 100644 --- a/src/tests/unit_tests/common/common.py +++ b/src/tests/unit_tests/common/common.py @@ -13,7 +13,7 @@ from quickapp.config.prompt import CustomSystemPromptConfig from quickapp.config.tools.base import AttachmentConfig from quickapp.config.toolsets.toolset import ToolSet -from quickapp.dial_prompt_skills import ResolvedDialPromptSkill +from quickapp.skills import ResolvedSkill, SkillFileReader from quickapp.skills._skill_metadata import SkillMetadata MODULE_TYPE: TypeAlias = Callable[[Binder], None] | Module | type[Module] @@ -113,17 +113,19 @@ def noop_timeout_resolver_provider(value: float = 300.0) -> MagicMock: return make_provider(noop_timeout_resolver(value=value)) -def make_resolved_dial_prompt_skill( +def make_resolved_skill( url: str, name: str, description: str = "A skill", content: str = "body", -) -> ResolvedDialPromptSkill: - """Builder for ``ResolvedDialPromptSkill`` fixtures shared by skill/registry - and dial-prompt-skills tests. - """ - return ResolvedDialPromptSkill( + files: tuple[str, ...] = (), + reader: SkillFileReader | None = None, +) -> ResolvedSkill: + """Builder for ``ResolvedSkill`` fixtures shared across skills tests.""" + return ResolvedSkill( url=url, metadata=SkillMetadata(name=name, description=description), content=content, + files=files, + reader=reader, ) diff --git a/src/tests/unit_tests/dial_files_tooling/test_offload_config_resolution.py b/src/tests/unit_tests/dial_files_tooling/test_offload_config_resolution.py index 5aea9e02..a144f3a4 100644 --- a/src/tests/unit_tests/dial_files_tooling/test_offload_config_resolution.py +++ b/src/tests/unit_tests/dial_files_tooling/test_offload_config_resolution.py @@ -66,13 +66,19 @@ def test_excluded_tools_is_frozenset_with_mandatory_read_back_tools(self): config = _resolve(_make_app_config(offload=_make_offload(excluded_tools={"tool_a"}))) assert isinstance(config.excluded_tools, frozenset) assert config.excluded_tools == frozenset( - {"tool_a", "internal_file_read_lines", "internal_file_search"} + { + "tool_a", + "internal_file_read_lines", + "internal_file_search", + "internal_skills_read_skill", + } ) def test_read_back_tools_always_excluded_even_when_config_omits_them(self): config = _resolve(_make_app_config(offload=_make_offload(excluded_tools=set()))) assert "internal_file_read_lines" in config.excluded_tools assert "internal_file_search" in config.excluded_tools + assert "internal_skills_read_skill" in config.excluded_tools def test_disabled_when_read_back_tools_not_in_enabled_list(self): config = _resolve(_make_app_config(enabled_tools=["write", "edit"])) diff --git a/src/tests/unit_tests/dial_prompt_skills_tests/test_dial_prompt_skill_initializer.py b/src/tests/unit_tests/dial_prompt_skills_tests/test_dial_prompt_skill_initializer.py index c1fc2403..47959288 100644 --- a/src/tests/unit_tests/dial_prompt_skills_tests/test_dial_prompt_skill_initializer.py +++ b/src/tests/unit_tests/dial_prompt_skills_tests/test_dial_prompt_skill_initializer.py @@ -10,7 +10,7 @@ from quickapp.dial_prompt_skills import _DialPromptSkillsContext from quickapp.dial_prompt_skills._dial_prompt_skill_initializer import _DialPromptSkillInitializer from quickapp.dial_prompt_skills._dial_prompt_skill_resolver import DialPromptSkillResolverOutput -from tests.unit_tests.common.common import make_resolved_dial_prompt_skill as _resolved +from tests.unit_tests.common.common import make_resolved_skill as _resolved def _make_config_provider(skills: list[DialPromptSkillConfig] | None = None) -> MagicMock: diff --git a/src/tests/unit_tests/dial_skills_tests/__init__.py b/src/tests/unit_tests/dial_skills_tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/tests/unit_tests/dial_skills_tests/test_dial_skill_resolver.py b/src/tests/unit_tests/dial_skills_tests/test_dial_skill_resolver.py new file mode 100644 index 00000000..2e4cff99 --- /dev/null +++ b/src/tests/unit_tests/dial_skills_tests/test_dial_skill_resolver.py @@ -0,0 +1,159 @@ +from unittest.mock import AsyncMock, MagicMock + +import pytest + +# `quickapp.common.*` must be imported before `quickapp.config.skill`: the two +# packages form a pre-existing import cycle (common/__init__ -> staged_base_tool +# -> config.application -> config.skill), which only bites when config.skill is +# the first of them to load. Keep this import above the config one. +from quickapp.common.exceptions import SkillInitializationException +from quickapp.config.skill import DialSkillConfig +from quickapp.dial_skills._dial_skill_reader import DialSkillReader +from quickapp.dial_skills._dial_skill_resolver import DialSkillResolver +from quickapp.dial_skills._dial_skills_client import SkillInventory +from quickapp.dial_skills._exceptions import DialSkillFileReadError + +MANIFEST = """--- +name: refund-policy +description: How to handle refunds. +--- + +# Refund Policy +""" + + +def _make_resolver( + *, + manifest: str = MANIFEST, + inventory: SkillInventory | None = None, + max_files: int = 200, +) -> tuple[DialSkillResolver, MagicMock]: + client = MagicMock() + client.max_files = max_files + client.read_manifest = AsyncMock(return_value=manifest) + client.list_text_files = AsyncMock(return_value=inventory or SkillInventory()) + return DialSkillResolver(client, MagicMock(spec=DialSkillReader)), client + + +def _config(url: str) -> DialSkillConfig: + return DialSkillConfig(url=url) + + +class TestResolve: + + @pytest.mark.asyncio + async def test_resolves_manifest_and_inventory(self): + inventory = SkillInventory(files=("references/eu.md", "references/us.md")) + resolver, _ = _make_resolver(inventory=inventory) + + output = await resolver.resolve([_config("skills/b/refund-policy")]) + + assert not output.exceptions + skill = output.resolved[0] + assert skill.metadata.name == "refund-policy" + assert skill.files == ("references/eu.md", "references/us.md") + assert "\nreferences/eu.md\nreferences/us.md\n" in skill.content + assert skill.content.startswith("---") + + @pytest.mark.asyncio + async def test_no_inventory_block_when_skill_has_no_files(self): + resolver, _ = _make_resolver() + + skill = (await resolver.resolve([_config("skills/b/plain")])).resolved[0] + + assert "" not in skill.content + assert skill.content == MANIFEST + + @pytest.mark.asyncio + async def test_truncated_inventory_is_flagged_to_the_model(self): + inventory = SkillInventory(files=("a.md",), truncated=True) + resolver, _ = _make_resolver(inventory=inventory, max_files=1) + + skill = (await resolver.resolve([_config("skills/b/big")])).resolved[0] + + assert "truncated at 1 entries" in skill.content + + @pytest.mark.asyncio + async def test_paths_are_not_xml_escaped(self): + inventory = SkillInventory(files=("references/user's-guide.md",)) + resolver, _ = _make_resolver(inventory=inventory) + + skill = (await resolver.resolve([_config("skills/b/x")])).resolved[0] + + # An escaped path would advertise a name no lookup resolves. + assert "references/user's-guide.md" in skill.content + assert "'" not in skill.content + + @pytest.mark.asyncio + async def test_deduplicates_by_url_before_fetching(self): + resolver, client = _make_resolver() + + output = await resolver.resolve([_config("skills/b/s"), _config("skills/b/s")]) + + assert len(output.resolved) == 1 + assert client.read_manifest.await_count == 1 + + @pytest.mark.asyncio + async def test_duplicate_name_keeps_first_and_reports(self): + resolver, _ = _make_resolver() + + output = await resolver.resolve([_config("skills/b/one"), _config("skills/b/two")]) + + assert len(output.resolved) == 1 + assert output.resolved[0].url == "skills/b/one" + assert "Duplicate skill name 'refund-policy'" in output.exceptions[0].reason + + @pytest.mark.asyncio + async def test_manifest_failure_drops_the_skill(self): + resolver, client = _make_resolver() + client.read_manifest = AsyncMock( + side_effect=DialSkillFileReadError("SKILL.md", "403 Forbidden") + ) + + output = await resolver.resolve([_config("skills/b/denied")]) + + assert output.resolved == [] + assert isinstance(output.exceptions[0], SkillInitializationException) + assert "403 Forbidden" in output.exceptions[0].reason + assert output.exceptions[0].url == "skills/b/denied" + + @pytest.mark.asyncio + async def test_invalid_frontmatter_drops_the_skill(self): + resolver, _ = _make_resolver(manifest="no frontmatter here") + + output = await resolver.resolve([_config("skills/b/broken")]) + + assert output.resolved == [] + assert "No YAML frontmatter found" in output.exceptions[0].reason + + @pytest.mark.asyncio + async def test_listing_failure_keeps_the_skill_with_a_warning(self): + resolver, client = _make_resolver() + client.list_text_files = AsyncMock(side_effect=TimeoutError()) + + output = await resolver.resolve([_config("skills/b/s")]) + + # The manifest read fine; the skill is still useful without its files. + assert output.resolved[0].files == () + warning = output.exceptions[0] + assert warning.severity == "warning" + assert "Could not list bundled files: TimeoutError" in warning.reason + + @pytest.mark.asyncio + async def test_parser_warnings_ride_as_warnings(self): + manifest = "---\nname: Refund_Policy\ndescription: d\n---\nbody" + resolver, _ = _make_resolver(manifest=manifest) + + output = await resolver.resolve([_config("skills/b/s")]) + + assert output.resolved + assert all(exc.severity == "warning" for exc in output.exceptions) + + @pytest.mark.asyncio + async def test_empty_config_list_does_no_io(self): + resolver, client = _make_resolver() + + output = await resolver.resolve([]) + + assert output.resolved == [] + assert client.read_manifest.await_count == 0 diff --git a/src/tests/unit_tests/dial_skills_tests/test_dial_skills_client.py b/src/tests/unit_tests/dial_skills_tests/test_dial_skills_client.py new file mode 100644 index 00000000..693292e3 --- /dev/null +++ b/src/tests/unit_tests/dial_skills_tests/test_dial_skills_client.py @@ -0,0 +1,280 @@ +from types import SimpleNamespace +from typing import cast + +import pytest +from aidial_client import AsyncDial + +from quickapp.dial_skills._dial_skills_client import _DialSkillsClient +from quickapp.dial_skills._exceptions import ( + DialSkillFileNotTextError, + DialSkillFileReadError, + DialSkillFileTooLargeError, +) +from quickapp.dial_skills._settings import DialSkillsSettings + +SKILL_URL = "skills/my-bucket/refund-policy" +FILES_PREFIX = f"{SKILL_URL}/files/" + + +def _page(urls: list[str], next_token: str | None = None) -> SimpleNamespace: + """A stand-in for the client's SkillFileMetadata page.""" + return SimpleNamespace( + url=FILES_PREFIX, + next_token=next_token, + items=[SimpleNamespace(url=url) for url in urls], + ) + + +class _FakeFilesRef: + """The tail of ``client.skills(url=...).files(path=...)``. + + Hand-written rather than a MagicMock chain: ``skill.files.list()`` and + ``skill.files(path=...).read()`` land on *different* auto-created mocks, + so a fixture that stubs one silently fails to intercept the other. The + signatures mirror the real reference, so a drift shows up as a TypeError. + """ + + def __init__(self, recorder: "_FakeSkills", url: str, path: str | None = None): + self._recorder = recorder + self._url = url + self._path = path + + def __call__(self, *, path: str) -> "_FakeFilesRef": + return _FakeFilesRef(self._recorder, self._url, path) + + async def list( + self, + *, + limit: int | None = None, + token: str | None = None, + recursive: bool | None = None, + ) -> SimpleNamespace: + self._recorder.list_calls.append({"url": self._url, "token": token, "recursive": recursive}) + pages = self._recorder.pages + return pages[min(len(self._recorder.list_calls) - 1, len(pages) - 1)] + + async def read(self) -> SimpleNamespace: + self._recorder.read_calls.append((self._url, self._path)) + if self._recorder.read_error is not None: + raise self._recorder.read_error + + async def aget_content() -> bytes: + return self._recorder.content + + return SimpleNamespace(aget_content=aget_content) + + +class _FakeSkillRef: + def __init__(self, recorder: "_FakeSkills", url: str): + self._recorder = recorder + self._url = url + + @property + def files(self) -> _FakeFilesRef: + return _FakeFilesRef(self._recorder, self._url) + + +class _FakeSkills: + """Stands in for ``AsyncDial.skills`` and records what was asked for.""" + + def __init__(self, pages: list[SimpleNamespace], content: bytes): + self.pages = pages + self.content = content + self.read_error: BaseException | None = None + self.list_calls: list[dict[str, object]] = [] + self.read_calls: list[tuple[str, str | None]] = [] + + def __call__(self, *, url: str) -> _FakeSkillRef: + return _FakeSkillRef(self, url) + + +def _make_client( + *, + pages: list[SimpleNamespace] | None = None, + content: bytes = b"hello", + settings: DialSkillsSettings | None = None, +) -> tuple[_DialSkillsClient, _FakeSkills]: + skills = _FakeSkills(pages or [_page([])], content) + dial_client = cast(AsyncDial, SimpleNamespace(skills=skills)) + client = _DialSkillsClient(dial_client, settings or DialSkillsSettings()) + return client, skills + + +class TestListTextFiles: + + @pytest.mark.asyncio + async def test_advertises_only_text_files(self): + client, _ = _make_client( + pages=[ + _page( + [ + f"{FILES_PREFIX}SKILL.md", + f"{FILES_PREFIX}references/eu-rules.md", + f"{FILES_PREFIX}data/prices.csv", + f"{FILES_PREFIX}assets/logo.png", + f"{FILES_PREFIX}scripts/run", + ] + ) + ] + ) + + inventory = await client.list_text_files(SKILL_URL) + + # SKILL.md is served by read_skill itself; the binary and the + # extension-less file are not text. + assert inventory.files == ("references/eu-rules.md", "data/prices.csv") + assert inventory.truncated is False + + @pytest.mark.asyncio + async def test_skips_folders_and_hidden_entries(self): + client, _ = _make_client( + pages=[ + _page( + [ + f"{FILES_PREFIX}references/", + f"{FILES_PREFIX}.dial-resource", + f"{FILES_PREFIX}.env", + f"{FILES_PREFIX}.hidden/secret.md", + f"{FILES_PREFIX}notes.md", + ] + ) + ] + ) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("notes.md",) + + @pytest.mark.asyncio + async def test_percent_decodes_paths(self): + client, _ = _make_client(pages=[_page([f"{FILES_PREFIX}references/api%20schema.md"])]) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("references/api schema.md",) + + @pytest.mark.asyncio + async def test_ignores_entries_outside_the_listed_folder(self): + client, _ = _make_client( + pages=[_page(["skills/other-bucket/their-skill/files/leak.md", f"{FILES_PREFIX}ok.md"])] + ) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("ok.md",) + + @pytest.mark.asyncio + async def test_follows_pagination(self): + client, dial = _make_client( + pages=[ + _page([f"{FILES_PREFIX}a.md"], next_token="t1"), + _page([f"{FILES_PREFIX}b.md"], next_token=None), + ] + ) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("a.md", "b.md") + assert len(dial.list_calls) == 2 + + @pytest.mark.asyncio + async def test_stops_on_repeated_token(self): + client, dial = _make_client( + pages=[ + _page([f"{FILES_PREFIX}a.md"], next_token="stuck"), + _page([f"{FILES_PREFIX}b.md"], next_token="stuck"), + ] + ) + + inventory = await client.list_text_files(SKILL_URL) + + # A cursor that does not advance must not spend the page budget. + assert inventory.files == ("a.md", "b.md") + assert len(dial.list_calls) == 2 + + @pytest.mark.asyncio + async def test_page_budget_marks_truncated(self): + settings = DialSkillsSettings(DIAL_SKILLS_LISTING_MAX_PAGES=2) + client, dial = _make_client( + pages=[ + _page([f"{FILES_PREFIX}a.md"], next_token="t1"), + _page([f"{FILES_PREFIX}b.md"], next_token="t2"), + _page([f"{FILES_PREFIX}c.md"], next_token="t3"), + ], + settings=settings, + ) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("a.md", "b.md") + assert inventory.truncated is True + assert len(dial.list_calls) == 2 + + @pytest.mark.asyncio + async def test_max_files_marks_truncated(self): + settings = DialSkillsSettings(DIAL_SKILLS_MAX_FILES=2) + client, _ = _make_client( + pages=[ + _page([f"{FILES_PREFIX}a.md", f"{FILES_PREFIX}b.md", f"{FILES_PREFIX}c.md"]), + ], + settings=settings, + ) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("a.md", "b.md") + assert inventory.truncated is True + + @pytest.mark.asyncio + async def test_deduplicates_replayed_paths(self): + client, _ = _make_client( + pages=[ + _page([f"{FILES_PREFIX}a.md"], next_token="t1"), + _page([f"{FILES_PREFIX}a.md", f"{FILES_PREFIX}b.md"], next_token=None), + ] + ) + + inventory = await client.list_text_files(SKILL_URL) + + assert inventory.files == ("a.md", "b.md") + + +class TestReadTextFile: + + @pytest.mark.asyncio + async def test_returns_decoded_text(self): + client, _ = _make_client(content=b"# Rules\n") + + assert await client.read_text_file(SKILL_URL, "references/eu.md") == "# Rules\n" + + @pytest.mark.asyncio + async def test_rejects_oversized_file(self): + settings = DialSkillsSettings(DIAL_SKILLS_FILE_MAX_BYTES=4) + client, _ = _make_client(content=b"too long", settings=settings) + + with pytest.raises(DialSkillFileTooLargeError, match="over the 4 byte limit"): + await client.read_text_file(SKILL_URL, "big.md") + + @pytest.mark.asyncio + async def test_rejects_non_utf8_file(self): + client, _ = _make_client(content=b"\xff\xfe\x00binary") + + with pytest.raises(DialSkillFileNotTextError, match="not UTF-8 text"): + await client.read_text_file(SKILL_URL, "weird.md") + + @pytest.mark.asyncio + async def test_transport_error_always_carries_a_reason(self): + client, dial = _make_client() + # str(TimeoutError()) is empty, which would otherwise produce a + # reason-less "Failed to read ...: " message. + dial.read_error = TimeoutError() + + with pytest.raises(DialSkillFileReadError, match="TimeoutError"): + await client.read_text_file(SKILL_URL, "slow.md") + + @pytest.mark.asyncio + async def test_read_manifest_targets_skill_md(self): + client, dial = _make_client(content=b"manifest") + + assert await client.read_manifest(SKILL_URL) == "manifest" + assert dial.read_calls == [(SKILL_URL, "SKILL.md")] diff --git a/src/tests/unit_tests/skills_tests/test_agent_skills_provider.py b/src/tests/unit_tests/skills_tests/test_agent_skills_provider.py index 284f872c..fb80362f 100644 --- a/src/tests/unit_tests/skills_tests/test_agent_skills_provider.py +++ b/src/tests/unit_tests/skills_tests/test_agent_skills_provider.py @@ -303,3 +303,35 @@ def test_unknown_skill_raises_file_not_found(self): with pytest.raises(FileNotFoundError, match="Skill not found"): asp.get_skill_content("nonexistent-skill") + + +# --------------------------------------------------------------------------- +# SkillsProvider conformance +# --------------------------------------------------------------------------- + + +class TestSkillsProvider: + """AgentSkillsProvider is the SkillsProvider for predefined skills.""" + + def test_order_is_lowest(self): + assert AgentSkillsProvider.order == 0 + + def test_display_name_is_human_readable(self): + assert AgentSkillsProvider.display_name == "predefined skills" + + def test_resolved_skills_carry_metadata_and_content_with_no_reader(self): + provider = PredefinedContentProvider(PredefinedSettings()) + asp = AgentSkillsProvider(provider) + + by_name = {s.metadata.name: s for s in asp.resolved_skills} + + assert "tool-call-file-parameter-formatting" in by_name + skill = by_name["tool-call-file-parameter-formatting"] + assert skill.content == asp.get_skill_content("tool-call-file-parameter-formatting") + assert skill.reader is None + + def test_resolved_skills_agrees_with_get_all_skills(self): + provider = PredefinedContentProvider(PredefinedSettings()) + asp = AgentSkillsProvider(provider) + + assert [s.metadata for s in asp.resolved_skills] == asp.get_all_skills() diff --git a/src/tests/unit_tests/skills_tests/test_skill_reader_tool.py b/src/tests/unit_tests/skills_tests/test_skill_reader_tool.py new file mode 100644 index 00000000..168e7833 --- /dev/null +++ b/src/tests/unit_tests/skills_tests/test_skill_reader_tool.py @@ -0,0 +1,96 @@ +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from quickapp.skills._exceptions import SkillFileNotFoundError, SkillFilesNotSupportedError +from quickapp.skills._skill_reader_tool import _SkillReaderTool +from quickapp.skills._tool_configs import SKILL_READER_TOOL_CONFIG + + +def _make_tool(registry: MagicMock) -> _SkillReaderTool: + return _SkillReaderTool( + stage_wrapper_builder=MagicMock(), + tool_config=SKILL_READER_TOOL_CONFIG, + perf_timer=MagicMock(), + skills_registry=registry, + ) + + +def _make_registry() -> MagicMock: + registry = MagicMock() + registry.get_skill_content.return_value = "# Manifest" + registry.read_skill_file = AsyncMock(return_value="# Bundled file") + return registry + + +class TestSkillReaderTool: + + @pytest.mark.asyncio + async def test_without_file_path_reads_the_manifest(self): + registry = _make_registry() + tool = _make_tool(registry) + + result = await tool._run_in_stage_async(skill_name="refunds") + + assert result.content == "# Manifest" + registry.read_skill_file.assert_not_awaited() + + @pytest.mark.asyncio + async def test_with_file_path_reads_the_bundled_file(self): + registry = _make_registry() + tool = _make_tool(registry) + + result = await tool._run_in_stage_async(skill_name="refunds", file_path="references/eu.md") + + assert result.content == "# Bundled file" + registry.read_skill_file.assert_awaited_once_with("refunds", "references/eu.md") + registry.get_skill_content.assert_not_called() + + @pytest.mark.asyncio + async def test_empty_file_path_falls_back_to_the_manifest(self): + registry = _make_registry() + tool = _make_tool(registry) + + result = await tool._run_in_stage_async(skill_name="refunds", file_path="") + + assert result.content == "# Manifest" + + @pytest.mark.asyncio + async def test_missing_skill_name_is_reported(self): + tool = _make_tool(_make_registry()) + + result = await tool._run_in_stage_async(skill_name=None) + + assert "Missing required parameter: skill_name" in result.content + + @pytest.mark.asyncio + async def test_unavailable_file_hands_the_inventory_back_to_the_model(self): + registry = _make_registry() + registry.read_skill_file = AsyncMock( + side_effect=SkillFileNotFoundError("refunds", "assets/logo.png", ["references/eu.md"]) + ) + tool = _make_tool(registry) + + result = await tool._run_in_stage_async(skill_name="refunds", file_path="assets/logo.png") + + assert "is not available in skill 'refunds'" in result.content + assert "references/eu.md" in result.content + + @pytest.mark.asyncio + async def test_source_without_files_is_explained(self): + registry = _make_registry() + registry.read_skill_file = AsyncMock(side_effect=SkillFilesNotSupportedError("predef")) + tool = _make_tool(registry) + + result = await tool._run_in_stage_async(skill_name="predef", file_path="a.md") + + assert "has no bundled files" in result.content + + +class TestToolSchema: + + def test_file_path_is_optional(self): + parameters = SKILL_READER_TOOL_CONFIG.open_ai_tool.function.parameters + + assert "file_path" in parameters.properties + assert parameters.required == ["skill_name"] diff --git a/src/tests/unit_tests/skills_tests/test_skills_provider_conformance.py b/src/tests/unit_tests/skills_tests/test_skills_provider_conformance.py new file mode 100644 index 00000000..ffd69c92 --- /dev/null +++ b/src/tests/unit_tests/skills_tests/test_skills_provider_conformance.py @@ -0,0 +1,67 @@ +"""Conformance tests: ``_DialPromptSkillsContext`` and ``_DialSkillsContext`` +each implement ``SkillsProvider`` directly. (``AgentSkillsProvider``'s is +covered in ``test_agent_skills_provider.py``.)""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from quickapp.dial_prompt_skills import _DialPromptSkillsContext +from quickapp.dial_skills import DialSkillReader, _DialSkillsContext +from quickapp.skills.agent_skills_provider import AgentSkillsProvider +from tests.unit_tests.common.common import make_resolved_skill as _skill + + +class TestOrdering: + + def test_precedence_is_predefined_then_prompt_then_dial_skill(self): + assert AgentSkillsProvider.order < _DialPromptSkillsContext.order < _DialSkillsContext.order + + def test_display_names_are_human_readable(self): + assert AgentSkillsProvider.display_name == "predefined skills" + assert _DialPromptSkillsContext.display_name == "DIAL prompt skills" + assert _DialSkillsContext.display_name == "DIAL skill resources" + + +class TestDialPromptSkillsContext: + + def test_resolved_skills_starts_empty_and_accumulates(self): + context = _DialPromptSkillsContext() + assert context.resolved_skills == [] + + skill = _skill("prompts/b/p", "p", content="body") + context.extend_resolved_skills([skill]) + + assert context.resolved_skills == [skill] + + @pytest.mark.asyncio + async def test_prompt_skills_have_no_reader(self): + context = _DialPromptSkillsContext() + context.extend_resolved_skills([_skill("prompts/b/p", "p")]) + + assert context.resolved_skills[0].reader is None + + +class TestDialSkillsContext: + + def test_resolved_skills_starts_empty_and_accumulates(self): + context = _DialSkillsContext() + assert context.resolved_skills == [] + + skill = _skill("skills/b/s", "s", files=("a.md",)) + context.extend_resolved_skills([skill]) + + assert context.resolved_skills == [skill] + + @pytest.mark.asyncio + async def test_read_file_delegates_to_the_skills_own_reader(self): + reader = MagicMock(spec=DialSkillReader) + reader.read_bundled_file = AsyncMock(return_value="file content") + skill = _skill("skills/b/s", "s", files=("a.md",), reader=reader) + context = _DialSkillsContext() + context.extend_resolved_skills([skill]) + + result = await context.resolved_skills[0].read_file("a.md") + + assert result == "file content" + reader.read_bundled_file.assert_awaited_once_with(skill, "a.md") diff --git a/src/tests/unit_tests/skills_tests/test_skills_providers_wiring.py b/src/tests/unit_tests/skills_tests/test_skills_providers_wiring.py new file mode 100644 index 00000000..c38fa910 --- /dev/null +++ b/src/tests/unit_tests/skills_tests/test_skills_providers_wiring.py @@ -0,0 +1,138 @@ +"""DI wiring for the three skill providers — each module contributes its own +adapter to ``list[SkillsProvider]`` via ``@multiprovider``.""" + +from unittest.mock import MagicMock + +from aidial_client import AsyncDial +from fastapi_injector import Injected, request_scope +from injector import Binder, Module, ProviderOf +from starlette.testclient import TestClient + +from quickapp.common.exceptions import InitializationException +from quickapp.dial_prompt_skills import _DialPromptSkillsContext +from quickapp.dial_prompt_skills.dial_prompt_skills_module import DialPromptSkillsModule +from quickapp.dial_skills.dial_skills_module import DialSkillsModule +from quickapp.skills._skill_metadata import SkillMetadata +from quickapp.skills._skills_registry import SkillsRegistry +from quickapp.skills.skills_module import SkillsModule +from quickapp.skills.skills_provider import ResolvedSkill, SkillsProvider +from tests.unit_tests.common.common import create_test_app + + +class _StubDialClientModule(Module): + """Binds the DIAL client the skill providers fetch through.""" + + def configure(self, binder: Binder) -> None: + binder.bind(AsyncDial, to=lambda: MagicMock(spec=AsyncDial), scope=request_scope) + + +def _make_client(modules: list[Module]) -> TestClient: + app = create_test_app([_StubDialClientModule(), *modules]) + + @app.get("/source-types") + async def source_types( + sources: list[SkillsProvider] = Injected(list[SkillsProvider]), + ) -> list[str]: + return sorted(type(s).__name__ for s in sources) + + @app.get("/prompt-part") + async def prompt_part(registry: SkillsRegistry = Injected(SkillsRegistry)) -> str: + return await registry.get_prompt_part() + + @app.get("/read-unknown-file") + async def read_unknown_file( + registry: SkillsRegistry = Injected(SkillsRegistry), + ) -> dict[str, str]: + try: + await registry.read_skill_file("nope", "a.md") + except FileNotFoundError as exc: + return {"error": type(exc).__name__} + return {"error": "none"} + + @app.get("/collision-reaches-aggregate") + async def collision_reaches_aggregate( + providers: list[SkillsProvider] = Injected(list[SkillsProvider]), + registry: SkillsRegistry = Injected(SkillsRegistry), + context: _DialPromptSkillsContext = Injected(_DialPromptSkillsContext), + # Resolved lazily, exactly as _InitializationErrorHandler takes it. + exceptions_provider: ProviderOf[list[InitializationException]] = Injected( + ProviderOf[list[InitializationException]] + ), + ) -> list[str]: + predefined_name = providers[0].resolved_skills[0].metadata.name + context.extend_resolved_skills( + [ + ResolvedSkill( + url="prompts/b/collides", + metadata=SkillMetadata(name=predefined_name, description="d"), + content="loser", + ) + ] + ) + # The merge runs during setup_messages... + await registry.get_prompt_part() + # ...and only afterwards does the handler resolve the aggregate. + return [ + exc.reason + for exc in exceptions_provider.get() + if "already provided by" in getattr(exc, "reason", "") + ] + + return TestClient(app) + + +class TestSkillSourcesWiring: + + def test_all_three_sources_are_injected(self): + client = _make_client([SkillsModule(), DialPromptSkillsModule(), DialSkillsModule()]) + + response = client.get("/source-types") + + assert response.status_code == 200 + assert response.json() == sorted( + [ + "AgentSkillsProvider", + "_DialPromptSkillsContext", + "_DialSkillsContext", + ] + ) + + def test_registry_resolves_with_only_always_on_sources(self): + # Mirrors ENABLE_PREVIEW_FEATURES=false: DialSkillsModule omitted. + client = _make_client([SkillsModule(), DialPromptSkillsModule()]) + + types_response = client.get("/source-types") + assert types_response.status_code == 200 + assert types_response.json() == sorted(["AgentSkillsProvider", "_DialPromptSkillsContext"]) + + prompt_response = client.get("/prompt-part") + assert prompt_response.status_code == 200 + + read_response = client.get("/read-unknown-file") + assert read_response.status_code == 200 + assert read_response.json() == {"error": "FileNotFoundError"} + + def test_registry_collisions_reach_the_aggregated_initialization_exceptions(self): + """The registry owns collision exceptions rather than pushing them back + into each provider, which only works because the aggregated + ``list[InitializationException]`` is resolved lazily — after the merge. + """ + client = _make_client([SkillsModule(), DialPromptSkillsModule()]) + + response = client.get("/collision-reaches-aggregate") + + assert response.status_code == 200 + assert response.json() == [ + "Skill 'tool-call-file-parameter-formatting' is already provided by" + " predefined skills; this definition is ignored." + ] + + def test_source_registration_order_does_not_depend_on_module_list_order(self): + # Real precedence data is covered by + # test_precedence_is_independent_of_source_list_order (registry_dial_skills tests). + forward = _make_client([SkillsModule(), DialPromptSkillsModule(), DialSkillsModule()]) + reversed_order = _make_client( + [DialSkillsModule(), DialPromptSkillsModule(), SkillsModule()] + ) + + assert forward.get("/source-types").json() == reversed_order.get("/source-types").json() diff --git a/src/tests/unit_tests/skills_tests/test_skills_registry.py b/src/tests/unit_tests/skills_tests/test_skills_registry.py index 844ac287..586bf983 100644 --- a/src/tests/unit_tests/skills_tests/test_skills_registry.py +++ b/src/tests/unit_tests/skills_tests/test_skills_registry.py @@ -6,16 +6,22 @@ from quickapp.dial_prompt_skills import _DialPromptSkillsContext from quickapp.skills._skill_metadata import SkillMetadata from quickapp.skills._skills_registry import SkillsRegistry -from tests.unit_tests.common.common import make_resolved_dial_prompt_skill as _resolved +from quickapp.skills.agent_skills_provider import AgentSkillsProvider +from tests.unit_tests.common.common import make_resolved_skill as _resolved -def _make_predefined_provider( +def _predefined_provider( skills: list[SkillMetadata] | None = None, contents: dict[str, str] | None = None, ) -> MagicMock: - provider = MagicMock() - provider.get_all_skills.return_value = skills or [] - provider.get_all_skill_contents.return_value = contents or {} + skills = skills or [] + contents = contents or {} + provider = MagicMock(spec=AgentSkillsProvider) + provider.order = AgentSkillsProvider.order + provider.display_name = AgentSkillsProvider.display_name + provider.resolved_skills = [ + _resolved(f"predefined:{m.name}", m.name, m.description, contents[m.name]) for m in skills + ] return provider @@ -24,16 +30,13 @@ def _skill(name: str, description: str = "A skill") -> SkillMetadata: class TestSkillsRegistryNoContext: - """Preview off: no _DialPromptSkillsContext is bound.""" + """Preview off: no dial-prompt provider is contributed.""" @pytest.mark.asyncio async def test_returns_predefined_only(self): predefined = [_skill("predefined-skill")] contents = {"predefined-skill": "# Predefined\nContent here"} - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(predefined, contents), - dial_prompt_skills_context=None, - ) + registry = SkillsRegistry(providers=[_predefined_provider(predefined, contents)]) xml = await registry.get_prompt_part() @@ -42,34 +45,25 @@ async def test_returns_predefined_only(self): def test_get_skill_content_returns_predefined(self): contents = {"my-skill": "# My Skill\nContent"} - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider([_skill("my-skill")], contents), - dial_prompt_skills_context=None, - ) + registry = SkillsRegistry(providers=[_predefined_provider([_skill("my-skill")], contents)]) assert registry.get_skill_content("my-skill") == "# My Skill\nContent" def test_get_skill_content_unknown_raises(self): - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(), - dial_prompt_skills_context=None, - ) + registry = SkillsRegistry(providers=[_predefined_provider()]) with pytest.raises(FileNotFoundError, match="Skill not found"): registry.get_skill_content("nonexistent") @pytest.mark.asyncio async def test_empty_predefined_returns_empty_xml(self): - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(), - dial_prompt_skills_context=None, - ) + registry = SkillsRegistry(providers=[_predefined_provider()]) assert await registry.get_prompt_part() == "" class TestSkillsRegistryWithContext: - """Preview on: _DialPromptSkillsContext is bound and pre-populated.""" + """Preview on: a dial-prompt provider is contributed and pre-populated.""" @pytest.mark.asyncio async def test_merges_predefined_and_context_skills(self): @@ -81,10 +75,7 @@ async def test_merges_predefined_and_context_skills(self): [_resolved("prompts/b/dial-skill", "dial-skill", "From DIAL", "DIAL skill content")] ) - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(predefined, contents), - dial_prompt_skills_context=context, - ) + registry = SkillsRegistry(providers=[_predefined_provider(predefined, contents), context]) xml = await registry.get_prompt_part() @@ -92,7 +83,7 @@ async def test_merges_predefined_and_context_skills(self): assert "dial-skill" in xml @pytest.mark.asyncio - async def test_predefined_wins_on_name_collision_and_appends_exception(self): + async def test_predefined_wins_on_name_collision_and_records_exception(self): predefined = [_skill("shared-name", "Predefined version")] contents = {"shared-name": "Predefined content"} @@ -101,20 +92,17 @@ async def test_predefined_wins_on_name_collision_and_appends_exception(self): [_resolved("prompts/b/collides", "shared-name", "DIAL version", "DIAL content")] ) - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(predefined, contents), - dial_prompt_skills_context=context, - ) + registry = SkillsRegistry(providers=[_predefined_provider(predefined, contents), context]) xml = await registry.get_prompt_part() assert "Predefined version" in xml assert registry.get_skill_content("shared-name") == "Predefined content" - assert len(context.exceptions) == 1 - collision = context.exceptions[0] + assert len(registry.collision_exceptions) == 1 + collision = registry.collision_exceptions[0] assert isinstance(collision, SkillInitializationException) assert collision.url == "prompts/b/collides" - assert "predefined" in collision.reason.lower() + assert "already provided by predefined skills" in collision.reason @pytest.mark.asyncio async def test_get_skill_content_for_context_skill(self): @@ -123,16 +111,13 @@ async def test_get_skill_content_for_context_skill(self): [_resolved("prompts/b/only", "dial-only", content="DIAL prompt skill content")] ) - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(), - dial_prompt_skills_context=context, - ) + registry = SkillsRegistry(providers=[_predefined_provider(), context]) assert registry.get_skill_content("dial-only") == "DIAL prompt skill content" @pytest.mark.asyncio async def test_merge_caches_result(self): - """Repeated calls must not re-run the merge (and so must not re-append + """Repeated calls must not re-run the merge (and so must not re-record collision warnings).""" predefined = [_skill("shared")] contents = {"shared": "Predefined content"} @@ -142,16 +127,13 @@ async def test_merge_caches_result(self): [_resolved("prompts/b/collides", "shared", "DIAL version", "DIAL content")] ) - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(predefined, contents), - dial_prompt_skills_context=context, - ) + registry = SkillsRegistry(providers=[_predefined_provider(predefined, contents), context]) await registry.get_prompt_part() await registry.get_prompt_part() registry.get_skill_content("shared") - assert len(context.exceptions) == 1 + assert len(registry.collision_exceptions) == 1 @pytest.mark.asyncio async def test_context_exceptions_preserved_through_merge(self): @@ -162,10 +144,7 @@ async def test_context_exceptions_preserved_through_merge(self): SkillInitializationException(url="prompts/b/broken", reason="Fetch failed") ) - registry = SkillsRegistry( - predefined_provider=_make_predefined_provider(), - dial_prompt_skills_context=context, - ) + registry = SkillsRegistry(providers=[_predefined_provider(), context]) await registry.get_prompt_part() assert len(context.exceptions) == 1 diff --git a/src/tests/unit_tests/skills_tests/test_skills_registry_dial_skills.py b/src/tests/unit_tests/skills_tests/test_skills_registry_dial_skills.py new file mode 100644 index 00000000..278791ad --- /dev/null +++ b/src/tests/unit_tests/skills_tests/test_skills_registry_dial_skills.py @@ -0,0 +1,178 @@ +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from quickapp.dial_prompt_skills import _DialPromptSkillsContext +from quickapp.dial_skills import DialSkillReader, _DialSkillsContext +from quickapp.skills._exceptions import SkillFileNotFoundError, SkillFilesNotSupportedError +from quickapp.skills._skill_metadata import SkillMetadata +from quickapp.skills._skills_registry import SkillsRegistry +from quickapp.skills.agent_skills_provider import AgentSkillsProvider +from tests.unit_tests.common.common import make_resolved_skill as _skill + + +def _predefined_provider( + skills: list[SkillMetadata] | None = None, + contents: dict[str, str] | None = None, +) -> MagicMock: + skills = skills or [] + contents = contents or {} + provider = MagicMock(spec=AgentSkillsProvider) + provider.order = AgentSkillsProvider.order + provider.display_name = AgentSkillsProvider.display_name + provider.resolved_skills = [ + _skill(f"predefined:{m.name}", m.name, m.description, contents[m.name]) for m in skills + ] + return provider + + +def _dial_skills_context( + urls_and_names: list[tuple[str, str]], + read_result: str = "file body", + content: str = "body", + files: tuple[str, ...] = (), +) -> tuple[_DialSkillsContext, MagicMock]: + """A dial-skills context whose skills carry a real (client-mocked) reader.""" + client = MagicMock() + client.read_text_file = AsyncMock(return_value=read_result) + reader = DialSkillReader(client) + context = _DialSkillsContext() + context.extend_resolved_skills( + [ + _skill(url, name, content=content, files=files, reader=reader) + for url, name in urls_and_names + ] + ) + return context, client + + +class TestMerge: + + @pytest.mark.asyncio + async def test_dial_skill_appears_in_available_skills(self): + dial_context, _ = _dial_skills_context([("skills/b/refunds", "refunds")]) + registry = SkillsRegistry(providers=[_predefined_provider(), dial_context]) + + xml = await registry.get_prompt_part() + + assert "refunds" in xml + + def test_predefined_wins_over_dial_skill(self): + predefined = [SkillMetadata(name="shared", description="predefined")] + predefined_provider = _predefined_provider(predefined, {"shared": "predefined"}) + dial_context, _ = _dial_skills_context([("skills/b/shared", "shared")]) + registry = SkillsRegistry(providers=[predefined_provider, dial_context]) + + assert registry.get_skill_content("shared") == "predefined" + assert "already provided by predefined skills" in registry.collision_exceptions[0].reason + + def test_dial_prompt_wins_over_dial_skill(self): + prompt_context = _DialPromptSkillsContext() + prompt_context.extend_resolved_skills( + [_skill("prompts/b/shared", "shared", content="from prompt")] + ) + dial_context, _ = _dial_skills_context([("skills/b/shared", "shared")]) + registry = SkillsRegistry(providers=[_predefined_provider(), prompt_context, dial_context]) + + assert registry.get_skill_content("shared") == "from prompt" + assert "already provided by DIAL prompt skills" in registry.collision_exceptions[0].reason + + def test_all_three_providers_coexist(self): + prompt_context = _DialPromptSkillsContext() + prompt_context.extend_resolved_skills([_skill("prompts/b/p", "from-prompt")]) + dial_context, _ = _dial_skills_context([("skills/b/s", "from-skill")]) + predefined_provider = _predefined_provider( + [SkillMetadata(name="predef", description="d")], {"predef": "body"} + ) + registry = SkillsRegistry(providers=[predefined_provider, prompt_context, dial_context]) + + for name in ("predef", "from-prompt", "from-skill"): + assert registry.get_skill_content(name) + + def test_precedence_is_independent_of_provider_list_order(self): + predefined = [SkillMetadata(name="shared", description="predefined")] + predefined_provider = _predefined_provider(predefined, {"shared": "predefined"}) + dial_context, _ = _dial_skills_context([("skills/b/shared", "shared")]) + + forward = SkillsRegistry(providers=[predefined_provider, dial_context]) + reversed_order = SkillsRegistry(providers=[dial_context, predefined_provider]) + + assert forward.get_skill_content("shared") == "predefined" + assert reversed_order.get_skill_content("shared") == "predefined" + + +class TestReadSkillFile: + + @pytest.mark.asyncio + async def test_reads_an_advertised_file(self): + dial_context, _ = _dial_skills_context( + [("skills/b/s", "s")], read_result="# EU rules", files=("references/eu.md",) + ) + registry = SkillsRegistry(providers=[dial_context]) + + assert await registry.read_skill_file("s", "references/eu.md") == "# EU rules" + + @pytest.mark.asyncio + async def test_unadvertised_path_is_refused_with_the_inventory(self): + dial_context, _ = _dial_skills_context([("skills/b/s", "s")], files=("references/eu.md",)) + registry = SkillsRegistry(providers=[dial_context]) + + with pytest.raises(SkillFileNotFoundError) as exc: + await registry.read_skill_file("s", "assets/logo.png") + + assert "references/eu.md" in str(exc.value) + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "path", + [ + "../../../other-bucket/their-skill/files/SKILL.md", + "/etc/passwd", + ".env", + "references/../../escape.md", + ], + ) + async def test_traversal_and_hidden_paths_are_refused(self, path: str): + # Containment is inventory membership: nothing that was not advertised + # can be reached, whatever its shape. + dial_context, _ = _dial_skills_context([("skills/b/s", "s")], files=("references/eu.md",)) + registry = SkillsRegistry(providers=[dial_context]) + + with pytest.raises(SkillFileNotFoundError): + await registry.read_skill_file("s", path) + + @pytest.mark.asyncio + async def test_manifest_path_returns_the_manifest(self): + dial_context, _ = _dial_skills_context( + [("skills/b/s", "s")], content="# Manifest", files=("a.md",) + ) + registry = SkillsRegistry(providers=[dial_context]) + + assert await registry.read_skill_file("s", "SKILL.md") == "# Manifest" + + @pytest.mark.asyncio + async def test_predefined_skill_has_no_bundled_files(self): + predefined_provider = _predefined_provider( + [SkillMetadata(name="predef", description="d")], {"predef": "body"} + ) + registry = SkillsRegistry(providers=[predefined_provider]) + + with pytest.raises(SkillFilesNotSupportedError, match="has no bundled files"): + await registry.read_skill_file("predef", "references/eu.md") + + @pytest.mark.asyncio + async def test_unknown_skill_raises(self): + registry = SkillsRegistry(providers=[]) + + with pytest.raises(FileNotFoundError, match="Skill not found"): + await registry.read_skill_file("nope", "a.md") + + @pytest.mark.asyncio + async def test_repeat_read_is_memoized(self): + dial_context, client = _dial_skills_context([("skills/b/s", "s")], files=("a.md",)) + registry = SkillsRegistry(providers=[dial_context]) + + await registry.read_skill_file("s", "a.md") + await registry.read_skill_file("s", "a.md") + + assert client.read_text_file.await_count == 1