From 4cdbe2a363b93fc527225ccc0133961a35e32661 Mon Sep 17 00:00:00 2001 From: David Bernheisel Date: Sun, 4 Oct 2026 18:56:33 -0400 Subject: [PATCH 1/4] Skills: serve the MCP Skills extension Route skill:// paths to actions that return a Phantom.Skill, and serve skills/list, skills/get, resources/read, and resources/directory/read. --- .formatter.exs | 3 + CHANGELOG.md | 8 + README.md | 40 ++ lib/phantom/cache.ex | 41 ++ lib/phantom/request.ex | 5 + lib/phantom/resource_plug.ex | 7 + lib/phantom/router.ex | 124 +++- lib/phantom/router/skills.ex | 336 ++++++++++ lib/phantom/skill.ex | 581 ++++++++++++++++++ mix.exs | 1 + mix.lock | 1 + test/phantom/skill_edge_test.exs | 387 ++++++++++++ test/phantom/skill_plug_test.exs | 82 +++ test/phantom/skill_router_test.exs | 430 +++++++++++++ test/phantom/skill_test.exs | 273 ++++++++ test/support/skills/git-workflow/SKILL.md | 7 + test/support/skills/refunds/SKILL.md.eex | 10 + test/support/skills/refunds/assets/logo.png | Bin 0 -> 1921 bytes test/support/skills/refunds/examples/email.md | 3 + .../skills/refunds/partial-refunds/SKILL.md | 5 + test/support/skills/study-review/SKILL.md.eex | 5 + 21 files changed, 2343 insertions(+), 6 deletions(-) create mode 100644 lib/phantom/router/skills.ex create mode 100644 lib/phantom/skill.ex create mode 100644 test/phantom/skill_edge_test.exs create mode 100644 test/phantom/skill_plug_test.exs create mode 100644 test/phantom/skill_router_test.exs create mode 100644 test/phantom/skill_test.exs create mode 100644 test/support/skills/git-workflow/SKILL.md create mode 100644 test/support/skills/refunds/SKILL.md.eex create mode 100644 test/support/skills/refunds/assets/logo.png create mode 100644 test/support/skills/refunds/examples/email.md create mode 100644 test/support/skills/refunds/partial-refunds/SKILL.md create mode 100644 test/support/skills/study-review/SKILL.md.eex diff --git a/.formatter.exs b/.formatter.exs index c5c4e9e..7d2640f 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -7,6 +7,9 @@ locals_without_parens = [ resource: 2, resource: 3, resource: 4, + skill: 2, + skill: 3, + embed_skills: 1, input_schema: 1, field: 2, field: 3, diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bfc813..6e9b34d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,14 @@ only runs as a task when the client did not declare the extension. - `Phantom.Request.missing_capability/1` accepts a `ClientCapabilities` map, for capabilities whose names contain dots. +- New: Skills, with the MCP Skills extension (`io.modelcontextprotocol/skills`). + `Phantom.Router.skill/3` routes a `skill://` path to an action that returns a + `Phantom.Skill`, and `Phantom.Skill.embed_skills/1` compiles skill + directories, rendering `.eex` files with assigns. Phantom serves + `skills/list`, `skills/get`, `resources/read`, and `resources/directory/read`. + Files render lazily, and caching is opt-in with `Phantom.Skill.with_cache/2`. + `Phantom.Cache.add_skill/2` adds a skill at runtime. The `skill://` scheme is + reserved for skills. Streamable HTTP transport fixes for the protocol versions before 2026-07-28: diff --git a/README.md b/README.md index cfdaf94..2b60f9f 100644 --- a/README.md +++ b/README.md @@ -537,6 +537,45 @@ It may return the allowed tuples or just their URI strings. Returning `nil` or ` returning an invalid value rejects the resources. URIs that do not resolve to an available resource template are rejected before the callback runs. +## Defining Skills + +Phantom serves [Agent Skills](https://agentskills.io/) with the MCP Skills +extension. Route a skill path under `skill://` to an action, the way a +Phoenix router routes to a controller: + +```elixir +defmodule MyApp.MCP.Router do + # ... + + # skill://acme/billing/refunds/SKILL.md + skill "acme/billing/refunds", MyApp.MCP.Skills, :refunds + skill "studies/:study_id/study-review", MyApp.MCP.Skills, :study_review +end +``` + +The action returns a `Phantom.Skill`. `embed_skills/1` compiles skill +directories into functions; files ending in `.eex` render with assigns. + +```elixir +defmodule MyApp.MCP.Skills do + use Phantom.Skill + + # skills/refunds/SKILL.md.eex, skills/refunds/examples/email.md, ... + embed_skills "skills/*" + + def refunds(_params, session) do + {:reply, refunds(%{user: session.assigns.user}), session} + end +end +``` + +Phantom calls the action for `skills/list`, `skills/get`, `resources/read`, +and `resources/directory/read`, and computes the digests clients verify from +what it returns. A file can be a binary or a function that renders it when +needed. Caching is opt-in with `Phantom.Skill.with_cache/2`. Parsing +`SKILL.md` frontmatter needs the optional `{:yamerl, "~> 0.10"}` dependency. +To add a skill at runtime, use `Phantom.Cache.add_skill/2`. See `Phantom.Skill`. + ## Eliciting input Two helpers, picked by how the dev wants to structure the handler. @@ -702,6 +741,7 @@ Phantom will implement these MCP requests on your behalf: - `resources/read` dispatch the request to your handler. `Phantom.Resource`. - `resources/subscribe` available if the MCP router is configured with `pubsub`. To notify of updates, prefer `Phantom.Tracker.notify_resources_updated(uris)`; `notify_resource_updated(uri)` remains available for individual changes. - `resources/unsubscribe` see above. +- `skills/list`, `skills/get`, and `resources/directory/read` serve skills from your router. Skills are resource templates, so `allow_resource_templates/2` limits them too. Read more in `Phantom.Skill`. - `logging/setLevel` available if the MCP router is configured with `pubsub`. Logs can be sent to client with `Session.log_{level}(session, map_content)`. [See docs](https://modelcontextprotocol.io/specification/20.6.03-26/server/utilities/logging#log-levels). - `tools/list` list either the allowed tools as provided in the `connect/2` callback or all tools by default. To disable, return `allow_tools(session, [])` in the `connect/2` callback. - `tools/call` dispatch the request to your handler. Read more in `Phantom.Tool`. diff --git a/lib/phantom/cache.ex b/lib/phantom/cache.ex index 4d5577e..d9c6838 100644 --- a/lib/phantom/cache.ex +++ b/lib/phantom/cache.ex @@ -74,6 +74,47 @@ defmodule Phantom.Cache do Phantom.Router.__create_resource_routers__(resource_templates, __CALLER__) end + @doc """ + Add a skill for the given router. Takes the same arguments as + `Phantom.Router.skill/3`: + + Phantom.Cache.add_skill(MyApp.MCP.Router, + path: "acme/billing/refunds", + handler: MyApp.MCP.Skills, + function: :refunds + ) + + This regenerates the router's skill routes. + """ + def add_skill(router, skill_spec) do + skill = skill_spec |> Map.new() |> Map.put(:router, router) |> Phantom.Router.skill_template() + validate!([skill]) + register(router) + + # Serialize updates on this node, so concurrent calls don't lose a skill or + # regenerate the routes over each other. + :global.trans( + {{__MODULE__, router}, self()}, + fn -> + existing = :persistent_term.get({Phantom, router, :resource_templates}, []) + resource_templates = Enum.sort_by(Enum.uniq([skill | existing]), & &1.name) + raise_if_duplicates(resource_templates) + + # Regenerating the routes redefines the module on purpose; don't warn about it. + Code.with_diagnostics(fn -> + resource_templates + |> Enum.filter(&(&1.scheme == "skill")) + |> Phantom.Router.__create_resource_routers__(__ENV__) + end) + + :persistent_term.put({Phantom, router, :resource_templates}, resource_templates) + end, + [node()] + ) + + :ok + end + @doc """ List all the entities for the given type. """ diff --git a/lib/phantom/request.ex b/lib/phantom/request.ex index e5eac8c..07468b4 100644 --- a/lib/phantom/request.ex +++ b/lib/phantom/request.ex @@ -42,12 +42,15 @@ defmodule Phantom.Request do resources/list resources/templates/list resources/read + resources/directory/read completion/complete subscriptions/listen notifications/cancelled tasks/get tasks/update tasks/cancel + skills/list + skills/get ] @modern_cacheable_methods ~w[ @@ -57,6 +60,8 @@ defmodule Phantom.Request do resources/list resources/templates/list resources/read + skills/list + skills/get ] import Phantom.Utils diff --git a/lib/phantom/resource_plug.ex b/lib/phantom/resource_plug.ex index 11f6914..0be74d4 100644 --- a/lib/phantom/resource_plug.ex +++ b/lib/phantom/resource_plug.ex @@ -20,6 +20,13 @@ defmodule Phantom.ResourcePlug do ) end + def call(%{assigns: %{resource_template: %{scheme: "skill"} = template}} = fake_conn, _opts) do + %{session: session, uri: uri} = fake_conn.assigns + session = %{session | request: %{session.request | spec: template}} + result = Phantom.Router.Skills.read(template, fake_conn.path_params, uri, session) + assign(fake_conn, :result, wrap(result, uri, session)) + end + def call(fake_conn, _opts) do session = %{ fake_conn.assigns.session diff --git a/lib/phantom/router.ex b/lib/phantom/router.ex index c9d8844..ce00c7d 100644 --- a/lib/phantom/router.ex +++ b/lib/phantom/router.ex @@ -41,6 +41,7 @@ defmodule Phantom.Router do alias Phantom.Resource alias Phantom.ResourceTemplate alias Phantom.Session + alias Phantom.Skill alias Phantom.Tasks alias Phantom.Tool alias Phantom.Tool.JSONSchema @@ -268,7 +269,9 @@ defmodule Phantom.Router do resource: 3, resource: 4, prompt: 2, - prompt: 3 + prompt: 3, + skill: 2, + skill: 3 ] require Phantom.ClientLogger @@ -505,7 +508,8 @@ defmodule Phantom.Router do |> Phantom.Router.resource_capability(__MODULE__, session) |> Phantom.Router.completion_capability(__MODULE__, session) |> Phantom.Router.logging_capability(__MODULE__, session) - |> Phantom.Router.ui_capability(__MODULE__, session), + |> Phantom.Router.ui_capability(__MODULE__, session) + |> Phantom.Router.skill_capability(__MODULE__, session), serverInfo: server_info, instructions: instructions }, session} @@ -534,6 +538,7 @@ defmodule Phantom.Router do |> Phantom.Router.logging_capability(__MODULE__, session) |> Phantom.Router.ui_capability(__MODULE__, session) |> Phantom.Router.tasks_capability(__MODULE__) + |> Phantom.Router.skill_capability(__MODULE__, session) {:reply, %{ @@ -672,6 +677,25 @@ defmodule Phantom.Router do list_resources(params["cursor"], %{session | request: request}) end + def dispatch_method("skills/list", params, request, session) do + Phantom.Router.Skills.list(__MODULE__, %{session | request: request}, params["cursor"]) + end + + def dispatch_method("skills/get", %{"uri" => uri}, request, session) + when is_binary(uri) do + Phantom.Router.Skills.get(__MODULE__, %{session | request: request}, uri) + end + + def dispatch_method("resources/directory/read", %{"uri" => uri}, request, session) + when is_binary(uri) do + Phantom.Router.Skills.read_directory(__MODULE__, %{session | request: request}, uri) + end + + def dispatch_method(method, _params, _request, session) + when method in ["skills/get", "resources/directory/read"] do + {:error, Request.invalid_params(%{uri: "is required"}), session} + end + def dispatch_method("notification" <> type, _params, _request, session) do {:reply, nil, session} end @@ -913,6 +937,9 @@ defmodule Phantom.Router do {:ok, %{scheme: "ui"}} -> raise "The ui:// scheme is reserved for MCP Apps" + {:ok, %{scheme: "skill"}} -> + raise "The skill:// scheme is reserved for skills, see `Phantom.Router.skill/3`" + {:ok, %{scheme: scheme, host: host, path: path}} when is_binary(scheme) and (is_binary(path) or (is_binary(host) and host != "")) -> scheme @@ -952,6 +979,64 @@ defmodule Phantom.Router do end end + @doc """ + Route a skill to an action that returns a `Phantom.Skill`. + + The path locates the skill under `skill://` and ends in the skill's name. + Segments after the first may be path params. The function defaults to the + skill's name with hyphens replaced by underscores. + + skill "git-workflow", MyApp.MCP.Skills + skill "acme/billing/refunds", MyApp.MCP.Skills, :refunds + skill "studies/:study_id/study-review", MyApp.MCP.Skills, :study_review + + See `Phantom.Skill` for writing actions. + """ + defmacro skill(path, handler, function \\ nil) do + meta = %{line: __CALLER__.line, file: __CALLER__.file} + + quote line: meta.line, file: meta.file, generated: true do + @phantom_resource_templates Phantom.Router.skill_template( + path: unquote(path), + handler: unquote(handler), + function: unquote(function), + router: __MODULE__, + meta: unquote(Macro.escape(meta)) + ) + end + end + + @doc false + # The resource template that routes a skill's files to its action. + def skill_template(attrs) do + attrs = Map.new(attrs) + [first | rest] = segments = String.split(attrs.path, "/") + + if String.starts_with?(first, ":") do + raise ArgumentError, + "skill #{inspect(attrs.path)}: the first segment of a skill path can't be a path param" + end + + if not (Regex.match?(~r/\A[a-z0-9._~-]+\z/, first) and + Enum.all?(rest, &Regex.match?(~r/\A(:[a-z_][A-Za-z0-9_]*|[A-Za-z0-9._~-]+)\z/, &1))) do + raise ArgumentError, + "invalid skill path #{inspect(attrs.path)}: segments are letters, digits, " <> + "- . _ ~ or path params, and the first segment is lowercase" + end + + name = List.last(segments) + Skill.validate_name!(name, "skill #{inspect(attrs.path)}") + + ResourceTemplate.build( + uri: "skill://#{attrs.path}/*file", + name: attrs.path, + router: Module.concat([attrs.router, ResourceRouter, "Skill"]), + handler: attrs.handler, + function: attrs[:function] || String.to_atom(String.replace(name, "-", "_")), + meta: attrs[:meta] || %{file: "nofile", line: 0} + ) + end + @doc false defp maybe_register_app(name, opts, caller) do {app_module, opts} = Keyword.pop(opts, :app) @@ -1236,7 +1321,8 @@ defmodule Phantom.Router do plug :match plug :dispatch - for resource_template <- unquote(Macro.escape(resource_templates)) do + for resource_template <- + unquote(Macro.escape(sort_skill_routes(resource_templates))) do match(Phantom.ResourceTemplate.route(resource_template), to: Phantom.ResourcePlug, assigns: %{resource_template: resource_template} @@ -1252,6 +1338,14 @@ defmodule Phantom.Router do ) end + # A skill's route globs its files, so a skill nested within it must match first. + defp sort_skill_routes([%ResourceTemplate{scheme: "skill"} | _] = skills) do + Phantom.Router.Skills.validate_nesting!(skills) + Enum.sort_by(skills, &Phantom.Router.Skills.route_order/1) + end + + defp sort_skill_routes(resource_templates), do: resource_templates + @doc """ Constructs a response map for the given resource with the provided parameters. This function is provided to your MCP Router that accepts the session instead. @@ -1421,7 +1515,8 @@ defmodule Phantom.Router do def validate_task_subscriptions(_notifications, _session), do: :ok - defp resolve_resource(router, session, uri) when is_binary(uri) do + @doc false + def resolve_resource(router, session, uri) when is_binary(uri) do with {:ok, %{scheme: scheme} = uri_struct} when is_binary(scheme) <- URI.new(uri), resource_router when not is_nil(resource_router) <- get_resource_router(router, session, scheme) do @@ -1443,7 +1538,7 @@ defmodule Phantom.Router do _ -> :error end - defp resolve_resource(_router, _session, _uri), do: :error + def resolve_resource(_router, _session, _uri), do: :error defp normalize_authorized_resources(_resources, nil), do: [] @@ -1544,6 +1639,21 @@ defmodule Phantom.Router do end end + @doc false + def skill_capability(capabilities, router, session) do + if Enum.any?(Cache.list(session, router, :resource_templates), &(&1.scheme == "skill")) do + extensions = Map.get(capabilities, :extensions, %{}) + + Map.put( + capabilities, + :extensions, + Map.put(extensions, "io.modelcontextprotocol/skills", %{directoryRead: true}) + ) + else + capabilities + end + end + @doc false def completion_capability(capabilities, router, session) do resource_templates = Cache.list(session, router, :resource_templates) @@ -1722,7 +1832,8 @@ defmodule Phantom.Router do def encode_request_state(result, _session), do: result - defp paginate(entities, cursor, fun) do + @doc false + def paginate(entities, cursor, fun) do if not is_nil(cursor) and not Enum.any?(entities, &(&1.name == cursor)) do {:error, Request.invalid_params(%{cursor: "Invalid cursor"})} else @@ -1779,6 +1890,7 @@ defmodule Phantom.Router do result = session |> Cache.list(router, :resource_templates) + |> Enum.reject(&(&1.scheme == "skill")) |> paginate(cursor, &ResourceTemplate.to_json/1) case result do diff --git a/lib/phantom/router/skills.ex b/lib/phantom/router/skills.ex new file mode 100644 index 0000000..df20aa0 --- /dev/null +++ b/lib/phantom/router/skills.ex @@ -0,0 +1,336 @@ +defmodule Phantom.Router.Skills do + @moduledoc false + # Serves the MCP Skills extension from a router's skill routes, which are + # resource templates under `skill:///*file`. + + require Logger + + alias Phantom.Cache + alias Phantom.Request + alias Phantom.ResourceTemplate + alias Phantom.Session + alias Phantom.Skill + + ## Routes + + # A route's path segments, without its `*file` glob. + def segments(%ResourceTemplate{authority: authority, path: path}) do + [authority | path |> String.replace_suffix("/*file", "") |> String.split("/", trim: true)] + end + + # Deepest first; at the same depth, static segments before path params. The + # generated resource router matches in this order. + def route_order(route), do: {-length(segments(route)), Enum.map(segments(route), ¶m?/1)} + + defp param?(":" <> _), do: true + defp param?(_segment), do: false + + defp skill_routes(routes) do + routes |> Enum.filter(&(&1.scheme == "skill")) |> Enum.sort_by(&route_order/1) + end + + # A route nested under another must not have path params below it, since its + # files are part of the other skill's manifest. + def validate_nesting!(routes) do + for outer <- routes, inner <- routes, outer != inner do + {prefix, rest} = Enum.split(segments(inner), length(segments(outer))) + + if length(rest) > 0 and compatible?(segments(outer), prefix) and Enum.any?(rest, ¶m?/1) do + raise ArgumentError, + "skill #{inspect(inner.name)} is nested in skill #{inspect(outer.name)}, " <> + "so it can't have path params below #{inspect(outer.name)}" + end + end + + :ok + end + + defp compatible?(a, b) do + length(a) == length(b) and + Enum.all?(Enum.zip(a, b), fn {x, y} -> x == y or param?(x) or param?(y) end) + end + + defp match(route_segments, segments, params \\ %{}) + defp match([], [], params), do: {:ok, params} + + defp match([":" <> param | route_segments], [value | segments], params), + do: match(route_segments, segments, Map.put(params, param, value)) + + defp match([same | route_segments], [same | segments], params), + do: match(route_segments, segments, params) + + defp match(_route_segments, _segments, _params), do: :error + + defp concrete(route, params) do + Enum.map(segments(route), fn + ":" <> param -> params[param] + segment -> segment + end) + end + + defp base_uri(segments), do: "skill://" <> Enum.map_join(segments, "/", &Skill.encode_segment/1) + + # The routes serving `segments`, deepest first: its own route, then the route of + # each skill it's nested in. + defp chain(routes, segments) do + for depth <- length(segments)..1//-1, + prefix = Enum.take(segments, depth), + route = Enum.find(routes, &match?({:ok, _}, match(segments(&1), prefix))), + route != nil, + do: {route, prefix} + end + + # Routed skills nested directly in the skill at `segments`. + defp nested(routes, segments) do + depth = length(segments) + + candidates = + routes + |> Enum.flat_map(fn route -> + {prefix, rest} = Enum.split(segments(route), depth) + + if rest != [] and match?({:ok, _}, match(prefix, segments)), + do: [{route, segments ++ rest}], + else: [] + end) + |> Enum.uniq_by(&elem(&1, 1)) + + Enum.reject(candidates, fn {_, inner} -> + Enum.any?(candidates, fn {_, outer} -> + length(outer) < length(inner) and Enum.take(inner, length(outer)) == outer + end) + end) + end + + ## Actions + + # Returns `{:ok, skill, session}`, `{:absent, session}` or `{:error, error, session}`. + defp call(route, segments, session) do + {:ok, params} = match(segments(route), segments) + route |> apply_action(params, session) |> action_result(route) + end + + defp apply_action(route, params, session) do + apply(route.handler, route.function, [params, session]) + rescue + error in FunctionClauseError -> + if {error.module, error.function, error.arity} == {route.handler, route.function, 2}, + do: {:reply, nil, session}, + else: reraise(error, __STACKTRACE__) + end + + defp action_result({:reply, %Skill{frontmatter: %{"name" => name}} = skill, session}, route) do + if name == List.last(segments(route)) do + {:ok, skill, session} + else + Logger.error( + "#{inspect(route.handler)}.#{route.function}/2 returned the skill #{inspect(name)} " <> + "for #{inspect(route.name)}; a skill's name must be the last segment of its path" + ) + + {:error, Request.internal_error(), session} + end + end + + defp action_result({:reply, nil, session}, _route), do: {:absent, session} + defp action_result({:error, _error, %Session{}} = error, _route), do: error + + defp action_result(other, route) do + raise ArgumentError, + "#{inspect(route.handler)}.#{route.function}/2 must return " <> + "{:reply, %Phantom.Skill{} | nil, session} or {:error, error, session}; " <> + "skill actions are synchronous. Got: #{inspect(other)}" + end + + # The skill's files, with each nested skill's files in place of the skill's own + # copy of its directory. Returns the files, as in `Skill.manifest/2`, and every + # skill they come from. + defp tree(routes, skill, segments, session) do + own = Map.new(skill.files, fn {path, _content} -> {path, {skill, path}} end) + + Enum.reduce(nested(routes, segments), {own, [skill]}, fn {route, inner}, {files, skills} -> + case call(route, inner, session) do + {:ok, nested_skill, _session} -> + {nested_files, nested_skills} = tree(routes, nested_skill, inner, session) + dir = inner |> Enum.drop(length(segments)) |> Enum.join("/") + + files = + files + |> Map.reject(fn {path, _} -> String.starts_with?(path, dir <> "/") end) + |> Map.merge( + Map.new(nested_files, fn {path, served} -> {dir <> "/" <> path, served} end) + ) + + {files, skills ++ nested_skills} + + _absent_or_error -> + {files, skills} + end + end) + end + + defp entry(router, skill, segments, session) do + {files, skills} = + tree(skill_routes(Cache.list(nil, router, :resource_templates)), skill, segments, session) + + base_uri = base_uri(segments) + + resources = + if Enum.any?(skills, & &1.dynamic), + do: {:ok, "dynamic"}, + else: Skill.manifest(files, base_uri) + + case resources do + {:ok, resources} -> + {:ok, + %{uri: base_uri <> "/SKILL.md", frontmatter: skill.frontmatter, resources: resources}, + skills} + + {:error, message} -> + Logger.error("Skill #{base_uri} can't be served: #{message}") + {:error, Request.internal_error(), session} + end + end + + ## Methods + + def list(router, session, cursor) do + routes = + session + |> Cache.list(router, :resource_templates) + |> Enum.filter( + &(&1.scheme == "skill" and not Enum.any?(segments(&1), fn s -> param?(s) end)) + ) + + case Phantom.Router.paginate(routes, cursor, & &1) do + {:ok, page, next_cursor} -> + results = Enum.map(page, &list_entry(router, &1, session)) + listed = for {:ok, entry, skills} <- results, do: {entry, skills} + + # A route that serves no skill to this session makes the listing specific to it. + hints = + if length(listed) == length(page), + do: Skill.cache_hints(Enum.flat_map(listed, &elem(&1, 1)), session), + else: [ttl_ms: 0, scope: :private] + + {:reply, + %{skills: Enum.map(listed, &elem(&1, 0))} + |> Map.merge(next_cursor || %{}) + |> Request.with_cache(hints), session} + + {:error, error} -> + {:error, error, session} + end + end + + # A skill that fails is left out, and the rest are listed. + defp list_entry(router, route, session) do + segments = segments(route) + + with {:ok, skill, session} <- call(route, segments, session) do + entry(router, skill, segments, session) + end + catch + kind, reason -> + Logger.error( + "Skill #{inspect(route.name)} was left out of skills/list: " <> + Exception.format(kind, reason, __STACKTRACE__) + ) + + :error + end + + def get(router, session, uri) do + with {:ok, route, segments, "SKILL.md"} <- resolve(router, session, uri), + true <- route in Cache.list(session, router, :resource_templates), + {:ok, skill, session} <- call(route, segments, session), + {:ok, entry, skills} <- entry(router, skill, segments, session) do + {:reply, Request.with_cache(%{skill: entry}, Skill.cache_hints(skills, session)), session} + else + {:error, _error, %Session{}} = error -> error + _ -> {:error, invalid_params("No skill is served at #{uri}"), session} + end + end + + def read_directory(router, session, uri) do + with {:ok, _route, segments, dir} <- resolve(router, session, uri), + {:ok, skill, skill_segments, session} <- serving(router, segments, session), + routes = skill_routes(Cache.list(nil, router, :resource_templates)), + {files, _skills} = tree(routes, skill, skill_segments, session), + dir = relative(segments, dir, skill_segments), + {:ok, resources} <- Skill.list_directory(files, base_uri(skill_segments), dir) do + # Directories are listed whole, so a cursor is never needed. + {:reply, %{resources: resources}, session} + else + {:error, _error, %Session{}} = error -> error + _ -> {:error, invalid_params("#{uri} is not a directory resource"), session} + end + end + + # Called by `Phantom.ResourcePlug` for `resources/read` of a skill route's file. + def read(route, %{"file" => file} = params, uri, session) do + segments = concrete(route, Map.delete(params, "file")) + + with true <- Skill.uri(base_uri(segments), Enum.join(file, "/")) == uri, + {:ok, skill, skill_segments, session} <- + serving(session.router, segments, session), + {:ok, content} <- + Skill.read(skill, relative(segments, Enum.join(file, "/"), skill_segments), uri) do + {:reply, content, session} + else + {:error, _error, %Session{}} = error -> error + _ -> {:reply, nil, session} + end + end + + # Resolves a canonical skill URI through the router's resource router to its + # route, the route's segments, and the path within the skill. + defp resolve(router, session, uri) do + with {:ok, {^uri, %{"file" => file} = params, %ResourceTemplate{scheme: "skill"} = route}} <- + Phantom.Router.resolve_resource(router, session, uri), + segments = concrete(route, Map.delete(params, "file")), + path = Enum.join(file, "/"), + true <- Skill.uri(base_uri(segments), path) == uri do + {:ok, route, segments, path} + else + _ -> :error + end + end + + # The skill that serves a path under `segments`: the route's own skill, or when it + # serves none, the skill it's nested in. Each must be allowed for the session, or + # be nested in a skill that is. + defp serving(router, segments, session) do + allowed = Cache.list(session, router, :resource_templates) + chain = chain(skill_routes(Cache.list(nil, router, :resource_templates)), segments) + serve(chain, allowed, session, nil) + end + + defp serve([], _allowed, session, nil), do: {:absent, session} + defp serve([], _allowed, _session, error), do: error + + defp serve([{route, segments} | outer] = chain, allowed, session, error) do + if Enum.any?(chain, &(elem(&1, 0) in allowed)), + do: route |> call(segments, session) |> served(segments, outer, allowed, error), + else: {:absent, session} + end + + defp served({:ok, skill, session}, segments, _outer, _allowed, _error), + do: {:ok, skill, segments, session} + + defp served({:absent, session}, _segments, outer, allowed, error), + do: serve(outer, allowed, session, error) + + defp served({:error, _error, session} = new_error, _segments, outer, allowed, error), + do: serve(outer, allowed, session, error || new_error) + + # A path under `segments`, made relative to the skill at `skill_segments`. + defp relative(segments, path, skill_segments) do + segments + |> Enum.drop(length(skill_segments)) + |> Enum.concat(String.split(path, "/", trim: true)) + |> Enum.join("/") + end + + defp invalid_params(message), do: %{Request.invalid_params() | message: message} +end diff --git a/lib/phantom/skill.ex b/lib/phantom/skill.ex new file mode 100644 index 0000000..67550fb --- /dev/null +++ b/lib/phantom/skill.ex @@ -0,0 +1,581 @@ +defmodule Phantom.Skill do + @moduledoc """ + A skill served with the MCP Skills extension (`io.modelcontextprotocol/skills`). + + A skill is a directory of files with a `SKILL.md` at its root, as defined by the + [Agent Skills specification](https://agentskills.io/specification). Route a skill + path to an action in your `Phantom.Router`, and return a `%Phantom.Skill{}` from it, + much like a Phoenix controller renders a template: + + defmodule MyApp.MCP.Router do + use Phantom.Router, name: "MyApp", vsn: "1.0" + + # skill://git-workflow/SKILL.md → MyApp.MCP.Skills.git_workflow/2 + skill "git-workflow", MyApp.MCP.Skills + + skill "acme/billing/refunds", MyApp.MCP.Skills, :refunds + skill "studies/:study_id/study-review", MyApp.MCP.Skills, :study_review + end + + The final segment of the path is the skill's name, and must equal the `name` in its + frontmatter. Earlier segments organize skills and may contain path params, except the + first. Skills with path params are left out of `skills/list`, but clients can still + get them by URI. + + Each skill is a resource template named by its path, so + `Phantom.Session.allow_resource_templates/2` limits skills too. + + ## Actions + + An action receives the path params and the session, and returns + `{:reply, %Phantom.Skill{}, session}`. Return `{:reply, nil, session}` when no skill + is served at that path. Actions are synchronous. + + Phantom calls the same action for `skills/list`, `skills/get`, `resources/read`, and + `resources/directory/read`, and serves the part each method asks for. Digests are + computed from what the action returns, so the action must return the same files for + the same session. If it cannot, mark the skill with `dynamic/1`. + + ## Nested skills + + A skill routed under another skill's path is nested in it: the parent's manifest and + directory listings include the nested skill's files in place of the parent's own + files in that directory, and reads there go to the nested skill's action. When that + action returns `nil` or an error, the parent's own files are used instead. A session + allowed the parent may read the nested skill's files. + + ## Rendering + + A file's contents are either a binary or a function that returns iodata. A function + is only called when Phantom needs that file: `resources/read` renders the one file + read, while `skills/list` and `skills/get` render every file for their digests. + + ## Caching + + Results are private and uncached unless the action calls `with_cache/2`. Only + declare `scope: :public` when the skill is the same for every user who can see it. + + {:reply, git_workflow(%{}) |> Phantom.Skill.with_cache(ttl_ms: 300_000, scope: :public), + session} + + ## Embedding skills from files + + `embed_skills/1` compiles skill directories into functions, the way + `Phoenix.Component.embed_templates/1` compiles templates. Files ending in `.eex` are + rendered with assigns; the frontmatter of `SKILL.md` is parsed at compile time and + must not contain EEx. + + defmodule MyApp.MCP.Skills do + use Phantom.Skill + + # skills/refunds/SKILL.md.eex, skills/refunds/examples/email.md + # → refunds(assigns) + embed_skills "skills/*" + + def refunds(_params, session) do + {:reply, refunds(%{user: session.assigns.user}), session} + end + + def git_workflow(_params, session) do + {:reply, + Phantom.Skill.new( + %{name: "git-workflow", description: "Follow this team's Git conventions"}, + %{"SKILL.md" => "# Git workflow\\n\\nBranch from `main`.\\n"} + ), session} + end + end + + Parsing frontmatter requires the optional `:yamerl` dependency: + + {:yamerl, "~> 0.10"} + """ + + @compile {:no_warn_undefined, :yamerl_constr} + + @max_files 512 + @max_size 16_777_216 + @name_pattern ~r/\A[a-z0-9]+(-[a-z0-9]+)*\z/ + @frontmatter_pattern ~r/\A---[ \t]*\R(?.*?)^---[ \t]*(?:\R|\z)(?.*)\z/ms + @simple_key ~r/\A[A-Za-z0-9_-]+\z/ + + # `digests` is internal: digests of static files, computed by `embed_skills/1`. + defstruct frontmatter: %{}, files: %{}, dynamic: false, cache: nil, digests: %{} + + @type t :: %__MODULE__{ + frontmatter: %{required(String.t()) => term()}, + files: %{required(String.t()) => binary() | (-> iodata())}, + dynamic: boolean(), + cache: nil | [ttl_ms: non_neg_integer(), scope: :public | :private], + digests: %{optional(String.t()) => {non_neg_integer(), String.t()}} + } + + @doc false + defmacro __using__(_opts) do + quote do + import Phantom.Skill, only: [embed_skills: 1] + Module.register_attribute(__MODULE__, :phantom_skill_hashes, accumulate: true) + @before_compile Phantom.Skill + end + end + + @doc false + defmacro __before_compile__(env) do + hashes = Module.get_attribute(env.module, :phantom_skill_hashes) + + quote do + @doc false + def __mix_recompile__? do + Enum.any?(unquote(hashes), fn {pattern, hash} -> + Phantom.Skill.__files_hash__(pattern) != hash + end) + end + end + end + + @doc """ + Build a skill from its frontmatter and files. + + `frontmatter` must contain `name` and `description`; every other field passes through + to clients as written. `files` maps paths relative to the skill's root to their + contents, and must include `"SKILL.md"` without its frontmatter. Contents are iodata, + or a function that returns iodata when the file is needed. + """ + @spec new(map(), %{required(String.t()) => iodata() | (-> iodata())}) :: t() + def new(frontmatter, files) do + frontmatter = stringify_keys(frontmatter) + files = Map.new(files, fn {path, content} -> {path, normalize_content(content)} end) + + validate_frontmatter!(frontmatter) + Enum.each(Map.keys(files), &validate_path!/1) + + if not Map.has_key?(files, "SKILL.md") do + raise ArgumentError, "a skill must have a SKILL.md file" + end + + skill = %__MODULE__{frontmatter: frontmatter, files: files} + validate_limits!(skill) + skill + end + + defp validate_path!(path) do + segments = if is_binary(path), do: String.split(path, "/"), else: [""] + + if Enum.any?(segments, &(&1 in ["", ".", ".."] or String.contains?(&1, "\\"))) do + raise ArgumentError, + "invalid file path #{inspect(path)}: paths are relative to the skill's root, " <> + "separated by /, without empty, . or .. segments or backslashes" + end + end + + @doc """ + Mark a skill as generated such that its files can't have stable digests. + + Clients receive `"resources": "dynamic"` instead of a manifest, and some + clients decline to load such skills. + """ + @spec dynamic(t()) :: t() + def dynamic(%__MODULE__{} = skill), do: %{skill | dynamic: true} + + @doc """ + Let clients cache the skill's `skills/list` and `skills/get` entries. + + Both `:ttl_ms` and `:scope` (`:public` or `:private`) are required, with the + meaning of `Phantom.Request.with_cache/2`. + """ + @spec with_cache(t(), ttl_ms: non_neg_integer(), scope: :public | :private) :: t() + def with_cache(%__MODULE__{} = skill, opts) do + with {:ok, ttl_ms} when is_integer(ttl_ms) and ttl_ms >= 0 <- Keyword.fetch(opts, :ttl_ms), + {:ok, scope} when scope in [:public, :private] <- Keyword.fetch(opts, :scope) do + %{skill | cache: [ttl_ms: ttl_ms, scope: scope]} + else + _ -> + raise ArgumentError, + "with_cache/2 requires :ttl_ms, a non-negative integer, and :scope, :public or :private" + end + end + + @doc false + # Cache hints for a result built from these skills. A listing is only public when + # every skill is and the session sees every skill. + def cache_hints(skills, session) do + hints = Enum.map(skills, & &1.cache) + + if hints == [] or nil in hints do + [ttl_ms: 0, scope: :private] + else + public? = + is_nil(session.allowed_resource_templates) and + Enum.all?(hints, &(&1[:scope] == :public)) + + [ + ttl_ms: hints |> Enum.map(& &1[:ttl_ms]) |> Enum.min(), + scope: if(public?, do: :public, else: :private) + ] + end + end + + defp normalize_content(fun) when is_function(fun, 0), do: fun + defp normalize_content(iodata), do: IO.iodata_to_binary(iodata) + + @doc """ + Compile every skill directory matching `pattern` into a function. + + The pattern is relative to the current file. A directory with a `SKILL.md` or + `SKILL.md.eex` becomes a function named after the directory, with hyphens replaced + by underscores, that takes assigns and returns a `%Phantom.Skill{}`. + """ + defmacro embed_skills(pattern) do + pattern = Path.expand(pattern, Path.dirname(__CALLER__.file)) + + skill_dirs = + pattern + |> Path.wildcard() + |> Enum.filter( + &(File.exists?(Path.join(&1, "SKILL.md")) or File.exists?(Path.join(&1, "SKILL.md.eex"))) + ) + + if skill_dirs == [] do + raise ArgumentError, "no skills found matching #{pattern}" + end + + quote do + @phantom_skill_hashes {unquote(pattern), unquote(__files_hash__(pattern))} + unquote_splicing(Enum.map(skill_dirs, &embed_skill/1)) + end + end + + defp embed_skill(dir) do + name = Path.basename(dir) + files = dir |> Path.join("**/*") |> Path.wildcard() |> Enum.filter(&File.regular?/1) + + skill_md_path = + Enum.find([Path.join(dir, "SKILL.md.eex"), Path.join(dir, "SKILL.md")], &File.exists?/1) + + {frontmatter, body} = embed_skill_md(skill_md_path) + + skill_md = + case body do + {:static, body} -> {"SKILL.md", {:static, frontmatter_block(frontmatter) <> body}, body} + eex -> {"SKILL.md", eex, nil} + end + + embedded = [skill_md | for(path <- files, path != skill_md_path, do: embed_file(path, dir))] + + embedded + |> Enum.map(&elem(&1, 0)) + |> Enum.frequencies() + |> Enum.each(fn + {_path, 1} -> :ok + {path, _} -> raise ArgumentError, "#{dir}: more than one file is served at #{path}" + end) + + # SKILL.md is served with its frontmatter, so its digest covers both. + digests = + for {path, {:static, served}, _} <- embedded, + into: %{}, + do: {path, {byte_size(served), digest(served)}} + + contents = for {path, content, body} <- embedded, do: {path, embed_content(content, body)} + + quote do + unquote_splicing(Enum.map(files, "e(do: @external_resource(unquote(&1))))) + + def unquote(String.to_atom(String.replace(name, "-", "_")))(var!(assigns)) do + _ = var!(assigns) + + %{ + Phantom.Skill.new(unquote(Macro.escape(frontmatter)), %{unquote_splicing(contents)}) + | digests: unquote(Macro.escape(digests)) + } + end + end + end + + # Returns the parsed frontmatter and the body: `{:eex, quoted}` or `{:static, binary}`. + defp embed_skill_md(path) do + source = File.read!(path) + + {frontmatter, body} = + case parse(source) do + {:ok, frontmatter, body} -> {frontmatter, body} + {:error, reason} -> raise ArgumentError, "#{path}: #{reason}" + end + + header = binary_part(source, 0, byte_size(source) - byte_size(body)) + eex? = Path.extname(path) == ".eex" + + if eex? and String.contains?(header, "<%") do + raise ArgumentError, "#{path}: EEx is not allowed in frontmatter" + end + + validate_frontmatter!(frontmatter, path) + + name = path |> Path.dirname() |> Path.basename() + + if frontmatter["name"] != name do + raise ArgumentError, + "#{path}: frontmatter name #{inspect(frontmatter["name"])} must match its directory #{inspect(name)}" + end + + if eex?, + do: + {frontmatter, {:eex, EEx.compile_string(body, file: path, line: count_lines(header) + 1)}}, + else: {frontmatter, {:static, body}} + end + + # EEx renders lazily, in a function that closes over the assigns. A static SKILL.md + # is stored without its frontmatter, which `new/2` adds when it's served. + defp embed_content({:eex, quoted}, _body), do: quote(do: fn -> unquote(quoted) end) + defp embed_content({:static, _served}, body) when is_binary(body), do: body + defp embed_content({:static, served}, nil), do: served + + # Returns `{served_path, content, skill_md_body}`. A nested skill's SKILL.md is also + # served by its own route, so it's written out the same way to serve the same bytes + # at the same URI. + defp embed_file(path, dir) do + relative = Path.relative_to(path, dir) + served_path = if Path.extname(relative) == ".eex", do: Path.rootname(relative), else: relative + + cond do + Path.basename(served_path) == "SKILL.md" -> + content = + case embed_skill_md(path) do + {frontmatter, {:eex, quoted}} -> {:eex, [frontmatter_block(frontmatter), quoted]} + {frontmatter, {:static, body}} -> {:static, frontmatter_block(frontmatter) <> body} + end + + {served_path, content, nil} + + Path.extname(relative) == ".eex" -> + {served_path, {:eex, EEx.compile_file(path)}, nil} + + true -> + {served_path, {:static, File.read!(path)}, nil} + end + end + + defp count_lines(text), do: text |> :binary.matches("\n") |> length() + + @doc false + def __files_hash__(pattern) do + pattern + |> Path.wildcard() + |> Enum.flat_map(&[&1 | Path.wildcard(Path.join(&1, "**/*"))]) + |> Enum.sort() + |> :erlang.md5() + end + + @doc """ + Split a `SKILL.md` into its parsed frontmatter and its body. + """ + @spec parse(String.t()) :: {:ok, map(), String.t()} | {:error, String.t()} + def parse(skill_md) do + case Regex.named_captures(@frontmatter_pattern, skill_md) do + %{"yaml" => yaml, "body" => body} -> + with {:ok, frontmatter} <- parse_yaml(yaml), do: {:ok, frontmatter, body} + + nil -> + {:error, "SKILL.md must begin with YAML frontmatter between --- lines"} + end + end + + defp parse_yaml(yaml) do + if not Code.ensure_loaded?(:yamerl_constr) do + raise "parsing SKILL.md frontmatter requires the :yamerl dependency, add {:yamerl, \"~> 0.10\"} to your deps" + end + + try do + :yamerl_constr.string(yaml, str_node_as_binary: true, map_node_format: :map) + catch + _kind, _reason -> {:error, "invalid YAML frontmatter"} + else + [%{} = frontmatter] -> {:ok, normalize_yaml(frontmatter)} + _ -> {:error, "frontmatter must be a YAML mapping"} + end + end + + # JSON objects have string keys, and YAML's null is nil. + defp normalize_yaml(:null), do: nil + + defp normalize_yaml(map) when is_map(map), + do: Map.new(map, fn {k, v} -> {to_string(normalize_yaml(k)), normalize_yaml(v)} end) + + defp normalize_yaml(list) when is_list(list), do: Enum.map(list, &normalize_yaml/1) + defp normalize_yaml(value), do: value + + @doc """ + The skill's files as served, with the frontmatter written ahead of `SKILL.md`. + Renders every file. + """ + @spec contents(t()) :: %{required(String.t()) => binary()} + def contents(%__MODULE__{files: files} = skill) do + Map.new(files, fn {path, _content} -> {path, render(skill, path)} end) + end + + defp render(%__MODULE__{frontmatter: frontmatter, files: files}, "SKILL.md"), + do: frontmatter_block(frontmatter) <> render_content(files["SKILL.md"]) + + defp render(%__MODULE__{files: files}, path), do: render_content(files[path]) + + defp render_content(fun) when is_function(fun, 0), do: IO.iodata_to_binary(fun.()) + defp render_content(binary), do: binary + + # Each value is written as JSON, which YAML 1.2 parses back to the same value. + defp frontmatter_block(frontmatter) do + {first, rest} = Map.split(frontmatter, ["name", "description"]) + + lines = + Enum.map( + [{"name", first["name"]}, {"description", first["description"]}] ++ Enum.sort(rest), + fn {key, value} -> [yaml_key(key), ": ", JSON.encode!(value), "\n"] end + ) + + IO.iodata_to_binary(["---\n", lines, "---\n"]) + end + + defp yaml_key(key) do + if Regex.match?(@simple_key, key), do: key, else: JSON.encode!(key) + end + + @doc false + def validate_name!(name, context \\ "skill") do + if not (is_binary(name) and String.length(name) in 1..64 and Regex.match?(@name_pattern, name)) do + raise ArgumentError, + "#{context}: invalid skill name #{inspect(name)}. Names are 1-64 lowercase " <> + "letters, digits, and single hyphens, and do not start or end with a hyphen" + end + + :ok + end + + defp validate_frontmatter!(frontmatter, context \\ "skill") do + validate_name!(frontmatter["name"], context) + + if not (is_binary(frontmatter["description"]) and frontmatter["description"] != "") do + raise ArgumentError, "#{context}: frontmatter must have a description" + end + end + + # Only static files are counted towards the size limit; rendering the others + # here would defeat rendering them lazily. + defp validate_limits!(%__MODULE__{files: files}) do + if map_size(files) > @max_files do + raise ArgumentError, "a skill may have at most #{@max_files} files" + end + + if files |> Map.values() |> Enum.filter(&is_binary/1) |> Enum.map(&byte_size/1) |> Enum.sum() > + @max_size do + raise ArgumentError, "a skill's files may total at most #{@max_size} bytes" + end + end + + defp stringify_keys(map) when is_map(map), + do: Map.new(map, fn {k, v} -> {to_string(k), stringify_keys(v)} end) + + defp stringify_keys(list) when is_list(list), do: Enum.map(list, &stringify_keys/1) + defp stringify_keys(value), do: value + + @doc false + # The URI of a file or directory within the skill at `base_uri`. + def uri(base_uri, ""), do: base_uri + + def uri(base_uri, path) do + base_uri <> "/" <> Enum.map_join(String.split(path, "/"), "/", &encode_segment/1) + end + + @doc false + def encode_segment(segment), do: URI.encode(segment, &URI.char_unreserved?/1) + + defp digest(bytes), do: "sha256:" <> Base.encode16(:crypto.hash(:sha256, bytes), case: :lower) + + # Reuses a digest computed by `embed_skills/1` while the file is still static. + defp digest(%__MODULE__{files: files, digests: digests}, path, bytes) do + case digests do + %{^path => {size, digest}} + when is_binary(:erlang.map_get(path, files)) and byte_size(bytes) == size -> + digest + + _ -> + digest(bytes) + end + end + + @doc false + # The manifest of a skill's files. `files` maps each path within the skill to the + # skill that serves it and the file's path within that skill. + def manifest(files, base_uri) do + resources = + for {path, {skill, own_path}} <- Enum.sort(files) do + bytes = render(skill, own_path) + + %{ + uri: uri(base_uri, path), + digest: digest(skill, own_path, bytes), + size: byte_size(bytes) + } + end + + cond do + length(resources) > @max_files -> + {:error, "#{base_uri} has more than #{@max_files} files"} + + resources |> Enum.map(& &1.size) |> Enum.sum() > @max_size -> + {:error, "#{base_uri} has more than #{@max_size} bytes of files"} + + true -> + {:ok, resources} + end + end + + @doc false + # The `resources/read` contents of one of the skill's own files. + def read(%__MODULE__{files: files} = skill, path, uri) when is_map_key(files, path) do + bytes = render(skill, path) + content = %{uri: uri, mimeType: MIME.from_path(path)} + + if String.valid?(bytes), + do: {:ok, Map.put(content, :text, bytes)}, + else: {:ok, Map.put(content, :blob, Base.encode64(bytes))} + end + + def read(%__MODULE__{}, _path, _uri), do: :error + + @doc false + # The `resources/directory/read` children of a directory, where "" is the skill's + # root. `files` is as in `manifest/2`. A skill's SKILL.md takes its name and + # description from its frontmatter. + def list_directory(files, base_uri, dir) do + prefix = if dir == "", do: "", else: dir <> "/" + + children = + for {path, served} <- files, String.starts_with?(path, prefix), uniq: true do + case path |> String.replace_prefix(prefix, "") |> String.split("/", parts: 2) do + [file] -> {file, child(file, served)} + [subdir, _] -> {subdir, %{name: subdir, mimeType: "inode/directory"}} + end + end + + if children == [] do + :error + else + dir_uri = uri(base_uri, dir) + + {:ok, + children + |> Enum.sort_by(&elem(&1, 0)) + |> Enum.map(fn {name, child} -> + Map.put(child, :uri, dir_uri <> "/" <> encode_segment(name)) + end)} + end + end + + defp child("SKILL.md", {%__MODULE__{frontmatter: frontmatter}, "SKILL.md"}) do + %{ + name: frontmatter["name"], + description: frontmatter["description"], + mimeType: "text/markdown" + } + end + + defp child(file, _served), do: %{name: file, mimeType: MIME.from_path(file)} +end diff --git a/mix.exs b/mix.exs index ef5a0d4..7bb44a0 100644 --- a/mix.exs +++ b/mix.exs @@ -51,6 +51,7 @@ defmodule Phantom.MixProject do {:telemetry, "~> 1.0"}, {:phoenix_pubsub, "~> 2.0", optional: true, only: [:dev, :test, :prod, :stdio]}, {:uuid_v7, "~> 0.6"}, + {:yamerl, "~> 0.10", optional: true}, ## Test {:phoenix_live_view, "~> 1.0", only: [:dev, :test, :stdio]}, {:ex_doc, "~> 0.31", only: :dev, warn_if_outdated: true, runtime: false}, diff --git a/mix.lock b/mix.lock index d2cedc6..765eb58 100644 --- a/mix.lock +++ b/mix.lock @@ -31,4 +31,5 @@ "uuid_v7": {:hex, :uuid_v7, "0.6.0", "1d65727ade8ca619ed40fdef90c4186b50c84657d2b412f7cb79777ab2d47559", [:mix], [{:ecto, "~> 3.0", [hex: :ecto, repo: "hexpm", optional: true]}], "hexpm", "1dc401134e61da847a7b2a3b28d2593893f457b9f2704893b1ba3ff7946ce91f"}, "websock": {:hex, :websock, "0.5.3", "2f69a6ebe810328555b6fe5c831a851f485e303a7c8ce6c5f675abeb20ebdadc", [:mix], [], "hexpm", "6105453d7fac22c712ad66fab1d45abdf049868f253cf719b625151460b8b453"}, "websock_adapter": {:hex, :websock_adapter, "0.6.0", "73db5ab8aaefd1a876a97ce3e6afc96562625de69ef17a4e04426e034849d0b8", [:mix], [{:bandit, ">= 0.6.0", [hex: :bandit, repo: "hexpm", optional: true]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:plug_cowboy, "~> 2.6", [hex: :plug_cowboy, repo: "hexpm", optional: true]}, {:websock, "~> 0.5", [hex: :websock, repo: "hexpm", optional: false]}], "hexpm", "50021a85bce8f203b086705d9e0c5415e2c7eb05d319111b0428fe71f9934617"}, + "yamerl": {:hex, :yamerl, "0.10.0", "4ff81fee2f1f6a46f1700c0d880b24d193ddb74bd14ef42cb0bcf46e81ef2f8e", [:rebar3], [], "hexpm", "346adb2963f1051dc837a2364e4acf6eb7d80097c0f53cbdc3046ec8ec4b4e6e"}, } diff --git a/test/phantom/skill_edge_test.exs b/test/phantom/skill_edge_test.exs new file mode 100644 index 0000000..9848383 --- /dev/null +++ b/test/phantom/skill_edge_test.exs @@ -0,0 +1,387 @@ +defmodule Phantom.SkillEdgeTest do + use ExUnit.Case, async: true + + import ExUnit.CaptureLog + + alias Phantom.Cache + alias Phantom.Skill + + defmodule Actions do + alias Phantom.Skill + + def outer(_params, session) do + {:reply, + Skill.new(%{name: "outer", description: "Outer skill"}, %{ + "SKILL.md" => "# Outer\n", + "inner/notes.md" => "hello Ada\n", + "gone/notes.md" => "kept\n" + }), session} + end + + def inner(_params, session) do + {:reply, + Skill.new(%{name: "inner", description: "Inner skill"}, %{ + "SKILL.md" => "# Inner\n", + "notes.md" => "hello\n" + }), session} + end + + def gone(_params, session), do: {:reply, nil, session} + + def spaced_files(_params, session) do + {:reply, + Skill.new(%{name: "spaced-files", description: "A file with a space"}, %{ + "SKILL.md" => "# Spaced\n", + "my file.md" => "spaced\n" + }), session} + end + + def buggy(_params, _session), do: String.upcase(Process.get(:missing)) + + def exploding(_params, session) do + {:reply, + Skill.new(%{name: "exploding", description: "Raises when rendered"}, %{ + "SKILL.md" => fn -> raise "boom" end + }), session} + end + + def async_skill(_params, session), do: {:noreply, session} + + def fine(_params, session) do + {:reply, Skill.new(%{name: "fine", description: "Fine"}, %{"SKILL.md" => "# Fine\n"}), + session} + end + + def pub(_params, session) do + {:reply, + Skill.new(%{name: "pub", description: "Public"}, %{"SKILL.md" => "# Pub\n"}) + |> Skill.with_cache(ttl_ms: 1_000, scope: :public), session} + end + + def admin(_params, %{assigns: %{admin: true}} = session) do + {:reply, + Skill.new(%{name: "admin", description: "Admins only"}, %{"SKILL.md" => "# Admin\n"}) + |> Skill.with_cache(ttl_ms: 1_000, scope: :public), session} + end + + def admin(_params, session), do: {:reply, nil, session} + + def featured_review(_params, session), do: review("featured", session) + def review_by_id(%{"id" => id}, session), do: review("by id #{id}", session) + + defp review(text, session) do + {:reply, + Skill.new(%{name: "review", description: "Review"}, %{"SKILL.md" => "# #{text}\n"}), + session} + end + + def git_workflow(_params, session) do + {:reply, Skill.new(%{name: "git-workflow", description: "Git"}, %{"SKILL.md" => "# Git\n"}), + session} + end + end + + defmodule NestRouter do + use Phantom.Router, name: "Nest", vsn: "1.0" + + skill "nest/outer", Actions + skill "nest/outer/inner", Actions + skill "nest/outer/gone", Actions + skill "files/spaced-files", Actions + skill "git-workflow", Actions + end + + defmodule FailRouter do + use Phantom.Router, name: "Fail", vsn: "1.0" + + skill "ok/fine", Actions + skill "bugs/buggy", Actions + skill "bugs/exploding", Actions + skill "bad/async-skill", Actions + end + + defmodule PublicRouter do + use Phantom.Router, name: "Public", vsn: "1.0" + + skill "a/pub", Actions + skill "z/admin", Actions + end + + defmodule StaticFirstRouter do + use Phantom.Router, name: "StaticFirst", vsn: "1.0" + + skill "studies/featured/review", Actions, :featured_review + skill "studies/:id/review", Actions, :review_by_id + end + + defmodule ParamFirstRouter do + use Phantom.Router, name: "ParamFirst", vsn: "1.0" + + skill "studies/:id/review", Actions, :review_by_id + skill "studies/featured/review", Actions, :featured_review + end + + defmodule LateRouter do + use Phantom.Router, name: "Late", vsn: "1.0" + end + + defmodule ConcurrentRouter do + use Phantom.Router, name: "Concurrent", vsn: "1.0" + end + + defmodule PagedRouter do + use Phantom.Router, name: "Paged", vsn: "1.0" + end + + setup do + for router <- [NestRouter, FailRouter, PublicRouter, StaticFirstRouter, ParamFirstRouter], + do: Cache.register(router) + + :ok + end + + defp dispatch(router, method, params, opts \\ []) do + session = + Phantom.Test.build_session(router, + assigns: Keyword.get(opts, :assigns, %{}), + allowed_resource_templates: opts[:allowed_resource_templates] + ) + + request = Phantom.Test.build_request(method, params: params) + router.dispatch_method(method, params, request, %{session | request: request}) + end + + defp read(router, uri, opts \\ []) do + case dispatch(router, "resources/read", %{"uri" => uri}, opts) do + {:reply, %{contents: [%{text: text}]}, _} -> {:ok, text} + {:error, error, _} -> {:error, error} + end + end + + defp get(router, uri, opts \\ []), do: dispatch(router, "skills/get", %{"uri" => uri}, opts) + + defp sha256(bytes), do: "sha256:" <> Base.encode16(:crypto.hash(:sha256, bytes), case: :lower) + + describe "nested skills with their own route" do + test "the parent's manifest lists the nested skill's files, matching what's read" do + assert {:reply, %{skill: %{resources: resources}}, _} = + get(NestRouter, "skill://nest/outer/SKILL.md") + + assert [ + "skill://nest/outer/SKILL.md", + "skill://nest/outer/gone/notes.md", + "skill://nest/outer/inner/SKILL.md", + "skill://nest/outer/inner/notes.md" + ] = Enum.map(resources, & &1.uri) + + for %{uri: uri, digest: digest, size: size} <- resources do + assert {:ok, bytes} = read(NestRouter, uri) + assert digest == sha256(bytes), uri + assert size == byte_size(bytes), uri + end + + assert {:ok, "hello\n"} = read(NestRouter, "skill://nest/outer/inner/notes.md") + end + + test "falls back to the parent's files when the nested action serves no skill" do + assert {:ok, "kept\n"} = read(NestRouter, "skill://nest/outer/gone/notes.md") + end + + test "directory reads list the nested skill's files" do + assert {:reply, %{resources: resources}, _} = + dispatch(NestRouter, "resources/directory/read", %{ + "uri" => "skill://nest/outer/inner" + }) + + assert [ + %{name: "inner", description: "Inner skill", mimeType: "text/markdown"}, + %{name: "notes.md"} + ] = resources + end + + test "the parent's permission covers the nested skill's files" do + assert {:ok, "hello\n"} = + read(NestRouter, "skill://nest/outer/inner/notes.md", + allowed_resource_templates: ["nest/outer"] + ) + end + + test "the nested skill's permission does not cover the parent's files" do + assert {:error, _} = + read(NestRouter, "skill://nest/outer/SKILL.md", + allowed_resource_templates: ["nest/outer/inner"] + ) + + assert {:error, _} = + read(NestRouter, "skill://nest/outer/gone/notes.md", + allowed_resource_templates: ["nest/outer/gone"] + ) + end + + test "a skill's path params can't continue below another skill's path" do + assert_raise ArgumentError, ~r/nested/, fn -> + Code.compile_quoted( + quote do + defmodule Phantom.SkillEdgeTest.BadNesting do + use Phantom.Router, name: "BadNesting" + skill "nest/outer", Phantom.SkillEdgeTest.Actions + skill "nest/outer/:x/inner", Phantom.SkillEdgeTest.Actions + end + end + ) + end + end + end + + describe "resources/read" do + test "is limited by the session's allowed resource templates" do + assert {:error, _} = + read(NestRouter, "skill://nest/outer/SKILL.md", + allowed_resource_templates: ["git-workflow"] + ) + end + end + + describe "URIs" do + test "percent-encodes file paths in manifests and reads them back" do + assert {:reply, %{skill: %{resources: resources}}, _} = + get(NestRouter, "skill://files/spaced-files/SKILL.md") + + assert "skill://files/spaced-files/my%20file.md" in Enum.map(resources, & &1.uri) + assert {:ok, "spaced\n"} = read(NestRouter, "skill://files/spaced-files/my%20file.md") + end + + test "only canonical URIs resolve, without logging errors" do + log = + capture_log(fn -> + for uri <- [ + "skill://git-workflow/SKILL.md/", + "skill://GIT-WORKFLOW/SKILL.md", + "skill://git-workflow/SKILL.md?x=1", + "skill://git-workflow//SKILL.md", + "skill://git-workflow/%53KILL.md" + ] do + assert {:error, %{code: -32602}, _} = get(NestRouter, uri) + assert {:error, _} = read(NestRouter, uri) + end + + for uri <- ["skill://nest/outer/", "skill://nest/outer/inner/"] do + assert {:error, %{code: -32602}, _} = + dispatch(NestRouter, "resources/directory/read", %{"uri" => uri}) + end + end) + + refute log =~ "[error]" + end + + test "the first segment of a skill path can't be a path param" do + assert_raise ArgumentError, ~r/first segment/, fn -> + Phantom.Router.skill_template( + path: ":tenant/git-workflow", + handler: Actions, + router: __MODULE__ + ) + end + end + end + + describe "missing params" do + test "skills/get and resources/directory/read require a uri" do + assert {:error, %{code: -32602}, _} = dispatch(NestRouter, "skills/get", %{}) + + assert {:error, %{code: -32602}, _} = + dispatch(NestRouter, "resources/directory/read", %{}) + end + end + + describe "failures" do + test "a FunctionClauseError raised inside an action is not treated as no skill" do + assert_raise FunctionClauseError, fn -> get(FailRouter, "skill://bugs/buggy/SKILL.md") end + end + + test "skills/list omits skills whose action or files raise, and logs them" do + log = + capture_log(fn -> + assert {:reply, %{skills: [%{uri: "skill://ok/fine/SKILL.md"}]}, _} = + dispatch(FailRouter, "skills/list", %{}) + end) + + assert log =~ "bugs/buggy" + assert log =~ "bugs/exploding" + assert log =~ "bad/async-skill" + end + + test "an unsupported return value raises a descriptive error" do + assert_raise ArgumentError, ~r/synchronous/, fn -> + get(FailRouter, "skill://bad/async-skill/SKILL.md") + end + end + end + + describe "public listings" do + test "are public only when every skill on the page is served and public" do + assert {:reply, %{skills: [_, _], cacheScope: "public"}, _} = + dispatch(PublicRouter, "skills/list", %{}, assigns: %{admin: true}) + + assert {:reply, %{skills: [%{uri: "skill://a/pub/SKILL.md"}], cacheScope: "private"}, _} = + dispatch(PublicRouter, "skills/list", %{}, assigns: %{admin: false}) + end + end + + describe "route precedence" do + test "a static route wins over a path param at the same depth" do + for router <- [StaticFirstRouter, ParamFirstRouter] do + assert {:ok, "---" <> _ = text} = read(router, "skill://studies/featured/review/SKILL.md") + assert text =~ "# featured" + + assert {:ok, text} = read(router, "skill://studies/42/review/SKILL.md") + assert text =~ "# by id 42" + end + end + end + + describe "Cache.add_skill/2" do + test "keeps skills added before the router is registered" do + Cache.add_skill(LateRouter, path: "late/git-workflow", handler: Actions) + Cache.register(LateRouter) + + assert {:reply, %{skill: %{frontmatter: %{"name" => "git-workflow"}}}, _} = + get(LateRouter, "skill://late/git-workflow/SKILL.md") + end + + test "keeps every skill added concurrently" do + Cache.register(ConcurrentRouter) + + 1..20 + |> Task.async_stream(fn i -> + Cache.add_skill(ConcurrentRouter, path: "c#{i}/git-workflow", handler: Actions) + end) + |> Stream.run() + + assert 20 = length(Cache.list(nil, ConcurrentRouter, :resource_templates)) + end + end + + describe "skills/list pagination" do + test "pages through more than 100 skills" do + Cache.register(PagedRouter) + + for i <- 1..105 do + path = "p#{String.pad_leading(to_string(i), 3, "0")}/git-workflow" + Cache.add_skill(PagedRouter, path: path, handler: Actions) + end + + assert {:reply, %{skills: first, nextCursor: cursor}, _} = + dispatch(PagedRouter, "skills/list", %{}) + + assert {:reply, %{skills: rest} = second, _} = + dispatch(PagedRouter, "skills/list", %{"cursor" => cursor}) + + refute Map.has_key?(second, :nextCursor) + assert length(first) == 100 + assert length(rest) == 5 + + assert Enum.uniq(Enum.map(first ++ rest, & &1.uri)) |> length() == 105 + end + end +end diff --git a/test/phantom/skill_plug_test.exs b/test/phantom/skill_plug_test.exs new file mode 100644 index 0000000..e39c9c8 --- /dev/null +++ b/test/phantom/skill_plug_test.exs @@ -0,0 +1,82 @@ +defmodule Phantom.SkillPlugTest do + use ExUnit.Case, async: true + + import Phantom.TestDispatcher + import Plug.Conn + import Plug.Test + + defmodule Skills do + alias Phantom.Skill + + def git_workflow(_params, session) do + {:reply, + Skill.new(%{name: "git-workflow", description: "Git"}, %{"SKILL.md" => "# Git\n"}) + |> Skill.with_cache(ttl_ms: 60_000, scope: :public), session} + end + end + + defmodule Router do + use Phantom.Router, name: "SkillPlug", vsn: "1.0" + + skill "git-workflow", Skills + end + + setup do + Phantom.Cache.register(Router) + :ok + end + + defp post(id, method, params) do + meta = %{ + "io.modelcontextprotocol/protocolVersion" => "2026-07-28", + "io.modelcontextprotocol/clientInfo" => %{"name" => "Test", "version" => "1.0.0"}, + "io.modelcontextprotocol/clientCapabilities" => %{} + } + + :post + |> conn("/mcp", %{ + jsonrpc: "2.0", + id: id, + method: method, + params: Map.put(params, "_meta", meta) + }) + |> put_req_header("content-type", "application/json") + |> put_req_header("mcp-protocol-version", "2026-07-28") + |> put_req_header("mcp-method", method) + |> call(router: Router) + + assert_receive {:response, ^id, "message", payload}, 1_000 + payload + end + + test "skills/list is served under 2026-07-28" do + assert %{ + result: %{ + resultType: "complete", + ttlMs: 60_000, + cacheScope: "public", + skills: [%{uri: "skill://git-workflow/SKILL.md"}] + } + } = post(1, "skills/list", %{}) + end + + test "skills/get is served under 2026-07-28" do + assert %{ + result: %{ + resultType: "complete", + ttlMs: 60_000, + cacheScope: "public", + skill: %{uri: "skill://git-workflow/SKILL.md"} + } + } = post(2, "skills/get", %{"uri" => "skill://git-workflow/SKILL.md"}) + end + + test "resources/directory/read is served under 2026-07-28" do + assert %{ + result: %{ + resultType: "complete", + resources: [%{uri: "skill://git-workflow/SKILL.md", name: "git-workflow"}] + } + } = post(3, "resources/directory/read", %{"uri" => "skill://git-workflow"}) + end +end diff --git a/test/phantom/skill_router_test.exs b/test/phantom/skill_router_test.exs new file mode 100644 index 0000000..6fde849 --- /dev/null +++ b/test/phantom/skill_router_test.exs @@ -0,0 +1,430 @@ +defmodule Phantom.SkillRouterTest do + use ExUnit.Case, async: true + + alias Phantom.Cache + + defmodule Skills do + use Phantom.Skill + + embed_skills "../support/skills/*" + embed_skills "../support/skills/refunds/partial-refunds" + + def git_workflow(_params, session) do + {:reply, git_workflow(%{}) |> Phantom.Skill.with_cache(ttl_ms: 300_000, scope: :public), + session} + end + + def refunds(_params, session), + do: {:reply, refunds(%{user: session.assigns.user}), session} + + def partial_refunds(_params, session) do + {:reply, partial_refunds(%{}) |> Phantom.Skill.with_cache(ttl_ms: 60_000, scope: :public), + session} + end + + def plain_strings(_params, session) do + {:reply, + Phantom.Skill.new(%{name: "plain-strings", description: "Only binaries"}, %{ + "SKILL.md" => "# Plain\n" + }), session} + end + + def study_review(%{"study_id" => "404"}, session), do: {:reply, nil, session} + + def study_review(%{"study_id" => id}, session), + do: {:reply, study_review(%{study_id: id}), session} + + def daily(_params, session) do + skill = + Phantom.Skill.new( + %{name: "daily", description: "Assemble today's report"}, + %{"SKILL.md" => "# Daily\n", "data/today.md" => "Today\n"} + ) + + {:reply, Phantom.Skill.dynamic(skill), session} + end + + def mismatched(_params, session), do: {:reply, git_workflow(%{}), session} + + def lazy_skill(_params, session) do + test = self() + + {:reply, + Phantom.Skill.new(%{name: "lazy-skill", description: "Renders when read"}, %{ + "SKILL.md" => fn -> + send(test, :rendered_skill_md) + "# Lazy\n" + end, + "notes.md" => "Notes\n" + }), session} + end + end + + defmodule TestRouter do + use Phantom.Router, name: "SkillTest", vsn: "1.0" + + skill "git-workflow", Skills + skill "acme/billing/refunds", Skills, :refunds + skill "acme/billing/refunds/partial-refunds", Skills, :partial_refunds + skill "studies/:study_id/study-review", Skills, :study_review + skill "reports/daily", Skills, :daily + skill "lazy/lazy-skill", Skills + skill "plain/plain-strings", Skills + end + + defmodule RuntimeRouter do + use Phantom.Router, name: "RuntimeSkills", vsn: "1.0" + end + + defmodule MismatchRouter do + use Phantom.Router, name: "SkillMismatch", vsn: "1.0" + + skill "acme/wrong-name", Skills, :mismatched + end + + defmodule EmptyRouter do + use Phantom.Router, name: "NoSkills", vsn: "1.0" + end + + setup do + Cache.register(TestRouter) + Cache.register(MismatchRouter) + Cache.register(EmptyRouter) + Cache.register(RuntimeRouter) + :ok + end + + defp dispatch(method, params, opts \\ []), do: dispatch_to(TestRouter, method, params, opts) + + defp dispatch_to(router, method, params, opts \\ []) do + session = + Phantom.Test.build_session(router, + assigns: %{user: "Ada"}, + allowed_resource_templates: opts[:allowed_resource_templates] + ) + + request = Phantom.Test.build_request(method, params: params) + router.dispatch_method(method, params, request, %{session | request: request}) + end + + defp sha256(bytes), do: "sha256:" <> Base.encode16(:crypto.hash(:sha256, bytes), case: :lower) + + defp read_bytes(uri) do + assert {:reply, %{contents: [content]}, _} = dispatch("resources/read", %{"uri" => uri}) + + case content do + %{text: text} -> text + %{blob: blob} -> Base.decode64!(blob) + end + end + + describe "skills/list" do + test "lists every skill without path params, sorted by path" do + assert {:reply, %{skills: skills, ttlMs: 0, cacheScope: "private"} = result, _} = + dispatch("skills/list", %{}) + + refute Map.has_key?(result, :nextCursor) + + assert [ + "skill://acme/billing/refunds/SKILL.md", + "skill://acme/billing/refunds/partial-refunds/SKILL.md", + "skill://git-workflow/SKILL.md", + "skill://lazy/lazy-skill/SKILL.md", + "skill://plain/plain-strings/SKILL.md", + "skill://reports/daily/SKILL.md" + ] = Enum.map(skills, & &1.uri) + end + + test "entries carry the frontmatter and a complete manifest" do + assert {:reply, %{skills: [refunds | _]}, _} = dispatch("skills/list", %{}) + + assert %{ + frontmatter: %{"name" => "refunds", "license" => "Apache-2.0"}, + resources: [ + %{uri: "skill://acme/billing/refunds/SKILL.md"}, + %{uri: "skill://acme/billing/refunds/assets/logo.png"}, + %{uri: "skill://acme/billing/refunds/examples/email.md"}, + %{uri: "skill://acme/billing/refunds/partial-refunds/SKILL.md"} + ] + } = refunds + end + + test "every digest and size matches the bytes resources/read returns" do + assert {:reply, %{skills: skills}, _} = dispatch("skills/list", %{}) + + for %{resources: resources} <- skills, is_list(resources), resource <- resources do + bytes = read_bytes(resource.uri) + assert resource.digest == sha256(bytes), resource.uri + assert resource.size == byte_size(bytes), resource.uri + end + end + + test "dynamic skills have no manifest" do + assert {:reply, %{skills: skills}, _} = dispatch("skills/list", %{}) + + assert %{resources: "dynamic"} = + Enum.find(skills, &(&1.uri == "skill://reports/daily/SKILL.md")) + end + + test "respects the session's allowed resource templates" do + assert {:reply, %{skills: [%{uri: "skill://git-workflow/SKILL.md"}]}, _} = + dispatch("skills/list", %{}, allowed_resource_templates: ["git-workflow"]) + end + + test "is cached for the shortest ttl when every listed skill has cache hints" do + assert {:reply, %{ttlMs: 60_000}, _} = + dispatch("skills/list", %{}, + allowed_resource_templates: [ + "git-workflow", + "acme/billing/refunds/partial-refunds" + ] + ) + end + + test "is private when the session has an allow-list" do + assert {:reply, %{ttlMs: 300_000, cacheScope: "private"}, _} = + dispatch("skills/list", %{}, allowed_resource_templates: ["git-workflow"]) + end + end + + describe "skills/get" do + test "returns the entry for a skill" do + assert {:reply, %{skill: skill}, _} = + dispatch("skills/get", %{"uri" => "skill://git-workflow/SKILL.md"}) + + bytes = read_bytes("skill://git-workflow/SKILL.md") + + assert %{ + uri: "skill://git-workflow/SKILL.md", + frontmatter: %{"name" => "git-workflow"}, + resources: [ + %{uri: "skill://git-workflow/SKILL.md", digest: digest, size: size} + ] + } = skill + + assert digest == sha256(bytes) + assert size == byte_size(bytes) + end + + test "uses the skill's cache hints" do + assert {:reply, %{ttlMs: 300_000, cacheScope: "public"}, _} = + dispatch("skills/get", %{"uri" => "skill://git-workflow/SKILL.md"}) + end + + test "is private and not cached without cache hints, even when every file is a string" do + for uri <- [ + "skill://plain/plain-strings/SKILL.md", + "skill://acme/billing/refunds/SKILL.md", + "skill://reports/daily/SKILL.md" + ] do + assert {:reply, %{ttlMs: 0, cacheScope: "private"}, _} = + dispatch("skills/get", %{"uri" => uri}) + end + end + + test "renders every file for the manifest" do + assert {:reply, %{skill: _}, _} = + dispatch("skills/get", %{"uri" => "skill://lazy/lazy-skill/SKILL.md"}) + + assert_received :rendered_skill_md + end + + test "passes path params to the action" do + assert {:reply, %{skill: %{frontmatter: %{"name" => "study-review"}}}, _} = + dispatch("skills/get", %{"uri" => "skill://studies/42/study-review/SKILL.md"}) + + assert read_bytes("skill://studies/42/study-review/SKILL.md") =~ "# Review study 42" + end + + test "returns invalid params for an unknown skill" do + for uri <- [ + "skill://acme/billing/chargebacks/SKILL.md", + "skill://git-workflow/README.md", + "skill://studies/404/study-review/SKILL.md", + "https://example.com/SKILL.md" + ] do + assert {:error, %{code: -32602, message: "No skill is served at " <> ^uri}, _} = + dispatch("skills/get", %{"uri" => uri}) + end + end + + test "respects the session's allowed resource templates" do + assert {:error, %{code: -32602}, _} = + dispatch("skills/get", %{"uri" => "skill://acme/billing/refunds/SKILL.md"}, + allowed_resource_templates: ["git-workflow"] + ) + end + + @tag :capture_log + test "returns an internal error when the name does not match the path" do + assert {:error, %{code: -32603}, _} = + dispatch_to(MismatchRouter, "skills/get", %{ + "uri" => "skill://acme/wrong-name/SKILL.md" + }) + end + end + + describe "resources/read" do + test "serves SKILL.md with its frontmatter" do + assert {:reply, %{contents: [content]}, _} = + dispatch("resources/read", %{"uri" => "skill://acme/billing/refunds/SKILL.md"}) + + assert %{ + uri: "skill://acme/billing/refunds/SKILL.md", + mimeType: "text/markdown", + text: "---\n" <> _ = text + } = content + + assert {:ok, %{"name" => "refunds"}, "# Refunds\n\nHello Ada." <> _} = + Phantom.Skill.parse(text) + end + + test "serves binary files as blobs" do + assert {:reply, %{contents: [%{mimeType: "image/png", blob: _}]}, _} = + dispatch("resources/read", %{ + "uri" => "skill://acme/billing/refunds/assets/logo.png" + }) + end + + test "renders only the file being read" do + assert read_bytes("skill://lazy/lazy-skill/notes.md") == "Notes\n" + refute_received :rendered_skill_md + end + + test "returns not found for a file the skill does not have" do + assert {:error, %{code: _}, _} = + dispatch("resources/read", %{"uri" => "skill://acme/billing/refunds/nope.md"}) + end + end + + describe "resources/directory/read" do + test "lists the direct children of a skill's root" do + assert {:reply, %{resources: resources}, _} = + dispatch("resources/directory/read", %{"uri" => "skill://acme/billing/refunds"}) + + assert [ + %{ + uri: "skill://acme/billing/refunds/SKILL.md", + name: "refunds", + description: "Process customer refund requests per company policy", + mimeType: "text/markdown" + }, + %{ + uri: "skill://acme/billing/refunds/assets", + name: "assets", + mimeType: "inode/directory" + }, + %{ + uri: "skill://acme/billing/refunds/examples", + name: "examples", + mimeType: "inode/directory" + }, + %{ + uri: "skill://acme/billing/refunds/partial-refunds", + name: "partial-refunds", + mimeType: "inode/directory" + } + ] = resources + end + + test "lists a subdirectory" do + assert {:reply, + %{ + resources: [ + %{uri: "skill://acme/billing/refunds/examples/email.md", name: "email.md"} + ] + }, _} = + dispatch("resources/directory/read", %{ + "uri" => "skill://acme/billing/refunds/examples" + }) + end + + test "returns invalid params for a file or a missing directory" do + for uri <- [ + "skill://acme/billing/refunds/SKILL.md", + "skill://acme/billing/refunds/nope", + "skill://acme/billing" + ] do + assert {:error, %{code: -32602}, _} = + dispatch("resources/directory/read", %{"uri" => uri}) + end + end + end + + describe "capabilities" do + test "declares the skills extension and resources" do + assert {:reply, %{capabilities: capabilities}, _} = dispatch("server/discover", %{}) + + assert %{ + resources: %{}, + extensions: %{"io.modelcontextprotocol/skills" => %{directoryRead: true}} + } = capabilities + end + + test "omits the extension without skills" do + assert {:reply, %{capabilities: capabilities}, _} = + dispatch_to(EmptyRouter, "server/discover", %{}) + + refute Map.has_key?(capabilities, :extensions) + end + end + + describe "Cache.add_skill/2" do + test "serves a skill added at runtime" do + Cache.add_skill(RuntimeRouter, path: "runtime/git-workflow", handler: Skills) + + assert {:reply, %{skill: %{frontmatter: %{"name" => "git-workflow"}}}, _} = + dispatch_to(RuntimeRouter, "skills/get", %{ + "uri" => "skill://runtime/git-workflow/SKILL.md" + }) + + assert {:reply, %{contents: [%{text: "---\n" <> _}]}, _} = + dispatch_to(RuntimeRouter, "resources/read", %{ + "uri" => "skill://runtime/git-workflow/SKILL.md" + }) + + assert {:reply, %{capabilities: %{extensions: %{"io.modelcontextprotocol/skills" => _}}}, _} = + dispatch_to(RuntimeRouter, "server/discover", %{}) + end + + test "rejects an invalid skill path" do + assert_raise ArgumentError, ~r/name/, fn -> + Cache.add_skill(RuntimeRouter, path: "runtime/Bad_Name", handler: Skills, function: :x) + end + end + end + + test "skill routes are generated in the router's ResourceRouter.Skill module" do + assert Code.ensure_loaded?(TestRouter.ResourceRouter.Skill) + end + + test "skill routes are not listed as resource templates" do + assert {:reply, %{resourceTemplates: []}, _} = dispatch("resources/templates/list", %{}) + end + + test "the skill:// scheme is reserved for skills" do + assert_raise RuntimeError, ~r/skill/, fn -> + Code.compile_quoted( + quote do + defmodule Phantom.SkillRouterTest.Reserved do + use Phantom.Router, name: "Reserved" + resource "skill://foo/:id", Phantom.SkillRouterTest.Skills, :git_workflow + end + end + ) + end + end + + test "skill paths must end in a valid skill name" do + assert_raise ArgumentError, ~r/name/, fn -> + Code.compile_quoted( + quote do + defmodule Phantom.SkillRouterTest.BadPath do + use Phantom.Router, name: "BadPath" + skill "acme/:name", Phantom.SkillRouterTest.Skills, :git_workflow + end + end + ) + end + end +end diff --git a/test/phantom/skill_test.exs b/test/phantom/skill_test.exs new file mode 100644 index 0000000..0534f9e --- /dev/null +++ b/test/phantom/skill_test.exs @@ -0,0 +1,273 @@ +defmodule Phantom.SkillTest do + use ExUnit.Case, async: true + + alias Phantom.Skill + + defmodule Embedded do + use Phantom.Skill + + embed_skills "../support/skills/*" + embed_skills "../support/skills/refunds/partial-refunds" + end + + @skills_dir Path.expand("../support/skills", __DIR__) + + describe "new/2" do + test "stringifies frontmatter keys and turns iodata into binaries" do + assert %Skill{ + frontmatter: %{ + "name" => "git-workflow", + "description" => "Git conventions", + "metadata" => %{"team" => "platform"} + }, + files: %{"SKILL.md" => "# Git workflow\n"}, + dynamic: false + } = + Skill.new( + %{ + name: "git-workflow", + description: "Git conventions", + metadata: %{team: "platform"} + }, + %{"SKILL.md" => ["# Git ", "workflow\n"]} + ) + end + + test "keeps functions to render when a file is needed" do + skill = + Skill.new(%{name: "a", description: "b"}, %{ + "SKILL.md" => fn -> ["# A", "\n"] end, + "notes.md" => "Notes\n" + }) + + assert %Skill{files: %{"SKILL.md" => fun, "notes.md" => "Notes\n"}} = skill + assert is_function(fun, 0) + assert %{"SKILL.md" => "---\n" <> _, "notes.md" => "Notes\n"} = Skill.contents(skill) + assert Skill.contents(skill)["SKILL.md"] =~ ~r/---\n# A\n\z/ + end + + test "rejects file paths that aren't relative paths within the skill" do + for path <- ["", "/a.md", "a.md/", "a//b.md", "./a.md", "../a.md", "a/../b.md", "a\\b.md"] do + assert_raise ArgumentError, ~r/path/, fn -> + Skill.new(%{name: "a", description: "b"}, %{"SKILL.md" => "", path => ""}) + end + end + end + + test "requires a SKILL.md" do + assert_raise ArgumentError, ~r/SKILL.md/, fn -> + Skill.new(%{name: "a", description: "b"}, %{"README.md" => ""}) + end + end + + test "requires a description" do + assert_raise ArgumentError, ~r/description/, fn -> + Skill.new(%{name: "a"}, %{"SKILL.md" => ""}) + end + end + + test "enforces the Agent Skills naming rules" do + for name <- [ + "Refunds", + "-refunds", + "refunds-", + "re--funds", + "re_funds", + "", + String.duplicate("a", 65) + ] do + assert_raise ArgumentError, ~r/name/, fn -> + Skill.new(%{name: name, description: "b"}, %{"SKILL.md" => ""}) + end + end + end + + test "rejects skills with more than 512 files" do + files = Map.new(1..512, &{"file-#{&1}.md", ""}) |> Map.put("SKILL.md", "") + + assert_raise ArgumentError, ~r/512/, fn -> + Skill.new(%{name: "a", description: "b"}, files) + end + end + end + + test "dynamic/1 marks a skill as dynamic" do + skill = Skill.new(%{name: "a", description: "b"}, %{"SKILL.md" => ""}) + assert %Skill{dynamic: true} = Skill.dynamic(skill) + end + + describe "with_cache/2" do + test "skills have no cache hints unless given" do + assert %Skill{cache: nil} = Skill.new(%{name: "a", description: "b"}, %{"SKILL.md" => ""}) + end + + test "stores the cache hints" do + skill = + %{name: "a", description: "b"} + |> Skill.new(%{"SKILL.md" => ""}) + |> Skill.with_cache(ttl_ms: 60_000, scope: :public) + + assert %Skill{cache: [ttl_ms: 60_000, scope: :public]} = skill + end + + test "requires a valid ttl and scope" do + skill = Skill.new(%{name: "a", description: "b"}, %{"SKILL.md" => ""}) + + for opts <- [ + [ttl_ms: 60_000], + [scope: :public], + [ttl_ms: -1, scope: :public], + [ttl_ms: 1, scope: :shared] + ] do + assert_raise ArgumentError, fn -> Skill.with_cache(skill, opts) end + end + end + end + + describe "embed_skills/1" do + test "serves the same files as new/2, rendering EEx with assigns" do + expected = + Skill.new( + %{ + "name" => "refunds", + "description" => "Process customer refund requests per company policy", + "license" => "Apache-2.0", + "metadata" => %{"team" => "billing"} + }, + %{ + "SKILL.md" => + "# Refunds\n\nHello Ada. Draft the reply from [the email template](examples/email.md).\n", + "examples/email.md" => "# Email\n\nDear customer,\n", + "assets/logo.png" => File.read!(Path.join(@skills_dir, "refunds/assets/logo.png")), + "partial-refunds/SKILL.md" => + Skill.contents(Embedded.partial_refunds(%{}))["SKILL.md"] + } + ) + + embedded = Embedded.refunds(%{user: "Ada"}) + assert embedded.frontmatter == expected.frontmatter + assert Skill.contents(embedded) == Skill.contents(expected) + end + + test "renders EEx files lazily and embeds other files as binaries" do + assert %Skill{files: %{"SKILL.md" => skill_md, "examples/email.md" => "# Email" <> _}} = + Embedded.refunds(%{user: "Ada"}) + + assert is_function(skill_md, 0) + end + + test "embeds static skills" do + assert %Skill{ + frontmatter: %{ + "name" => "git-workflow", + "description" => "Follow this team's Git conventions: branching and commits" + }, + files: %{"SKILL.md" => "# Git workflow\n\nBranch from `main`.\n"} + } = Embedded.git_workflow(%{}) + end + + test "embeds a nested skill on its own" do + assert %Skill{frontmatter: %{"name" => "partial-refunds"}} = Embedded.partial_refunds(%{}) + end + + test "raises when the frontmatter name does not match the directory" do + dir = Path.join(System.tmp_dir!(), "phantom-skill-#{System.unique_integer([:positive])}") + File.mkdir_p!(Path.join(dir, "refunds")) + on_exit(fn -> File.rm_rf!(dir) end) + + File.write!(Path.join(dir, "refunds/SKILL.md"), "---\nname: other\ndescription: x\n---\n") + + assert_raise ArgumentError, ~r/other/, fn -> + Code.compile_quoted( + quote do + defmodule Phantom.SkillTest.Mismatch do + use Phantom.Skill + embed_skills unquote(Path.join(dir, "*")) + end + end + ) + end + end + + test "raises when two files are served at the same path" do + dir = Path.join(System.tmp_dir!(), "phantom-skill-#{System.unique_integer([:positive])}") + File.mkdir_p!(Path.join(dir, "refunds")) + on_exit(fn -> File.rm_rf!(dir) end) + + File.write!(Path.join(dir, "refunds/SKILL.md"), "---\nname: refunds\ndescription: x\n---\n") + + File.write!( + Path.join(dir, "refunds/SKILL.md.eex"), + "---\nname: refunds\ndescription: x\n---\n" + ) + + assert_raise ArgumentError, ~r/SKILL.md/, fn -> + Code.compile_quoted( + quote do + defmodule Phantom.SkillTest.Duplicate do + use Phantom.Skill + embed_skills unquote(Path.join(dir, "*")) + end + end + ) + end + end + + test "raises when EEx appears in the frontmatter" do + dir = Path.join(System.tmp_dir!(), "phantom-skill-#{System.unique_integer([:positive])}") + File.mkdir_p!(Path.join(dir, "refunds")) + on_exit(fn -> File.rm_rf!(dir) end) + + File.write!( + Path.join(dir, "refunds/SKILL.md.eex"), + "---\nname: refunds\ndescription: <%= @x %>\n---\n" + ) + + assert_raise ArgumentError, ~r/frontmatter/, fn -> + Code.compile_quoted( + quote do + defmodule Phantom.SkillTest.EExFrontmatter do + use Phantom.Skill + embed_skills unquote(Path.join(dir, "*")) + end + end + ) + end + end + end + + describe "parse/1" do + test "stringifies keys that YAML parses as other types" do + assert {:ok, %{"name" => "a", "1" => "x", "true" => %{"2" => "y"}}, ""} = + Skill.parse("---\nname: a\n1: x\ntrue:\n 2: y\n---\n") + end + end + + describe "contents/1" do + test "writes frontmatter that parses back to the same map" do + frontmatter = %{ + "name" => "tricky", + "description" => "Quotes \" and colons: here\nand a newline, unicode é ✓", + "license" => nil, + "allowed-tools" => "Bash(git:*) Read", + "metadata" => %{"version" => 2, "ratio" => 1.5, "beta" => true, "tags" => ["a", "b: c"]} + } + + skill = Skill.new(frontmatter, %{"SKILL.md" => "# Body\n"}) + %{"SKILL.md" => skill_md} = Skill.contents(skill) + + assert {:ok, ^frontmatter, "# Body\n"} = Skill.parse(skill_md) + end + end + + describe "embedded skills" do + test "static files have digests computed at compile time" do + assert %Skill{digests: %{"SKILL.md" => {_size, "sha256:" <> _}}} = + Embedded.git_workflow(%{}) + + assert %Skill{digests: digests} = Embedded.refunds(%{user: "Ada"}) + assert Map.has_key?(digests, "examples/email.md") + refute Map.has_key?(digests, "SKILL.md") + end + end +end diff --git a/test/support/skills/git-workflow/SKILL.md b/test/support/skills/git-workflow/SKILL.md new file mode 100644 index 0000000..3af4869 --- /dev/null +++ b/test/support/skills/git-workflow/SKILL.md @@ -0,0 +1,7 @@ +--- +name: git-workflow +description: "Follow this team's Git conventions: branching and commits" +--- +# Git workflow + +Branch from `main`. diff --git a/test/support/skills/refunds/SKILL.md.eex b/test/support/skills/refunds/SKILL.md.eex new file mode 100644 index 0000000..30542e8 --- /dev/null +++ b/test/support/skills/refunds/SKILL.md.eex @@ -0,0 +1,10 @@ +--- +name: refunds +description: Process customer refund requests per company policy +license: Apache-2.0 +metadata: + team: billing +--- +# Refunds + +Hello <%= @user %>. Draft the reply from [the email template](examples/email.md). diff --git a/test/support/skills/refunds/assets/logo.png b/test/support/skills/refunds/assets/logo.png new file mode 100644 index 0000000000000000000000000000000000000000..3fedeb0ccb026f5003f9bf439767cc63dd6e5416 GIT binary patch literal 1921 zcmZvd4>;3#9LInDY%Xo1r~Nw1iu}3DCi!zGrDz!?BjjAJQ&~*HFp8uayRxQash*^g zbgYszBusX>g|q&hT^t$rvr|)aBjVC_-_1SOv*)?z`Tf4X=llJAKi|*$`+9%Azu)oT zpp975#ikGhVf{BzLm|kZKzn~?1V-wk9;!hnCYToH3qcw-#MVF%o6Xkf9+0-xKpHR( z20#aLb8}-b7)3=zQmIs<(NM`&t?7ZIHAiLHd;V&?^}bIA0tkd6H@6xZP0C>W30AU! zPepl_?~iz}cYRGps zWI@|i+k8>@l`j5IzxzeJJi=C9z5Jo8V+Z+3+@`?<+sah$mb8#3hvF0lR;`9!xzGw3 z^xbvKwf(CyL~emf#)Yl}9t9K0HU}xErgE%i$`@;-fsMW23@+m_+=|w$_#$iT2{xlV zGVw;T`Oj_y>5@f-+mGdULlCOlpGpbK=zY_RVj8xC{Um*N4V@F)Hxibha_sCBkC3YG;}V#m zX=~Qk&wrP>zQqiK7AJpp!Y^sIS7mXY##Z9bKVr`iw>eI?x(+;LSt>4FvmWml?+ERS zMe`})eb#S{jrnC`0&=oXbit&1yQcv~A@CeAePK?BuECB!ZqmK&#JX@XHXIA;c$=Jd zNjJ-`#1W-FW6wzgONgAg3PDmj`L4tMOxP3O^pO|IYoOiK?Y+&S!r48ques&5F$F^q4mh3|@xfs_3Fx#A(-&j^9#_}nCi3RCiF5Fq% zOeuC8r(^Rkp$1$VoQvOB$g|}Q#YtQw9hJxrNZO8+%_(;U9zzkmCDv=bO`M$AodJU* zruXinnao)QLOwZ{d&{{k=M23YmUC#mdL-iXwP=-p(lQLR80NHxq|5AEd<<92MP>={ z(|M&BmK=ScfSff>K^4NQOIHE$B z>J{M?v3W)kjBe21bd|oS*LwCShAZ}u8u|+DPvj(53i=P#_V26hPt5wTW#QsZMQD!! zVx-0rNxKzyDe^e1O&L>WKLToI2j14hSPkf+(*h)&Y zPXae|v5iSlB5=x?d!jMq+g&~i>r|~%Xnx455z_=NG5WqvvFnImWHy%1ajgAcGI;@T z9=?+dcj9lx!#DA-E#cSr;_YxTzRwfxOB2=_S5`vt@#2T*>N?b$H&9cPSiZEn#f(UV za&yJc_*yd8K_1y08L5`cSv@ks=Xzw^^~r?mkqH95GZa83=A$GSC+q{o)n$D;ejo`E&>oNcFD=JPn0Mf?j z451`LVPCDIxq1Q=KE<%qh7RKI>fz^9{jttx*DDc>RJ2`R|o@k-KT z2dCR4Cx@yeQk4VqEs3dAD2zQkjlfB}NgfkFD!wK literal 0 HcmV?d00001 diff --git a/test/support/skills/refunds/examples/email.md b/test/support/skills/refunds/examples/email.md new file mode 100644 index 0000000..1a52fbc --- /dev/null +++ b/test/support/skills/refunds/examples/email.md @@ -0,0 +1,3 @@ +# Email + +Dear customer, diff --git a/test/support/skills/refunds/partial-refunds/SKILL.md b/test/support/skills/refunds/partial-refunds/SKILL.md new file mode 100644 index 0000000..aac926c --- /dev/null +++ b/test/support/skills/refunds/partial-refunds/SKILL.md @@ -0,0 +1,5 @@ +--- +name: partial-refunds +description: Refund part of an order +--- +# Partial refunds diff --git a/test/support/skills/study-review/SKILL.md.eex b/test/support/skills/study-review/SKILL.md.eex new file mode 100644 index 0000000..8dd38e4 --- /dev/null +++ b/test/support/skills/study-review/SKILL.md.eex @@ -0,0 +1,5 @@ +--- +name: study-review +description: Review a study for gaps in its questions +--- +# Review study <%= @study_id %> From efc70b365e77e5930dbd2fb6a7517ffad80e08b8 Mon Sep 17 00:00:00 2001 From: David Bernheisel Date: Sun, 4 Oct 2026 19:46:27 -0400 Subject: [PATCH 2/4] Skills: nested skills own their files, harden add_skill A file belongs to the deepest skill route that contains it, as the resource router decides. A nested skill that serves nothing serves no files, and a skill's permission covers the skills nested in it. Cache.add_skill/2 takes a list, shares its lock with add_resource_template, logs compiler diagnostics, and waits for reads still running in the old routes. Digests are always computed from the served bytes, and descriptions are limited to 1024 characters. --- README.md | 5 +- lib/phantom/cache.ex | 110 ++++++++++---- lib/phantom/router.ex | 18 ++- lib/phantom/router/skills.ex | 236 ++++++++++++----------------- lib/phantom/skill.ex | 57 +++---- test/phantom/skill_edge_test.exs | 163 +++++++++++++++++++- test/phantom/skill_router_test.exs | 35 +++++ test/phantom/skill_test.exs | 22 +-- 8 files changed, 420 insertions(+), 226 deletions(-) diff --git a/README.md b/README.md index 2b60f9f..ab11dd1 100644 --- a/README.md +++ b/README.md @@ -574,7 +574,10 @@ and `resources/directory/read`, and computes the digests clients verify from what it returns. A file can be a binary or a function that renders it when needed. Caching is opt-in with `Phantom.Skill.with_cache/2`. Parsing `SKILL.md` frontmatter needs the optional `{:yamerl, "~> 0.10"}` dependency. -To add a skill at runtime, use `Phantom.Cache.add_skill/2`. See `Phantom.Skill`. +Skill routes are resource templates named by their path: they share names and +allow-lists with your resource templates, and are not listed in +`resources/templates/list`. To add skills at runtime, use +`Phantom.Cache.add_skill/2`. See `Phantom.Skill`. ## Eliciting input diff --git a/lib/phantom/cache.ex b/lib/phantom/cache.ex index d9c6838..b2b2fe6 100644 --- a/lib/phantom/cache.ex +++ b/lib/phantom/cache.ex @@ -7,6 +7,8 @@ defmodule Phantom.Cache do to use the functions herein. """ + require Logger + @doc """ Initialize the cache with compiled tooling for the given router. @@ -63,20 +65,22 @@ defmodule Phantom.Cache do resource_templates = resource_template_spec |> List.wrap() |> Enum.map(&Phantom.ResourceTemplate.build/1) - existing = :persistent_term.get({Phantom, router, :resource_templates}, []) + with_lock(router, fn -> + existing = :persistent_term.get({Phantom, router, :resource_templates}, []) - resource_templates = - Enum.sort_by(Enum.uniq(resource_templates ++ existing), &{&1.scheme, &1.name}) + resource_templates = + Enum.sort_by(Enum.uniq(resource_templates ++ existing), &{&1.scheme, &1.name}) - validate!(resource_templates) - raise_if_duplicates(resource_templates) - :persistent_term.put({Phantom, router, :resource_templates}, resource_templates) - Phantom.Router.__create_resource_routers__(resource_templates, __CALLER__) + validate!(resource_templates) + raise_if_duplicates(resource_templates) + :persistent_term.put({Phantom, router, :resource_templates}, resource_templates) + Phantom.Router.__create_resource_routers__(resource_templates, __CALLER__) + end) end @doc """ - Add a skill for the given router. Takes the same arguments as - `Phantom.Router.skill/3`: + Add one skill, or a list of skills, for the given router. Each takes the same + arguments as `Phantom.Router.skill/3`: Phantom.Cache.add_skill(MyApp.MCP.Router, path: "acme/billing/refunds", @@ -84,37 +88,77 @@ defmodule Phantom.Cache do function: :refunds ) - This regenerates the router's skill routes. + This regenerates the router's skill routes once for each call. """ - def add_skill(router, skill_spec) do - skill = skill_spec |> Map.new() |> Map.put(:router, router) |> Phantom.Router.skill_template() - validate!([skill]) + def add_skill(router, skill_specs) do + skill_specs = + if is_map(skill_specs) or Keyword.keyword?(skill_specs), + do: [skill_specs], + else: skill_specs + + skills = + Enum.map(skill_specs, fn spec -> + spec |> Map.new() |> Map.put(:router, router) |> Phantom.Router.skill_template() + end) + + validate!(skills) register(router) - # Serialize updates on this node, so concurrent calls don't lose a skill or - # regenerate the routes over each other. - :global.trans( - {{__MODULE__, router}, self()}, - fn -> - existing = :persistent_term.get({Phantom, router, :resource_templates}, []) - resource_templates = Enum.sort_by(Enum.uniq([skill | existing]), & &1.name) - raise_if_duplicates(resource_templates) - - # Regenerating the routes redefines the module on purpose; don't warn about it. - Code.with_diagnostics(fn -> - resource_templates - |> Enum.filter(&(&1.scheme == "skill")) - |> Phantom.Router.__create_resource_routers__(__ENV__) - end) - - :persistent_term.put({Phantom, router, :resource_templates}, resource_templates) - end, - [node()] - ) + with_lock(router, fn -> + existing = :persistent_term.get({Phantom, router, :resource_templates}, []) + resource_templates = Enum.sort_by(Enum.uniq(skills ++ existing), & &1.name) + raise_if_duplicates(resource_templates) + + __redefine_modules__(fn -> + resource_templates + |> Enum.filter(&(&1.scheme == "skill")) + |> Phantom.Router.__create_resource_routers__(__ENV__) + end) + + :persistent_term.put({Phantom, router, :resource_templates}, resource_templates) + end) :ok end + # Serializes updates to a router's resource templates on this node, which is + # where `:persistent_term` and the generated routers live. + defp with_lock(router, fun), do: :global.trans({{__MODULE__, router}, self()}, fun, [node()]) + + @doc false + # Runs `fun`, which redefines modules on purpose. Drops the warning about that, + # and logs every other diagnostic, so a compile error isn't lost. + def __redefine_modules__(fun) do + {result, diagnostics} = + Code.with_diagnostics(fn -> + try do + {:ok, fun.()} + rescue + exception -> {:error, exception, __STACKTRACE__} + end + end) + + for %{message: message, severity: severity} = diagnostic <- diagnostics, + not String.starts_with?(message, "redefining module") do + Logger.log( + if(severity == :error, do: :error, else: :warning), + Exception.format_file_line( + Path.relative_to_cwd(diagnostic.file || "nofile"), + line(diagnostic) + ) <> + " " <> message + ) + end + + case result do + {:ok, value} -> value + {:error, exception, stacktrace} -> reraise exception, stacktrace + end + end + + defp line(%{position: {line, _column}}), do: line + defp line(%{position: line}), do: line + @doc """ List all the entities for the given type. """ diff --git a/lib/phantom/router.ex b/lib/phantom/router.ex index ce00c7d..0560bb5 100644 --- a/lib/phantom/router.ex +++ b/lib/phantom/router.ex @@ -1332,12 +1332,28 @@ defmodule Phantom.Router do match(_, to: Phantom.ResourcePlug.NotFound) end - true = :code.soft_purge(resource_router) + soft_purge!(resource_router) Module.create(resource_router, body, Macro.Env.location(env)) end ) end + # A request still running the router's old code blocks replacing it, so wait + # for it to finish rather than kill it. + defp soft_purge!(module, attempts \\ 50) do + cond do + :code.soft_purge(module) -> + :ok + + attempts > 0 -> + Process.sleep(20) + soft_purge!(module, attempts - 1) + + true -> + raise "can't replace #{inspect(module)}: requests are still running its old code" + end + end + # A skill's route globs its files, so a skill nested within it must match first. defp sort_skill_routes([%ResourceTemplate{scheme: "skill"} | _] = skills) do Phantom.Router.Skills.validate_nesting!(skills) diff --git a/lib/phantom/router/skills.ex b/lib/phantom/router/skills.ex index df20aa0..d6f5d86 100644 --- a/lib/phantom/router/skills.ex +++ b/lib/phantom/router/skills.ex @@ -1,7 +1,9 @@ defmodule Phantom.Router.Skills do @moduledoc false # Serves the MCP Skills extension from a router's skill routes, which are - # resource templates under `skill:///*file`. + # resource templates under `skill:///*file`. The router's generated + # resource router resolves every skill URI, so a file always belongs to the + # deepest skill route whose path contains it. require Logger @@ -25,17 +27,13 @@ defmodule Phantom.Router.Skills do defp param?(":" <> _), do: true defp param?(_segment), do: false - defp skill_routes(routes) do - routes |> Enum.filter(&(&1.scheme == "skill")) |> Enum.sort_by(&route_order/1) - end - # A route nested under another must not have path params below it, since its # files are part of the other skill's manifest. def validate_nesting!(routes) do for outer <- routes, inner <- routes, outer != inner do {prefix, rest} = Enum.split(segments(inner), length(segments(outer))) - if length(rest) > 0 and compatible?(segments(outer), prefix) and Enum.any?(rest, ¶m?/1) do + if rest != [] and compatible?(segments(outer), prefix) and Enum.any?(rest, ¶m?/1) do raise ArgumentError, "skill #{inspect(inner.name)} is nested in skill #{inspect(outer.name)}, " <> "so it can't have path params below #{inspect(outer.name)}" @@ -50,16 +48,9 @@ defmodule Phantom.Router.Skills do Enum.all?(Enum.zip(a, b), fn {x, y} -> x == y or param?(x) or param?(y) end) end - defp match(route_segments, segments, params \\ %{}) - defp match([], [], params), do: {:ok, params} - - defp match([":" <> param | route_segments], [value | segments], params), - do: match(route_segments, segments, Map.put(params, param, value)) - - defp match([same | route_segments], [same | segments], params), - do: match(route_segments, segments, params) - - defp match(_route_segments, _segments, _params), do: :error + defp skill_routes(router) do + Enum.filter(Cache.list(nil, router, :resource_templates), &(&1.scheme == "skill")) + end defp concrete(route, params) do Enum.map(segments(route), fn @@ -70,43 +61,57 @@ defmodule Phantom.Router.Skills do defp base_uri(segments), do: "skill://" <> Enum.map_join(segments, "/", &Skill.encode_segment/1) - # The routes serving `segments`, deepest first: its own route, then the route of - # each skill it's nested in. - defp chain(routes, segments) do - for depth <- length(segments)..1//-1, - prefix = Enum.take(segments, depth), - route = Enum.find(routes, &match?({:ok, _}, match(segments(&1), prefix))), - route != nil, - do: {route, prefix} + # Resolves a canonical skill URI through the resource router to the route that + # serves it, the route's params and segments, and the path within the skill. + defp resolve(router, uri) do + with {:ok, {^uri, %{"file" => file} = params, %ResourceTemplate{scheme: "skill"} = route}} <- + Phantom.Router.resolve_resource(router, nil, uri), + params = Map.delete(params, "file"), + segments = concrete(route, params), + path = Enum.join(file, "/"), + true <- Skill.uri(base_uri(segments), path) == uri do + {:ok, route, params, segments, path} + else + _ -> :error + end + end + + # The skill route whose root is exactly `segments`, as the resource router resolves it. + defp root(router, segments) do + case resolve(router, base_uri(segments)) do + {:ok, route, params, ^segments, ""} -> {:ok, route, params} + _ -> :error + end end - # Routed skills nested directly in the skill at `segments`. - defp nested(routes, segments) do + # Skill routes nested in the skill at `segments`, at any depth. + defp descendants(router, segments) do depth = length(segments) - candidates = - routes - |> Enum.flat_map(fn route -> - {prefix, rest} = Enum.split(segments(route), depth) - - if rest != [] and match?({:ok, _}, match(prefix, segments)), - do: [{route, segments ++ rest}], - else: [] - end) - |> Enum.uniq_by(&elem(&1, 1)) - - Enum.reject(candidates, fn {_, inner} -> - Enum.any?(candidates, fn {_, outer} -> - length(outer) < length(inner) and Enum.take(inner, length(outer)) == outer - end) - end) + for route <- skill_routes(router), + {_prefix, rest} = Enum.split(segments(route), depth), + rest != [] and not Enum.any?(rest, ¶m?/1), + inner = segments ++ rest, + {:ok, ^route, params} <- [root(router, inner)], + do: {route, params, inner} + end + + # A route is accessible when it, or a skill route it's nested in, is allowed. + defp accessible?(router, session, route, segments) do + allowed = Cache.list(session, router, :resource_templates) + route in allowed or Enum.any?(ancestors(router, segments), &(&1 in allowed)) + end + + defp ancestors(router, segments) do + for depth <- (length(segments) - 1)..1//-1, + {:ok, route, _params} <- [root(router, Enum.take(segments, depth))], + do: route end ## Actions # Returns `{:ok, skill, session}`, `{:absent, session}` or `{:error, error, session}`. - defp call(route, segments, session) do - {:ok, params} = match(segments(route), segments) + defp call(route, params, session) do route |> apply_action(params, session) |> action_result(route) end @@ -142,37 +147,41 @@ defmodule Phantom.Router.Skills do "skill actions are synchronous. Got: #{inspect(other)}" end - # The skill's files, with each nested skill's files in place of the skill's own - # copy of its directory. Returns the files, as in `Skill.manifest/2`, and every - # skill they come from. - defp tree(routes, skill, segments, session) do - own = Map.new(skill.files, fn {path, _content} -> {path, {skill, path}} end) - - Enum.reduce(nested(routes, segments), {own, [skill]}, fn {route, inner}, {files, skills} -> - case call(route, inner, session) do - {:ok, nested_skill, _session} -> - {nested_files, nested_skills} = tree(routes, nested_skill, inner, session) - dir = inner |> Enum.drop(length(segments)) |> Enum.join("/") - - files = - files - |> Map.reject(fn {path, _} -> String.starts_with?(path, dir <> "/") end) - |> Map.merge( - Map.new(nested_files, fn {path, served} -> {dir <> "/" <> path, served} end) - ) - - {files, skills ++ nested_skills} - - _absent_or_error -> - {files, skills} - end - end) + # The files of the skill at `segments`, each from the skill that owns it: the + # deepest of the skill and its nested skills whose directory contains the file. + # Returns the files, as in `Skill.manifest/2`, and every skill they come from. + defp tree(router, skill, segments, session) do + owners = + for {route, params, inner} <- descendants(router, segments), + do: {inner |> Enum.drop(length(segments)) |> Enum.join("/"), route, params} + + dirs = Enum.map(owners, &elem(&1, 0)) + + skills = + [{"", skill}] ++ + for {dir, route, params} <- owners, + {:ok, nested, _session} <- [call(route, params, session)], + do: {dir, nested} + + files = + for {dir, owner} <- skills, + {path, _content} <- owner.files, + full = if(dir == "", do: path, else: dir <> "/" <> path), + owner_dir(full, dirs) == dir, + into: %{}, + do: {full, {owner, path}} + + {files, Enum.map(skills, &elem(&1, 1))} end - defp entry(router, skill, segments, session) do - {files, skills} = - tree(skill_routes(Cache.list(nil, router, :resource_templates)), skill, segments, session) + defp owner_dir(path, dirs) do + dirs + |> Enum.filter(&String.starts_with?(path, &1 <> "/")) + |> Enum.max_by(&String.length/1, fn -> "" end) + end + defp entry(router, skill, segments, session) do + {files, skills} = tree(router, skill, segments, session) base_uri = base_uri(segments) resources = @@ -227,7 +236,7 @@ defmodule Phantom.Router.Skills do defp list_entry(router, route, session) do segments = segments(route) - with {:ok, skill, session} <- call(route, segments, session) do + with {:ok, skill, session} <- call(route, %{}, session) do entry(router, skill, segments, session) end catch @@ -241,9 +250,9 @@ defmodule Phantom.Router.Skills do end def get(router, session, uri) do - with {:ok, route, segments, "SKILL.md"} <- resolve(router, session, uri), - true <- route in Cache.list(session, router, :resource_templates), - {:ok, skill, session} <- call(route, segments, session), + with {:ok, route, params, segments, "SKILL.md"} <- resolve(router, uri), + true <- accessible?(router, session, route, segments), + {:ok, skill, session} <- call(route, params, session), {:ok, entry, skills} <- entry(router, skill, segments, session) do {:reply, Request.with_cache(%{skill: entry}, Skill.cache_hints(skills, session)), session} else @@ -253,12 +262,11 @@ defmodule Phantom.Router.Skills do end def read_directory(router, session, uri) do - with {:ok, _route, segments, dir} <- resolve(router, session, uri), - {:ok, skill, skill_segments, session} <- serving(router, segments, session), - routes = skill_routes(Cache.list(nil, router, :resource_templates)), - {files, _skills} = tree(routes, skill, skill_segments, session), - dir = relative(segments, dir, skill_segments), - {:ok, resources} <- Skill.list_directory(files, base_uri(skill_segments), dir) do + with {:ok, route, params, segments, dir} <- resolve(router, uri), + true <- accessible?(router, session, route, segments), + {:ok, skill, session} <- call(route, params, session), + {files, _skills} = tree(router, skill, segments, session), + {:ok, resources} <- Skill.list_directory(files, base_uri(segments), dir) do # Directories are listed whole, so a cursor is never needed. {:reply, %{resources: resources}, session} else @@ -269,13 +277,14 @@ defmodule Phantom.Router.Skills do # Called by `Phantom.ResourcePlug` for `resources/read` of a skill route's file. def read(route, %{"file" => file} = params, uri, session) do - segments = concrete(route, Map.delete(params, "file")) - - with true <- Skill.uri(base_uri(segments), Enum.join(file, "/")) == uri, - {:ok, skill, skill_segments, session} <- - serving(session.router, segments, session), - {:ok, content} <- - Skill.read(skill, relative(segments, Enum.join(file, "/"), skill_segments), uri) do + params = Map.delete(params, "file") + segments = concrete(route, params) + path = Enum.join(file, "/") + + with true <- Skill.uri(base_uri(segments), path) == uri, + true <- accessible?(session.router, session, route, segments), + {:ok, skill, session} <- call(route, params, session), + {:ok, content} <- Skill.read(skill, path, uri) do {:reply, content, session} else {:error, _error, %Session{}} = error -> error @@ -283,54 +292,5 @@ defmodule Phantom.Router.Skills do end end - # Resolves a canonical skill URI through the router's resource router to its - # route, the route's segments, and the path within the skill. - defp resolve(router, session, uri) do - with {:ok, {^uri, %{"file" => file} = params, %ResourceTemplate{scheme: "skill"} = route}} <- - Phantom.Router.resolve_resource(router, session, uri), - segments = concrete(route, Map.delete(params, "file")), - path = Enum.join(file, "/"), - true <- Skill.uri(base_uri(segments), path) == uri do - {:ok, route, segments, path} - else - _ -> :error - end - end - - # The skill that serves a path under `segments`: the route's own skill, or when it - # serves none, the skill it's nested in. Each must be allowed for the session, or - # be nested in a skill that is. - defp serving(router, segments, session) do - allowed = Cache.list(session, router, :resource_templates) - chain = chain(skill_routes(Cache.list(nil, router, :resource_templates)), segments) - serve(chain, allowed, session, nil) - end - - defp serve([], _allowed, session, nil), do: {:absent, session} - defp serve([], _allowed, _session, error), do: error - - defp serve([{route, segments} | outer] = chain, allowed, session, error) do - if Enum.any?(chain, &(elem(&1, 0) in allowed)), - do: route |> call(segments, session) |> served(segments, outer, allowed, error), - else: {:absent, session} - end - - defp served({:ok, skill, session}, segments, _outer, _allowed, _error), - do: {:ok, skill, segments, session} - - defp served({:absent, session}, _segments, outer, allowed, error), - do: serve(outer, allowed, session, error) - - defp served({:error, _error, session} = new_error, _segments, outer, allowed, error), - do: serve(outer, allowed, session, error || new_error) - - # A path under `segments`, made relative to the skill at `skill_segments`. - defp relative(segments, path, skill_segments) do - segments - |> Enum.drop(length(skill_segments)) - |> Enum.concat(String.split(path, "/", trim: true)) - |> Enum.join("/") - end - defp invalid_params(message), do: %{Request.invalid_params() | message: message} end diff --git a/lib/phantom/skill.ex b/lib/phantom/skill.ex index 67550fb..ab78679 100644 --- a/lib/phantom/skill.ex +++ b/lib/phantom/skill.ex @@ -22,8 +22,9 @@ defmodule Phantom.Skill do first. Skills with path params are left out of `skills/list`, but clients can still get them by URI. - Each skill is a resource template named by its path, so - `Phantom.Session.allow_resource_templates/2` limits skills too. + Each skill is a resource template named by its path, so skill paths share names with + resource templates, and `Phantom.Session.allow_resource_templates/2` limits skills too. + Skill routes are not listed in `resources/templates/list`. ## Actions @@ -38,11 +39,12 @@ defmodule Phantom.Skill do ## Nested skills - A skill routed under another skill's path is nested in it: the parent's manifest and - directory listings include the nested skill's files in place of the parent's own - files in that directory, and reads there go to the nested skill's action. When that - action returns `nil` or an error, the parent's own files are used instead. A session - allowed the parent may read the nested skill's files. + A skill routed under another skill's path is nested in it. Each file belongs to the + deepest skill whose directory contains it: the parent's manifest and directory + listings take each nested skill's files from its own action, and leave out the + parent's own files in that directory. A nested action that returns `nil` or an error + serves no files; one that raises fails the parent's `skills/get` and is left out of + `skills/list`. A session allowed a skill may use every skill nested in it. ## Rendering @@ -52,8 +54,9 @@ defmodule Phantom.Skill do ## Caching - Results are private and uncached unless the action calls `with_cache/2`. Only - declare `scope: :public` when the skill is the same for every user who can see it. + `skills/list` and `skills/get` results are private and uncached unless the action + calls `with_cache/2`. Only declare `scope: :public` when the skill is the same for + every user who can see it. {:reply, git_workflow(%{}) |> Phantom.Skill.with_cache(ttl_ms: 300_000, scope: :public), session} @@ -98,15 +101,13 @@ defmodule Phantom.Skill do @frontmatter_pattern ~r/\A---[ \t]*\R(?.*?)^---[ \t]*(?:\R|\z)(?.*)\z/ms @simple_key ~r/\A[A-Za-z0-9_-]+\z/ - # `digests` is internal: digests of static files, computed by `embed_skills/1`. - defstruct frontmatter: %{}, files: %{}, dynamic: false, cache: nil, digests: %{} + defstruct frontmatter: %{}, files: %{}, dynamic: false, cache: nil @type t :: %__MODULE__{ frontmatter: %{required(String.t()) => term()}, files: %{required(String.t()) => binary() | (-> iodata())}, dynamic: boolean(), - cache: nil | [ttl_ms: non_neg_integer(), scope: :public | :private], - digests: %{optional(String.t()) => {non_neg_integer(), String.t()}} + cache: nil | [ttl_ms: non_neg_integer(), scope: :public | :private] } @doc false @@ -269,12 +270,6 @@ defmodule Phantom.Skill do {path, _} -> raise ArgumentError, "#{dir}: more than one file is served at #{path}" end) - # SKILL.md is served with its frontmatter, so its digest covers both. - digests = - for {path, {:static, served}, _} <- embedded, - into: %{}, - do: {path, {byte_size(served), digest(served)}} - contents = for {path, content, body} <- embedded, do: {path, embed_content(content, body)} quote do @@ -283,10 +278,7 @@ defmodule Phantom.Skill do def unquote(String.to_atom(String.replace(name, "-", "_")))(var!(assigns)) do _ = var!(assigns) - %{ - Phantom.Skill.new(unquote(Macro.escape(frontmatter)), %{unquote_splicing(contents)}) - | digests: unquote(Macro.escape(digests)) - } + Phantom.Skill.new(unquote(Macro.escape(frontmatter)), %{unquote_splicing(contents)}) end end end @@ -451,8 +443,9 @@ defmodule Phantom.Skill do defp validate_frontmatter!(frontmatter, context \\ "skill") do validate_name!(frontmatter["name"], context) - if not (is_binary(frontmatter["description"]) and frontmatter["description"] != "") do - raise ArgumentError, "#{context}: frontmatter must have a description" + if not (is_binary(frontmatter["description"]) and + String.length(frontmatter["description"]) in 1..1024) do + raise ArgumentError, "#{context}: frontmatter must have a description of 1-1024 characters" end end @@ -488,18 +481,6 @@ defmodule Phantom.Skill do defp digest(bytes), do: "sha256:" <> Base.encode16(:crypto.hash(:sha256, bytes), case: :lower) - # Reuses a digest computed by `embed_skills/1` while the file is still static. - defp digest(%__MODULE__{files: files, digests: digests}, path, bytes) do - case digests do - %{^path => {size, digest}} - when is_binary(:erlang.map_get(path, files)) and byte_size(bytes) == size -> - digest - - _ -> - digest(bytes) - end - end - @doc false # The manifest of a skill's files. `files` maps each path within the skill to the # skill that serves it and the file's path within that skill. @@ -510,7 +491,7 @@ defmodule Phantom.Skill do %{ uri: uri(base_uri, path), - digest: digest(skill, own_path, bytes), + digest: digest(bytes), size: byte_size(bytes) } end diff --git a/test/phantom/skill_edge_test.exs b/test/phantom/skill_edge_test.exs index 9848383..c1ea400 100644 --- a/test/phantom/skill_edge_test.exs +++ b/test/phantom/skill_edge_test.exs @@ -79,6 +79,38 @@ defmodule Phantom.SkillEdgeTest do {:reply, Skill.new(%{name: "git-workflow", description: "Git"}, %{"SKILL.md" => "# Git\n"}), session} end + + def top(_params, session) do + {:reply, + Skill.new(%{name: "top", description: "Top skill"}, %{ + "SKILL.md" => "# Top\n", + "mid/notes.md" => "top copy\n", + "mid/bottom/notes.md" => "TOP COPY\n" + }), session} + end + + def mid(_params, session), do: {:reply, nil, session} + + def bottom(_params, session) do + {:reply, + Skill.new(%{name: "bottom", description: "Bottom skill"}, %{ + "SKILL.md" => "# Bottom\n", + "notes.md" => "BOTTOM\n" + }), session} + end + + def raiser(_params, _session), do: raise("nested boom") + + def slow_skill(_params, session) do + send(session.assigns.test, {:blocked, self()}) + + receive do + :go -> + {:reply, + Skill.new(%{name: "slow-skill", description: "Slow"}, %{"SKILL.md" => "# Slow\n"}), + session} + end + end end defmodule NestRouter do @@ -89,6 +121,16 @@ defmodule Phantom.SkillEdgeTest do skill "nest/outer/gone", Actions skill "files/spaced-files", Actions skill "git-workflow", Actions + skill "deep/top", Actions + skill "deep/top/mid", Actions + skill "deep/top/mid/bottom", Actions + end + + defmodule RaiseRouter do + use Phantom.Router, name: "Raise", vsn: "1.0" + + skill "boom/outer", Actions + skill "boom/outer/raiser", Actions end defmodule FailRouter do @@ -133,8 +175,23 @@ defmodule Phantom.SkillEdgeTest do use Phantom.Router, name: "Paged", vsn: "1.0" end + defmodule BatchRouter do + use Phantom.Router, name: "Batch", vsn: "1.0" + end + + defmodule InFlightRouter do + use Phantom.Router, name: "InFlight", vsn: "1.0" + end + setup do - for router <- [NestRouter, FailRouter, PublicRouter, StaticFirstRouter, ParamFirstRouter], + for router <- [ + NestRouter, + RaiseRouter, + FailRouter, + PublicRouter, + StaticFirstRouter, + ParamFirstRouter + ], do: Cache.register(router) :ok @@ -169,7 +226,6 @@ defmodule Phantom.SkillEdgeTest do assert [ "skill://nest/outer/SKILL.md", - "skill://nest/outer/gone/notes.md", "skill://nest/outer/inner/SKILL.md", "skill://nest/outer/inner/notes.md" ] = Enum.map(resources, & &1.uri) @@ -183,8 +239,44 @@ defmodule Phantom.SkillEdgeTest do assert {:ok, "hello\n"} = read(NestRouter, "skill://nest/outer/inner/notes.md") end - test "falls back to the parent's files when the nested action serves no skill" do - assert {:ok, "kept\n"} = read(NestRouter, "skill://nest/outer/gone/notes.md") + test "files owned by a nested skill that serves none are neither listed nor read" do + assert {:error, _} = read(NestRouter, "skill://nest/outer/gone/notes.md") + end + + test "a deeper skill under an absent nested skill is still part of the manifest" do + assert {:reply, %{skill: %{resources: resources}}, _} = + get(NestRouter, "skill://deep/top/SKILL.md") + + assert [ + "skill://deep/top/SKILL.md", + "skill://deep/top/mid/bottom/SKILL.md", + "skill://deep/top/mid/bottom/notes.md" + ] = Enum.map(resources, & &1.uri) + + for %{uri: uri, digest: digest, size: size} <- resources do + assert {:ok, bytes} = read(NestRouter, uri) + assert digest == sha256(bytes), uri + assert size == byte_size(bytes), uri + end + + assert {:ok, "BOTTOM\n"} = read(NestRouter, "skill://deep/top/mid/bottom/notes.md") + assert {:error, _} = read(NestRouter, "skill://deep/top/mid/notes.md") + end + + test "skills/get on a nested skill is allowed by the parent's permission" do + assert {:reply, %{skill: %{frontmatter: %{"name" => "inner"}}}, _} = + get(NestRouter, "skill://nest/outer/inner/SKILL.md", + allowed_resource_templates: ["nest/outer"] + ) + end + + @tag :capture_log + test "a nested action that raises fails skills/get, and is left out of skills/list" do + assert_raise RuntimeError, "nested boom", fn -> + get(RaiseRouter, "skill://boom/outer/SKILL.md") + end + + assert {:reply, %{skills: []}, _} = dispatch(RaiseRouter, "skills/list", %{}) end test "directory reads list the nested skill's files" do @@ -349,6 +441,69 @@ defmodule Phantom.SkillEdgeTest do get(LateRouter, "skill://late/git-workflow/SKILL.md") end + test "adds a list of skills" do + Cache.add_skill(BatchRouter, [ + [path: "b1/git-workflow", handler: Actions], + [path: "b2/git-workflow", handler: Actions] + ]) + + for uri <- ["skill://b1/git-workflow/SKILL.md", "skill://b2/git-workflow/SKILL.md"] do + assert {:reply, %{skill: %{frontmatter: %{"name" => "git-workflow"}}}, _} = + get(BatchRouter, uri) + end + end + + test "waits for a read still running in the old routes" do + Cache.add_skill(InFlightRouter, + path: "slow/slow-skill", + handler: Actions, + function: :slow_skill + ) + + test = self() + + reader = + Task.async(fn -> + read(InFlightRouter, "skill://slow/slow-skill/SKILL.md", assigns: %{test: test}) + end) + + assert_receive {:blocked, action} + + Cache.add_skill(InFlightRouter, path: "one/git-workflow", handler: Actions) + + adding = + Task.async(fn -> + Cache.add_skill(InFlightRouter, path: "two/git-workflow", handler: Actions) + end) + + Process.sleep(50) + send(action, :go) + + assert {:ok, "---\n" <> _} = Task.await(reader) + assert :ok = Task.await(adding) + + for uri <- ["skill://one/git-workflow/SKILL.md", "skill://two/git-workflow/SKILL.md"] do + assert {:reply, %{skill: _}, _} = get(InFlightRouter, uri) + end + end + + test "logs compile errors from regenerating the routes" do + log = + capture_log(fn -> + assert_raise CompileError, fn -> + Cache.__redefine_modules__(fn -> + Module.create( + Phantom.SkillEdgeTest.Broken, + quote(do: def(broken, do: undefined_variable)), + __ENV__ + ) + end) + end + end) + + assert log =~ "undefined_variable" + end + test "keeps every skill added concurrently" do Cache.register(ConcurrentRouter) diff --git a/test/phantom/skill_router_test.exs b/test/phantom/skill_router_test.exs index 6fde849..6cfc4fd 100644 --- a/test/phantom/skill_router_test.exs +++ b/test/phantom/skill_router_test.exs @@ -9,6 +9,20 @@ defmodule Phantom.SkillRouterTest do embed_skills "../support/skills/*" embed_skills "../support/skills/refunds/partial-refunds" + # Same-size edits to an embedded skill, which must not reuse stale digests. + def edited(_params, session) do + skill = git_workflow(%{}) + + skill = %{ + skill + | frontmatter: + Map.update!(skill.frontmatter, "description", &String.replace(&1, "Git", "Hg!")), + files: Map.update!(skill.files, "SKILL.md", &String.replace(&1, "main", "trnk")) + } + + {:reply, skill, session} + end + def git_workflow(_params, session) do {:reply, git_workflow(%{}) |> Phantom.Skill.with_cache(ttl_ms: 300_000, scope: :public), session} @@ -76,6 +90,12 @@ defmodule Phantom.SkillRouterTest do use Phantom.Router, name: "RuntimeSkills", vsn: "1.0" end + defmodule EditedRouter do + use Phantom.Router, name: "SkillEdited", vsn: "1.0" + + skill "edited/git-workflow", Skills, :edited + end + defmodule MismatchRouter do use Phantom.Router, name: "SkillMismatch", vsn: "1.0" @@ -89,6 +109,7 @@ defmodule Phantom.SkillRouterTest do setup do Cache.register(TestRouter) Cache.register(MismatchRouter) + Cache.register(EditedRouter) Cache.register(EmptyRouter) Cache.register(RuntimeRouter) :ok @@ -229,6 +250,20 @@ defmodule Phantom.SkillRouterTest do assert_received :rendered_skill_md end + test "digests cover the bytes the action returns" do + uri = "skill://edited/git-workflow/SKILL.md" + + assert {:reply, %{skill: %{resources: [%{digest: digest, size: size}]}}, _} = + dispatch_to(EditedRouter, "skills/get", %{"uri" => uri}) + + assert {:reply, %{contents: [%{text: text}]}, _} = + dispatch_to(EditedRouter, "resources/read", %{"uri" => uri}) + + assert text =~ "Hg!" and text =~ "trnk" + assert digest == sha256(text) + assert size == byte_size(text) + end + test "passes path params to the action" do assert {:reply, %{skill: %{frontmatter: %{"name" => "study-review"}}}, _} = dispatch("skills/get", %{"uri" => "skill://studies/42/study-review/SKILL.md"}) diff --git a/test/phantom/skill_test.exs b/test/phantom/skill_test.exs index 0534f9e..fa8bd08 100644 --- a/test/phantom/skill_test.exs +++ b/test/phantom/skill_test.exs @@ -66,6 +66,17 @@ defmodule Phantom.SkillTest do end end + test "limits the description to 1024 characters" do + assert %Skill{} = + Skill.new(%{name: "a", description: String.duplicate("é", 1024)}, %{ + "SKILL.md" => "" + }) + + assert_raise ArgumentError, ~r/description/, fn -> + Skill.new(%{name: "a", description: String.duplicate("a", 1025)}, %{"SKILL.md" => ""}) + end + end + test "enforces the Agent Skills naming rules" do for name <- [ "Refunds", @@ -259,15 +270,4 @@ defmodule Phantom.SkillTest do assert {:ok, ^frontmatter, "# Body\n"} = Skill.parse(skill_md) end end - - describe "embedded skills" do - test "static files have digests computed at compile time" do - assert %Skill{digests: %{"SKILL.md" => {_size, "sha256:" <> _}}} = - Embedded.git_workflow(%{}) - - assert %Skill{digests: digests} = Embedded.refunds(%{user: "Ada"}) - assert Map.has_key?(digests, "examples/email.md") - refute Map.has_key?(digests, "SKILL.md") - end - end end From c74ebb052c353f1af01c29909cd8041b9855fa33 Mon Sep 17 00:00:00 2001 From: David Bernheisel Date: Sun, 4 Oct 2026 19:51:38 -0400 Subject: [PATCH 3/4] Skills: simplify --- lib/phantom/cache.ex | 5 +- lib/phantom/router.ex | 8 +- lib/phantom/router/skills.ex | 219 +++++++++++++++++++++------------ lib/phantom/skill.ex | 226 +++++++++-------------------------- 4 files changed, 202 insertions(+), 256 deletions(-) diff --git a/lib/phantom/cache.ex b/lib/phantom/cache.ex index b2b2fe6..51a1a6e 100644 --- a/lib/phantom/cache.ex +++ b/lib/phantom/cache.ex @@ -121,13 +121,10 @@ defmodule Phantom.Cache do :ok end - # Serializes updates to a router's resource templates on this node, which is - # where `:persistent_term` and the generated routers live. + # The lock is local, because :persistent_term and the generated routers are local to each node. defp with_lock(router, fun), do: :global.trans({{__MODULE__, router}, self()}, fun, [node()]) @doc false - # Runs `fun`, which redefines modules on purpose. Drops the warning about that, - # and logs every other diagnostic, so a compile error isn't lost. def __redefine_modules__(fun) do {result, diagnostics} = Code.with_diagnostics(fn -> diff --git a/lib/phantom/router.ex b/lib/phantom/router.ex index 0560bb5..691ef15 100644 --- a/lib/phantom/router.ex +++ b/lib/phantom/router.ex @@ -1007,7 +1007,6 @@ defmodule Phantom.Router do end @doc false - # The resource template that routes a skill's files to its action. def skill_template(attrs) do attrs = Map.new(attrs) [first | rest] = segments = String.split(attrs.path, "/") @@ -1032,7 +1031,7 @@ defmodule Phantom.Router do name: attrs.path, router: Module.concat([attrs.router, ResourceRouter, "Skill"]), handler: attrs.handler, - function: attrs[:function] || String.to_atom(String.replace(name, "-", "_")), + function: attrs[:function] || Skill.function_name(name), meta: attrs[:meta] || %{file: "nofile", line: 0} ) end @@ -1338,8 +1337,7 @@ defmodule Phantom.Router do ) end - # A request still running the router's old code blocks replacing it, so wait - # for it to finish rather than kill it. + # Wait for old code to finish, because a hard purge kills the processes that run it. defp soft_purge!(module, attempts \\ 50) do cond do :code.soft_purge(module) -> @@ -1354,7 +1352,7 @@ defmodule Phantom.Router do end end - # A skill's route globs its files, so a skill nested within it must match first. + # A nested skill must match before its parent, because the parent route globs all files. defp sort_skill_routes([%ResourceTemplate{scheme: "skill"} | _] = skills) do Phantom.Router.Skills.validate_nesting!(skills) Enum.sort_by(skills, &Phantom.Router.Skills.route_order/1) diff --git a/lib/phantom/router/skills.ex b/lib/phantom/router/skills.ex index d6f5d86..e9f7098 100644 --- a/lib/phantom/router/skills.ex +++ b/lib/phantom/router/skills.ex @@ -1,9 +1,6 @@ defmodule Phantom.Router.Skills do @moduledoc false - # Serves the MCP Skills extension from a router's skill routes, which are - # resource templates under `skill:///*file`. The router's generated - # resource router resolves every skill URI, so a file always belongs to the - # deepest skill route whose path contains it. + # This module serves the MCP Skills extension from the skill routes of a router. require Logger @@ -13,22 +10,16 @@ defmodule Phantom.Router.Skills do alias Phantom.Session alias Phantom.Skill - ## Routes - - # A route's path segments, without its `*file` glob. def segments(%ResourceTemplate{authority: authority, path: path}) do [authority | path |> String.replace_suffix("/*file", "") |> String.split("/", trim: true)] end - # Deepest first; at the same depth, static segments before path params. The - # generated resource router matches in this order. def route_order(route), do: {-length(segments(route)), Enum.map(segments(route), ¶m?/1)} defp param?(":" <> _), do: true defp param?(_segment), do: false - # A route nested under another must not have path params below it, since its - # files are part of the other skill's manifest. + # A nested route cannot have path params, because its files are in the manifest of its parent. def validate_nesting!(routes) do for outer <- routes, inner <- routes, outer != inner do {prefix, rest} = Enum.split(segments(inner), length(segments(outer))) @@ -52,6 +43,10 @@ defmodule Phantom.Router.Skills do Enum.filter(Cache.list(nil, router, :resource_templates), &(&1.scheme == "skill")) end + defp listable?(route) do + route.scheme == "skill" and not Enum.any?(segments(route), ¶m?/1) + end + defp concrete(route, params) do Enum.map(segments(route), fn ":" <> param -> params[param] @@ -59,24 +54,36 @@ defmodule Phantom.Router.Skills do end) end - defp base_uri(segments), do: "skill://" <> Enum.map_join(segments, "/", &Skill.encode_segment/1) + defp base_uri(segments), do: "skill://" <> Enum.map_join(segments, "/", &encode_segment/1) + + defp uri(base_uri, ""), do: base_uri + + defp uri(base_uri, path) do + base_uri <> "/" <> Enum.map_join(String.split(path, "/"), "/", &encode_segment/1) + end + + defp encode_segment(segment), do: URI.encode(segment, &URI.char_unreserved?/1) - # Resolves a canonical skill URI through the resource router to the route that - # serves it, the route's params and segments, and the path within the skill. defp resolve(router, uri) do - with {:ok, {^uri, %{"file" => file} = params, %ResourceTemplate{scheme: "skill"} = route}} <- - Phantom.Router.resolve_resource(router, nil, uri), - params = Map.delete(params, "file"), - segments = concrete(route, params), - path = Enum.join(file, "/"), - true <- Skill.uri(base_uri(segments), path) == uri do - {:ok, route, params, segments, path} - else - _ -> :error + case Phantom.Router.resolve_resource(router, nil, uri) do + {:ok, {^uri, params, %ResourceTemplate{scheme: "skill"} = route}} -> + locate(route, params, uri) + + _ -> + :error end end - # The skill route whose root is exactly `segments`, as the resource router resolves it. + defp locate(route, %{"file" => file} = params, uri) do + params = Map.delete(params, "file") + segments = concrete(route, params) + path = Enum.join(file, "/") + + if uri(base_uri(segments), path) == uri, + do: {:ok, route, params, segments, path}, + else: :error + end + defp root(router, segments) do case resolve(router, base_uri(segments)) do {:ok, route, params, ^segments, ""} -> {:ok, route, params} @@ -84,11 +91,10 @@ defmodule Phantom.Router.Skills do end end - # Skill routes nested in the skill at `segments`, at any depth. - defp descendants(router, segments) do + defp descendants(router, routes, segments) do depth = length(segments) - for route <- skill_routes(router), + for route <- routes, {_prefix, rest} = Enum.split(segments(route), depth), rest != [] and not Enum.any?(rest, ¶m?/1), inner = segments ++ rest, @@ -96,7 +102,6 @@ defmodule Phantom.Router.Skills do do: {route, params, inner} end - # A route is accessible when it, or a skill route it's nested in, is allowed. defp accessible?(router, session, route, segments) do allowed = Cache.list(session, router, :resource_templates) route in allowed or Enum.any?(ancestors(router, segments), &(&1 in allowed)) @@ -108,9 +113,6 @@ defmodule Phantom.Router.Skills do do: route end - ## Actions - - # Returns `{:ok, skill, session}`, `{:absent, session}` or `{:error, error, session}`. defp call(route, params, session) do route |> apply_action(params, session) |> action_result(route) end @@ -147,29 +149,27 @@ defmodule Phantom.Router.Skills do "skill actions are synchronous. Got: #{inspect(other)}" end - # The files of the skill at `segments`, each from the skill that owns it: the - # deepest of the skill and its nested skills whose directory contains the file. - # Returns the files, as in `Skill.manifest/2`, and every skill they come from. - defp tree(router, skill, segments, session) do - owners = - for {route, params, inner} <- descendants(router, segments), + defp tree(router, routes, skill, segments, session) do + nested = + for {route, params, inner} <- descendants(router, routes, segments), do: {inner |> Enum.drop(length(segments)) |> Enum.join("/"), route, params} - dirs = Enum.map(owners, &elem(&1, 0)) + nested_dirs = Enum.map(nested, &elem(&1, 0)) - skills = - [{"", skill}] ++ - for {dir, route, params} <- owners, - {:ok, nested, _session} <- [call(route, params, session)], - do: {dir, nested} + nested_skills = + for {dir, route, params} <- nested, + {:ok, nested_skill, _session} <- [call(route, params, session)], + do: {dir, nested_skill} + + skills = [{"", skill} | nested_skills] files = for {dir, owner} <- skills, {path, _content} <- owner.files, - full = if(dir == "", do: path, else: dir <> "/" <> path), - owner_dir(full, dirs) == dir, + full_path = if(dir == "", do: path, else: dir <> "/" <> path), + owner_dir(full_path, nested_dirs) == dir, into: %{}, - do: {full, {owner, path}} + do: {full_path, {owner, path}} {files, Enum.map(skills, &elem(&1, 1))} end @@ -180,14 +180,14 @@ defmodule Phantom.Router.Skills do |> Enum.max_by(&String.length/1, fn -> "" end) end - defp entry(router, skill, segments, session) do - {files, skills} = tree(router, skill, segments, session) + defp entry(router, routes, skill, segments, session) do + {files, skills} = tree(router, routes, skill, segments, session) base_uri = base_uri(segments) resources = if Enum.any?(skills, & &1.dynamic), do: {:ok, "dynamic"}, - else: Skill.manifest(files, base_uri) + else: manifest(files, base_uri) case resources do {:ok, resources} -> @@ -201,25 +201,53 @@ defmodule Phantom.Router.Skills do end end - ## Methods + defp manifest(files, base_uri) do + resources = + for {path, {skill, own_path}} <- Enum.sort(files) do + bytes = Skill.render(skill, own_path) + %{uri: uri(base_uri, path), digest: digest(bytes), size: byte_size(bytes)} + end + + total_size = resources |> Enum.map(& &1.size) |> Enum.sum() + + case Skill.check_limits(length(resources), total_size) do + :ok -> {:ok, resources} + {:error, reason} -> {:error, "#{base_uri} has #{reason}"} + end + end + + defp digest(bytes), do: "sha256:" <> Base.encode16(:crypto.hash(:sha256, bytes), case: :lower) + + defp cache_hints(skills, session) do + hints = Enum.map(skills, & &1.cache) + + if hints == [] or nil in hints do + [ttl_ms: 0, scope: :private] + else + public? = + is_nil(session.allowed_resource_templates) and + Enum.all?(hints, &(&1[:scope] == :public)) + + [ + ttl_ms: hints |> Enum.map(& &1[:ttl_ms]) |> Enum.min(), + scope: if(public?, do: :public, else: :private) + ] + end + end def list(router, session, cursor) do - routes = - session - |> Cache.list(router, :resource_templates) - |> Enum.filter( - &(&1.scheme == "skill" and not Enum.any?(segments(&1), fn s -> param?(s) end)) - ) + routes = session |> Cache.list(router, :resource_templates) |> Enum.filter(&listable?/1) case Phantom.Router.paginate(routes, cursor, & &1) do {:ok, page, next_cursor} -> - results = Enum.map(page, &list_entry(router, &1, session)) + all_routes = skill_routes(router) + results = Enum.map(page, &list_entry(router, all_routes, &1, session)) listed = for {:ok, entry, skills} <- results, do: {entry, skills} - # A route that serves no skill to this session makes the listing specific to it. + # The listing is private when a route serves no skill to this session. hints = if length(listed) == length(page), - do: Skill.cache_hints(Enum.flat_map(listed, &elem(&1, 1)), session), + do: cache_hints(Enum.flat_map(listed, &elem(&1, 1)), session), else: [ttl_ms: 0, scope: :private] {:reply, @@ -232,12 +260,9 @@ defmodule Phantom.Router.Skills do end end - # A skill that fails is left out, and the rest are listed. - defp list_entry(router, route, session) do - segments = segments(route) - + defp list_entry(router, routes, route, session) do with {:ok, skill, session} <- call(route, %{}, session) do - entry(router, skill, segments, session) + entry(router, routes, skill, segments(route), session) end catch kind, reason -> @@ -253,8 +278,8 @@ defmodule Phantom.Router.Skills do with {:ok, route, params, segments, "SKILL.md"} <- resolve(router, uri), true <- accessible?(router, session, route, segments), {:ok, skill, session} <- call(route, params, session), - {:ok, entry, skills} <- entry(router, skill, segments, session) do - {:reply, Request.with_cache(%{skill: entry}, Skill.cache_hints(skills, session)), session} + {:ok, entry, skills} <- entry(router, skill_routes(router), skill, segments, session) do + {:reply, Request.with_cache(%{skill: entry}, cache_hints(skills, session)), session} else {:error, _error, %Session{}} = error -> error _ -> {:error, invalid_params("No skill is served at #{uri}"), session} @@ -265,9 +290,8 @@ defmodule Phantom.Router.Skills do with {:ok, route, params, segments, dir} <- resolve(router, uri), true <- accessible?(router, session, route, segments), {:ok, skill, session} <- call(route, params, session), - {files, _skills} = tree(router, skill, segments, session), - {:ok, resources} <- Skill.list_directory(files, base_uri(segments), dir) do - # Directories are listed whole, so a cursor is never needed. + {files, _skills} = tree(router, skill_routes(router), skill, segments, session), + {:ok, resources} <- list_directory(files, base_uri(segments), dir) do {:reply, %{resources: resources}, session} else {:error, _error, %Session{}} = error -> error @@ -275,16 +299,11 @@ defmodule Phantom.Router.Skills do end end - # Called by `Phantom.ResourcePlug` for `resources/read` of a skill route's file. - def read(route, %{"file" => file} = params, uri, session) do - params = Map.delete(params, "file") - segments = concrete(route, params) - path = Enum.join(file, "/") - - with true <- Skill.uri(base_uri(segments), path) == uri, + def read(route, params, uri, session) do + with {:ok, route, params, segments, path} <- locate(route, params, uri), true <- accessible?(session.router, session, route, segments), {:ok, skill, session} <- call(route, params, session), - {:ok, content} <- Skill.read(skill, path, uri) do + {:ok, content} <- file_content(skill, path, uri) do {:reply, content, session} else {:error, _error, %Session{}} = error -> error @@ -292,5 +311,51 @@ defmodule Phantom.Router.Skills do end end + defp file_content(%Skill{files: files} = skill, path, uri) when is_map_key(files, path) do + bytes = Skill.render(skill, path) + content = %{uri: uri, mimeType: MIME.from_path(path)} + + if String.valid?(bytes), + do: {:ok, Map.put(content, :text, bytes)}, + else: {:ok, Map.put(content, :blob, Base.encode64(bytes))} + end + + defp file_content(%Skill{}, _path, _uri), do: :error + + defp list_directory(files, base_uri, dir) do + prefix = if dir == "", do: "", else: dir <> "/" + + children = + for {path, served} <- files, String.starts_with?(path, prefix), uniq: true do + case path |> String.replace_prefix(prefix, "") |> String.split("/", parts: 2) do + [file] -> {file, child(file, served)} + [subdir, _] -> {subdir, %{name: subdir, mimeType: "inode/directory"}} + end + end + + if children == [] do + :error + else + dir_uri = uri(base_uri, dir) + + {:ok, + children + |> Enum.sort_by(&elem(&1, 0)) + |> Enum.map(fn {name, child} -> + Map.put(child, :uri, dir_uri <> "/" <> encode_segment(name)) + end)} + end + end + + defp child("SKILL.md", {%Skill{frontmatter: frontmatter}, "SKILL.md"}) do + %{ + name: frontmatter["name"], + description: frontmatter["description"], + mimeType: "text/markdown" + } + end + + defp child(file, _served), do: %{name: file, mimeType: MIME.from_path(file)} + defp invalid_params(message), do: %{Request.invalid_params() | message: message} end diff --git a/lib/phantom/skill.ex b/lib/phantom/skill.ex index ab78679..7ceb62d 100644 --- a/lib/phantom/skill.ex +++ b/lib/phantom/skill.ex @@ -158,16 +158,23 @@ defmodule Phantom.Skill do skill end - defp validate_path!(path) do - segments = if is_binary(path), do: String.split(path, "/"), else: [""] - - if Enum.any?(segments, &(&1 in ["", ".", ".."] or String.contains?(&1, "\\"))) do - raise ArgumentError, - "invalid file path #{inspect(path)}: paths are relative to the skill's root, " <> - "separated by /, without empty, . or .. segments or backslashes" + defp validate_path!(path) when is_binary(path) do + if path |> String.split("/") |> Enum.any?(&invalid_segment?/1) do + raise_invalid_path!(path) end end + defp validate_path!(path), do: raise_invalid_path!(path) + + defp invalid_segment?(segment), + do: segment in ["", ".", ".."] or String.contains?(segment, "\\") + + defp raise_invalid_path!(path) do + raise ArgumentError, + "invalid file path #{inspect(path)}: paths are relative to the skill's root, " <> + "separated by /, without empty, . or .. segments or backslashes" + end + @doc """ Mark a skill as generated such that its files can't have stable digests. @@ -195,26 +202,6 @@ defmodule Phantom.Skill do end end - @doc false - # Cache hints for a result built from these skills. A listing is only public when - # every skill is and the session sees every skill. - def cache_hints(skills, session) do - hints = Enum.map(skills, & &1.cache) - - if hints == [] or nil in hints do - [ttl_ms: 0, scope: :private] - else - public? = - is_nil(session.allowed_resource_templates) and - Enum.all?(hints, &(&1[:scope] == :public)) - - [ - ttl_ms: hints |> Enum.map(& &1[:ttl_ms]) |> Enum.min(), - scope: if(public?, do: :public, else: :private) - ] - end - end - defp normalize_content(fun) when is_function(fun, 0), do: fun defp normalize_content(iodata), do: IO.iodata_to_binary(iodata) @@ -252,15 +239,12 @@ defmodule Phantom.Skill do skill_md_path = Enum.find([Path.join(dir, "SKILL.md.eex"), Path.join(dir, "SKILL.md")], &File.exists?/1) - {frontmatter, body} = embed_skill_md(skill_md_path) - - skill_md = - case body do - {:static, body} -> {"SKILL.md", {:static, frontmatter_block(frontmatter) <> body}, body} - eex -> {"SKILL.md", eex, nil} - end + {frontmatter, skill_md} = embed_skill_md(skill_md_path) - embedded = [skill_md | for(path <- files, path != skill_md_path, do: embed_file(path, dir))] + embedded = [ + {"SKILL.md", skill_md} + | for(path <- files, path != skill_md_path, do: embed_file(path, dir)) + ] embedded |> Enum.map(&elem(&1, 0)) @@ -270,12 +254,12 @@ defmodule Phantom.Skill do {path, _} -> raise ArgumentError, "#{dir}: more than one file is served at #{path}" end) - contents = for {path, content, body} <- embedded, do: {path, embed_content(content, body)} + contents = for {path, content} <- embedded, do: {path, embed_content(content)} quote do unquote_splicing(Enum.map(files, "e(do: @external_resource(unquote(&1))))) - def unquote(String.to_atom(String.replace(name, "-", "_")))(var!(assigns)) do + def unquote(function_name(name))(var!(assigns)) do _ = var!(assigns) Phantom.Skill.new(unquote(Macro.escape(frontmatter)), %{unquote_splicing(contents)}) @@ -283,7 +267,6 @@ defmodule Phantom.Skill do end end - # Returns the parsed frontmatter and the body: `{:eex, quoted}` or `{:static, binary}`. defp embed_skill_md(path) do source = File.read!(path) @@ -309,40 +292,32 @@ defmodule Phantom.Skill do "#{path}: frontmatter name #{inspect(frontmatter["name"])} must match its directory #{inspect(name)}" end - if eex?, - do: - {frontmatter, {:eex, EEx.compile_string(body, file: path, line: count_lines(header) + 1)}}, - else: {frontmatter, {:static, body}} + content = + if eex?, + do: {:eex, EEx.compile_string(body, file: path, line: count_lines(header) + 1)}, + else: {:static, body} + + {frontmatter, content} end - # EEx renders lazily, in a function that closes over the assigns. A static SKILL.md - # is stored without its frontmatter, which `new/2` adds when it's served. - defp embed_content({:eex, quoted}, _body), do: quote(do: fn -> unquote(quoted) end) - defp embed_content({:static, _served}, body) when is_binary(body), do: body - defp embed_content({:static, served}, nil), do: served + defp embed_content({:eex, quoted}), do: quote(do: fn -> unquote(quoted) end) + defp embed_content({:static, iodata}), do: iodata - # Returns `{served_path, content, skill_md_body}`. A nested skill's SKILL.md is also - # served by its own route, so it's written out the same way to serve the same bytes - # at the same URI. defp embed_file(path, dir) do relative = Path.relative_to(path, dir) served_path = if Path.extname(relative) == ".eex", do: Path.rootname(relative), else: relative cond do + # A nested SKILL.md keeps its frontmatter, because its own route serves the same bytes. Path.basename(served_path) == "SKILL.md" -> - content = - case embed_skill_md(path) do - {frontmatter, {:eex, quoted}} -> {:eex, [frontmatter_block(frontmatter), quoted]} - {frontmatter, {:static, body}} -> {:static, frontmatter_block(frontmatter) <> body} - end - - {served_path, content, nil} + {frontmatter, {kind, body}} = embed_skill_md(path) + {served_path, {kind, [frontmatter_block(frontmatter), body]}} Path.extname(relative) == ".eex" -> - {served_path, {:eex, EEx.compile_file(path)}, nil} + {served_path, {:eex, EEx.compile_file(path)}} true -> - {served_path, {:static, File.read!(path)}, nil} + {served_path, {:static, File.read!(path)}} end end @@ -381,20 +356,11 @@ defmodule Phantom.Skill do catch _kind, _reason -> {:error, "invalid YAML frontmatter"} else - [%{} = frontmatter] -> {:ok, normalize_yaml(frontmatter)} + [%{} = frontmatter] -> {:ok, stringify_keys(frontmatter)} _ -> {:error, "frontmatter must be a YAML mapping"} end end - # JSON objects have string keys, and YAML's null is nil. - defp normalize_yaml(:null), do: nil - - defp normalize_yaml(map) when is_map(map), - do: Map.new(map, fn {k, v} -> {to_string(normalize_yaml(k)), normalize_yaml(v)} end) - - defp normalize_yaml(list) when is_list(list), do: Enum.map(list, &normalize_yaml/1) - defp normalize_yaml(value), do: value - @doc """ The skill's files as served, with the frontmatter written ahead of `SKILL.md`. Renders every file. @@ -404,21 +370,22 @@ defmodule Phantom.Skill do Map.new(files, fn {path, _content} -> {path, render(skill, path)} end) end - defp render(%__MODULE__{frontmatter: frontmatter, files: files}, "SKILL.md"), + @doc false + def render(%__MODULE__{frontmatter: frontmatter, files: files}, "SKILL.md"), do: frontmatter_block(frontmatter) <> render_content(files["SKILL.md"]) - defp render(%__MODULE__{files: files}, path), do: render_content(files[path]) + def render(%__MODULE__{files: files}, path), do: render_content(files[path]) defp render_content(fun) when is_function(fun, 0), do: IO.iodata_to_binary(fun.()) defp render_content(binary), do: binary - # Each value is written as JSON, which YAML 1.2 parses back to the same value. + # YAML 1.2 parses each JSON value back to the same value. defp frontmatter_block(frontmatter) do {first, rest} = Map.split(frontmatter, ["name", "description"]) lines = Enum.map( - [{"name", first["name"]}, {"description", first["description"]}] ++ Enum.sort(rest), + [{"name", first["name"]}, {"description", first["description"]} | Enum.sort(rest)], fn {key, value} -> [yaml_key(key), ": ", JSON.encode!(value), "\n"] end ) @@ -449,114 +416,33 @@ defmodule Phantom.Skill do end end - # Only static files are counted towards the size limit; rendering the others - # here would defeat rendering them lazily. + # This check counts only static files, so that file functions stay lazy. defp validate_limits!(%__MODULE__{files: files}) do - if map_size(files) > @max_files do - raise ArgumentError, "a skill may have at most #{@max_files} files" - end + static_bytes = files |> Map.values() |> Enum.filter(&is_binary/1) |> Enum.map(&byte_size/1) - if files |> Map.values() |> Enum.filter(&is_binary/1) |> Enum.map(&byte_size/1) |> Enum.sum() > - @max_size do - raise ArgumentError, "a skill's files may total at most #{@max_size} bytes" + case check_limits(map_size(files), Enum.sum(static_bytes)) do + :ok -> :ok + {:error, reason} -> raise ArgumentError, "a skill has #{reason}" end end - defp stringify_keys(map) when is_map(map), - do: Map.new(map, fn {k, v} -> {to_string(k), stringify_keys(v)} end) - - defp stringify_keys(list) when is_list(list), do: Enum.map(list, &stringify_keys/1) - defp stringify_keys(value), do: value - @doc false - # The URI of a file or directory within the skill at `base_uri`. - def uri(base_uri, ""), do: base_uri + def check_limits(file_count, _bytes) when file_count > @max_files, + do: {:error, "more than #{@max_files} files"} - def uri(base_uri, path) do - base_uri <> "/" <> Enum.map_join(String.split(path, "/"), "/", &encode_segment/1) - end + def check_limits(_file_count, bytes) when bytes > @max_size, + do: {:error, "more than #{@max_size} bytes of files"} - @doc false - def encode_segment(segment), do: URI.encode(segment, &URI.char_unreserved?/1) - - defp digest(bytes), do: "sha256:" <> Base.encode16(:crypto.hash(:sha256, bytes), case: :lower) - - @doc false - # The manifest of a skill's files. `files` maps each path within the skill to the - # skill that serves it and the file's path within that skill. - def manifest(files, base_uri) do - resources = - for {path, {skill, own_path}} <- Enum.sort(files) do - bytes = render(skill, own_path) - - %{ - uri: uri(base_uri, path), - digest: digest(bytes), - size: byte_size(bytes) - } - end - - cond do - length(resources) > @max_files -> - {:error, "#{base_uri} has more than #{@max_files} files"} - - resources |> Enum.map(& &1.size) |> Enum.sum() > @max_size -> - {:error, "#{base_uri} has more than #{@max_size} bytes of files"} - - true -> - {:ok, resources} - end - end + def check_limits(_file_count, _bytes), do: :ok @doc false - # The `resources/read` contents of one of the skill's own files. - def read(%__MODULE__{files: files} = skill, path, uri) when is_map_key(files, path) do - bytes = render(skill, path) - content = %{uri: uri, mimeType: MIME.from_path(path)} - - if String.valid?(bytes), - do: {:ok, Map.put(content, :text, bytes)}, - else: {:ok, Map.put(content, :blob, Base.encode64(bytes))} - end + def function_name(skill_name), do: skill_name |> String.replace("-", "_") |> String.to_atom() - def read(%__MODULE__{}, _path, _uri), do: :error + defp stringify_keys(:null), do: nil - @doc false - # The `resources/directory/read` children of a directory, where "" is the skill's - # root. `files` is as in `manifest/2`. A skill's SKILL.md takes its name and - # description from its frontmatter. - def list_directory(files, base_uri, dir) do - prefix = if dir == "", do: "", else: dir <> "/" - - children = - for {path, served} <- files, String.starts_with?(path, prefix), uniq: true do - case path |> String.replace_prefix(prefix, "") |> String.split("/", parts: 2) do - [file] -> {file, child(file, served)} - [subdir, _] -> {subdir, %{name: subdir, mimeType: "inode/directory"}} - end - end - - if children == [] do - :error - else - dir_uri = uri(base_uri, dir) - - {:ok, - children - |> Enum.sort_by(&elem(&1, 0)) - |> Enum.map(fn {name, child} -> - Map.put(child, :uri, dir_uri <> "/" <> encode_segment(name)) - end)} - end - end - - defp child("SKILL.md", {%__MODULE__{frontmatter: frontmatter}, "SKILL.md"}) do - %{ - name: frontmatter["name"], - description: frontmatter["description"], - mimeType: "text/markdown" - } - end + defp stringify_keys(map) when is_map(map), + do: Map.new(map, fn {k, v} -> {to_string(stringify_keys(k)), stringify_keys(v)} end) - defp child(file, _served), do: %{name: file, mimeType: MIME.from_path(file)} + defp stringify_keys(list) when is_list(list), do: Enum.map(list, &stringify_keys/1) + defp stringify_keys(value), do: value end From f10288210db9ae7defa1a220197b07783d9364e1 Mon Sep 17 00:00:00 2001 From: David Bernheisel Date: Sun, 4 Oct 2026 20:53:50 -0400 Subject: [PATCH 4/4] Skills: let the router decide what skills/list lists Add the overridable list_skills/2 router callback. The default lists every skill route without path params. --- CHANGELOG.md | 1 + README.md | 6 +- lib/phantom/router.ex | 27 +++++++ lib/phantom/router/skills.ex | 51 +++++++----- lib/phantom/skill.ex | 17 +++- test/phantom/skill_list_test.exs | 131 +++++++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 23 deletions(-) create mode 100644 test/phantom/skill_list_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e9b34d..c1e8c93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ directories, rendering `.eex` files with assigns. Phantom serves `skills/list`, `skills/get`, `resources/read`, and `resources/directory/read`. Files render lazily, and caching is opt-in with `Phantom.Skill.with_cache/2`. + `c:Phantom.Router.list_skills/2` decides which skills `skills/list` returns. `Phantom.Cache.add_skill/2` adds a skill at runtime. The `skill://` scheme is reserved for skills. diff --git a/README.md b/README.md index ab11dd1..0f22b9b 100644 --- a/README.md +++ b/README.md @@ -576,8 +576,10 @@ needed. Caching is opt-in with `Phantom.Skill.with_cache/2`. Parsing `SKILL.md` frontmatter needs the optional `{:yamerl, "~> 0.10"}` dependency. Skill routes are resource templates named by their path: they share names and allow-lists with your resource templates, and are not listed in -`resources/templates/list`. To add skills at runtime, use -`Phantom.Cache.add_skill/2`. See `Phantom.Skill`. +`resources/templates/list`. Override `list_skills/2` in your router to decide +which skills `skills/list` returns; by default it lists every skill route without +path params. To add skills at runtime, use `Phantom.Cache.add_skill/2`. See +`Phantom.Skill`. ## Eliciting input diff --git a/lib/phantom/router.ex b/lib/phantom/router.ex index 691ef15..b9590b7 100644 --- a/lib/phantom/router.ex +++ b/lib/phantom/router.ex @@ -160,6 +160,28 @@ defmodule Phantom.Router do | {:noreply, Session.t()} | {:error, any(), Session.t()} + @doc """ + List the skills that `skills/list` returns, as `SKILL.md` URIs. + + Return `Phantom.Skill.list/2` with the URIs of one page and the cursor of the next + page. The cursor is opaque to Phantom. Phantom builds each entry as `skills/get` does, + and leaves out a URI that serves no skill to the session. + + The default implementation lists every skill route without path params, by path. + + def list_skills(cursor, session) do + {studies, next_cursor} = MyApp.Studies.page(session.assigns.user, cursor) + uris = Enum.map(studies, &"skill://studies/\#{&1.id}/study-review/SKILL.md") + + {:reply, Phantom.Skill.list(["skill://git-workflow/SKILL.md" | uris], next_cursor), + session} + end + """ + @callback list_skills(String.t() | nil, Session.t()) :: + {:reply, %{required(:skills) => [String.t()], optional(:nextCursor) => String.t()}, + Session.t()} + | {:error, any(), Session.t()} + @doc """ Authorize subscriptions and update notifications for resolved resources. @@ -327,6 +349,10 @@ defmodule Phantom.Router do {:error, Request.not_found(), session} end + def list_skills(cursor, session) do + Phantom.Router.Skills.default_list(__MODULE__, session, cursor) + end + @doc """ Return the Resource URI for the resource by name and params. @@ -718,6 +744,7 @@ defmodule Phantom.Router do @doc false defoverridable list_resources: 2, + list_skills: 2, authorize_resource_subscriptions: 2, authorize_task_subscriptions: 2, server_info: 1, diff --git a/lib/phantom/router/skills.ex b/lib/phantom/router/skills.ex index e9f7098..6878530 100644 --- a/lib/phantom/router/skills.ex +++ b/lib/phantom/router/skills.ex @@ -19,6 +19,8 @@ defmodule Phantom.Router.Skills do defp param?(":" <> _), do: true defp param?(_segment), do: false + defp static?(route), do: not Enum.any?(segments(route), ¶m?/1) + # A nested route cannot have path params, because its files are in the manifest of its parent. def validate_nesting!(routes) do for outer <- routes, inner <- routes, outer != inner do @@ -43,10 +45,6 @@ defmodule Phantom.Router.Skills do Enum.filter(Cache.list(nil, router, :resource_templates), &(&1.scheme == "skill")) end - defp listable?(route) do - route.scheme == "skill" and not Enum.any?(segments(route), ¶m?/1) - end - defp concrete(route, params) do Enum.map(segments(route), fn ":" <> param -> params[param] @@ -235,39 +233,54 @@ defmodule Phantom.Router.Skills do end end - def list(router, session, cursor) do - routes = session |> Cache.list(router, :resource_templates) |> Enum.filter(&listable?/1) + def default_list(router, session, cursor) do + routes = + session + |> Cache.list(router, :resource_templates) + |> Enum.filter(&(&1.scheme == "skill" and static?(&1))) - case Phantom.Router.paginate(routes, cursor, & &1) do - {:ok, page, next_cursor} -> - all_routes = skill_routes(router) - results = Enum.map(page, &list_entry(router, all_routes, &1, session)) + case Phantom.Router.paginate(routes, cursor, &(base_uri(segments(&1)) <> "/SKILL.md")) do + {:ok, uris, next_cursor} -> + {:reply, Skill.list(uris, next_cursor[:nextCursor]), session} + + {:error, error} -> + {:error, error, session} + end + end + + def list(router, session, cursor) do + case router.list_skills(cursor, session) do + {:reply, %{skills: uris} = page, session} -> + routes = skill_routes(router) + results = Enum.map(uris, &list_entry(router, routes, &1, session)) listed = for {:ok, entry, skills} <- results, do: {entry, skills} - # The listing is private when a route serves no skill to this session. + # The listing is private when a URI serves no skill to this session. hints = - if length(listed) == length(page), + if length(listed) == length(uris), do: cache_hints(Enum.flat_map(listed, &elem(&1, 1)), session), else: [ttl_ms: 0, scope: :private] {:reply, %{skills: Enum.map(listed, &elem(&1, 0))} - |> Map.merge(next_cursor || %{}) + |> Map.merge(Map.take(page, [:nextCursor])) |> Request.with_cache(hints), session} - {:error, error} -> - {:error, error, session} + {:error, _error, %Session{}} = error -> + error end end - defp list_entry(router, routes, route, session) do - with {:ok, skill, session} <- call(route, %{}, session) do - entry(router, routes, skill, segments(route), session) + defp list_entry(router, routes, uri, session) do + with {:ok, route, params, segments, "SKILL.md"} <- resolve(router, uri), + true <- accessible?(router, session, route, segments), + {:ok, skill, session} <- call(route, params, session) do + entry(router, routes, skill, segments, session) end catch kind, reason -> Logger.error( - "Skill #{inspect(route.name)} was left out of skills/list: " <> + "Skill #{uri} was left out of skills/list: " <> Exception.format(kind, reason, __STACKTRACE__) ) diff --git a/lib/phantom/skill.ex b/lib/phantom/skill.ex index 7ceb62d..065c34a 100644 --- a/lib/phantom/skill.ex +++ b/lib/phantom/skill.ex @@ -19,8 +19,9 @@ defmodule Phantom.Skill do The final segment of the path is the skill's name, and must equal the `name` in its frontmatter. Earlier segments organize skills and may contain path params, except the - first. Skills with path params are left out of `skills/list`, but clients can still - get them by URI. + first. The router's `c:Phantom.Router.list_skills/2` decides which skills + `skills/list` returns; by default, every route without path params. Clients can get + any served skill by URI. Each skill is a resource template named by its path, so skill paths share names with resource templates, and `Phantom.Session.allow_resource_templates/2` limits skills too. @@ -184,6 +185,18 @@ defmodule Phantom.Skill do @spec dynamic(t()) :: t() def dynamic(%__MODULE__{} = skill), do: %{skill | dynamic: true} + @doc """ + The result of `c:Phantom.Router.list_skills/2`: the `SKILL.md` URIs to list, and + the cursor of the next page, or `nil` on the last page. + """ + @spec list([String.t()], String.t() | nil) :: %{ + required(:skills) => [String.t()], + optional(:nextCursor) => String.t() + } + def list(uris, next_cursor) do + Phantom.Utils.remove_nils(%{skills: List.wrap(uris), nextCursor: next_cursor}) + end + @doc """ Let clients cache the skill's `skills/list` and `skills/get` entries. diff --git a/test/phantom/skill_list_test.exs b/test/phantom/skill_list_test.exs new file mode 100644 index 0000000..99c16f5 --- /dev/null +++ b/test/phantom/skill_list_test.exs @@ -0,0 +1,131 @@ +defmodule Phantom.SkillListTest do + use ExUnit.Case, async: true + + alias Phantom.Cache + alias Phantom.Skill + + defmodule Skills do + alias Phantom.Skill + + def git_workflow(_params, session) do + {:reply, + Skill.new(%{name: "git-workflow", description: "Git"}, %{"SKILL.md" => "# Git\n"}) + |> Skill.with_cache(ttl_ms: 300_000, scope: :public), session} + end + + def study_review(%{"study_id" => "404"}, session), do: {:reply, nil, session} + + def study_review(%{"study_id" => id}, session) do + {:reply, + Skill.new(%{name: "study-review", description: "Review a study"}, %{ + "SKILL.md" => "# Review study #{id}\n" + }) + |> Skill.with_cache(ttl_ms: 60_000, scope: :public), session} + end + end + + defmodule CustomRouter do + use Phantom.Router, name: "CustomList", vsn: "1.0" + + skill "git-workflow", Skills + skill "studies/:study_id/study-review", Skills, :study_review + + def list_skills("fail", session), do: {:error, Phantom.Request.internal_error(), session} + + def list_skills("studies", session) do + {:reply, Skill.list(["skill://studies/42/study-review/SKILL.md"], "more"), session} + end + + def list_skills("unresolved", session) do + uris = [ + "skill://git-workflow/SKILL.md", + "skill://studies/404/study-review/SKILL.md", + "skill://nope/SKILL.md", + "skill://git-workflow/README.md" + ] + + {:reply, Skill.list(uris, nil), session} + end + + def list_skills(_cursor, session) do + uris = ["skill://git-workflow/SKILL.md", "skill://studies/42/study-review/SKILL.md"] + {:reply, Skill.list(uris, nil), session} + end + end + + defmodule DefaultRouter do + use Phantom.Router, name: "DefaultList", vsn: "1.0" + + skill "git-workflow", Skills + skill "studies/:study_id/study-review", Skills, :study_review + end + + setup do + Cache.register(CustomRouter) + Cache.register(DefaultRouter) + :ok + end + + defp list(router, cursor, opts \\ []) do + session = + Phantom.Test.build_session(router, + allowed_resource_templates: opts[:allowed_resource_templates] + ) + + params = if cursor, do: %{"cursor" => cursor}, else: %{} + request = Phantom.Test.build_request("skills/list", params: params) + router.dispatch_method("skills/list", params, request, %{session | request: request}) + end + + defp uris(%{skills: skills}), do: Enum.map(skills, & &1.uri) + + test "Skill.list/2 omits nextCursor when there is no next page" do + assert Skill.list(["skill://a/SKILL.md"], nil) == %{skills: ["skill://a/SKILL.md"]} + + assert Skill.list(["skill://a/SKILL.md"], "next") == %{ + skills: ["skill://a/SKILL.md"], + nextCursor: "next" + } + end + + test "the default lists every skill route without path params" do + assert {:reply, result, _} = list(DefaultRouter, nil) + assert uris(result) == ["skill://git-workflow/SKILL.md"] + assert %{ttlMs: 300_000, cacheScope: "public"} = result + end + + test "the router can list a skill with path params" do + assert {:reply, result, _} = list(CustomRouter, nil) + + assert uris(result) == [ + "skill://git-workflow/SKILL.md", + "skill://studies/42/study-review/SKILL.md" + ] + + assert %{ttlMs: 60_000, cacheScope: "public"} = result + end + + test "the router can leave out a static skill and pass its cursor through" do + assert {:reply, %{nextCursor: "more"} = result, _} = list(CustomRouter, "studies") + assert uris(result) == ["skill://studies/42/study-review/SKILL.md"] + end + + test "a URI that serves no skill is left out and makes the listing private" do + assert {:reply, result, _} = list(CustomRouter, "unresolved") + assert uris(result) == ["skill://git-workflow/SKILL.md"] + assert %{ttlMs: 0, cacheScope: "private"} = result + refute Map.has_key?(result, :nextCursor) + end + + test "a URI the session may not use is left out and makes the listing private" do + assert {:reply, result, _} = + list(CustomRouter, nil, allowed_resource_templates: ["git-workflow"]) + + assert uris(result) == ["skill://git-workflow/SKILL.md"] + assert %{ttlMs: 0, cacheScope: "private"} = result + end + + test "an error from the router is returned" do + assert {:error, %{code: -32603}, _} = list(CustomRouter, "fail") + end +end