feat: skills as DIAL resource (#418) - #524
Merged
Merged
Conversation
andrii-novikov
force-pushed
the
feat/418-skills-as-dial-resource
branch
from
August 28, 2026 14:05
7253fda to
a2b5914
Compare
andrii-novikov
changed the title
andrii-novikov
force-pushed
the
feat/418-skills-as-dial-resource
branch
from
August 31, 2026 09:54
cdbd4f3 to
775e3a0
Compare
Designs a minimal first cut at consuming DIAL Core's folder-shaped skill resources (epam/ai-dial-core#1633), scoped to ship without refactoring anything that already works. Phase 1: a new `dial-skill` config type over `skills/<bucket>/<path>`, resolved per request into a manifest plus an inventory of the skill's bundled text files, and `read_skill(skill_name, file_path?)` for progressive disclosure — one round-trip per file the model actually asks for. `dial_skills/` mirrors the existing `dial_prompt_skills/` package shape; `SkillsRegistry` gains one merge pass and one async method. Predefined skills and `dial-prompt` are untouched. Readability is enforced by inventory membership: a `file_path` is served only if it was advertised, which subsumes traversal, encoded-separator and dotfile handling without a bespoke validator. Deferred to phase 2 with reasons: binary/asset files, cross-request caching, `read_skill` offload exclusion, editor skill browsing. No QuickApps-side validator for `dial-skill` — Core validates `SKILL.md` on write — though `/skills/validate` must be narrowed to the prompt config so the published schema stops advertising a type it rejects. Records the two blockers verified against the source: the `client.skills` resource is written but unreleased in ai-dial-client-python, and ai-dial-core has no skills auto-share (no `getSkills`, `shareApplicationSkills`, or `ApiKeyData.attachedSkills`), which limits phase 1 to skills the caller's own key can already reach. Refs #418 Claude-Session: https://claude.ai/code/session_011mUSHykzn4b9y97fpjkskn
Phase 1 of the design in docs/designs/skills_as_dial_resource.md: an app config can reference a DIAL skill resource, and the agent can open the files bundled with it on demand. A new `dial-skill` config type over `skills/<bucket>/<path>` joins `SkillConfig`. At request initialization each configured skill costs one `SKILL.md` read plus one recursive file listing; the listing is appended to the manifest as a `<skill_files>` block, so the model learns what the skill contains the moment it calls `read_skill`. It then opens one with `read_skill(skill_name, file_path)` — one round-trip per file it actually asks for, memoized for the rest of the request. `dial_skills/` deliberately mirrors the existing `dial_prompt_skills/` package shape rather than factoring out a shared lifecycle: the two are identical in outline, and unifying them would mean rewriting the prompt path. `SkillsRegistry` gains one merge pass and one async method. Predefined skills and `dial-prompt` are untouched. Readability is inventory membership: a `file_path` is served only if it was advertised, and the inventory is a text-extension allowlist minus folders, dotfiles and `SKILL.md`. That single check subsumes traversal, encoded separators and hidden-file leakage without a bespoke validator — the model can only ask for what it was told exists. Binary files are not advertised and not readable in this phase. `/skills/validate` is narrowed to `DialPromptSkillConfig` rather than gaining a `dial-skill` branch: Core validates `SKILL.md` on write, so a stored skill is already valid, and keeping the union would have published an OpenAPI advertising a request the handler answers 400 to. The dead `isinstance` check goes with it. Also corrects a stale claim in docs/skills.md: the `skills` config field is a plain `Field`, not a `PreviewField`, so it never required ENABLE_PREVIEW_FEATURES. Known incomplete, both recorded in the design doc: - `aidial-client` is still pinned to 0.16.x, which has no `skills` resource — it lives on an unreleased branch. Access funnels through one `TODO(#418)` shim, so this cannot run against a real DIAL Core until that client ships. - DIAL Core does not auto-share config-declared skills to the app's per-request key, so a `dial-skill` resolves only where the caller's own key already has access. Verified: `make lint` clean, 1959 unit tests pass (54 new). Refs #418 Claude-Session: https://claude.ai/code/session_011mUSHykzn4b9y97fpjkskn
Offloading a large read_skill result into a DIAL-file pointer would strip the manifest's <skill_files> inventory from the tool result the model sees, breaking progressive disclosure. Add internal_skills_read_skill to _MANDATORY_EXCLUDED_TOOLS alongside the existing read-back guards.
DialSkillConfig and DialSkillsModule were unconditionally active regardless of ENABLE_PREVIEW_FEATURES, exposing a config type whose resolution depends on an aidial-client attribute that does not exist in any released client version yet. Mark DialSkillConfig with @preview_model (strips it from the discriminated union / schema when preview is off) and DialSkillsModule with @preview_module (drops it from the DI module list), matching the pattern used by FolderContextConfig / other preview-gated modules.
Replace the local editable path dependency on aidial-client (which only resolved on a machine with both repos checked out side by side) with a git dependency pinned to the feat/skills-read branch of epam/ai-dial-client-python. This carries the skills resource (/v2/skills reads) the feature needs but that has not been released yet, and is resolvable by CI and other machines. Still TEMPORARY - MUST NOT MERGE: must be swapped for a released version once aidial-client ships the skills resource.
andrii-novikov
force-pushed
the
feat/418-skills-as-dial-resource
branch
from
August 31, 2026 10:10
472fea5 to
713db26
Compare
andrii-novikov
changed the title
Status: Draft -> Approved. The D-1 client-release gate remains open (aidial-client feat/skills-read is still unmerged/unreleased) and is tracked as an implementation/merge blocker, not a design defect.
- docs: clarify dynamic skill registration is Partial (DIAL-prompt and DIAL-skill sources resolve per request) - fix stage title formatting (remove spaces around /) - remove stale TODO comment about pinned aidial-client version
andrii-novikov
marked this pull request as ready for review
August 31, 2026 13:47
korotaav48
requested changes
Sep 1, 2026
korotaav48
left a comment
korotaav48
left a comment
Contributor
There was a problem hiding this comment.
The core architectural issue is that the new dial_skills module leaks
into the existing skills layer — SkillsRegistry imports ResolvedDialSkill, _DialSkillsContext and MANIFEST_NAME directly from dial_skills
SkillsRegistry previously reached into dial_skills' private submodules for ResolvedDialSkill, _DialSkillsContext and MANIFEST_NAME, and did the bundled-file read itself. dial_skills likewise reached into skills' private _frontmatter/_skill_metadata modules. - skills/__init__.py now exports SkillMetadata, parse_frontmatter and SkillFileNotFoundError as its public surface - dial_skills/__init__.py exports DialSkillReader, DialSkillResolver, ResolvedDialSkill and _DialSkillsContext - New DialSkillReader owns the manifest/inventory check, path normalization and cache-then-fetch for one bundled file; the cache is its own private state (request-scoped, used by no one else) - _DialSkillsContext is pure state again (resolved_skills + exceptions), mirroring _DialPromptSkillsContext - SkillsRegistry.read_skill_file collapses to a merge lookup plus one delegated call - DialSkillResolver._fetch_one results now pair with their own URL via _fetch_labeled instead of correlating asyncio.gather output by index - rename build_skill_files_block to _build_skill_files_block (internal) Addresses PR #524 review comments.
4 tasks
Follows aidial-client's move to chained references: get_file(url, path) and list_files(url, ...) become skills(url=...).files(path=...).read() and .files.list(...). The files reference is built once per listing, so the url is parsed once rather than on every page. Replaces the MagicMock-based test doubles with hand-written fakes: skills.files.list() and skills.files(path=...).read() land on different auto-created mocks, so stubbing one silently failed to intercept the other. The fakes mirror the real signatures, so drift surfaces as a TypeError instead of a passing test. Refs #418
This was referenced Sep 8, 2026
Closed
sovadim
previously approved these changes
Sep 10, 2026
Replace the temporary git-branch override (feat/skills-read) with the released aidial-client 0.17.0, which ships the skills resource. Claude-Session: https://claude.ai/code/session_01TqzW8dqXH92sC28kX6WoxQ
andrii-novikov
changed the title
Collaborator
Author
|
/deploy-review
|
Collaborator
Author
|
/deploy-review
|
2 tasks
VinShurik
approved these changes
Sep 14, 2026
korotaav48
approved these changes
Sep 14, 2026
andrii-novikov
enabled auto-merge (squash)
September 14, 2026 08:28
andrii-novikov
changed the title
andrii-novikov
added a commit
that referenced
this pull request
Sep 14, 2026
Update docs/designs/skill_invocation.md to match the current repo state: - skills_as_dial_resource.md's dependency (feat/418) is merged to development (#524), not pending - document the known gap where a skill picked on a non-first user message doesn't survive the next turn, matching what docs/skills.md and the injector's docstring already say #549
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applicable issues
Description of changes
Phase 1 of the design in
docs/designs/skills_as_dial_resource.md: an app config can reference a DIAL skill resource — a folder holdingSKILL.mdplus bundled text files stored via Core's/v2/skillsAPI — and the agent reads it on demand with progressive disclosure.dial-skillconfig type overskills/<bucket>/<path>joinsSkillConfig. At request initialization each configured skill costs oneSKILL.mdread plus one recursive file listing; the listing is appended to the manifest as a<skill_files>block, so the model learns what the skill contains the moment it callsread_skill. It then opens a file withread_skill(skill_name, file_path)— one round-trip per file it actually asks for, memoized for the rest of the request.dial_skills/mirrors the existingdial_prompt_skills/package shape rather than factoring out a shared lifecycle (the two are identical in outline; unifying them would mean rewriting the prompt path).SkillsRegistrygains one merge pass and one async method. Predefined skills anddial-promptare untouched, and precedence stayspredefined > dial-prompt > dial-skill.file_pathis served only if it was advertised in the listing, and the inventory is a text-extension allowlist minus folders, dotfiles, andSKILL.md. That single check subsumes traversal, encoded separators, and hidden-file leakage without a bespoke validator. Binary files are not advertised and not readable in this phase./skills/validateis narrowed toDialPromptSkillConfigrather than gaining adial-skillbranch: Core validatesSKILL.mdon write, so a stored skill is already valid, and keeping the union would have published an OpenAPI advertising a request the handler answers 400 to.read_skillis excluded fromtool_call_result_offload: offloading a largeread_skillresult into a DIAL-file pointer would strip the manifest's<skill_files>inventory from the tool result the model actually sees, breaking progressive disclosure.DialSkillConfigandDialSkillsModuleare gated behindENABLE_PREVIEW_FEATURES(@preview_model/@preview_module) — they were unconditionally active, which would have exposed a config type whose resolution depends on anaidial-clientattribute not present in any released client version.docs/skills.md: theskillsconfig field is a plainField, not aPreviewField, so it never requiredENABLE_PREVIEW_FEATURES.Known incomplete / temporary (tracked in the design doc, Status still
Draft):aidial-clientis pinned to thefeat/skills-readbranch ofepam/ai-dial-client-python(not a release) — the branch carries theskillsresource this feature needs, but a released version does not exist yet. This is markedTEMPORARY - MUST NOT MERGEinpyproject.tomland must be swapped for a released version before this ships.Checklist
Draft— not yet approved, andaidial-clientrelease is a blocking dependency before this can merge)dial-skillconfig type is additive and preview-gated)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.