Skip to content

feat(server): serve the skill references as MCP best-practices resources - #995

Merged
ragnorc merged 20 commits into
ModernRelay:mainfrom
ragnorc:mcp-best-practices
Oct 16, 2026
Merged

ragnorc merged 20 commits into
ModernRelay:mainfrom
ragnorc:mcp-best-practices

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

The last part of R8 in the RFC 0003 amendment (#992, issue #991): the npm package's omnigraph://best-practices/* resources, now served by the server itself.

Stacked on #994 (which is stacked on #993) and on #986, whose omnigraph-skill crate embeds skills/omnigraph byte for byte. Only the last commit is new. I'll merge main in as those land.

What changes

  • omnigraph-server depends on omnigraph-skill (path + version, as the CLI does).
  • resources/list adds omnigraph://best-practices/index and omnigraph://best-practices/{queries,data,schema,search,changes}. The bodies are skills/omnigraph/references/<topic>.md from the embedded skill, so the server always ships the references written beside its source; the npm package fetched them from this repository at build time instead. The curated descriptions move here from the package's cookbook-descriptions.json, rewritten for the server's tool names. The package's excluded references (CLI, cluster, policy, migrations, blobs, stored queries, remote ops) stay excluded for the same reasons.
  • These static resources need no graph, policy check or I/O; the request is still authenticated by /mcp's middleware.
  • MCP guide and changelog note.

Tests

  • cargo test -p omnigraph-server --locked: lib 202 passed (2 ignored), integration 262 passed, boot_settings 39, multi_graph 8. mcp_resources_read_graphs_schema_and_branches_under_policy now asserts the full resource list and that each topic's text equals the skill file byte for byte; the unit test every_best_practice_resource_is_an_embedded_skill_reference fails the build if a listed topic leaves the skill.
  • cargo fmt --all --check, clippy on omnigraph-server with -D warnings, check-docs.py, check-dependency-sources.py, typos: clean.
  • Live check (official MCP TypeScript SDK against the built binary): resources/list returns the seven URIs and omnigraph://best-practices/queries reads back # Query Authoring & Linting.

…kill

The binary now carries `skills/omnigraph` byte for byte. `omnigraph skill
export <dir> [--force] [--json]` writes it where an agent loads skills, and
`omnigraph skill show [<path>]` prints one file, so a consumer gets the skill
written for the CLI it runs.

A new `omnigraph-skill` crate has its package root at `skills/`, keeping the
skill tree its one copy while still packaging; its build script embeds every
file. An export records each written file's digest in `.omnigraph-skill.json`,
so a later `--force` export removes only unchanged files the skill no longer
has and never touches anything else.

In-source tests hold the skill's command index and condensed reference to the
clap tree; integration tests check byte identity and the `--force` rules;
check-docs.py now also holds the skill's declared HTTP contract to the CLI's.
…5ff0

# Conflicts:
#	crates/omnigraph-cli/src/planes.rs
#	docs/user/cli/reference.md
…5ff0

# Conflicts:
#	crates/omnigraph-cli/src/cli.rs
#	crates/omnigraph-cli/src/main.rs
#	crates/omnigraph-cli/src/planes.rs
#	docs/user/cli/reference.md
@cursor

cursor Bot commented Oct 14, 2026

Copy link
Copy Markdown

Review: Serve skill references as MCP best-practices resources

What this PR does. This is the last piece of RFC 0003 R8: the omnigraph-server now serves the agent skill's topic references (queries, data, schema, search, changes) as omnigraph://best-practices/* MCP resources. It also introduces the omnigraph-skill crate, the omnigraph skill export CLI command, expands MCP to every credential source (not just OIDC), adds read-parity tools (health, schema, snapshot, branches_list, commits_list, commits_get, commits_changes, changes_poll), and opt-in write tools (mutate, load, branches_create, branches_delete, branches_merge). These are from the stacked PRs (#993, #994, #986); only the top commit (0f2f39e2) is new here.

Architecture. The omnigraph-skill crate (rooted at skills/) uses a build script to embed skills/omnigraph files byte for byte at compile time. The server takes the crate as a dependency and serves five topic references as MCP resources with curated descriptions. Static resources bypass graph admission (no I/O, no policy check needed) but are still behind the authenticated /mcp middleware, so only callers with valid credentials can read them. Graph-backed resources (omnigraph://graphs, omnigraph://graphs/{graph}/schema, omnigraph://graphs/{graph}/branches) continue to run their handler's policy action.

Invariant compliance. This change is additive and does not touch the graph-write path, manifest publication, snapshot coherence, or recovery. The MCP write tools follow the existing handlers and policy actions. outcome_unknown reporting for writes whose wait times out is a sound design that does not acknowledge before durable visibility. The if_graph_commit conditional write protocol is correctly threaded through the MCP layer.

Test coverage. All tests pass at the exact head commit.

Test suite Scope Result
cargo test -p omnigraph-server --locked --test integration mcp:: 10 MCP integration tests 10 passed
cargo test -p omnigraph-server --locked --lib mcp 5 MCP unit tests 5 passed
cargo test -p omnigraph-cli --locked --test integration cli_schema_config:: 29 tests including skill_export_writes_the_built_in_skill_byte_for_byte and the force/remove test 29 passed
cargo clippy -p omnigraph-server -p omnigraph-skill --all-targets --locked -- -D warnings Lint Clean
python3 scripts/check-dependency-sources.py Dependency integrity Clean (848 crates.io, 21 workspace)

The every_best_practice_resource_is_an_embedded_skill_reference unit test and the mcp_resources_read_graphs_schema_and_branches_under_policy integration test together verify that every served reference exists in the skill, starts with a markdown heading, appears in the index, and is byte-equal to the source file.

Tradeoffs. Embedding the skill text into the server binary increases binary size by the sum of the five reference files (about 54 KiB). This is negligible. The alternative, reading files at runtime, would add I/O and a filesystem dependency that the server otherwise does not need for these static resources. Embedding is the right choice.

Recommendation: approve once the one blocking finding below is resolved.


Blocking

  1. Publish-order bug in publish-crates.yml: omnigraph-server now depends on omnigraph-skill, but the publish workflow publishes omnigraph-server before omnigraph-skill (line 182 vs line 181). The publish_if_new omnigraph-skill line must move above publish_if_new omnigraph-server. Without this fix, the next crates.io release will fail because omnigraph-server references a version of omnigraph-skill that has not been published yet. The comment on line 169 already acknowledges "omnigraph-skill has no internal deps" but it is itself a dependency of both omnigraph-server and omnigraph-cli, so it must precede both in the publish sequence.

Optional / Nits

  1. BestPractice::body() uses expect() (mcp.rs:740): This panics at runtime if a served key is missing from the embedded skill. The unit test (every_best_practice_resource_is_an_embedded_skill_reference) catches the mismatch at test time, so the expect is safe in practice. However, since the MCP request has already been authenticated, a panic here becomes a 500 with a lost connection to the caller. Returning an ErrorData::internal_error instead of panicking would be more defensive. Not blocking because the test gate covers this.

  2. Linear search in omnigraph_skill::file() (skills/src/lib.rs:30): FILES.iter().find(|file| file.path == path) is O(n). With only ~15 files this is negligible, but since FILES is sorted by path (the build script sorts), a binary search (FILES.binary_search_by_key(&path, |f| f.path)) would be a zero-cost improvement. Not blocking.


Validated at commit 0f2f39e2b6bb880356065a5ac5624ebf6f9bb8ea.

Comment thread .github/workflows/publish-crates.yml
…claude/cool-newton-c25ff0

# Conflicts:
#	Cargo.lock
#	crates/omnigraph-cli/src/main.rs
…claude/cool-newton-c25ff0

# Conflicts:
#	crates/omnigraph-cli/Cargo.toml
@ragnorc

ragnorc commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Review at 0f2f39e, addressed at 542c8fd:

  • Blocking, publish order: fixed. omnigraph-skill publishes before omnigraph-server (thread resolved).
  • Optional, expect() in BestPractice::body(): taken. body() returns Option, and a missing or non-UTF-8 reference becomes a JSON-RPC internal error for that one read instead of a panic. The unit test still requires every listed key to be embedded.
  • Optional, binary search in omnigraph_skill::file(): that function belongs to feat(cli): omnigraph skill export ships the version-matched agent skill #986's crate, so I left it to that PR rather than change it here.

The branch now also has main (including #994, cea15256) and #986's latest head (343dc4bd) merged in, no rebase. Checks at 542c8fd with CARGO_INCREMENTAL=0:

  • cargo test -p omnigraph-server --locked: lib 205, boot_settings 40, integration 263, multi_graph 8, all passing.
  • cargo clippy -p omnigraph-server -p omnigraph-skill -p omnigraph-cli --all-targets -D warnings: clean.
  • cargo fmt --check, check-docs.py, check-dependency-sources.py and typos: clean.

I'll merge #986's branch again when it pushes its review fixes.

The exported skill now states the storage formats this binary reports, and
an in-source test holds that line to the value `omnigraph version` prints,
through one shared formatter. Elsewhere the skill refers to that line
instead of repeating the numbers.

A `--force` export no longer removes a recorded file that differs from a
current file only in ASCII case, which on a case-insensitive file system is
the file it just wrote. Skill paths and record paths must be ASCII. The
skill crate's package includes exactly what its build embeds. The command
index lists `cluster list` and `cluster pull`.
…aph into mcp-best-practices

# Conflicts:
#	crates/omnigraph-server/Cargo.toml
@ragnorc

ragnorc commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Merged #986's head b002ed82 again, which already contains main, at d5c2f1cb (no rebase). The one conflict was the server's Cargo.toml: main moved the workspace to 0.14.0, so the server's omnigraph-skill dependency is now 0.14.0 too, matching the CLI. This PR adds no CLI command, so the command index is unaffected.

Checks with CARGO_INCREMENTAL=0:

  • cargo test -p omnigraph-server --locked: lib 205, boot_settings 40, integration 263, multi_graph 8, all passing.
  • cargo test -p omnigraph-cli --bin omnigraph skill: 5 passing.
  • cargo clippy -p omnigraph-server -p omnigraph-skill -p omnigraph-cli --all-targets -D warnings: clean.
  • cargo fmt --check, check-docs.py, check-dependency-sources.py and typos: clean.

@ragnorc

ragnorc commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

#986's review fixes are pushed: ragnorc:claude/cool-newton-c25ff0 at 62a83e7 (main merged at fe69c03). Changes in omnigraph-skill that this PR builds on:

  • The crate version is 0.14.0, and its package has an explicit include = ["/build.rs", "/src/**", "/omnigraph/**"].
  • FILES is now a static (the embedded bytes exist once in a binary), and file() binary-searches it; the build sorts paths byte-wise. The API names are unchanged: FILES, file, NAME, ENTRY.
  • build.rs refuses non-ASCII skill path names.
  • Every public CLI command, including nested ones such as cluster members list, needs a row in skills/omnigraph/references/commands.md (## Command Index), or skill_command_index_lists_every_public_command fails.

@ragnorc

ragnorc commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Merged #986's head 62a83e79 again, which contains main up to fe69c039, the binary-search file() and the cluster members list index row, at 8b2a4fbf (no rebase, no conflicts). That resolves the last optional point from the review.

Checks with CARGO_INCREMENTAL=0:

  • cargo test -p omnigraph-skill --locked: 1 passing.
  • cargo test -p omnigraph-server --locked: lib 205, boot_settings 40, integration 263, multi_graph 8, all passing.
  • cargo test -p omnigraph-cli --bin omnigraph skill: 5 passing.
  • cargo clippy -p omnigraph-server -p omnigraph-skill -p omnigraph-cli --all-targets -D warnings: clean.
  • cargo fmt --check, check-docs.py, check-dependency-sources.py and typos: clean.

@ragnorc
ragnorc merged commit 4102cc8 into ModernRelay:main Oct 16, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant