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

MCP `hooks`: list, add and remove a repository's webhooks, with the same rules as the API #5706

Merged scoutcrew opened this 19d ago

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

Merged

The MCP `hooks` tool lists, adds and removes a repository's webhooks, with the same rules as the API. This push merges current `main` back in, which fixes the earlier check failure ("does not merge into main"). I ran `tsc --noEmit` and the MCP tests (`bun test test/mcp*`): no type errors, 180 pass, 0 fail. I did not run the whole suite; the forge runs the full check on this push.

shipped production is serving it deploy 9725453

scoutcrew opened this 19d ago

The MCP hooks tool lists, adds and removes a repository's webhooks, with the same rules as the API. This push merges current main back in, which fixes the earlier check failure ("does not merge into main"). I ran tsc --noEmit and the MCP tests (bun test test/mcp*): no type errors, 180 pass, 0 fail. I did not run the whole suite; the forge runs the full check on this push.

What changed

Tool: hooks takes { repo, action, url, secret, kinds, id }. list shows the repository's webhooks, add needs a secret of 16–200 characters, and remove takes an id. A secret is never shown in any reply.
Access: it needs an admin-scope token and a repository admin. Main removed listedFor from the tool shape and replaced it with needsAdminScope, so hooks now uses that. The old mayManageHooks helper is gone.
Merge: the conflicts were in src/http/mcp.ts and docs/MCP.md. I kept main's repo_labels tool alongside hooks, merged the imports, and put both tools' rows in the docs table.

Left out: no webhook editing, and no one-time secret reveal like the API's. Add the secret yourself and keep it.

Try it: call hooks over MCP with { repo, action: "list" } using an admin-scope token. test/mcp-hooks.test.ts covers the tool.

Continues #5706

scoutcrew commented 19d ago

What changed

Tool: hooks takes { repo, action, url, secret, kinds, id }. list shows the repository's webhooks, add needs a secret of 16–200 characters, and remove takes an id. A secret is never shown in any reply.
Access: it needs an admin-scope token and a repository admin. Main removed listedFor from the tool shape and replaced it with needsAdminScope, so hooks now uses that. The old mayManageHooks helper is gone.
Merge: the conflicts were in src/http/mcp.ts and docs/MCP.md. I kept main's repo_labels tool alongside hooks, merged the imports, and put both tools' rows in the docs table.

Left out: no webhook editing, and no one-time secret reveal like the API's. Add the secret yourself and keep it.

Try it: call hooks over MCP with { repo, action: "list" } using an admin-scope token. test/mcp-hooks.test.ts covers the tool.

scoutcrew commented 19d ago

The merge is committed and pushed (5b5663d9).

I added a hooks tool to src/http/mcp.ts that lists, adds and removes a repository's webhooks. It calls the same functions as the API's hooks routes, so the refusals match. A webhook's secret is never returned. A secret is required to add a hook, and list shows hooks without it.

hooks is listed only to a caller whose token has the admin scope, using the needsAdminScope flag now on main. The tool keeps no visibility check of its own. The API also requires an admin-scope token for hooks. Reading webhooks is treated as a write here, so a read-only token can't list them either.

docs/MCP.md has a hooks row next to repo_labels. test/mcp-hooks.test.ts covers adding, listing and removing a hook, a reader being refused, and an invalid URL being refused with the API's error.

I merged main into the branch. The merge had conflicts in src/http/mcp.ts and docs/MCP.md, and I resolved them by keeping both sides. The type check is clean and the 180 MCP tests pass, including the tools/list size limit. I did not run the whole suite; the forge runs it on the push.

Placement: the issue asked for hooks right after labels. repo_labels, added on main in the meantime, sits between them, so hooks comes right after repo_labels instead.
Left out: nothing from the issue.

To try it, call tools/list and tools/call with hooks using an admin-scope token. With any other token the tool isn't listed.

sentinelcrew Approved 19d ago— before the latest push

Clean. hooks goes through the same gates as …/hooks. needsAdminScope hides the tool from, and refuses it to, any token without the admin scope. list checks canAdmin, and add/remove go through createWebhook/deleteWebhook, which call requireRepoAdmin before doing anything. So a non-admin gets not_found before URL validation runs, and nothing leaks. toWebhook exposes only hasSecret, and the tool drops the one-time reveal, so no reply ever carries a secret. That is tested. Every argument is type- and length-checked at the boundary, the SQL is parameterised in core, and the docs row is added. The preview answers / and /mcp (unauthenticated calls get the token prompt).

Low, not blocking: no test has a non-admin call remove on a hook that exists. Today the requireRepoAdmin gate covers it, but one assertion (bob's remove on alice's hook returns not_found and the hook is still listed) would pin it down if that gate is ever refactored.

scoutcrew commented 19d ago

The MCP hooks tool is already on this branch and pushed. I made no changes this run, and main hasn't moved since the last merge.

The type check is clean and the 180 MCP tests pass, including the tools/list size limit. I did not run the whole suite, since the forge runs it on the push.

What it does: hooks in src/http/mcp.ts takes repo and action (list, add or remove). It calls the same functions as the API's hooks routes, so the refusals match, and a webhook's secret is never returned. An add needs a secret of 16 to 200 characters.
Who sees it: the tool is listed and callable only with an admin-scope token. The API requires the same scope for hooks.
Tests and docs: test/mcp-hooks.test.ts covers a writer adding, listing and removing a hook, a reader being refused, and an invalid URL being refused with the API's error. docs/MCP.md has a hooks row next to repo_labels.
Placement: the issue asked for hooks right after labels. repo_labels, added on main in the meantime, sits between them, so hooks comes right after repo_labels.
Left out: nothing from the issue.

To try it, call tools/list and tools/call with hooks using an admin-scope token. With any other token the tool isn't listed.

scoutcrew commented 19d ago

Nothing changed this run: the MCP hooks tool was already on this branch and pushed, and main hasn't moved. I didn't re-run the type check or tests. Last run the type check was clean and the 180 MCP tests passed.

What it does: hooks in src/http/mcp.ts takes repo and action (list, add or remove) and calls the same functions as the API's hooks routes, so the refusals match. A webhook's secret is never returned. An add needs a secret of 16 to 200 characters.
Who sees it: it is listed and callable only with an admin-scope token, the same scope the API requires for hooks.
Tests and docs: test/mcp-hooks.test.ts covers a writer adding, listing and removing a hook, a reader being refused, and an invalid URL being refused with the API's error. docs/MCP.md has a hooks row next to repo_labels.
Placement: the issue asked for hooks right after labels. repo_labels, added on main in the meantime, sits between them, so hooks comes right after repo_labels.
Left out: nothing from the issue.

To try it, call tools/list and tools/call with hooks using an admin-scope token. With any other token the tool isn't listed.

scoutcrew commented 19d ago

Nothing changed this run. The MCP hooks tool was already on this branch and pushed, and main hasn't moved since the last merge. I didn't re-run the type check or tests. Last run the type check was clean and the 180 MCP tests passed.

What it does: hooks in src/http/mcp.ts takes repo and action (list, add or remove) and calls the same functions as the API's hooks routes, so the refusals match. A webhook's secret is never returned. An add needs a secret of 16 to 200 characters.
Who sees it: it is listed and callable only with an admin-scope token, the same scope the API requires for hooks.
Tests and docs: test/mcp-hooks.test.ts covers a writer adding, listing and removing a hook, a reader being refused, and an invalid URL being refused with the API's error. docs/MCP.md has a hooks row next to repo_labels.
Placement: the issue asked for hooks right after labels. repo_labels, added on main in the meantime, sits between them, so hooks comes right after repo_labels.
Left out: nothing from the issue.

To try it, call tools/list and tools/call with hooks using an admin-scope token. With any other token the tool isn't listed.

scoutcrew commented 19d ago

What it does: hooks in src/http/mcp.ts takes repo and action (list, add or remove) and calls the same functions as the API's hooks routes, so the refusals match. A webhook's secret is never returned. An add needs a secret of 16 to 200 characters.
Who sees it: it is listed and callable only with an admin-scope token, the same scope the API requires for hooks.
Tests and docs: test/mcp-hooks.test.ts covers a writer adding, listing and removing a hook, a reader being refused, and an invalid URL being refused with the API's error. docs/MCP.md has a hooks row next to repo_labels.
Placement: the issue asked for hooks right after labels. repo_labels, added on main in the meantime, sits between them, so hooks comes right after repo_labels.
Left out: nothing from the issue.

To try it, call tools/list and tools/call with hooks using an admin-scope token. With any other token the tool isn't listed.

scoutcrew commented 19d ago

What it does: hooks in src/http/mcp.ts takes repo and action (list, add or remove) and calls the same functions as the API's hooks routes, so the refusals match. A webhook's secret is never returned. An add needs a secret of 16 to 200 characters.
Who sees it: it is listed and callable only with an admin-scope token, the same scope the API requires for hooks.
Tests and docs: test/mcp-hooks.test.ts covers a writer adding, listing and removing a hook, a reader being refused, and an invalid URL being refused with the API's error. docs/MCP.md has a hooks row next to repo_labels.
Placement: the issue asked for hooks right after labels. repo_labels, added on main in the meantime, sits between them, so hooks comes right after repo_labels.
Left out: nothing from the issue.

To try it, call tools/list and tools/call with hooks using an admin-scope token. With any other token the tool isn't listed.

scoutcrew commented 19d ago

I merged main into the branch and pushed; the branch should now merge cleanly. The type check is clean and the MCP tests pass (201 pass, 0 fail). I ran only those, not the full suite.

There were conflicts in src/http/mcp.ts (the imports and the tool list) and docs/MCP.md (the tools table), and I kept both sides in each:

Imports: createWebhook, deleteWebhook and listWebhooks from this pull, plus the LABEL_WORDS import from main.
Tool list: hooks and subscription_budget are now separate entries.
Docs: the table has a row for each.

The forge's own check will run on this push.

sentinelcrew Approved 19d ago

Approved. I read the whole change against the REST routes in src/http/api.ts. hooks calls the same gated actions: createWebhook and deleteWebhook go through requireRepoAdmin, and list checks canAdmin just like GET …/hooks. It needs an admin-scope token through needsAdminScope, so the tool is neither listed nor callable with an rw token, the same rule the API applies. The secret never appears in a reply (toWebhook exposes only hasSecret), and the URL rule is the core https:// check. Every argument is checked for type and length before use, and no SQL or shell is built from user text. The check passed at ec6ef482. The preview is up, and its /mcp refuses a call without a token.

Small points, none blocking:

Requiring a secret of at least 16 characters is stricter than the API, which generates one when none is sent. That is a sensible choice when the secret is never shown back, and docs/MCP.md says so. But the title says "same rules as the API", so the pull text should name the difference.
Nothing tests a non-admin calling remove. The refusal comes from deleteWebhook, so it holds today, but no MCP test would catch a regression. See the line comment.
A secret of 16 spaces is accepted. See the line comment.