Repository navigation
agent: MCP Skills extension (SEP-2640) client support - #131
Merged
Merged
Conversation
jaredLunde
added a commit
that referenced
this pull request
Oct 6, 2026
…agnostics Closes the gaps PR #131 listed against SEP-2640: - Approval (spec MUST, no implicit local execution). Activating a model-chosen MCP skill needs the user's approval, bound to the entry's manifest fingerprint and asked before SKILL.md is fetched; a `/skill:` the user typed is that consent. A nested skill's approval is its own and the question names the enclosing skill. While a session is acting on an MCP skill, every `bash`/`execute` call needs approval too, in serve, run and subagent hook paths. serve asks through its approval gate (now always present) as `approval_request` frames with an `mcp_skill` object; run has no one to ask and denies; `--approve-mcp-skills` approves in advance. - Loaded-skill state is per session (`SkillSession` on `McpEnabledSet`), not per connection: a daemon's sessions share connections, so each session's registry rebinds the loading tools to its own state, reset on a session switch and shared with its subagents. - Listings refresh when needed and due: `ttlMs` expiry (absent/0 = stale) or `notifications/resources/list_changed`; held entries past `ttlMs` are re-fetched before a load; `cacheScope: "private"` listings are never written to the on-disk manifest. - Subagents get their parent session's MCP skills listing. - The 512-file / 16 MiB per-skill limits are enforced from the entry. - Declined/invalid entries, failed listings, same-name entries and on-disk name clashes are surfaced via get_commands' `collisions` and run's warnings, and survive a boot from the manifest cache. New e2e suites: mcp_skills_approval, mcp_skills_sessions, mcp_skills_listing (shared harness in tests/common/skills_env.rs). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
Contributor
Author
|
CI status for 8e89010: build, clippy, fmt, all mutants shards, core-providers, exec-live, gateway-unit, gateway-e2e and gateway stress pass. The three agent test shards (agent-lib, agent-rest, agent-code-mode) have not completed in CI: they wedged and were cancelled at 30:00 twice on this commit (22:00 and 22:31 UTC), as they did on fcd72c2. In the same 22:16–22:54 window the agent shards of the Apps, Events and Tasks PRs were cancelled the same way, so this looks environmental, but none of this PR's e2e tests have completed in CI yet. Locally on 8e89010: |
jaredLunde
force-pushed
the
jared/mcp-skills
branch
from
October 7, 2026 01:12
80de26f to
d3a829c
Compare
Client side of `io.modelcontextprotocol/skills`: a declaring server's `skills/list` entries become `<server>:<name>` skills in a separate `<available_skills origin="mcp">` block, loaded (size, digest and frontmatter verified, never prefetched) through `mcp__<server>__skill__read`, invocable as `/skill:<server>:<name>`, listed by `get_commands`, re-listed on `ttlMs` expiry or list-changed, and cached in the manifest. Activation of a model-chosen skill and code execution while one is active need the user's approval (`run` denies unless `--approve-mcp-skills`); state is per session and restored only from host-authored transcript records. Rebased onto MCP Events (#133): `tools/mcp_stdio.rs` is the one stdio transport, and its `rescue`/`unwrap_rescued` is the one workaround for rmcp's untagged-result bug (rust-sdk#1197), for events and skills results alike. `tools/mcp_wire.rs` keeps only the streamable-HTTP client, which applies the same `rescue` to `skills/*` JSON bodies and SSE events. Rebased onto MCP Tasks (#130) too: every skills request (skills/list, skills/get, resources/read) goes through `mcp::request_tracked` (`track_call` under the calling session's host), so a nested request is attributed or refused, never sent to a shared connection's own host. A steered `/skill:` expands as its session (its command loop can answer); a re-list or expansion between runs runs under a client-less host, so a nested request is declined at once instead of stalling the prompt. Rebased onto MCP Apps (#132): the skills resource hiding and the Apps `ui://` filter both run in `tools_from_client`; one resource-list-changed handler clears the Apps views and invalidates the skills listing; and the skills invalidation flag rides the dial into `McpHandler::for_dial`. Round-3 fixes: a steer waiting behind a `/skill:` when its run is cancelled is acked as not queued (the cancellation already dropped the run's lanes), and an expansion that finished but was never queued activates nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
jaredLunde
force-pushed
the
jared/mcp-skills
branch
from
October 7, 2026 01:31
d3a829c to
f514ebf
Compare
This was referenced Oct 7, 2026
jaredLunde
added a commit
that referenced
this pull request
Oct 7, 2026
* agent: MCP skills follow-ups, and the serve suite back in CI MCP skills (SEP-2640) gaps from #131: - A subagent's skill-load question names that subagent (`origin`), the same provenance its code-execution questions carry: its registry binds the loading tools in its own name (`filter_by_enabled_as`). - A listing re-fetched after connect is written back to the on-disk manifest cache at once (`mcp_manifest::store_skills`), or forgotten if it is now `cacheScope: private`. - Remembered approvals persist with the session (`mcp_skill_approval` custom entries, journaled at run end) and are restored before each prompt. Keys embed the content fingerprint, so an unchanged skill is not asked about again after a restart and a changed one is. The serve test suite: - `serve_get_tree_since_works_from_the_busy_loop_mid_prompt` hung on main too, deterministically: the session-title request serve makes after a first run took a reply from the in-order mock model's script, shifting every later turn. The same cause failed or hung 48 serve tests, unseen because CI has excluded `binary(~serve)` since #39. The scripted mock servers now answer title requests themselves, off the script and out of the record; title tests match `SESSION_TITLE_MARKER` on a routed server. - Two real bugs it had been hiding: session listings tied on `updated_at` (one-second resolution) had no stable order, so `offset` paging could repeat a session (tie broken by id); and since #131 an `approve` in a session without `--approve` was acknowledged even when it matched no question (refused again, pointing at `--approve`). - CI runs the serve tests again, as an `agent-serve` shard. Lib tests that assert "no enclosing repo / context file" now use a temp root with no project above it (`test_support::isolated_tempdir`), so they pass with TMPDIR inside a checkout or under a CLAUDE.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk * agent: approvals never persisted, host custom kinds reserved, race-free test ports Audit of 237d9a5: - F1: persisted MCP-skill approvals were forgeable. A model with write/edit tools can append an `mcp_skill_approval` entry to the user's session file (it knows the fingerprint from the loaded tag), and a client could `append_custom` one; an `execute` approval would then run unasked after a restart. Approvals are no longer persisted or restored: session-lifetime, in memory. A client's `append_custom` of a kind the agent writes itself (`mcp_task`, `mcp_task_result`, `mcp_skill_approval`: `session_store::HOST_CUSTOM_KINDS`) is refused. - F4: a typed `/skill:` is consent for that invocation only; it no longer approves the model's later loads of the skill. - F3: same-second session listings tie-break by id descending, so the newest (generated ids lead with their creation time) comes first. - F5: manifest-cache writes are read-modify-writes of one shared file; they now run under a lockfile with a per-writer temporary name. - F6: `serve_state_reporting` reads stdout with a deadline instead of hanging on a stall, and the scripted servers count title calls, so a change in title frequency shows (one per session, pinned). Test ports: `free_port()` released the port it picked and hoped the child bound it first; under parallel suites another process could take it. `serve` children now bind `--listen 127.0.0.1:0` and the test reads the port from serve's announcement (`spawn_listening`); a daemon whose own arguments name its port (an MCP Events callback URL) is handed a listener the test bound, by socket activation (`HeldPort`), kept across a restart; a "nothing listens here" port is held bound, never listening (`DeadPort`). `free_port` remains only for the gateway and nats-server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
jaredLunde
added a commit
that referenced
this pull request
Oct 7, 2026
…title-proof test mock - run journals the MCP tasks it starts (the same mcp_task entries serve writes) and `run --continue` resumes the session's pending tasks before its turn, journaling each result and splicing it into the turn it sends. - A session file's mode is tightened to 0600 by the next append if it was looser (create already set 0600; an existing file kept its mode). Task journal entries are confirmed sealed with the tenant key in service storage and read back intact. - The in-order test mock answers serve's session-title call outside its script and does not record it: the title call raced the next prompt and took its scripted reply under load (serve_session_tree, serve_compaction_ retry, serve_stop_after_turn and others). The title tests route titles. - The sessionId ownership rule now has an e2e test that fails without it (a session file copied under a new id carries the journal; no fork does). - serve_approval: a session without --approve has had a gate since #131 (MCP skills ask through it); the stale "no gate" expectation is updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This PR adds client-side support for the MCP Skills extension (SEP-2640,
io.modelcontextprotocol/skills, Final) tocrates/agent.When a connected server declares the extension, its skills now work like this:
serve'sget_commands./skill:<server>:<name>.set_mcp_enabledgates them.Servers that don't declare the extension behave exactly as before.
The code lives mostly in
tools/mcp_skills.rs. It is rebased onto MCP Events (#133), MCP Tasks (#130) and MCP Apps (#132): stdio servers go through their one stdio transport (tools/mcp_stdio.rs), the rmcp_metabug has one workaround (mcp_stdio::rescue), and nested requests are attributed withtrack_call.tools/mcp_wire.rskeeps only the streamable-HTTP client that applies that same rescue toskills/*responses.Design
Server state vs. session state.
ServerSkillsexists once per connection. It holds what the server published: the listing, and every entry learned since.SkillSessionexists once per session, onMcpEnabledSet. It holds what that session loaded, what its user approved, and who to ask.SkillSession. Subagents share their parent's.<skill name server location manifest>tag. As long as that tag is in context, the session counts as acting on that skill, whether it was resumed, switched to, forked or reopened after a restart. The spec allows the window to run longer than the skill's time in context, never shorter.mcp__<server>__skill__readcall bytool_use_id, or a user turn that is itself a/skill:expansion.Approval (the spec's "no implicit local execution").
SKILL.mdis fetched, and the answer is bound to the manifest fingerprint (full SHA-256 over every{uri, digest}), so a changed skill is asked about again./skill:the user types counts as consent.bash/executecall needs approval, asked per call and naming the active skills.execute), and only under that exact set of manifests.servesendsapproval_requestframes with anmcp_skillobject, through its existing approval gate (now always present).runhas no one to ask, so it denies.--approve-mcp-skills(on bothrunandserve) approves everything in advance.ServeHooks,RunHooksandChildHooks.allowed-toolsis not honoured for any skill.Integrity.
SKILL.mda field-by-field frontmatter comparison.1equals1.0, YAML 1.1'syesequalstrue, and the null spellings are equal. This deliberately loosens the spec's "identical in content" from one JSON rendering to the values. The fields the host acts on (disable-model-invocation) are read with the same normalization, so anything that verifies astrueis treated astrue.skills/get; after that the read is refused.No early fetch. Discovery reads only
skills/list.Freshness.
ttlMsran out, or the server sentnotifications/resources/list_changed. SEP-2640 has no skills-specific notification;notifications/skills/list_changedis accepted as well.cacheScope: "private"listings are never cached on disk.Limits.
skill://, and any.../SKILL.mdunder any scheme together with everything under its directory.Diagnostics. These appear in
get_commands'collisionsand asrunwarnings, and survive a boot from the cache:Names. Skills are named
<server>:<name>. Two same-named entries on one server become<server>:<skill-path>. They never shadow local skills.--no-skillssuppresses MCP skills too./skill:during a run. Steers and follow-ups received during a run go throughPendingSteers:/skill:one./skill:expansion runs on its own task, so the busy loop keeps polling the run and readingabort/approve.new_session/switch_session/fork/clone), any expansion still in flight is cancelled and awaited, and acked as not queued with the reason.The rmcp bug: one workaround, shared with Events
The bug. rmcp 3.2–3.5.1 decodes a result that carries
_meta(which every2026-07-28result does) as an emptyCallToolResult, so skills results disappear (rust-sdk#1197, open). The Python SDK's reference server exposed it.The workaround is
mcp_stdio::rescue/unwrap_rescued, merged with #133. A lossy result is wrapped as{"x-beyond-raw-result": …}before rmcp parses it, and each custom-request caller unwraps it. This PR's own stdio transport and its_metastripping are gone.tools/mcp_stdio.rs, unchanged. It applies the rescue to every line, skips non-UTF-8 lines, and keeps the exit grace and process-group sweep.mcp_wire::HttpClientanswersskills/*POSTs itself and applies the samerescue. It builds requests and handles statuses the way rmcp does:AuthRequired, 403 toInsufficientScope, and 404 with a session toSessionExpired;\n,\r\nand\ralike.Nested-request attribution (rebased onto #130)
Every skills request (
skills/list,skills/get,resources/read) now goes throughmcp::request_tracked. It registers the request withtrack_callunder the calling session's host for as long as the request is outstanding. A nested request the server raises meanwhile is attributed to that session or refused. It is never sent to a shared connection's own host./skill:steered into a running prompt expands as its session (McpSkills::in_run). That session's command loop is live, so it can answer.McpSkills::outside_run), so a nested request is declined at once. Nothing could answer it:serve's command loop is waiting on that very read, so routing the request to the session only stalled the prompt until the 30s expansion timeout. A first version did exactly that, and the new test caught it.Composition with MCP Apps (rebased onto #132)
tools_from_clientdrops skill resources from the generic resource tools, and Apps'ui://filter runs in the same function. Both stay.mcp_skills::run_lists_mcp_skills_without_fetching_any_skill_filenow also serves aui://resource and asserts it is not a resource tool either.on_resource_list_changedhandler clears the Apps views and also invalidates the skills listing.DialintoMcpHandler::for_dial, next toapps.reset_session(enablement and skill state) runs alongside the Apps close.Audit findings: all fixed, each with its proving test
Every test below fails without its fix. Each fix was reverted in turn by a scripted mutation run, and all 36 mutations were killed:
mcp_skills_resume:a_resumed_run_is_still_gated,switching_back_to_a_session_that_loaded_a_skill_restores_the_gate,a_fork_of_a_session_that_loaded_a_skill_is_gated(clone + fork),a_restarted_serve_reopening_the_session_is_gated/skill:steer freezes the busy loopmcp_skills_steer::a_skill_steered_mid_run_does_not_stall_the_command_loop(slow fixture read,get_stateanswered in under 3s)mcp_stdiotransport, which skips bad linesmcp_skills_integrity::a_stdio_server_writing_non_utf8_lines_keeps_working(bad line before the handshake and before every reply), against main's transportmcp_wire::an_sse_response_is_streamed_rescued_and_does_not_wait_for_the_server_to_close,an_oversized_sse_event_ends_the_stream_with_an_errormcp_wire::statuses_map_to_the_errors_rmcp_acts_on,a_reserved_custom_header_is_refusedexecutekeyed astool:executemcp_skills::execute_is_gated_and_remembered_per_program_not_per_tool(also kills M12b)skill://files hidden; per-call cross-origin gatemcp_skills::run_lists_mcp_skills_without_fetching_any_skill_file;mcp_skills_approval:run_denies_a_cross_origin_read_while_acting_on_another_servers_skill,serve_asks_per_call_before_a_cross_origin_readmcp_skills_integrity::tampered_frontmatter_is_refused_on_the_real_load_pathServewait intests/common/skills_env.rs(exercised by the mutation run: regressions fail, not hang)mcp_skills_integrity::an_honest_skill_is_not_refused_for_yaml_versus_json_representation,mcp_skills::frontmatter_equality_is_of_yaml_values_not_their_renderingmcp_skills_integrity:a_failed_re_list_keeps_the_last_good_listing,a_re_list_cut_short_keeps_the_notice_pending-32602mcp_skills_integrity::an_unlisted_nested_skill_inside_a_loaded_one_can_be_activated_by_uri--no-skillsmcp_skills_integrity::no_skills_suppresses_mcp_skills_toomcp_skills_integrity::a_listing_that_never_ends_is_cut_off_and_said_somcp_skills::a_content_block_labelled_as_another_resource_is_refusedmcp_skills::the_fingerprint_is_a_full_sha256handle_approve's no-gate branch removed (the gate always exists)manifest_filesin the questionmcp_skills_approval::serve_asks_to_activate_a_skill_and_to_run_code_while_it_is_activemcp_skills_integrity::a_wrong_size_is_refused_even_when_the_digest_matchesmcp_skills_integrity::a_file_the_loaded_skills_manifest_does_not_list_is_never_readmcp_skills_approval::a_skill_whose_manifest_changed_is_asked_about_againmcp_skills_integrity::a_held_entry_that_goes_stale_is_refreshed_and_the_current_body_served/skill:steer leaked across abort and sessions, acked after its run endedmcp_skills_steer:a_skill_steer_from_an_aborted_run_never_reaches_the_next_session(the audit's steer_leak sequence: no body, and no active skill gatingbash),a_skill_steer_from_an_aborted_run_never_reaches_the_next_prompt,a_skill_steered_mid_run_does_not_stall_the_command_loop(late ack issuccess: falsewith the reason)mcp_skills_steer::a_plain_steer_does_not_overtake_an_earlier_skill_steer(the audit's steer_order sequence)disable-model-invocation: yesmcp_skills_integrity::a_yaml_1_1_disable_model_invocation_is_honouredmcp_skills_integrity::a_forged_skill_tag_in_other_content_activates_nothing_on_resumeSKILL.mdunder another scheme stayed a generic resource.../SKILL.mdand its directory hidden; loads go through the skill toolmcp_skills_integrity::an_unlisted_skill_md_under_another_scheme_is_not_a_generic_resource\n\nonly within one chunk; JSON body uncapped\n/\r\n/\r); capped bodymcp_wire:event_boundaries_are_found_across_chunks_and_line_endings,a_small_events_stream_split_with_crlf_is_not_mistaken_for_an_oversized_one,a_json_body_over_the_cap_is_refused/skill:survived into the next promptmcp_skills_steer::a_plain_steer_behind_a_skill_steer_dies_with_the_aborted_runmcp_skills_steer::a_finished_expansion_that_was_never_queued_does_not_activate_its_skill(a fast skill done behind a slow one at abort; the nextbashruns ungated)request_tracked+in_run/outside_runmcp_skills_steer:a_nested_request_during_a_steered_skill_expansion_reaches_the_session,a_nested_request_during_an_expansion_between_runs_is_refused_at_oncemcp_stdio::rescuefor stdio and skills HTTPmcp_wire::rmcp_still_shadows_a_skills_result_that_carries_meta, the SSE test above, the Python SDK runs belowProof
The end-to-end tests drive the real
run/servebinaries against a real subprocess fixture,src/bin/mcp_skills_fixture_server.rs(stdio or--http,_metaon every result, request log). Every child is started withspawn_guarded.Local results (f514ebf, rebased on 7b7e9ef):
tests/mcp_*.rstargets: 1730 passed, 0 failed. That covers skills, apps, tasks, events andmcp_stdio_transport.--all-targets --features code-mode -D warnings),cargo fmt --checkanddprint check: clean.CI (f514ebf): green on the first attempt: all 20 jobs passed (run 37557496219), including the three agent test shards: agent-lib, agent-rest and agent-code-mode, which finished in under 5 minutes each. This is the first CI run on this branch in which the e2e tests completed.
Independent checks (run locally, not in CI)
c37eec8, the SEP-2640 client scenarios): 4/4 passed. The driver usesrun --approve-mcp-skills; without the flag, a load is correctly denied before any read.no-prefetchverify-digestverify-sizeverify-frontmatterRemaining limits
origin: "main". The skill tool has no per-call origin. Code-execution and cross-origin questions do name the subagent.🤖 Generated with Claude Code
https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk