From 5e72191cac9469c2ade91aaba1e62f69fa1ad94a Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Mon, 1 Jun 2026 09:28:21 +0900 Subject: feat(core): ntfy.sh notification dispatcher module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a transport-agnostic NotificationDispatcher and a fire-and-forget ntfy.sh transport (no SDK; just fetch). Configuration is persisted as a single global JSON blob under the 'ntfy_config' settings key. Event taxonomy (per-event toggles): - turn-completed — assistant turn finished cleanly - turn-error — final turn error (after all fallbacks) - permission-required — new permission prompt was created - agent-spawned — top-level user-agent tab spawned via 'summon' Design: - Single internal notify(event) interface so a future transport (email, webhook) plugs in without changing call sites. - attachToAgentManager + attachToPermissionManager subscribe to the existing event streams via narrow listener interfaces (no @dispatch/api dependency back into core). - 5s in-memory dedupe window on dedupeKey suppresses permission re-emits. - 10s per-request abort timeout so a hung ntfy server can't pin a worker. - All sends are fire-and-forget: void Promise.resolve(...).catch(warn). Tests (39 new): - ntfy transport: URL/headers/body/auth/click, header sanitization, per-event-type defaults, error paths. - config: defaults, normalization tolerance, round-trip, redaction. - dispatcher: master switch, per-event toggle, dedupe, agent/permission hookups, top-level-only filtering for agent-spawned, dispose. --- packages/core/tests/notifications/config.test.ts | 128 ++++++++ .../core/tests/notifications/dispatcher.test.ts | 323 +++++++++++++++++++++ packages/core/tests/notifications/ntfy.test.ts | 168 +++++++++++ 3 files changed, 619 insertions(+) create mode 100644 packages/core/tests/notifications/config.test.ts create mode 100644 packages/core/tests/notifications/dispatcher.test.ts create mode 100644 packages/core/tests/notifications/ntfy.test.ts (limited to 'packages/core/tests') diff --git a/packages/core/tests/notifications/config.test.ts b/packages/core/tests/notifications/config.test.ts new file mode 100644 index 0000000..64a9637 --- /dev/null +++ b/packages/core/tests/notifications/config.test.ts @@ -0,0 +1,128 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// In-memory fake for the settings table — mounted before the module under +// test is imported (vi.mock is hoisted). +const fakeSettings = new Map(); + +vi.mock("../../src/db/settings.js", () => ({ + getSetting: vi.fn((key: string) => fakeSettings.get(key) ?? null), + setSetting: vi.fn((key: string, value: string) => { + fakeSettings.set(key, value); + }), + deleteSetting: vi.fn((key: string) => { + fakeSettings.delete(key); + }), +})); + +const { + clearNtfyConfig, + defaultNtfyConfig, + loadNtfyConfig, + normalizeNtfyConfig, + NTFY_CONFIG_KEY, + redactNtfyConfig, + saveNtfyConfig, +} = await import("../../src/notifications/config.js"); + +describe("defaultNtfyConfig", () => { + it("disables notifications and ships sane per-event defaults", () => { + const cfg = defaultNtfyConfig(); + expect(cfg.enabled).toBe(false); + expect(cfg.topicUrl).toBe(""); + expect(cfg.authToken).toBe(""); + expect(cfg.events["turn-completed"]).toBe(true); + expect(cfg.events["turn-error"]).toBe(true); + expect(cfg.events["permission-required"]).toBe(true); + expect(cfg.events["agent-spawned"]).toBe(false); + }); +}); + +describe("normalizeNtfyConfig", () => { + it("returns defaults for non-object input", () => { + expect(normalizeNtfyConfig(null)).toEqual(defaultNtfyConfig()); + expect(normalizeNtfyConfig(undefined)).toEqual(defaultNtfyConfig()); + expect(normalizeNtfyConfig(42)).toEqual(defaultNtfyConfig()); + }); + + it("fills in missing event toggles with defaults (newly-added types default OFF)", () => { + const normalized = normalizeNtfyConfig({ + enabled: true, + topicUrl: "https://ntfy.sh/x", + events: { "turn-completed": false }, + }); + expect(normalized.events["turn-completed"]).toBe(false); + // Defaults preserved for fields the persisted blob doesn't have. + expect(normalized.events["turn-error"]).toBe(true); + expect(normalized.events["agent-spawned"]).toBe(false); + }); + + it("ignores extraneous fields and wrong-typed values", () => { + const normalized = normalizeNtfyConfig({ + enabled: "yes", // wrong type ⇒ default + topicUrl: 42, // wrong type ⇒ default + authToken: null, // wrong type ⇒ default + events: { "turn-completed": "no", bogus: true }, + extra: "ignored", + }); + expect(normalized.enabled).toBe(false); + expect(normalized.topicUrl).toBe(""); + expect(normalized.authToken).toBe(""); + expect(normalized.events["turn-completed"]).toBe(true); // default kept + expect((normalized.events as Record).bogus).toBeUndefined(); + }); +}); + +describe("load/save round-trip", () => { + beforeEach(() => { + fakeSettings.clear(); + }); + + it("returns defaults when nothing is persisted", () => { + expect(loadNtfyConfig()).toEqual(defaultNtfyConfig()); + }); + + it("round-trips a complete config", () => { + const cfg = { + enabled: true, + topicUrl: "https://ntfy.sh/team", + authToken: "tk_abc", + events: { + "turn-completed": false, + "turn-error": true, + "permission-required": true, + "agent-spawned": true, + }, + } as const; + saveNtfyConfig({ ...cfg }); + const loaded = loadNtfyConfig(); + expect(loaded).toEqual(cfg); + // Persisted as a JSON string under the documented key. + expect(fakeSettings.has(NTFY_CONFIG_KEY)).toBe(true); + }); + + it("returns defaults when stored JSON is corrupt", () => { + fakeSettings.set(NTFY_CONFIG_KEY, "{ not json"); + expect(loadNtfyConfig()).toEqual(defaultNtfyConfig()); + }); + + it("clearNtfyConfig removes the persisted entry", () => { + saveNtfyConfig({ ...defaultNtfyConfig(), enabled: true, topicUrl: "https://ntfy.sh/x" }); + expect(fakeSettings.has(NTFY_CONFIG_KEY)).toBe(true); + clearNtfyConfig(); + expect(fakeSettings.has(NTFY_CONFIG_KEY)).toBe(false); + }); +}); + +describe("redactNtfyConfig", () => { + it("strips authToken and surfaces a hasAuthToken flag", () => { + const cfg = { ...defaultNtfyConfig(), authToken: "tk_secret" }; + const redacted = redactNtfyConfig(cfg); + expect(redacted.authToken).toBe(""); + expect(redacted.hasAuthToken).toBe(true); + }); + + it("hasAuthToken is false for blank tokens", () => { + expect(redactNtfyConfig({ ...defaultNtfyConfig(), authToken: "" }).hasAuthToken).toBe(false); + expect(redactNtfyConfig({ ...defaultNtfyConfig(), authToken: " " }).hasAuthToken).toBe(false); + }); +}); diff --git a/packages/core/tests/notifications/dispatcher.test.ts b/packages/core/tests/notifications/dispatcher.test.ts new file mode 100644 index 0000000..db05de4 --- /dev/null +++ b/packages/core/tests/notifications/dispatcher.test.ts @@ -0,0 +1,323 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { NotificationEvent, NtfyConfig } from "../../src/notifications/types.js"; + +// The dispatcher imports `loadNtfyConfig` from config.ts, which transitively +// pulls in `db/index.js` (bun:sqlite). Stub the DB so vitest under Node can +// load this file. All tests inject `loadConfig` explicitly, so the real +// settings table is never read. +vi.mock("../../src/db/index.js", () => ({ + getDatabase: vi.fn(() => ({ + query: () => ({ get: () => null, run: () => {} }), + run: () => {}, + })), +})); + +const { NotificationDispatcher } = await import("../../src/notifications/dispatcher.js"); + +function makeConfig(overrides: Partial = {}): NtfyConfig { + return { + enabled: true, + topicUrl: "https://ntfy.sh/topic", + authToken: "", + events: { + "turn-completed": true, + "turn-error": true, + "permission-required": true, + "agent-spawned": true, + }, + ...overrides, + }; +} + +interface FakeAgentSource { + onEvent( + listener: (event: { type: string; tabId: string; [k: string]: unknown }) => void, + ): () => void; + emit(event: { type: string; tabId: string; [k: string]: unknown }): void; +} + +function makeAgentSource(): FakeAgentSource { + let l: ((event: { type: string; tabId: string; [k: string]: unknown }) => void) | null = null; + return { + onEvent(listener) { + l = listener; + return () => { + l = null; + }; + }, + emit(event) { + l?.(event); + }, + }; +} + +interface FakePermissionSource { + onPromptAdded( + listener: (prompt: { id: string; permission: string; description: string }) => void, + ): () => void; + emit(prompt: { id: string; permission: string; description: string }): void; +} + +function makePermissionSource(): FakePermissionSource { + let l: ((prompt: { id: string; permission: string; description: string }) => void) | null = null; + return { + onPromptAdded(listener) { + l = listener; + return () => { + l = null; + }; + }, + emit(p) { + l?.(p); + }, + }; +} + +// Microtask flush so the dispatcher's `void Promise.resolve(...).catch(...)` +// has a chance to settle before assertions. +async function flush(): Promise { + await Promise.resolve(); + await Promise.resolve(); +} + +describe("NotificationDispatcher.notify", () => { + let warnSpy: ReturnType; + beforeEach(() => { + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warnSpy.mockRestore(); + }); + + it("does not send when master switch is disabled", async () => { + const send = vi.fn(async () => ({ ok: true })); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ enabled: false }), + send, + }); + d.notify({ type: "turn-completed", title: "x", message: "y" }); + await flush(); + expect(send).not.toHaveBeenCalled(); + }); + + it("does not send when per-event-type toggle is off", async () => { + const send = vi.fn(async () => ({ ok: true })); + const d = new NotificationDispatcher({ + loadConfig: () => + makeConfig({ + events: { + "turn-completed": false, + "turn-error": true, + "permission-required": true, + "agent-spawned": false, + }, + }), + send, + }); + d.notify({ type: "turn-completed", title: "x", message: "y" }); + await flush(); + expect(send).not.toHaveBeenCalled(); + }); + + it("sends when enabled and toggle is on", async () => { + const send = vi.fn(async () => ({ ok: true })); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.notify({ type: "turn-completed", title: "x", message: "y" }); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + }); + + it("does not throw or block when the transport rejects", async () => { + const send = vi.fn(async () => { + throw new Error("boom"); + }); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + expect(() => d.notify({ type: "turn-completed", title: "x", message: "y" })).not.toThrow(); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + expect(warnSpy).toHaveBeenCalled(); + }); + + it("dedupes events with the same dedupeKey within the window", async () => { + const send = vi.fn(async () => ({ ok: true })); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig(), + send, + dedupeWindowMs: 1000, + }); + const event: NotificationEvent = { + type: "permission-required", + title: "p", + message: "p", + dedupeKey: "permission:42", + }; + d.notify(event); + d.notify(event); + d.notify(event); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + }); + + it("does not dedupe events without a dedupeKey", async () => { + const send = vi.fn(async () => ({ ok: true })); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.notify({ type: "turn-completed", title: "x", message: "y" }); + d.notify({ type: "turn-completed", title: "x", message: "y" }); + await flush(); + expect(send).toHaveBeenCalledTimes(2); + }); +}); + +describe("NotificationDispatcher.attachToAgentManager", () => { + let warnSpy: ReturnType; + beforeEach(() => { + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warnSpy.mockRestore(); + }); + + it("maps `done` → turn-completed (with tab title in the body)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig(), + send, + getTabTitle: (id) => (id === "tab-1" ? "My chat" : null), + }); + d.attachToAgentManager(source); + source.emit({ type: "done", tabId: "tab-1", message: { role: "assistant", chunks: [] } }); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + const event = send.mock.calls[0][1] as NotificationEvent; + expect(event.type).toBe("turn-completed"); + expect(event.title).toContain("My chat"); + expect(event.tabId).toBe("tab-1"); + }); + + it("maps `error` → turn-error and includes the error text", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.attachToAgentManager(source); + source.emit({ type: "error", tabId: "tab-1", error: "Rate limit", statusCode: 429 }); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + const event = send.mock.calls[0][1] as NotificationEvent; + expect(event.type).toBe("turn-error"); + expect(event.message).toContain("Rate limit"); + expect(event.message).toContain("429"); + }); + + it("ignores `status` events (would spam every transition)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.attachToAgentManager(source); + source.emit({ type: "status", tabId: "tab-1", status: "running" }); + source.emit({ type: "status", tabId: "tab-1", status: "idle" }); + await flush(); + expect(send).not.toHaveBeenCalled(); + }); + + it("maps `tab-created` to agent-spawned only for top-level user agents (parentTabId=null AND agentSlug set)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.attachToAgentManager(source); + + // Manual "new tab" with no agent slug ⇒ no notification. + source.emit({ + type: "tab-created", + tabId: "tab-1", + id: "tab-1", + title: "New Tab", + parentTabId: null, + agentSlug: null, + }); + // Subagent (has a parent) ⇒ no notification. + source.emit({ + type: "tab-created", + tabId: "tab-2", + id: "tab-2", + title: "Subagent", + parentTabId: "tab-1", + agentSlug: "researcher", + }); + // Top-level user agent ⇒ notify. + source.emit({ + type: "tab-created", + tabId: "tab-3", + id: "tab-3", + title: "Refactor auth code", + parentTabId: null, + agentSlug: "engineer", + }); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + const event = send.mock.calls[0][1] as NotificationEvent; + expect(event.type).toBe("agent-spawned"); + expect(event.message).toBe("Refactor auth code"); + expect(event.title).toContain("engineer"); + }); + + it("respects the per-event-type toggle (turn-completed off ⇒ silent)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => + makeConfig({ + events: { + "turn-completed": false, + "turn-error": true, + "permission-required": true, + "agent-spawned": false, + }, + }), + send, + }); + d.attachToAgentManager(source); + source.emit({ type: "done", tabId: "tab-1", message: { role: "assistant", chunks: [] } }); + await flush(); + expect(send).not.toHaveBeenCalled(); + }); +}); + +describe("NotificationDispatcher.attachToPermissionManager", () => { + let warnSpy: ReturnType; + beforeEach(() => { + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warnSpy.mockRestore(); + }); + + it("notifies once per unique prompt id (dedupes re-emits)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makePermissionSource(); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.attachToPermissionManager(source); + + source.emit({ id: "1", permission: "bash", description: "Run git status" }); + source.emit({ id: "1", permission: "bash", description: "Run git status" }); + source.emit({ id: "2", permission: "read", description: "Read /etc/hosts" }); + await flush(); + expect(send).toHaveBeenCalledTimes(2); + const events = send.mock.calls.map((c) => c[1] as NotificationEvent); + expect(events.map((e) => e.type)).toEqual(["permission-required", "permission-required"]); + expect(events.every((e) => e.dedupeKey?.startsWith("permission:"))).toBe(true); + }); +}); + +describe("NotificationDispatcher.dispose", () => { + it("releases attached subscriptions", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ loadConfig: () => makeConfig(), send }); + d.attachToAgentManager(source); + d.dispose(); + source.emit({ type: "done", tabId: "tab-1", message: { role: "assistant", chunks: [] } }); + await flush(); + expect(send).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/tests/notifications/ntfy.test.ts b/packages/core/tests/notifications/ntfy.test.ts new file mode 100644 index 0000000..3fb1d51 --- /dev/null +++ b/packages/core/tests/notifications/ntfy.test.ts @@ -0,0 +1,168 @@ +import { describe, expect, it, vi } from "vitest"; +import { sendNtfy, validateTopicUrl } from "../../src/notifications/ntfy.js"; +import type { NotificationEvent, NtfyConfig } from "../../src/notifications/types.js"; + +function makeConfig(overrides: Partial = {}): NtfyConfig { + return { + enabled: true, + topicUrl: "https://ntfy.sh/my-topic", + authToken: "", + events: { + "turn-completed": true, + "turn-error": true, + "permission-required": true, + "agent-spawned": true, + }, + ...overrides, + }; +} + +function makeEvent(overrides: Partial = {}): NotificationEvent { + return { + type: "turn-completed", + title: "Done", + message: "all good", + ...overrides, + }; +} + +function makeFetch( + response: Partial<{ ok: boolean; status: number; statusText: string; body: string }> = {}, +) { + const fetchImpl = vi.fn(async () => ({ + ok: response.ok ?? true, + status: response.status ?? 200, + statusText: response.statusText ?? "OK", + text: async () => response.body ?? "", + })); + return fetchImpl; +} + +describe("validateTopicUrl", () => { + it("accepts ntfy.sh-style URLs", () => { + expect(validateTopicUrl("https://ntfy.sh/my-topic")).toBeNull(); + expect(validateTopicUrl("http://ntfy.example.com/team-alerts")).toBeNull(); + }); + + it("rejects empty / whitespace", () => { + expect(validateTopicUrl("")).toMatch(/required/); + expect(validateTopicUrl(" ")).toMatch(/required/); + }); + + it("rejects malformed URLs", () => { + expect(validateTopicUrl("not a url")).toMatch(/valid URL/); + }); + + it("rejects non-http(s) schemes", () => { + expect(validateTopicUrl("ftp://ntfy.sh/topic")).toMatch(/http/); + }); + + it("rejects URLs missing a topic path", () => { + expect(validateTopicUrl("https://ntfy.sh")).toMatch(/topic/); + expect(validateTopicUrl("https://ntfy.sh/")).toMatch(/topic/); + }); +}); + +describe("sendNtfy", () => { + it("POSTs to the topic URL with Title/Priority/Tags/Content-Type headers and body", async () => { + const fetchImpl = makeFetch(); + const result = await sendNtfy( + makeConfig(), + makeEvent({ title: "Hello", message: "World", tags: ["bell"], priority: 4 }), + fetchImpl, + ); + expect(result.ok).toBe(true); + expect(fetchImpl).toHaveBeenCalledTimes(1); + const [url, init] = fetchImpl.mock.calls[0]; + expect(url).toBe("https://ntfy.sh/my-topic"); + expect(init.method).toBe("POST"); + expect(init.headers.Title).toBe("Hello"); + expect(init.headers.Priority).toBe("4"); + expect(init.headers.Tags).toBe("bell"); + expect(init.headers["Content-Type"]).toMatch(/text\/plain/); + expect(init.body).toBe("World"); + }); + + it("uses per-event-type defaults for priority and tags", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig(), makeEvent({ type: "turn-error" }), fetchImpl); + const init = fetchImpl.mock.calls[0][1]; + expect(init.headers.Priority).toBe("4"); // NTFY_DEFAULT_PRIORITIES["turn-error"] + expect(init.headers.Tags).toBe("rotating_light"); + }); + + it("attaches Authorization header when authToken is set", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig({ authToken: "tk_secret " }), makeEvent(), fetchImpl); + const init = fetchImpl.mock.calls[0][1]; + expect(init.headers.Authorization).toBe("Bearer tk_secret"); + }); + + it("omits Authorization when authToken is blank", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig({ authToken: " " }), makeEvent(), fetchImpl); + const init = fetchImpl.mock.calls[0][1]; + expect(init.headers.Authorization).toBeUndefined(); + }); + + it("attaches Click header when clickUrl is set", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig(), makeEvent({ clickUrl: "https://example.com/tab/abc" }), fetchImpl); + const init = fetchImpl.mock.calls[0][1]; + expect(init.headers.Click).toBe("https://example.com/tab/abc"); + }); + + it("appends short tab tag when tabId is set", async () => { + const fetchImpl = makeFetch(); + await sendNtfy( + makeConfig(), + makeEvent({ tabId: "abcdef0123456789", tags: ["bell"] }), + fetchImpl, + ); + const init = fetchImpl.mock.calls[0][1]; + expect(init.headers.Tags).toBe("bell,tab-abcdef01"); + }); + + it("strips CR/LF/control chars from header values (injection guard)", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig(), makeEvent({ title: "line1\r\nInjected: yes" }), fetchImpl); + const init = fetchImpl.mock.calls[0][1]; + expect(init.headers.Title).not.toContain("\n"); + expect(init.headers.Title).not.toContain("\r"); + expect(init.headers.Title).toBe("line1 Injected: yes"); + }); + + it("returns ok:false when notifications are disabled", async () => { + const fetchImpl = makeFetch(); + const result = await sendNtfy(makeConfig({ enabled: false }), makeEvent(), fetchImpl); + expect(result.ok).toBe(false); + expect(result.error).toMatch(/disabled/); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("returns ok:false on invalid topic URL without calling fetch", async () => { + const fetchImpl = makeFetch(); + const result = await sendNtfy(makeConfig({ topicUrl: "not a url" }), makeEvent(), fetchImpl); + expect(result.ok).toBe(false); + expect(result.error).toBeDefined(); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("returns ok:false with status on non-2xx response", async () => { + const fetchImpl = makeFetch({ ok: false, status: 403, statusText: "Forbidden", body: "nope" }); + const result = await sendNtfy(makeConfig(), makeEvent(), fetchImpl); + expect(result.ok).toBe(false); + expect(result.status).toBe(403); + expect(result.error).toMatch(/403/); + expect(result.error).toMatch(/nope/); + }); + + it("returns ok:false with error message on fetch throwing", async () => { + const fetchImpl = vi.fn(async () => { + throw new Error("ECONNREFUSED"); + }); + const result = await sendNtfy(makeConfig(), makeEvent(), fetchImpl); + expect(result.ok).toBe(false); + expect(result.error).toMatch(/ECONNREFUSED/); + }); +}); -- cgit v1.2.3 From 1870d0b86e797077b2a86c39d93fd34004b39e40 Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Mon, 1 Jun 2026 09:50:30 +0900 Subject: fix(notifications): address Gemini review — tighten validation, sanitize Click, support Basic auth, non-optimistic UI clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acted on 4 of 6 findings from the gemini-3-flash-preview second-opinion review (the other 2 were verified-wrong or judged not worth the complexity — see HANDOFF.md). core/src/notifications/ntfy.ts: - validateTopicUrl now enforces ntfy's actual topic-name constraints: exactly one path segment, 1–64 chars, charset [A-Za-z0-9_-]. Prevents users from saving topic URLs that look fine but silently 404 at publish time (cf. binwiederhier/ntfy#1451 for the 64-char limit and binwiederhier/ntfy's topic-name regex for the charset). - Click header now passes through sanitizeHeader, closing the same CRLF-injection vector that Title/Tags already had. - Authorization header construction now factors through a small buildAuthHeaderValue helper: a value that already starts with a scheme token ("Bearer xyz", "Basic dXNlcjpwYXNz") is used verbatim, so users of private ntfy servers that want Basic auth can paste the full header value. Bare tokens still get the "Bearer " prefix automatically. frontend/SettingsPanel.svelte: - clearNtfyAuthToken() was optimistic: it flipped hasAuthToken=false locally before awaiting the network call. If the request failed the UI lied about server state, and worse — a subsequent Save() with authToken:undefined would silently re-arm the original token. Now awaits the response, surfaces failures via the existing ntfySaveError banner, and only mutates local state on success. Adds a ntfyClearingToken loading flag so the button disables + spins during the request. Tests: +6 in ntfy.test.ts (multi-segment rejection, charset rejection, length boundary, 64-char acceptance, Basic auth pass-through, Click sanitization). All 442 tests pass; biome clean; svelte-check clean; manual ntfy.sh end-to-end re-verified. --- packages/core/src/notifications/ntfy.ts | 56 ++++++++++++++++----- packages/core/tests/notifications/ntfy.test.ts | 45 ++++++++++++++++- .../src/lib/components/SettingsPanel.svelte | 57 ++++++++++++++++------ 3 files changed, 130 insertions(+), 28 deletions(-) (limited to 'packages/core/tests') diff --git a/packages/core/src/notifications/ntfy.ts b/packages/core/src/notifications/ntfy.ts index 07ce33b..1f5101c 100644 --- a/packages/core/src/notifications/ntfy.ts +++ b/packages/core/src/notifications/ntfy.ts @@ -28,9 +28,17 @@ export type FetchLike = ( ) => Promise<{ ok: boolean; status: number; statusText?: string; text(): Promise }>; /** - * Validate a ntfy topic URL. Accepts only `http(s)://host/topic` with a - * non-empty topic path. Returns `null` on success, a human-readable error - * string on failure. + * ntfy topic-name rules: 1–64 chars, ASCII alphanumerics + `-` and `_`. Sourced + * from the ntfy server (cf. binwiederhier/ntfy issue #1451 — longer names + * silently 404). Matching this client-side keeps users from saving topic URLs + * that look fine but only fail at publish time. + */ +const NTFY_TOPIC_RE = /^[A-Za-z0-9_-]{1,64}$/; + +/** + * Validate a ntfy topic URL. Accepts only `http(s)://host/topic` where + * `topic` is a single path segment of 1–64 chars matching `[A-Za-z0-9_-]`. + * Returns `null` on success, a human-readable error string on failure. */ export function validateTopicUrl(topicUrl: string): string | null { const trimmed = topicUrl.trim(); @@ -44,9 +52,15 @@ export function validateTopicUrl(topicUrl: string): string | null { if (url.protocol !== "http:" && url.protocol !== "https:") { return "Topic URL must use http:// or https://"; } - // Path must be a non-empty topic (more than just "/") + // Path must be exactly one topic segment. const topic = url.pathname.replace(/^\/+|\/+$/g, ""); if (!topic) return "Topic URL must include a topic name (e.g. https://ntfy.sh/my-topic)"; + if (topic.includes("/")) { + return "Topic URL must point at a single topic (no extra path segments)"; + } + if (!NTFY_TOPIC_RE.test(topic)) { + return "Topic name must be 1–64 characters, letters/numbers/underscore/hyphen only"; + } return null; } @@ -79,17 +93,17 @@ export async function sendNtfy( } const headers: Record = { - // ntfy treats the title/priority/tags/click headers as ASCII-only. Strip - // control chars from the title; the body is sent UTF-8 verbatim. + // ntfy is tolerant of non-ASCII in the Title header but many proxies + // aren't — sanitizeHeader strips CR/LF/control chars (injection guard) + // and leaves UTF-8 in place. Body is sent verbatim as UTF-8. Title: sanitizeHeader(event.title), Priority: String(priority), "Content-Type": "text/plain; charset=utf-8", }; if (tags.length > 0) headers.Tags = tags.map(sanitizeHeader).join(","); - if (event.clickUrl) headers.Click = event.clickUrl; - if (config.authToken && config.authToken.trim() !== "") { - headers.Authorization = `Bearer ${config.authToken.trim()}`; - } + if (event.clickUrl) headers.Click = sanitizeHeader(event.clickUrl); + const authValue = buildAuthHeaderValue(config.authToken); + if (authValue) headers.Authorization = authValue; // Per-request abort so a hung server doesn't pin a Bun worker forever. const controller = new AbortController(); @@ -119,9 +133,27 @@ export async function sendNtfy( } } +/** + * Build the `Authorization` header value from the user-configured token. + * + * - Empty/blank ⇒ no header. + * - Already starts with `Bearer ` / `Basic ` / etc. (RFC 7235 scheme + space) + * ⇒ used verbatim. Lets users paste a complete header for Basic auth or + * any other scheme their private ntfy server supports. + * - Otherwise ⇒ prefixed with `Bearer ` (the common case). + */ +function buildAuthHeaderValue(rawToken: string): string | null { + const trimmed = (rawToken ?? "").trim(); + if (!trimmed) return null; + if (/^[A-Za-z][A-Za-z0-9._~+/-]*\s+\S/.test(trimmed)) { + // Already includes a scheme token (e.g. "Bearer xyz", "Basic dXNlcjpw"). + return sanitizeHeader(trimmed); + } + return `Bearer ${sanitizeHeader(trimmed)}`; +} + function sanitizeHeader(value: string): string { - // Strip CR/LF (header injection guard) and trim. ntfy is tolerant of - // non-ASCII in titles, but we still drop control chars. + // Strip CR/LF (header injection guard) and other control chars, then trim. // biome-ignore lint/suspicious/noControlCharactersInRegex: intentional return value.replace(/[\r\n\u0000-\u001f]+/g, " ").trim(); } diff --git a/packages/core/tests/notifications/ntfy.test.ts b/packages/core/tests/notifications/ntfy.test.ts index 3fb1d51..c183847 100644 --- a/packages/core/tests/notifications/ntfy.test.ts +++ b/packages/core/tests/notifications/ntfy.test.ts @@ -61,6 +61,27 @@ describe("validateTopicUrl", () => { expect(validateTopicUrl("https://ntfy.sh")).toMatch(/topic/); expect(validateTopicUrl("https://ntfy.sh/")).toMatch(/topic/); }); + + it("rejects multi-segment paths (topic must be a single segment)", () => { + expect(validateTopicUrl("https://ntfy.sh/team/sub")).toMatch(/single topic/); + }); + + it("rejects topic names with disallowed characters", () => { + expect(validateTopicUrl("https://ntfy.sh/has.dot")).toMatch(/letters\/numbers/); + expect(validateTopicUrl("https://ntfy.sh/has space")).toMatch(/letters\/numbers/); + expect(validateTopicUrl("https://ntfy.sh/has%20enc")).toMatch(/letters\/numbers/); + }); + + it("rejects topic names longer than 64 chars", () => { + const long = "a".repeat(65); + expect(validateTopicUrl(`https://ntfy.sh/${long}`)).toMatch(/64/); + }); + + it("accepts 64-char topic names and underscore/hyphen mixes", () => { + const maxlen = "a".repeat(64); + expect(validateTopicUrl(`https://ntfy.sh/${maxlen}`)).toBeNull(); + expect(validateTopicUrl("https://ntfy.sh/My_Topic-123")).toBeNull(); + }); }); describe("sendNtfy", () => { @@ -91,13 +112,23 @@ describe("sendNtfy", () => { expect(init.headers.Tags).toBe("rotating_light"); }); - it("attaches Authorization header when authToken is set", async () => { + it("attaches Authorization header with Bearer prefix when authToken is a bare token", async () => { const fetchImpl = makeFetch(); await sendNtfy(makeConfig({ authToken: "tk_secret " }), makeEvent(), fetchImpl); const init = fetchImpl.mock.calls[0][1]; expect(init.headers.Authorization).toBe("Bearer tk_secret"); }); + it("passes a pre-prefixed Authorization value (Basic, custom schemes) through verbatim", async () => { + const fetchImpl = makeFetch(); + await sendNtfy(makeConfig({ authToken: "Basic dXNlcjpwYXNz" }), makeEvent(), fetchImpl); + expect(fetchImpl.mock.calls[0][1].headers.Authorization).toBe("Basic dXNlcjpwYXNz"); + + const fetchImpl2 = makeFetch(); + await sendNtfy(makeConfig({ authToken: "Bearer already_prefixed" }), makeEvent(), fetchImpl2); + expect(fetchImpl2.mock.calls[0][1].headers.Authorization).toBe("Bearer already_prefixed"); + }); + it("omits Authorization when authToken is blank", async () => { const fetchImpl = makeFetch(); await sendNtfy(makeConfig({ authToken: " " }), makeEvent(), fetchImpl); @@ -112,6 +143,18 @@ describe("sendNtfy", () => { expect(init.headers.Click).toBe("https://example.com/tab/abc"); }); + it("sanitizes Click header (CR/LF injection guard)", async () => { + const fetchImpl = makeFetch(); + await sendNtfy( + makeConfig(), + makeEvent({ clickUrl: "https://example.com/\r\nInjected: yes" }), + fetchImpl, + ); + const v = fetchImpl.mock.calls[0][1].headers.Click; + expect(v).not.toContain("\n"); + expect(v).not.toContain("\r"); + }); + it("appends short tab tag when tabId is set", async () => { const fetchImpl = makeFetch(); await sendNtfy( diff --git a/packages/frontend/src/lib/components/SettingsPanel.svelte b/packages/frontend/src/lib/components/SettingsPanel.svelte index 1d9ebf7..8031500 100644 --- a/packages/frontend/src/lib/components/SettingsPanel.svelte +++ b/packages/frontend/src/lib/components/SettingsPanel.svelte @@ -72,6 +72,7 @@ let ntfySaveOk = $state(false); let ntfyTesting = $state(false); let ntfyTestResult = $state(null); let ntfyTestOk = $state(false); +let ntfyClearingToken = $state(false); function onChunkLimitChange(e: Event): void { const input = e.target as HTMLInputElement; @@ -212,22 +213,43 @@ async function sendNtfyTest(): Promise { } } -function clearNtfyAuthToken(): void { +async function clearNtfyAuthToken(): Promise { // `""` ⇒ explicit clear on save (vs. `undefined` which keeps existing). - ntfyAuthTokenInput = ""; - ntfy = { ...ntfy, hasAuthToken: false }; - // Send a save with explicit empty string to clear server-side. - void (async () => { - try { - await fetch(`${apiBase}/notifications`, { - method: "PUT", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ authToken: "" }), - }); - } catch { - // ignore + // Optimistic local state on failure caused a real bug pre-review: UI showed + // the token cleared while the server still held it, then "Save" treated the + // blank input as "keep existing" and silently re-armed the old token. Await + // the response and only flip local state on success. + ntfyClearingToken = true; + ntfySaveError = null; + try { + const res = await fetch(`${apiBase}/notifications`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ authToken: "" }), + }); + const data = (await res.json().catch(() => ({}))) as { + config?: NtfyConfigView; + error?: string; + }; + if (!res.ok) { + ntfySaveError = data.error ?? `Clear failed (HTTP ${res.status})`; + return; } - })(); + ntfyAuthTokenInput = ""; + if (data.config) { + ntfy = { + ...DEFAULT_NTFY, + ...data.config, + events: { ...DEFAULT_NTFY.events, ...(data.config.events ?? {}) }, + }; + } else { + ntfy = { ...ntfy, hasAuthToken: false }; + } + } catch (e) { + ntfySaveError = e instanceof Error ? e.message : "Network error"; + } finally { + ntfyClearingToken = false; + } } async function toggleAutoExpand(): Promise { @@ -421,9 +443,14 @@ $effect(() => { {/if} -- cgit v1.2.3 From 9c93086c0d4acaa1ed753488b12f72c2ca86a22c Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Mon, 1 Jun 2026 10:12:11 +0900 Subject: feat(notifications): add notifySubagents toggle to suppress subagent turn pings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A parent agent that spawns 8 subagents was producing 9 "Turn complete" notifications per round — almost always noise. New `notifySubagents` config flag (defaults to false) gates `turn-completed` and `turn-error` from any tab with a `parentTabId`. The flag is intentionally NOT applied to `permission-required` — a subagent's permission prompt still needs a human tap to proceed, so suppressing it would silently hang the subagent. `agent-spawned` is already top-level-only by construction. Wiring: - core/notifications/types.ts: NtfyConfig.notifySubagents: boolean - core/notifications/config.ts: defaults to false; normalize() tolerates missing / wrong-typed values and falls back to false - core/notifications/dispatcher.ts: new optional TabParentLookup option (getTabParentId). When notifySubagents=false AND the lookup returns a non-empty parent id string, turn-completed/turn-error are dropped. Lookup failures (no lookup configured, throws, returns undefined) fall back to "treat as top-level" so legitimate top-level events are never silently dropped when the DB is briefly unreadable. - api/app.ts: wires getTabParentId via core's getTab(id)?.parentTabId - frontend SettingsPanel.svelte: "Include subagent tabs" checkbox with an explanatory hint that permission prompts still fire Tests (+9): - 3 in config.test.ts: default-false, explicit-true, wrong-typed fallback - 6 in dispatcher.test.ts: suppression of turn-completed/turn-error from subagents, no suppression when flag is true, permission-required not gated, graceful fallback when lookup is missing/throws/returns undefined Live ntfy.sh round-trip re-verified (status: 200). --- packages/api/src/app.ts | 11 ++ packages/api/tests/routes.test.ts | 2 + packages/core/src/notifications/config.ts | 3 + packages/core/src/notifications/dispatcher.ts | 49 ++++++++ packages/core/src/notifications/index.ts | 1 + packages/core/src/notifications/types.ts | 8 ++ packages/core/tests/notifications/config.test.ts | 30 +++++ .../core/tests/notifications/dispatcher.test.ts | 138 +++++++++++++++++++++ .../src/lib/components/SettingsPanel.svelte | 17 +++ 9 files changed, 259 insertions(+) (limited to 'packages/core/tests') diff --git a/packages/api/src/app.ts b/packages/api/src/app.ts index 24cef24..0dabb0d 100644 --- a/packages/api/src/app.ts +++ b/packages/api/src/app.ts @@ -24,6 +24,17 @@ export const notificationDispatcher = new NotificationDispatcher({ return null; } }, + getTabParentId: (tabId) => { + try { + // `undefined` when the lookup fails (tab not found / DB unavailable) + // so the dispatcher falls back to "treat as top-level" rather than + // silently dropping notifications. + const row = getTab(tabId); + return row ? row.parentTabId : undefined; + } catch { + return undefined; + } + }, }); notificationDispatcher.attachToAgentManager(agentManager); notificationDispatcher.attachToPermissionManager(permissionManager); diff --git a/packages/api/tests/routes.test.ts b/packages/api/tests/routes.test.ts index c07f932..1fad690 100644 --- a/packages/api/tests/routes.test.ts +++ b/packages/api/tests/routes.test.ts @@ -290,6 +290,7 @@ vi.mock("@dispatch/core", () => ({ "permission-required": true, "agent-spawned": false, }, + notifySubagents: false, }; }, saveNtfyConfig() {}, @@ -307,6 +308,7 @@ vi.mock("@dispatch/core", () => ({ "permission-required": true, "agent-spawned": false, }, + notifySubagents: false, }; }, redactNtfyConfig(c: { authToken?: string }) { diff --git a/packages/core/src/notifications/config.ts b/packages/core/src/notifications/config.ts index 310c606..faa316e 100644 --- a/packages/core/src/notifications/config.ts +++ b/packages/core/src/notifications/config.ts @@ -17,6 +17,7 @@ export function defaultNtfyConfig(): NtfyConfig { topicUrl: "", authToken: "", events: { ...NTFY_DEFAULT_EVENTS }, + notifySubagents: false, }; } @@ -34,6 +35,8 @@ export function normalizeNtfyConfig(raw: unknown): NtfyConfig { topicUrl: typeof obj.topicUrl === "string" ? obj.topicUrl : base.topicUrl, authToken: typeof obj.authToken === "string" ? obj.authToken : base.authToken, events: { ...base.events }, + notifySubagents: + typeof obj.notifySubagents === "boolean" ? obj.notifySubagents : base.notifySubagents, }; const rawEvents = obj.events; if (rawEvents && typeof rawEvents === "object") { diff --git a/packages/core/src/notifications/dispatcher.ts b/packages/core/src/notifications/dispatcher.ts index 4f4fc79..01ce00c 100644 --- a/packages/core/src/notifications/dispatcher.ts +++ b/packages/core/src/notifications/dispatcher.ts @@ -31,6 +31,15 @@ export interface PermissionPromptSource { /** Look up a human-readable tab title for nicer notification text. */ export type TabTitleLookup = (tabId: string) => string | null; +/** + * Look up a tab's `parentTabId`. Returns `null` for top-level tabs (no + * parent) and `undefined` when the lookup can't be performed (no DB, tab + * not found). Both non-strings cause the dispatcher to fall back to + * "treat as top-level" to avoid silently dropping notifications when the + * lookup is broken. + */ +export type TabParentLookup = (tabId: string) => string | null | undefined; + export interface DispatcherOptions { /** Override the config loader (tests). Defaults to `loadNtfyConfig`. */ loadConfig?: () => NtfyConfig; @@ -40,6 +49,13 @@ export interface DispatcherOptions { fetchImpl?: FetchLike; /** Look up a tab title for richer titles. */ getTabTitle?: TabTitleLookup; + /** + * Look up a tab's `parentTabId`. Used to honour the + * `notifySubagents` config flag — when false, `turn-completed` / + * `turn-error` from subagent tabs (those with a parent) are + * suppressed. + */ + getTabParentId?: TabParentLookup; /** * How long (ms) a dedupeKey is suppressed for. Permission prompts re-emit * the whole pending list on every change, so dedupe is essential. @@ -51,6 +67,7 @@ export class NotificationDispatcher { private loadConfig: () => NtfyConfig; private send: (config: NtfyConfig, event: NotificationEvent) => Promise; private getTabTitle: TabTitleLookup | undefined; + private getTabParentId: TabParentLookup | undefined; private dedupeWindowMs: number; /** Recently-sent dedupeKey → expiresAt epoch ms. */ private recentlySent = new Map(); @@ -61,6 +78,7 @@ export class NotificationDispatcher { this.send = opts.send ?? ((config, event) => sendNtfy(config, event, opts.fetchImpl ?? undefined)); this.getTabTitle = opts.getTabTitle; + this.getTabParentId = opts.getTabParentId; this.dedupeWindowMs = opts.dedupeWindowMs ?? 5_000; } @@ -101,12 +119,22 @@ export class NotificationDispatcher { * * `status` events are ignored — they fire on every transition and we'd * either spam or duplicate the `done`/`error` notifications. + * + * Turn events from subagent tabs are suppressed when + * `config.notifySubagents === false` (the default). A parent agent + * spawning 8 subagents would otherwise produce 9 "Turn complete" + * pushes per round; almost always noise. Permission prompts are NOT + * gated this way — a subagent's permission request still needs human + * input to proceed, so suppressing those would silently hang the + * subagent. */ attachToAgentManager(source: AgentEventSource): () => void { const unsub = source.onEvent((event) => { if (event.type === "done") { + if (this.isSubagentSuppressed(event.tabId)) return; this.notify(this.buildTurnCompleted(event)); } else if (event.type === "error") { + if (this.isSubagentSuppressed(event.tabId)) return; this.notify(this.buildTurnError(event)); } else if (event.type === "tab-created") { const ev = event as unknown as { @@ -213,6 +241,27 @@ export class NotificationDispatcher { return `tab ${tabId.slice(0, 8)}`; } + /** + * Returns true when this `tabId` belongs to a subagent AND the user has + * opted out of subagent turn notifications. On lookup failure + * (`getTabParentId` returns `undefined` or throws) we err on the side + * of "not a subagent" — better to over-notify than to silently drop + * legitimate top-level events when the DB is briefly unreadable. + */ + private isSubagentSuppressed(tabId: string): boolean { + const config = this.loadConfig(); + if (config.notifySubagents) return false; + if (!this.getTabParentId) return false; + let parent: string | null | undefined; + try { + parent = this.getTabParentId(tabId); + } catch { + return false; + } + // Only a non-empty string parent id means "this tab is a subagent". + return typeof parent === "string" && parent.length > 0; + } + // ─── Dedupe helpers ─────────────────────────────────────────── private isDuplicate(key: string): boolean { diff --git a/packages/core/src/notifications/index.ts b/packages/core/src/notifications/index.ts index ea99a58..d9d934d 100644 --- a/packages/core/src/notifications/index.ts +++ b/packages/core/src/notifications/index.ts @@ -14,6 +14,7 @@ export { type DispatcherOptions, NotificationDispatcher, type PermissionPromptSource, + type TabParentLookup, type TabTitleLookup, } from "./dispatcher.js"; export { type FetchLike, type NtfySendResult, sendNtfy, validateTopicUrl } from "./ntfy.js"; diff --git a/packages/core/src/notifications/types.ts b/packages/core/src/notifications/types.ts index f6baa27..238cb68 100644 --- a/packages/core/src/notifications/types.ts +++ b/packages/core/src/notifications/types.ts @@ -57,12 +57,20 @@ export interface NotificationEvent { * - `authToken` — optional bearer token for private ntfy servers. * - `events` — per-event-type enable map. Missing entries default to OFF * so a newly-added event type doesn't silently start firing. + * - `notifySubagents` — when false (default), `turn-completed` and + * `turn-error` notifications from subagent tabs (tabs with a + * `parentTabId`) are suppressed. A parent agent that spawns 8 + * subagents would otherwise push 9 "Turn complete" notifications per + * round — usually noise. `permission-required` is NOT gated: even a + * subagent's permission prompt needs a human tap to proceed. + * `agent-spawned` is already top-level-only by construction. */ export interface NtfyConfig { enabled: boolean; topicUrl: string; authToken: string; events: Record; + notifySubagents: boolean; } /** All event types this build knows about (the source of truth for UI). */ diff --git a/packages/core/tests/notifications/config.test.ts b/packages/core/tests/notifications/config.test.ts index 64a9637..c110788 100644 --- a/packages/core/tests/notifications/config.test.ts +++ b/packages/core/tests/notifications/config.test.ts @@ -34,6 +34,7 @@ describe("defaultNtfyConfig", () => { expect(cfg.events["turn-error"]).toBe(true); expect(cfg.events["permission-required"]).toBe(true); expect(cfg.events["agent-spawned"]).toBe(false); + expect(cfg.notifySubagents).toBe(false); }); }); @@ -72,6 +73,34 @@ describe("normalizeNtfyConfig", () => { }); }); +describe("normalizeNtfyConfig — notifySubagents", () => { + it("defaults notifySubagents to false when absent", () => { + const normalized = normalizeNtfyConfig({ + enabled: true, + topicUrl: "https://ntfy.sh/x", + }); + expect(normalized.notifySubagents).toBe(false); + }); + + it("respects an explicit notifySubagents=true", () => { + const normalized = normalizeNtfyConfig({ + enabled: true, + topicUrl: "https://ntfy.sh/x", + notifySubagents: true, + }); + expect(normalized.notifySubagents).toBe(true); + }); + + it("falls back to default when notifySubagents is wrong-typed", () => { + const normalized = normalizeNtfyConfig({ + enabled: true, + topicUrl: "https://ntfy.sh/x", + notifySubagents: "yes" as unknown, + }); + expect(normalized.notifySubagents).toBe(false); + }); +}); + describe("load/save round-trip", () => { beforeEach(() => { fakeSettings.clear(); @@ -92,6 +121,7 @@ describe("load/save round-trip", () => { "permission-required": true, "agent-spawned": true, }, + notifySubagents: true, } as const; saveNtfyConfig({ ...cfg }); const loaded = loadNtfyConfig(); diff --git a/packages/core/tests/notifications/dispatcher.test.ts b/packages/core/tests/notifications/dispatcher.test.ts index db05de4..750552c 100644 --- a/packages/core/tests/notifications/dispatcher.test.ts +++ b/packages/core/tests/notifications/dispatcher.test.ts @@ -25,6 +25,10 @@ function makeConfig(overrides: Partial = {}): NtfyConfig { "permission-required": true, "agent-spawned": true, }, + // Default to true in the test config so existing tests (which never + // configure a getTabParentId lookup) keep firing for tab-1 / tab-2 / etc. + // Tests of the new subagent gating override this explicitly. + notifySubagents: true, ...overrides, }; } @@ -321,3 +325,137 @@ describe("NotificationDispatcher.dispose", () => { expect(send).not.toHaveBeenCalled(); }); }); + +describe("NotificationDispatcher subagent suppression (notifySubagents flag)", () => { + let warnSpy: ReturnType; + beforeEach(() => { + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warnSpy.mockRestore(); + }); + + const parents = new Map([ + ["top-level", null], + ["subagent", "top-level"], + ]); + const getTabParentId = (id: string): string | null | undefined => parents.get(id); + + it("suppresses turn-completed from subagent tabs when notifySubagents=false (default)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: false }), + send, + getTabParentId, + }); + d.attachToAgentManager(source); + + source.emit({ type: "done", tabId: "subagent", message: { role: "assistant", chunks: [] } }); + source.emit({ type: "done", tabId: "top-level", message: { role: "assistant", chunks: [] } }); + await flush(); + + expect(send).toHaveBeenCalledTimes(1); + expect((send.mock.calls[0][1] as NotificationEvent).tabId).toBe("top-level"); + }); + + it("suppresses turn-error from subagent tabs when notifySubagents=false", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: false }), + send, + getTabParentId, + }); + d.attachToAgentManager(source); + + source.emit({ type: "error", tabId: "subagent", error: "boom" }); + source.emit({ type: "error", tabId: "top-level", error: "boom" }); + await flush(); + + expect(send).toHaveBeenCalledTimes(1); + expect((send.mock.calls[0][1] as NotificationEvent).tabId).toBe("top-level"); + }); + + it("still notifies subagents when notifySubagents=true", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: true }), + send, + getTabParentId, + }); + d.attachToAgentManager(source); + + source.emit({ type: "done", tabId: "subagent", message: { role: "assistant", chunks: [] } }); + source.emit({ type: "done", tabId: "top-level", message: { role: "assistant", chunks: [] } }); + await flush(); + + expect(send).toHaveBeenCalledTimes(2); + }); + + it("does NOT gate permission-required (subagents must still get human input)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const psource = makePermissionSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: false }), + send, + getTabParentId, + }); + d.attachToPermissionManager(psource); + + psource.emit({ id: "p1", permission: "bash", description: "git status" }); + await flush(); + + expect(send).toHaveBeenCalledTimes(1); + expect((send.mock.calls[0][1] as NotificationEvent).type).toBe("permission-required"); + }); + + it("falls back to notifying when getTabParentId is not provided (treat as top-level)", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: false }), + send, + // intentionally NO getTabParentId + }); + d.attachToAgentManager(source); + + source.emit({ type: "done", tabId: "anything", message: { role: "assistant", chunks: [] } }); + await flush(); + + // Without a lookup, the dispatcher can't prove this is a subagent; it + // must err on the side of notifying so legitimate top-level events + // aren't silently dropped. + expect(send).toHaveBeenCalledTimes(1); + }); + + it("falls back to notifying when getTabParentId throws or returns undefined", async () => { + const send = vi.fn(async () => ({ ok: true })); + const source = makeAgentSource(); + const d = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: false }), + send, + getTabParentId: () => { + throw new Error("db unavailable"); + }, + }); + d.attachToAgentManager(source); + + source.emit({ type: "done", tabId: "x", message: { role: "assistant", chunks: [] } }); + await flush(); + expect(send).toHaveBeenCalledTimes(1); + + const send2 = vi.fn(async () => ({ ok: true })); + const source2 = makeAgentSource(); + const d2 = new NotificationDispatcher({ + loadConfig: () => makeConfig({ notifySubagents: false }), + send: send2, + getTabParentId: () => undefined, + }); + d2.attachToAgentManager(source2); + source2.emit({ type: "done", tabId: "x", message: { role: "assistant", chunks: [] } }); + await flush(); + expect(send2).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/frontend/src/lib/components/SettingsPanel.svelte b/packages/frontend/src/lib/components/SettingsPanel.svelte index 8031500..08e86ac 100644 --- a/packages/frontend/src/lib/components/SettingsPanel.svelte +++ b/packages/frontend/src/lib/components/SettingsPanel.svelte @@ -36,6 +36,7 @@ interface NtfyConfigView { authToken: string; hasAuthToken?: boolean; events: Record; + notifySubagents: boolean; } const NTFY_EVENT_LABELS: Record = { @@ -56,6 +57,7 @@ const DEFAULT_NTFY: NtfyConfigView = { "permission-required": true, "agent-spawned": false, }, + notifySubagents: false, }; let ntfy = $state({ ...DEFAULT_NTFY, events: { ...DEFAULT_NTFY.events } }); @@ -162,6 +164,7 @@ async function saveNtfy(): Promise { enabled: ntfy.enabled, topicUrl: ntfy.topicUrl, events: ntfy.events, + notifySubagents: ntfy.notifySubagents, }; if (ntfyAuthTokenInput !== "") payload.authToken = ntfyAuthTokenInput; const res = await fetch(`${apiBase}/notifications`, { @@ -469,6 +472,20 @@ $effect(() => { {/each} +
+ + + Off (default): turn-completed/turn-error from subagents are suppressed. Permission prompts still fire so subagents don't silently hang. + +
+