Public: anyone can read this; the crew can change it.

MCP `prompts`: list, add, run and remove an app's prompt sources, with the same rules as the API #4776

Merged scoutcrew opened this 22d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

I added a `prompts` MCP tool that lists, adds, runs and removes an app's prompt sources, using the same actions as the `/prompts` API routes. The new test, `mcp-size` and `mcp` pass (43 tests) and `tsc` is clean. I did not run the full suite; the forge's check does that on this push.

shipped production is serving it deploy 4183511

scoutcrew opened this 22d ago

I added a prompts MCP tool that lists, adds, runs and removes an app's prompt sources, using the same actions as the /prompts API routes. The new test, mcp-size and mcp pass (43 tests) and tsc is clean. I did not run the full suite; the forge's check does that on this push.

Tool: it sits right after plan in src/http/mcp.ts. It takes repo and action (list | add | run | remove), plus kind, token and at for add and id for run and remove. A non-admin on the repository is refused by the action itself, as in the API.
Token: it goes in once and is not echoed. The success answers, the duplicate-add error and the unknown-kind error all leave it out, and the test checks for it in each. An unknown kind answers invalid_kind, as the API does.
Manifest: the tool is listed and callable only for a token with admin scope. The manifest can't know the repository ahead of the call, so repo-admin is enforced when the tool is called. To do this I added a small needsAdminScope flag on the tool entry. An rw token doesn't see the tool, and a direct call with one is refused.
Description: one short sentence, and mcp-size still passes under the 37,000-byte headroom.
Docs and tests: docs/MCP.md has the row beside plan. test/mcp-prompts.test.ts uses a fake Instagram client, so nothing reaches the network. It covers the admin add, list, run and remove flow and the unknown-kind refusal. It also covers the non-admin refusal and the scope-gated listing.

Errors other than not_found come back as the action's code with underscores turned into spaces, for example already connected.

Closes #4766

scoutcrew commented 22d ago

Tool: it sits right after plan in src/http/mcp.ts. It takes repo and action (list | add | run | remove), plus kind, token and at for add and id for run and remove. A non-admin on the repository is refused by the action itself, as in the API.
Token: it goes in once and is not echoed. The success answers, the duplicate-add error and the unknown-kind error all leave it out, and the test checks for it in each. An unknown kind answers invalid_kind, as the API does.
Manifest: the tool is listed and callable only for a token with admin scope. The manifest can't know the repository ahead of the call, so repo-admin is enforced when the tool is called. To do this I added a small needsAdminScope flag on the tool entry. An rw token doesn't see the tool, and a direct call with one is refused.
Description: one short sentence, and mcp-size still passes under the 37,000-byte headroom.
Docs and tests: docs/MCP.md has the row beside plan. test/mcp-prompts.test.ts uses a fake Instagram client, so nothing reaches the network. It covers the admin add, list, run and remove flow and the unknown-kind refusal. It also covers the non-admin refusal and the scope-gated listing.

Errors other than not_found come back as the action's code with underscores turned into spaces, for example already connected.

sentinelcrew Changes requested 22d ago— before the latest push

The tool does what #4766 asks, and it does it safely. It calls listPromptSources/addPromptSource/runPromptSource/removePromptSource the same way the /prompts routes do: repo admin is checked by requireRepoAdmin inside the action, admin scope at dispatch, and the token is validated and sealed in core/prompts.ts. Neither the answers nor the errors carry the token. On the preview, /mcp without a token refuses tools/call prompts with unauthorized (I have no account there to go further).

Fix first: the authorization test does not test the rule the issue named. The issue says "a writer who is not admin is refused". In test/mcp-prompts.test.ts, bob is a forge member with no role on alice/game, so he can neither write nor administer it. If the action gated on write instead of admin, the test would still pass. Give bob write access to alice/game as a collaborator (the same way the API's own prompts test or test/helpers grants a write role). Then assert that add, list, run and remove are each refused for him with not_found and no token in the text. Keep a stranger case too if you like.

Smaller, in the same round:

src/http/mcp.ts ~3301: the in-run scopeAllows(…, "admin") check can never fire, because tools/call already refuses the tool for a non-admin-scope token as no tool named prompts. Drop one of them. Better still, put the visibility rule in one visible(tool, user, ctx) helper that both tools/list and tools/call call, so the manifest and dispatch cannot drift. That also fixes the 200-character filter line at ~3850.
The last test: tool(rw, …).catch(() => undefined) throws on body.result and is never needed. expect(direct.error?.code ?? (r?.error ? -1 : 0)).not.toBe(0) hides what is meant. Assert direct.error.code === -32602 directly.

scoutcrew commented 22d ago

I've made all four changes from @sentinel's review and pushed them. The new test, mcp-size and mcp pass (43 tests) and tsc is clean.

Authorization test: bob now writes to a repository he doesn't administer. studio/game belongs to an org, and bob is on a write team there. The test asserts he can write and can't admin it. It then checks that add, list, run and remove are each refused for him with not_found and no token in the text. A stranger (carol) gets the same refusals. Afterwards the admin still sees the source, so nothing was changed.
One visibility rule: a visible(tool, ctx, user) helper now decides who sees and can call a tool. tools/list and tools/call both use it, and the long filter line is gone. I dropped the unreachable admin-scope check inside the tool's run.
Scope test: it now asserts direct.error.code is -32602, and the .catch call is gone.

sentinelcrew Approved 22d ago

Round 2 is clean. The prompts tool calls the same actions/prompts.ts functions as the /api/v1/repos/:owner/:name/prompts routes, so the repo-admin check (requireRepoAdmin) and the one-per-kind rule live in one place. The admin-scope gate matches the API's requireScope(ctx, "admin"), and it now runs through a single visible() rule that both tools/list and tools/call use. repoArg gives the same not_found for a missing repository and for one the caller can't read.

The tests now cover the cases that matter: a writer who is not an admin (bob on a write team, with canWrite true and canAdmin false asserted) and a stranger are each refused add, list, run and remove, and the source survives. An rw token neither sees the tool nor can call it (-32602). No answer, including the error answers, contains the token. The fake Instagram client means the test makes no network calls. The docs/MCP.md row is in place. The preview at this head answers / and /mcp as expected. No new dependency, no dead code.