Unify MCP and agent tool surfaces onto one registry in NovelAgentToolset
Fixes NovelAgentService continuing a conversation under the wrong novel's route, since FindConversationAsync matched by id alone. Then extends NovelAgentToolset to all 45 tools the stdio MCP server offered (tag/location CRUD, character relationships, arc-stage beat pinning, question editing, cross-novel novel listing/creation), tagging each with whether it needs an explicit novel scope so a later MCP adapter can inject it. Renames the toolset's 33 existing schemas from snake_case to camelCase to match .NET/REST convention, since nothing external consumes them. Lays the groundwork to serve this same registry over MCP at /mcp and retire the separate stdio Novelly.Mcp project (docs/plans/api/mcp_http_merge_plan.md).
This commit is contained in:
James Wampler
@@ -0,0 +1,180 @@
|
||||
# Merge the MCP server into Novelly.Api as Streamable HTTP `/mcp`
|
||||
|
||||
## Context
|
||||
|
||||
Novelly has two duplicate tool surfaces that must be kept in sync by hand:
|
||||
|
||||
- **`src/Novelly.Mcp/`** — 45 tools as `[McpServerTool]` static methods, stdio-only, each calling the REST API back over HTTP via `NovelApiClient`. Built by no CI job, in no container, unknown to the AppHost. It only works if someone remembers to run `scripts/publish-mcp.sh` and re-point `.mcp.json` at the published binary. Zero tests.
|
||||
- **`src/Novelly.Api/Agent/NovelAgentToolset.cs`** — 33 tools for the embedded web agent, calling application services directly in-process.
|
||||
|
||||
The 33 are a **strict subset** of the 45, matching name-for-name. The gap is pure capability loss for the web agent, not a design distinction.
|
||||
|
||||
Two problems follow. The MCP server is unreachable from anything but a local stdio subprocess, so the QA deploy can't serve it without shipping a binary around. And every new capability has to be written twice, in two idioms, with nothing enforcing that they agree.
|
||||
|
||||
**Outcome:** one tool registry, called directly by both surfaces. The API serves MCP over Streamable HTTP at `/mcp`, so any MCP client reaches it over the network with an API key and no binary to distribute. The stdio project is deleted. The web agent gains all 12 tools it was missing.
|
||||
|
||||
## Decisions taken
|
||||
|
||||
- **Delete `src/Novelly.Mcp` entirely.** No stdio proxy is kept.
|
||||
- **The web agent gets all 45 tools**, including cross-novel `list_novels` and `create_novel`. Full parity, one list, no filtering.
|
||||
- **camelCase argument names** everywhere. MCP specifies nothing about argument naming — `inputSchema` is plain JSON Schema — so this is a free choice, and camelCase matches .NET convention, the REST API's `JsonSerializerDefaults.Web` output, and the current MCP surface. Existing MCP clients keep working unchanged. The cost lands on `NovelAgentToolset`'s 33 hand-built schemas, which are today the only snake_case thing in the repo and must be rewritten.
|
||||
- **Tool names stay snake_case** (`list_novels`, `get_novel_brief`) — that part *is* genuine MCP convention, and both surfaces already agree on it.
|
||||
|
||||
## Design
|
||||
|
||||
A single registry — the existing `AgentTool` shape in `NovelAgentToolset`, extended to all 45 tools — with two thin adapters over it.
|
||||
|
||||
**MCP adapter.** Register the SDK's dynamic-tools handlers rather than 45 attribute-decorated methods:
|
||||
|
||||
```csharp
|
||||
services.AddMcpServer(o => o.ServerInfo = new Implementation { Name = "novelly", Version = "1.0.0" })
|
||||
.WithHttpTransport()
|
||||
.WithListToolsHandler((request, ct) => ...)
|
||||
.WithCallToolHandler((request, ct) => ...);
|
||||
```
|
||||
|
||||
Both handlers resolve `request.Services!.GetRequiredService<NovelAgentToolset>()` per request and delegate to a pure adapter class. This avoids fighting the SDK's schema inference (`McpServerToolCreateOptions` has no `InputSchema` property) and avoids hoisting the scoped, nine-dependency toolset into a static catalog, which `WithTools(IEnumerable<McpServerTool>)` would force.
|
||||
|
||||
**Agent adapter.** Unchanged — `NovelAgentService.SendMessageAsync` → `toolset.ExecuteAsync(name, novelId, input, ct)`, with `novelId` still ambient from the route and never shown to the model.
|
||||
|
||||
**Novel scoping, one entry / two shapes.** `AgentTool` and `AgentToolDefinition` each gain a trailing `bool RequiresNovelId = false`, so the 33 existing construction sites keep compiling. The MCP adapter injects a required `novelId` property into the advertised schema for those tools and extracts it at call time; the agent adapter supplies it from the route. `list_novels` / `create_novel` need no scope at all.
|
||||
|
||||
The 11 currently novel-scoped tools are identifiable mechanically — `grep -n 'async (novelId' src/Novelly.Api/Agent/NovelAgentToolset.cs`: `get_novel_brief`, `update_novel_brief`, `list_characters`, `create_character`, `list_tags`, `list_locations`, `list_chapters`, `create_chapter`, `get_character_beats`, `list_open_questions`, `raise_open_question`. Of the 12 new tools, `create_tag` and `create_location` are novel-scoped; the other ten are child-id-scoped or unscoped.
|
||||
|
||||
### Verified before planning
|
||||
|
||||
The design rests on SDK behaviour that build-and-test would not catch, so it was checked against a running probe app rather than inferred from docs:
|
||||
|
||||
- `WithListToolsHandler` / `WithCallToolHandler` exist in `ModelContextProtocol` 2.1.0; `Tool.InputSchema` is a settable `JsonElement` whose setter validates exactly what `JsonSchemaBuilder.Build()` already emits.
|
||||
- **`capabilities.tools` *is* advertised** on `initialize` with handlers and no `ToolCollection` — this was the main open risk and it is closed.
|
||||
- **`request.Services` is non-null and yields a fresh DI scope per request** (three calls returned three distinct scope ids). This is what makes the scoped `NovelDbContext` and `NovelAgentToolset` correct here. `HttpServerTransportOptions.Stateless` defaults to `true` and `PerSessionExecutionContext` to `false` in 2.1.0, so no options need restating.
|
||||
- A hand-built `JsonElement` schema survives verbatim onto `tools/list` output, and `IsError` maps to `result.isError` rather than a JSON-RPC error object.
|
||||
- `GET /mcp` returns 405 in stateless mode. Harmless; clients only POST.
|
||||
|
||||
## Chunks
|
||||
|
||||
Each builds, tests and commits independently.
|
||||
|
||||
### Chunk 0 — Fix the cross-novel conversation leak
|
||||
|
||||
Independent; do it first to keep it out of the main diff.
|
||||
|
||||
`NovelAgentService.FindConversationAsync(Guid conversationId, ...)` matches on id alone, so a conversation belonging to novel X can be continued under novel Y's route, after which every tool call runs against Y with X's transcript. Add an optional novel filter, passed from `SendMessageAsync` only — `GetConversationAsync`/`DeleteConversationAsync` are reached via `/api/conversations/{id}`, which has no novel in the route, and keep passing `null`. Log the miss at Warning with `{ConversationId}`/`{NovelId}`.
|
||||
|
||||
**Files:** `src/Novelly.Api/Agent/NovelAgentService.cs`, `tests/Novelly.Api.Tests/NovelAgentServiceTests.cs`
|
||||
|
||||
**Tests:** `Continuing_a_conversation_under_a_different_novel_is_rejected`, plus `Continuing_a_conversation_under_its_own_novel_still_works` as the guard against over-tightening.
|
||||
|
||||
### Chunk 1 — One registry, all 45 tools (no MCP wiring yet)
|
||||
|
||||
Delivers the parity decision on its own, verifiable through the existing web agent.
|
||||
|
||||
- `src/Novelly.Api/Agent/AgentContracts.cs` — add `RequiresNovelId` to `AgentToolDefinition`. Safe: `AnthropicAgentModelClient.ToSdkTool` maps `Name`/`Description`/`InputSchema` explicitly, so the flag never reaches the model.
|
||||
- `src/Novelly.Api/Agent/NovelAgentToolset.cs` — add `RequiresNovelId` to `AgentTool`, set it on the 11 tools above, flow it into `Definitions`, and add the 12 new tools reusing the existing `OrNotFound` / `DeletedOrNotFound` / `ToolNotFound` idioms and the descriptions from the corresponding `src/Novelly.Mcp/Tools/*.cs` methods.
|
||||
- **Rename the 33 existing schemas to camelCase** in the same file — both the `JsonSchemaBuilder` property keys and the matching `JsonInput` lookup strings, which must stay in lockstep (`.Str("character_id", …)` / `JsonInput.RequiredGuid(input, "character_id")` → `"characterId"`). Mechanical and contained to this one file, but it is the bulk of the chunk's diff and a mismatched pair fails silently as a missing argument rather than a compile error — so the per-tool tests below are what actually catch it. The 12 new tools are written camelCase from the start, matching the names their `src/Novelly.Mcp/Tools/*.cs` equivalents already used.
|
||||
|
||||
The 12 new tools and their existing service calls — no service-layer work is needed:
|
||||
|
||||
| tool | scope | service call |
|
||||
|---|---|---|
|
||||
| `list_novels` | none | `novels.ListAsync` |
|
||||
| `create_novel` | none | `novels.CreateAsync` |
|
||||
| `get_character` | child | `characters.GetAsync` |
|
||||
| `relate_characters` | child | `characters.AddRelationshipAsync` — note `CreateRelationshipRequest`'s parameter order differs from the old MCP method's |
|
||||
| `set_arc_stage_beats` | child | `arcs.SetBeatsAsync` |
|
||||
| `create_tag` | **novel** | `tags.CreateAsync` |
|
||||
| `update_tag` / `delete_tag` | child | `tags.UpdateAsync` / `DeleteAsync` |
|
||||
| `create_location` | **novel** | `locations.CreateAsync` |
|
||||
| `update_location` / `delete_location` | child | `locations.UpdateAsync` / `DeleteAsync` |
|
||||
| `update_open_question` | child | `questions.UpdateAsync` |
|
||||
|
||||
Guid-list arguments follow the existing `JsonInput.Guids` idiom used by `reorder_beats`.
|
||||
|
||||
**Tests:** new `tests/Novelly.Api.Tests/NovelAgentToolsetTests.cs`, driving `ExecuteAsync` directly in the style of `ImportAgentToolsetTests.cs` (`ServiceTestFixture` already wires every service the toolset needs). BDD names, one per new capability — e.g. `Relating_two_characters_shows_the_pair_on_both_dossiers`, `Deleting_a_tag_leaves_the_characters_that_carried_it_alone`, `Updating_an_open_question_can_detach_it_from_its_chapter`. Two structural tests carry the most weight:
|
||||
|
||||
- `The_toolset_offers_every_tool_the_stdio_server_offered` — assert the 45 names against a hard-coded array. This is the anti-drift test.
|
||||
- Assert no `RequiresNovelId` tool's schema already declares `novelId`, since the MCP adapter injects it and a duplicate would be silent.
|
||||
|
||||
**Runtime verify:** AppHost up, open a novel's agent panel, ask it to create a tag and list tags; confirm in the Aspire trace.
|
||||
|
||||
### Chunk 2 — Serve the registry at `/mcp`
|
||||
|
||||
- `src/Novelly.Api/Novelly.Api.csproj` — add `ModelContextProtocol.AspNetCore` 2.1.0 (brings Core transitively; don't reference it directly). **Not in the local NuGet cache — first restore needs network.** Pin 2.1.0 to match the verified surface.
|
||||
- New feature folder `src/Novelly.Api/Mcp/`:
|
||||
- `NovelMcpTools.cs` — the adapter, as pure static methods: `Describe(definitions)` maps to `Tool` records, injecting `novelId` where `RequiresNovelId`; `CallAsync(toolset, parameters, ct)` serializes arguments to a `JsonElement`, extracts `novelId` when required (`Guid.Empty` otherwise), calls `ExecuteAsync`, and maps `AgentToolResult` → `CallToolResult`. Catch the `ArgumentException` from a missing/malformed `novelId` and return it as `IsError` rather than letting it escape as a JSON-RPC error. **Log `{Tool}` and `{NovelId}` only — never the arguments, which carry prose (`what_happened`, `synopsis`, `notes`).**
|
||||
- `McpEndpoints.cs` — `MapNovelMcp()` calling `app.MapMcp("/mcp")`, matching the repo's `Map*Endpoints` convention.
|
||||
- `src/Novelly.Api/Common/NovellyServiceRegistration.cs` — the `AddMcpServer(...)` registration shown above.
|
||||
- `src/Novelly.Api/Program.cs` — `.MapNovelMcp()` after `UseAuthentication()`/`UseAuthorization()`.
|
||||
|
||||
**Auth — the trap.** Do **not** chain `.RequireAuthorization()` onto `MapMcp`. The parameterless overload applies the *default* policy, which authenticates `IdentityConstants.ApplicationScheme` only and would reject the API key. The fallback policy already registered in `NovellyServiceRegistration` lists both that scheme *and* `ServiceApiKeyAuthenticationHandler.SchemeName`, and applies to any endpoint carrying no authorization metadata — which `MapMcp` adds none of. `/mcp` inherits the right protection by doing nothing. If explicitness is wanted, register a named policy listing both schemes; never the parameterless call.
|
||||
|
||||
External clients send `X-Novelly-Api-Key: <Auth:ServiceApiKey>`, resolving to `ServiceUser` (Admin), which sees every novel. If `Auth:ServiceApiKey` is unset the service user is never seeded and every call 401s.
|
||||
|
||||
CORS needs no change — the origin-restricted default policy is irrelevant to non-browser clients, and stateless mode exposes no `Mcp-Session-Id` header to read.
|
||||
|
||||
**Tests:** new `tests/Novelly.Api.Tests/NovelMcpToolsTests.cs`, against the pure adapter methods — no live session, no `WebApplicationFactory`. Cover: all 45 advertised with unique names; novel-scoped tools declare a required `novelId` and child-id tools don't; `list_novels` needs none; every advertised schema is a valid `type: object` (what `Tool.InputSchema`'s setter enforces, worth asserting before the SDK throws at startup); a missing `novelId` and a not-found id both come back as `IsError` results.
|
||||
|
||||
**Deliberately not adding a `WebApplicationFactory` harness.** None exists in the repo; adding one means an MVC.Testing reference, overriding the connection string, working around `Program.cs`'s boot-time `MigrateAsync` + `Environment.Exit(1)`, seeding the key, and parsing SSE. Its main payoff — proving the SDK wires up — is delivered more honestly by the curl walkthrough below, which exercises real Kestrel including auth, Serilog and the exception handler. Worth a separate chunk later if a regression harness is wanted.
|
||||
|
||||
### Chunk 3 — Delete the stdio server
|
||||
|
||||
Only after Chunk 2 is verified, so there's never a window with no MCP surface.
|
||||
|
||||
Remove `src/Novelly.Mcp/`, `scripts/publish-mcp.sh`, the `mcp-server/` publish output (gitignored; working-tree cleanup only), and the project line in `Novelly.slnx`. Verified as *not* referencing it: `scripts/ci/build.sh` (publishes the API by path), `prepush.sh`, `test.sh`, both Dockerfiles, the AppHost.
|
||||
|
||||
Docs to update:
|
||||
- `README.md` — fold the `Novelly.Mcp` stack-table row into the API's; rewrite "The MCP server" section (same 45 tools, now in-process at `POST /mcp`, no build step, `X-Novelly-Api-Key` auth). Note that MCP argument names are unchanged (camelCase), so existing clients need no edits, and that `create_novel` is owned by the signed-in user over the web agent but by the service user over MCP.
|
||||
- `.mcp.json` / `.mcp.json.example` — switch to `type: "http"`, `url: http://localhost:5080/mcp`, with the key in `headers`.
|
||||
- `.claude/agents/outline-importer.md` — drop the published-binary requirement. Its `tools:` frontmatter is **already stale** (`mcp__novelly__list_projects`, `get_project_brief`, `create_project` exist in neither surface); fix to real names while here.
|
||||
- `CLAUDE.md` — the Structure list still calls `src/Novelly.Mcp/` the "MCP stdio server" and Verifying still says "drive over stdio JSON-RPC". Propose these edits rather than slipping them in; CLAUDE.md is user-owned.
|
||||
|
||||
Leave `docs/plans/api/users_and_roles_plan.md` alone — historical.
|
||||
|
||||
**Verify:** `dotnet build Novelly.slnx` and `./scripts/ci/prepush.sh` both pass.
|
||||
|
||||
## Verification
|
||||
|
||||
Streamable HTTP needs `Accept: application/json, text/event-stream` and replies SSE-framed, so pipe through `sed -n 's/^data: //p'`.
|
||||
|
||||
```bash
|
||||
dotnet user-secrets set Auth:ServiceApiKey devkey -p src/Novelly.Api
|
||||
ASPNETCORE_URLS=http://localhost:5080 dotnet run --project src/Novelly.Api
|
||||
|
||||
MCP=http://localhost:5080/mcp
|
||||
H=(-H "Content-Type: application/json" -H "Accept: application/json, text/event-stream" -H "X-Novelly-Api-Key: devkey")
|
||||
|
||||
# handshake — expect capabilities.tools present
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"curl","version":"1"}}}' | sed -n 's/^data: //p' | jq .
|
||||
curl -sS "${H[@]}" -o /dev/null -w '%{http_code}\n' "$MCP" -d '{"jsonrpc":"2.0","method":"notifications/initialized"}'
|
||||
|
||||
# expect exactly 45
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":2,"method":"tools/list","params":{}}' | sed -n 's/^data: //p' | jq '.result.tools | length'
|
||||
|
||||
# novelId injection: present on list_tags, absent on delete_tag and list_novels
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":3,"method":"tools/list","params":{}}' | sed -n 's/^data: //p' \
|
||||
| jq '.result.tools[] | select(.name=="list_tags" or .name=="delete_tag" or .name=="list_novels") | {name, props:(.inputSchema.properties|keys), required:.inputSchema.required}'
|
||||
|
||||
# unscoped call, then a novel-scoped one (proves NovelUserContext resolved the service user in-handler)
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":4,"method":"tools/call","params":{"name":"list_novels","arguments":{}}}' | sed -n 's/^data: //p' | jq .
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":5,"method":"tools/call","params":{"name":"get_novel_brief","arguments":{"novelId":"<id from above>"}}}' | sed -n 's/^data: //p' | jq .
|
||||
|
||||
# error mapping — expect result.isError true, not a JSON-RPC error
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":6,"method":"tools/call","params":{"name":"list_tags","arguments":{}}}' | sed -n 's/^data: //p' | jq .
|
||||
|
||||
# auth — expect 401 with no key
|
||||
curl -sS -o /dev/null -w '%{http_code}\n' -H "Content-Type: application/json" -H "Accept: application/json, text/event-stream" \
|
||||
"$MCP" -d '{"jsonrpc":"2.0","id":7,"method":"tools/list","params":{}}'
|
||||
|
||||
# one write, end to end
|
||||
curl -sS "${H[@]}" "$MCP" -d '{"jsonrpc":"2.0","id":8,"method":"tools/call","params":{"name":"create_tag","arguments":{"novelId":"<id>","name":"Salt","color":"#9a4a2f"}}}' | sed -n 's/^data: //p' | jq .
|
||||
```
|
||||
|
||||
Then reconnect a real client: rewrite `.mcp.json` to the `type: "http"` form and run `/mcp` in Claude Code to confirm 45 tools. Finally confirm the web agent still drives the same registry (Aspire up, agent panel, exercise one of the 12 new tools) and that the dashboard shows `/mcp` requests with no prose in the log lines.
|
||||
|
||||
## Risks
|
||||
|
||||
- **`ModelContextProtocol.AspNetCore` is not cached locally.** First restore needs network.
|
||||
- **The web agent's argument names change** (snake_case → camelCase) across all 33 existing tools. Nothing external consumes those schemas — they are built fresh per request and handed to the model each turn — so there is no compatibility surface, but a `JsonSchemaBuilder` key left out of step with its `JsonInput` lookup fails silently as a missing argument rather than a compile error. External MCP clients are unaffected: their argument names were already camelCase.
|
||||
- **`create_novel` ownership differs by surface** — signed-in user vs service user. Not a bug, but surprising; document it.
|
||||
- **Not fixed here:** child-id-scoped tools carry no novel context in *either* surface, so a caller holding a foreign beat/tag/question id can reach across novels, subject only to `NovelAccessService`. Pre-existing for 20+ tools and unchanged by this work. Worth a follow-up.
|
||||
- **QA deploy:** once merged, `/mcp` rides the existing API container and its already-exposed port. `MCP_API_KEY` must be set in Gitea for `Auth__ServiceApiKey`, or every MCP call 401s.
|
||||
Reference in New Issue
Block a user