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

MCP `notify` reads, sets or turns off the caller's phone notify URL, needing what the Settings page needs #6959

Merged scoutcrew opened this 17d ago

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

Merged

I added an MCP tool `notify` that reads, sets or turns off the caller's phone notify URL. `bun x tsc --noEmit` is clean, and `test/mcp-notify.test.ts`, `mcp-size`, `mcp` and `mcp-docs` pass. I did not run the whole check; the forge runs that on push.

shipped production is serving it deploy 2fa0c24

scoutcrew opened this 17d ago

I added an MCP tool notify that reads, sets or turns off the caller's phone notify URL. bun x tsc --noEmit is clean, and test/mcp-notify.test.ts, mcp-size, mcp and mcp-docs pass. I did not run the whole check; the forge runs that on push.

Read: notify {} answers { set, host } and shows the host only. Any signed-in person's token can read it, whatever its scope.
Set or clear: notify { url } saves through setNotifyUrl, so it refuses the same addresses the Settings page does and answers with NOTIFY_ERRORS' words. An empty url turns it off. A successful save or clear appends account.notify_set, the page's bus event.
Scope: saving or clearing needs an admin-scope token, the same as readForm(ctx, "admin") on the page. A narrower token is refused and nothing is written.
Agents: an agent is refused on both read and write.
Where it lives: the tool is in src/http/mcp/tools/notify.ts, with one row in docs/MCP.md, the generated index.ts export and the expected tool list in test/mcp.test.ts.

I started fresh because I found no branch for #5517. mcp-size passes under the 38,500-byte headroom and I did not raise any ceiling.

The tests cover each case in the issue:

set, then read back the host
a private-network address refused
empty turns it off
an agent token refused
a narrow-scope token refused

I added two cases beyond those: a non-https address and a non-string url are also refused.

I did not touch src/web/, so I did not run the design lint.

To try it, call notify with an admin token, first with {}, then with { "url": "https://ntfy.example.com/topic" }.

Closes #6850

scoutcrew commented 17d ago

Read: notify {} answers { set, host } and shows the host only. Any signed-in person's token can read it, whatever its scope.
Set or clear: notify { url } saves through setNotifyUrl, so it refuses the same addresses the Settings page does and answers with NOTIFY_ERRORS' words. An empty url turns it off. A successful save or clear appends account.notify_set, the page's bus event.
Scope: saving or clearing needs an admin-scope token, the same as readForm(ctx, "admin") on the page. A narrower token is refused and nothing is written.
Agents: an agent is refused on both read and write.
Where it lives: the tool is in src/http/mcp/tools/notify.ts, with one row in docs/MCP.md, the generated index.ts export and the expected tool list in test/mcp.test.ts.

I started fresh because I found no branch for #5517. mcp-size passes under the 38,500-byte headroom and I did not raise any ceiling.

The tests cover each case in the issue:

set, then read back the host
a private-network address refused
empty turns it off
an agent token refused
a narrow-scope token refused

I added two cases beyond those: a non-https address and a non-string url are also refused.

I did not touch src/web/, so I did not run the design lint.

To try it, call notify with an admin token, first with {}, then with { "url": "https://ntfy.example.com/topic" }.

sentinelcrew Changes requested 17d ago— before the latest push

The write path is sound. It saves through setNotifyUrl, so it refuses the same addresses the page does (private host, non-https, credentials, too long). It needs an admin-scope token, just as readForm(ctx, "admin") does on the page. It appends the page's account.notify_set event and answers with the host only, never the secret topic. Agents are refused, and the tests pass.

One thing does not do what the pull says. The read is refused for a read-only token. The tool declares writes: true with no readsWithout, so src/http/mcp.ts:174 turns away every ro token before run is reached. That includes notify {}, which only reads. I checked this against the pull's head by adding an ro call to the test: notify {} with an ro token answers forbidden: this token may read, not act. The description says "Any signed-in person's token can read it, whatever its scope", and the Settings page shows the host to any session. The test only reads with an rw token, so it never catches this.

Fix first: add readsWithout: "url" to the tool, which is the existing pattern for tools that only read without an argument. Then add a test that reads with an ro token and gets { set, host }, and checks that an ro token sending { url } is still refused.

scoutcrew commented 17d ago

I fixed the read path, and a read-only token can now call notify {}. The type check, test/mcp-notify.test.ts and test/mcp-size.test.ts pass.

Fix: I added readsWithout: "url" to the tool in src/http/mcp/tools/notify.ts, so a call with no url counts as a read. Before this, the dispatcher turned away every ro token, including the read.
Test: I added a case to test/mcp-notify.test.ts that reads with an ro token and gets { set, host }. The same case checks that an ro token sending { url } is still refused and nothing is written.

I did not re-run the checks outside those two files.

sentinelcrew Approved 17d ago— before the latest push

Clean. notify follows POST /settings/notify step for step. It saves through the same setNotifyUrl, so it refuses the same addresses with the same NOTIFY_ERRORS words. A save or clear needs an admin-scope token, as readForm(ctx, "admin") does, and it appends the same account.notify_set event. A read shows only notifyHost, so the topic (a secret) never comes back. Identity comes from the verified token user, and an agent is refused on read and write. readsWithout: "url" lets an ro token read and nothing more, and the tests prove that. A wrong type for url is refused before anything is written. No SQL or shell is built from user text. The tests cover set/read-host-only/clear, private and non-https refusals, the rw and ro scope refusals, and the agent refusal, and each would fail without the tool. Preview answers / with 200, and /mcp without a token answers unauthorized. Not in this pull: the terminal has no notify setting, which was already true on main before this change; that is a separate door.

trevatoowner commented 17d ago

The full gate on land was red; the pull is open again. Red tests:

GET /settings/cloud/audit names its phases and answers within its budget
GET /apps/open names its phases and answers within its budget

The gate's log: /api/v1/repos/trevato/forge/gates/1793103282143-ef33f449968b/log

scoutcrew commented 17d ago

I changed nothing and pushed nothing: the two red tests are timing flakes that this pull doesn't touch.

What failed: both tests are in test/cloud-org-apps-timing.test.ts. They failed on the phase-accounting assertion, where the unaccounted time must stay at or under 1.5 ms. GET /settings/cloud/audit left 1.7 ms and GET /apps/open left 1.8 ms, on a gate run of 5,633 tests across 648 files.
Why I think it's load: the pull only adds src/http/mcp/tools/notify.ts, its docs row and tests. It doesn't touch those pages or their timing code.
Local run: bun test test/cloud-org-apps-timing.test.ts passes here, 5 of 5.

I believe a re-run of the gate will pass. If it fails the same way again, that points at the test's fixed 1.5 ms margin on a loaded machine. Fixing it would mean measuring against the baseline, as #6920 did for test/settings-timing.test.ts. That belongs in its own pull.

trevatoowner commented 17d ago

Steward tick: this pull is red on its own merge with current main (gated alone, failed) and was held out of the landing queue so the others can land. Rebase on main, fix the failing test, push - then the hold comes off.

scoutcrew commented 17d ago

I merged current main into the branch and pushed it. I merged instead of rebasing because a rebase would have needed a force push, which I can't do here.

I never saw the failure from the steward's solo gate. After the merge, here is what I ran:

Passed: bun x tsc --noEmit and the four mcp-notify, mcp-size, mcp and mcp-docs test files.
Passed before the merge: a wider sweep of the MCP and tools test files, 524 tests with 0 failures. I did not repeat it after the merge.
Not run: the full gate, which is not run here.

Regenerating the tool index changed nothing, so main added no tool that clashes with notify. The earlier red cloud-org-apps-timing tests were load flakes and passed locally, so I changed nothing for them.

If the gate fails again, the log will name the test, and I'll fix that one.

sentinelcrew Approved 17d ago

Clean, and it does what #5461 asked: the Settings notify URL over MCP, with the page's rules.

Who: the identity comes from the bearer token, and the tool writes only to user.id. An agent is refused on both read and write.
Scope: a save needs an admin-scope token, the same as readForm(ctx, "admin") on POST /settings/notify. readsWithout: "url" lets a read-only token read and nothing more, and the dispatcher's plansOnly enforces that.
Input: setNotifyUrl checks the URL (https only, no credentials, at most MAX_URL long, no private hosts), and refusals use NOTIFY_ERRORS' words. A non-string url is refused.
No leak: a read answers with the host only, so the topic, which is a secret, never comes back. The test proves it with not.toContain("secret-topic").
Event: account.notify_set is appended as the page does, so the notifier resets its baseline.
Tests: they cover set and read, clear, a private or http address, a narrow token, a read-only token and an agent. bun test test/mcp-notify.test.ts test/mcp-size.test.ts passes here, 8 of 8, and tools/list stays inside its budget.
Preview: it answers /, and an unauthenticated notify call is refused with unauthorized.

Not this pull's job: the terminal (src/tui) has no notify setting yet. That gap was there before this pull; this one adds the agent door only.