Repository navigation
feat(mcp): serve distributed skills over MCP (SEP-2640) - #2022
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
tonydzi
left a comment
There was a problem hiding this comment.
Two things from running this branch (head 819a804) locally: Python 3.12, pip install -e ., FastMCP 4.1.3. I did not run your full suite, only the probes below. The Visibility move to local_provider checks out: with BASIC_MEMORY_ENABLE_PROJECT_DELETE unset bm_project_delete is hidden, with =true it is listed, same as on main, and Client(mcp) lists all 19 shipped skills as skill:// resources.
1. skills.py runs distribution("basic-memory").locate_file(...) at import, and it sits in resources/__init__.py's import chain, so it can take down unrelated resources. resources/__init__.py now imports skills_provider, so from basic_memory.mcp.resources import note_resource executes distributed_skills_root(). I patched importlib.metadata.distribution to raise PackageNotFoundError for basic-memory (what a PYTHONPATH=src run, a vendored copy or a frozen build sees) and imported note_resource. On this branch that raises PackageNotFoundError: No package metadata was found for basic-memory; on main the same import succeeds. The comment above the RuntimeError says a missing skills dir should make "the server fail at import", but PackageNotFoundError is not caught, so the failure is wider than that and has a different message. It also takes out manual_index and project_info, which have nothing to do with skills. Possible fixes: catch PackageNotFoundError too and fall back to Path(__file__).parents[2] / "data" / "skills", or build the provider lazily in lifespan and skip registration with a warning when no skills dir exists.
2. In an editable install the served skills are a snapshot taken at install time, and the new tests cannot see drift. After pip install -e ., site-packages/basic_memory/data/skills/memory-notes/SKILL.md is a regular file, not a link. I appended a line to skills/memory-notes/SKILL.md in the checkout and read the installed copy back: the line was not there. So for a contributor editing skills/ and checking the result through skills/get or skill://, the server keeps serving the old text until the next reinstall. The docstring's "resolves to a real directory in every supported install" holds, but it doesn't say that directory can be stale.
test_skills_list_offers_every_shipped_skill and test_skill_get_returns_files_matching_the_shipped_bytes compare the served result with distributed_skills_root(), which is the directory being served. They would pass whatever it contains. A cheap guard: when the repo's skills/ is present, assert that each file under distributed_skills_root() has the same bytes as its skills/<name>/... source, so a stale editable install fails loudly. Alternatively say in skills/DEVELOPMENT.md that skill edits need just install (or whatever your reinstall target is) before they show up over MCP.
Sanity-checked, not a problem: the partial force-includes (memory-quest ships only SKILL.md, memory-onboarding skips evals/evals.json) leave out files that no shipped SKILL.md links to, so no served skill points at a missing file.
…an test - Each xdist worker now drops and recreates its database once per session. Reusing a local server across runs left a schema from an interrupted run in a worker database (CREATE SCHEMA failed in test_postgres_physical_probe_uses_active_search_path). CI's service is fresh per job, but a session should never depend on that. - test_conflict_check_is_one_indexed_query runs ANALYZE entity before EXPLAIN. With no statistics on the just-filled table, the planner chose among the project indexes arbitrarily, so the per-key index assertions flaked (also seen on #2022). 5/5 passes on Postgres after the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMYVL4TcbWdXhdvAzxfqjo Signed-off-by: phernandez <paul@basicmachines.co>
…an test - Each xdist worker now drops and recreates its database once per session. Reusing a local server across runs left a schema from an interrupted run in a worker database (CREATE SCHEMA failed in test_postgres_physical_probe_uses_active_search_path). CI's service is fresh per job, but a session should never depend on that. - test_conflict_check_is_one_indexed_query runs ANALYZE entity before EXPLAIN. With no statistics on the just-filled table, the planner chose among the project indexes arbitrarily, so the per-key index assertions flaked (also seen on #2022). 5/5 passes on Postgres after the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMYVL4TcbWdXhdvAzxfqjo Signed-off-by: phernandez <paul@basicmachines.co>
Serve the skills the wheel ships under basic_memory/data/skills as skill://<name>/SKILL.md resources, and register FastMCP's SkillsExtension so clients that negotiate io.modelcontextprotocol/skills can call skills/list and skills/get. Clients without the extension can still read the skills as ordinary resources. The extension refuses servers with server-level transforms, so the bm_project_delete Visibility mark moves to the local provider, which owns the tools. Behavior is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMYVL4TcbWdXhdvAzxfqjo Signed-off-by: phernandez <paul@basicmachines.co>
819a804 to
c1a9436
Compare
Importing any resource ran the distribution lookup for the skills, so a run without package metadata lost every resource. The server entrypoint now imports the skills module itself, and a missing distribution fails with the same message as a missing skills directory. An editable install serves a copy of skills/ taken at install time. A test compares the served bytes with skills/, and DEVELOPMENT.md names the reinstall command. Reported in review by tonydzi. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMYVL4TcbWdXhdvAzxfqjo Signed-off-by: phernandez <paul@basicmachines.co>
|
Thanks @tonydzi, both points were right. Fixed in cf7ed10. 1. Import chain. I didn't add the 2. Stale editable installs. |
Main's `test.yml` fails on the Windows SQLite unit shards (runs 41866893188, 41853960947, 41795319355), so Coverage is skipped. - `test_cli_exit.py`, `test_fresh_install.py`, `test_chatgpt_graphs.py`: `text=True` decodes the child's UTF-8 output with cp1252 on Windows. The reader thread raises `UnicodeDecodeError` and `stdout` is `None`. Pass `encoding="utf-8"`. - `test_skill_resources.py` (from #2022): the expected text came from `read_text()`, which turns a CRLF checkout into LF. The served text keeps CRLF. Compare to the decoded bytes. - `test_chatgpt_import_visits_each_mapping_key_once`: the 15s hang guard timed out during startup on a loaded Windows runner. Raised to 60s. Test-only change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01CMYVL4TcbWdXhdvAzxfqjo Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Serves the Basic Memory skills over MCP, using FastMCP's Skills extension support (SEP-2640). The pinned FastMCP 4.1.3 already includes it, so no upgrade is needed.
src/basic_memory/mcp/resources/skills.pyserves the skills the wheel ships underbasic_memory/data/skillsthroughSkillsDirectoryProvider(experimental_skills=True)plusSkillsExtension.io.modelcontextprotocol/skillsgetskills/list(description plus a digest manifest per skill) andskills/get.skill://<name>/SKILL.mdas an ordinary resource.tests/test_skills_dist.pyalready pins it. Internal skills such asmemory-ci-captureare never offered.set_project_delete_visibilitynow puts theVisibilitymark onserver.local_provider, not on the server.SkillsExtensionraises on any server-level transform. The tools live on the local provider, sobm_project_deletebehaves the same, and the existing visibility tests pass unchanged.Client support (checked 2026-11-01)
skills/list/skills/getclient forio.modelcontextprotocol/skills, but its enable check returnsfalse. It is built and switched off.skill://handling is its own internal skill provider.Test plan
tests/mcp/test_skill_resources.py:skills/listoffers exactly the shipped skills;skills/getsizes match the shipped files; a client without the extension can list and readskill://memory-notes/SKILL.mdjust typecheckjust fast-test -m "not benchmark": 5058 passed. The 8 failures were allCan't locate revision '0043'from a local home DB that another branch had migrated, and they reproduce without this change.🤖 Generated with Claude Code
https://claude.ai/code/session_01CMYVL4TcbWdXhdvAzxfqjo