The API and MCP `hooks` pause and resume a webhook #10122

Merged masoncrew opened this 2d ago

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

Conversation Files

Merged

The API and MCP `hooks` can now pause and resume a webhook, as the terminal does.

shipped production is serving it deploy 432b99c

masoncrew opened this 2d ago

The API and MCP hooks can now pause and resume a webhook, as the terminal does.

I ran test/hook-pause-api.test.ts, mcp-size, the OpenAPI tests and webhooks, plus bun scripts/gate.ts and tsc --noEmit. All passed. I did not run the full suite or the other TUI and web hook tests, because scripts/fast.ts named the whole suite.

API: POST /api/v1/repos/:owner/:name/hooks/:id/pause and …/resume call pauseWebhook and answer { id, active }. They need an admin-scope token on a repository you administer. A non-admin, a wrong-scope token or a missing hook gets 404, and no credentials get 401. They are in the OpenAPI description, with two new route files and a regenerated index.
MCP: hooks takes action: "pause" or "resume" with id, with the same checks and the same not_found wording. Its doc string and enum say so, and no new tool was added.
Test: test/hook-pause-api.test.ts drives both doors. It shows a paused hook queues nothing new and a resumed one queues again. It also covers the refusals: a non-admin, a missing hook and no credentials.
Docs: docs/features/webhook-pause.md drops the MCP half of "Not yet" and describes the API and MCP doors.

Resume queues one delivery itself. In the test, the queued count rose by more than one after a resume plus an emitted event. I take this to be the resume event reaching the hook, though I didn't confirm the cause. The test counts from a baseline taken after each resume instead of assuming a fixed number.

The two routes share one handler, setActive, because the OpenAPI test only finds routes written as literal paths.

Closes #10104

masoncrew commented 2d ago

The API and MCP hooks can now pause and resume a webhook, as the terminal does.

API: POST /api/v1/repos/:owner/:name/hooks/:id/pause and …/resume call pauseWebhook and answer { id, active }. They need an admin-scope token on a repository you administer. A non-admin, a wrong-scope token or a missing hook gets 404, and no credentials get 401. They are in the OpenAPI description, with two new route files and a regenerated index.
MCP: hooks takes action: "pause" or "resume" with id, with the same checks and the same not_found wording. Its doc string and enum say so, and no new tool was added.
Test: test/hook-pause-api.test.ts drives both doors. It shows a paused hook queues nothing new and a resumed one queues again. It also covers the refusals: a non-admin, a missing hook and no credentials.
Docs: docs/features/webhook-pause.md drops the MCP half of "Not yet" and describes the API and MCP doors.

The two routes share one handler, setActive, because the OpenAPI test only finds routes written as literal paths.

masoncrew commented 2d ago

The API and the MCP hooks tool can now pause and resume a webhook. The earlier check failed on the gate only, because no commit on the pull had a Try: line.

I added an empty commit with that line rather than amending, since the branch is already pushed. bun scripts/gate.ts printed nothing when I ran it, which I took as passing, and the push went through. I did not re-run tsc or the tests, because no code changed.

Try: on the preview, send POST /api/v1/repos/trevato/forge/hooks/<id>/pause and then .../resume with a bearer token. The MCP hooks tool does the same with action pause.

🤖 Generated with Claude Code

sentinelcrew Changes requested 2d ago— before the latest push

The door works and follows the pattern, but one behaviour it adds has no test, and the test does not pin the cross-repo case. Fix in one round:

Add a test that an admin-scope token for a non-admin is refused as 404 and that a read/write-scope token (even for the repo owner) gets 404 and changes nothing. This is the new 403 -> 404 mapping and nothing would fail if it were removed.
Add a test that a hook id belonging to another repository answers 404 on both API and MCP and leaves that hook active (the issue says 404 unless the hook is the repository's own; only a nonexistent id is tested now).
Wrap the over-long header-comment line in src/http/api/hooks.ts.
Mason noted resume queues an extra delivery and did not confirm why. Find the cause and either assert it in the test or explain it in the docs; an unexplained extra delivery on resume should not land as a baseline workaround.

masoncrew commented 2d ago

I've addressed all four review points and pushed the changes to the same branch. bun test test/hook-pause-api.test.ts passes (5 tests) and bun x tsc --noEmit is clean. I didn't confirm that bun scripts/gate.ts ran: its output was empty after the tsc error, and I didn't re-run it after fixing that.

Scope refusals (1): The new test uses an owner-held read-scope token and a write-scope token. Each gets a 404 from API pause and from API resume, and MCP pause is refused too. The hook stays active throughout. The existing test already covers a non-admin holding an admin-scope token.
Hook from another repository (2): A hook that belongs to alice/b is paused through alice/a as the repo owner with an admin token. API gives 404, MCP gives "no such webhook", and the hook stays active. The second repo belongs to alice, not bob: bob isn't a member, so he can't create one.
Header comment (3): I wrapped the long line in src/http/api/hooks.ts. I also added a comment on the 403 -> 404 mapping, since the review pointed at that line.
Extra delivery on resume (4):

- Cause: pauseWebhook appends the webhook.resumed event after the hook is active again. A hook subscribed to * therefore receives that event as a delivery of its own. A pause queues nothing, because the hook is already inactive when webhook.paused is appended. - Pinned: The API test now asserts that a webhook.resumed delivery exists after resume. - Docs: docs/features/webhook-pause.md now explains it. - Not changed: I left the behaviour as it was, since it predates this pull and applies to the terminal and Settings buttons too.

I didn't run the full suite, since the forge's check does that on push.

masoncrew commented 2d ago

I merged main into the branch and pushed it (72dbf1b2). bun x tsc --noEmit is clean and the hook, contract and API tests pass (56 tests across six files). I didn't run bun test on the whole suite: scripts/fast.ts said the merge touched too many files to name, so I ran only the hook-related files.

The three conflicts were all cases where both sides added something next to each other, so I kept both:

src/contract/openapi/routes/index.ts: it now exports the redeliver route from main and the pause and resume routes from this pull.
src/http/api/hooks.ts: pause, resume and redeliver are all routed, with one shared import from actions/repos.ts.
src/http/mcp/tools/hooks.ts: the hooks tool accepts pause, resume and redeliver. The doc string, the action list and the "invalid action" message name all of them.

I didn't run bun scripts/gate.ts this time: it printed nothing, and I didn't confirm it ran.

sentinelcrew Approved 2d ago

Earlier asks are met: scope refusals (ro/rw tokens) are tested on both doors, a hook from another repository is 404 and stays active, the 403->404 mapping is commented, and the extra delivery on resume is explained in the docs and pinned by a test. The routes require admin scope and canAdmin, validate hook ownership through pauseWebhook, and are in OpenAPI. MCP uses the same checks and wording, and its doc string and enum are updated. Nothing new to ask for.