From 338f0e460d01a5463b357d9b9ff3e8c1d9886247 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 9 Aug 2026 16:03:27 +0200 Subject: [PATCH] feat(approvals): gate push Approve/Deny buttons on the opt-in setting too One switch now governs the whole feature: with approvalsInboxEnabled off (the default), sendPushNotifications strips the actions and approvalId from permission push payloads, so the buttons no longer render at all (pre-inbox they rendered and did nothing). The page-side action relay is gated the same way for stale notifications sent before the toggle flipped. Only the store and answer endpoints keep running, so enabling the toggle surfaces anything already pending immediately. sendPushNotifications is async now (cached settings read); all call sites were already fire-and-forget. Covered by three new payload tests alongside the existing hostTitle suite. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 2 +- docs/approvals-inbox-plan.md | 6 +- src/web/public/approvals-ui.js | 6 +- src/web/schemas.ts | 10 ++-- src/web/server.ts | 21 ++++++- test/push-payload-host-title.test.ts | 85 ++++++++++++++++++++++++---- 6 files changed, 106 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 09f17949..5b8961b7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -206,7 +206,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Hook events**: Claude Code hooks trigger via `/api/hook-event`. Key events: `permission_prompt`, `elicitation_dialog`, `elicitation_complete`, `elicitation_response`, `idle_prompt`, `stop`, `teammate_idle`, `task_completed`. See `src/hooks-config.ts`; upstream hook semantics mirrored in `docs/claude-code-hooks-reference.md`. -**Approvals Inbox** (cross-session queue of prompts waiting on a human; `approvalsInboxEnabled`, SYNCED, default OFF: every UI surface is opt-in, though the store/endpoints/push actions run regardless): `web/approval-inbox.ts` is a `sessionWaits`-style singleton fed by `/api/hook-event`, holding at most ONE item per session (a new prompt supersedes), claude-mode only, in-memory. Cards are answered via `POST /api/approvals/:id/answer`, which sends a digit / Esc / idle-prompt text through `writeViaMux` (menu answers never carry `\r`). ⚠️ `option` digits are accepted ONLY when they match options parsed from the captured pane frame, and the answer path RE-CAPTURES the pane first (a dialog that no longer parses on screen means the keystroke would land in the composer, so refuse with 409). ⚠️ Resolution on the heuristic `working` signal is restricted to `idle` items; permission/question items clear only on definitive signals (`stop`, `elicitation_complete`/`elicitation_response`, exit/delete, answer, supersede, 12h TTL). The frontend seeds from `GET /api/approvals` in `handleInit` (which is what makes tab alerts survive reloads), but only with the setting ON; push Approve/Deny actions are answered from `sw.js` directly so they work with no tab open, setting-independent. Surfaces (all gated on the setting): header bell (marker-hidden until count > 0, phones never show it) + drawer (`approvals-ui.js`), phone overview NEEDS YOU answer strips (`mobile-overview.js`). Design: `docs/approvals-inbox-plan.md`. +**Approvals Inbox** (cross-session queue of prompts waiting on a human; `approvalsInboxEnabled`, SYNCED, default OFF: every surface is opt-in; only the store and answer endpoints run regardless, so flipping it ON shows anything already pending): `web/approval-inbox.ts` is a `sessionWaits`-style singleton fed by `/api/hook-event`, holding at most ONE item per session (a new prompt supersedes), claude-mode only, in-memory. Cards are answered via `POST /api/approvals/:id/answer`, which sends a digit / Esc / idle-prompt text through `writeViaMux` (menu answers never carry `\r`). ⚠️ `option` digits are accepted ONLY when they match options parsed from the captured pane frame, and the answer path RE-CAPTURES the pane first (a dialog that no longer parses on screen means the keystroke would land in the composer, so refuse with 409). ⚠️ Resolution on the heuristic `working` signal is restricted to `idle` items; permission/question items clear only on definitive signals (`stop`, `elicitation_complete`/`elicitation_response`, exit/delete, answer, supersede, 12h TTL). The frontend seeds from `GET /api/approvals` in `handleInit` (which is what makes tab alerts survive reloads), but only with the setting ON; push Approve/Deny buttons are also gated on it (`sendPushNotifications` strips `actions`/`approvalId` when OFF) and are answered from `sw.js` directly so they work with no tab open. Surfaces (all gated on the setting): header bell (marker-hidden until count > 0, phones never show it) + drawer (`approvals-ui.js`), phone overview NEEDS YOU answer strips (`mobile-overview.js`). Design: `docs/approvals-inbox-plan.md`. **Agent Teams**: `TeamWatcher` polls `~/.claude/teams/`, matches to sessions via `leadSessionId`. Teammates are in-process threads appearing as subagents. Enable: `CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1`. See `docs/agent-teams/`. diff --git a/docs/approvals-inbox-plan.md b/docs/approvals-inbox-plan.md index 02602f4b..7efa96db 100644 --- a/docs/approvals-inbox-plan.md +++ b/docs/approvals-inbox-plan.md @@ -75,9 +75,9 @@ Normal authed API (NOT the hook-secret bypass), `ApiResponse` envelope, Zod sche ### Push -- `sendPushNotifications` payload gains `approvalId` for the three hook events. +- `sendPushNotifications` payload gains `approvalId` for the three hook events. Both `approvalId` and the Approve/Deny `actions` are **gated on the opt-in setting**: with it off, permission pushes carry no buttons at all (pre-inbox they rendered and did nothing, so stripping them is the honest shape). - `sw.js` `notificationclick`: when `event.action` is `approve`/`deny`, POST `/api/approvals/:id/answer` directly from the worker (same-origin, cookie credentials) so the buttons work **with no tab open**; on failure fall back to focusing/opening a tab. Non-action clicks keep today's behavior. -- Page-side `notification-click` handler: honor `action` instead of dropping it. +- Page-side `notification-click` handler: honor `action` instead of dropping it (also setting-gated, for stale notifications sent before the toggle flipped). - Question/idle pushes keep no action buttons (options vary per dialog); tapping opens the inbox. ## Frontend @@ -88,7 +88,7 @@ New module `approvals-ui.js` (@loadorder 11.2, after panels-ui.js), prettier-for - **Desktop**: header bell `btn-approvals` with count badge. Ships default-hidden via marker class `btn-approvals--hidden` (same policy as the attachments button, so `test/mobile-header-buttons-policy.test.ts` excludes it from the default-visible enumeration); JS shows it only while count > 0. Click toggles a drawer of cards: session name + kind, tool/message summary, mono context block, buttons rendered from parsed options (else Approve/Deny), plus Dismiss and Open session. Esc closes; existing z-index layers respected. - **Phone**: header button stays hidden (`mobile.css`); the phone surface is the overview's NEEDS YOU section, whose rows gain inline ✓/✗ buttons for permission items (tap-through to the session remains the row's main action). Toolbar classes/status language rules from the mobile-overview section of CLAUDE.md apply. - **i18n**: new strings registered in i18n.js (en + zh-CN); status words carry `data-i18n-skip` where they would collide (mirroring the overview pills). -- **Setting**: `approvalsInboxEnabled`, synced (in `SettingsUpdateSchema`), **default OFF** (owner decision: every UI surface is opt-in, meaning no bell, no drawer, no overview strips, no seeding until enabled in App Settings → Panels). The store and answer endpoints keep running regardless, so the push Approve/Deny actions work either way (they are already opt-in per-subscription via push preferences). +- **Setting**: `approvalsInboxEnabled`, synced (in `SettingsUpdateSchema`), **default OFF** (owner decision: the entire feature is opt-in, meaning no bell, no drawer, no overview strips, no seeding, and no push action buttons until enabled in App Settings → Panels). Only the store and answer endpoints keep running regardless, so flipping the toggle ON surfaces anything already pending immediately, with no restart. ## Race honesty diff --git a/src/web/public/approvals-ui.js b/src/web/public/approvals-ui.js index 97c4a5c0..066ad17e 100644 --- a/src/web/public/approvals-ui.js +++ b/src/web/public/approvals-ui.js @@ -126,10 +126,12 @@ Object.assign(CodemanApp.prototype, { /** * Push-notification action relay (sw.js → settings-ui notification-click → - * here). Falls back to opening the session when the item is unknown. + * here). Falls back to opening the session when the item is unknown, or + * when the inbox is disabled (a stale notification from before the toggle + * flipped can still carry an action). */ handleNotificationAction(action, approvalId, sessionId) { - if ((action === 'approve' || action === 'deny') && approvalId) { + if ((action === 'approve' || action === 'deny') && approvalId && this.approvalsInboxEnabled()) { this.answerApproval(approvalId, action); return; } diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 5bbc3239..675f9d0f 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -791,10 +791,12 @@ export const SettingsUpdateSchema = z */ agentSkillEnabled: z.boolean().optional(), /** - * Approvals Inbox UI (header bell + drawer, phone overview answer buttons). - * SYNCED, default OFF (opt-in): even with items pending, no surface renders - * until this is enabled. The server-side store and answer endpoints run - * regardless, so push Approve/Deny actions keep working either way. + * Approvals Inbox (header bell + drawer, phone overview answer buttons, + * push Approve/Deny action buttons). SYNCED, default OFF (opt-in): even + * with items pending, no surface renders and push payloads carry no + * actions/approvalId until this is enabled. The server-side store and the + * answer endpoints run regardless, so flipping it ON shows anything + * already pending immediately. */ approvalsInboxEnabled: z.boolean().optional(), tunnelEnabled: z.boolean().optional(), diff --git a/src/web/server.ts b/src/web/server.ts index 3edc1b2f..b46d06ea 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -2104,13 +2104,27 @@ export class WebServer extends EventEmitter { * Only events in PUSH_EVENT_MAP trigger push. Per-subscription preferences are checked. * Expired subscriptions (410/404) are auto-removed. */ - private sendPushNotifications(event: string, data: Record): void { + // Async only for the Approvals Inbox settings read below; every call site is + // fire-and-forget (the EventPort signature stays `void`). + private async sendPushNotifications(event: string, data: Record): Promise { const template = WebServer.PUSH_EVENT_MAP[event]; if (!template) return; const subscriptions = this.pushStore.getAll(); if (subscriptions.length === 0) return; + // Approvals Inbox gating: the Approve/Deny action buttons answer through + // the inbox, so both the buttons and the approvalId they act on ship only + // when the OPT-IN `approvalsInboxEnabled` setting is on (default OFF). + // Pre-inbox these buttons rendered and did nothing; stripping them when + // the feature is off is the honest shape. Cheap: the settings read is + // cached (~2s TTL) and only taken for events that carry approval parts. + let approvalsEnabled = false; + if (template.actions || typeof data.approvalId === 'string') { + const settings = await this.readSettings(); + approvalsEnabled = settings.approvalsInboxEnabled === true; + } + const vapidKeys = this.pushStore.getVapidKeys(); webpush.setVapidDetails('mailto:codeman@localhost', vapidKeys.publicKey, vapidKeys.privateKey); @@ -2154,9 +2168,10 @@ export class WebServer extends EventEmitter { sessionId, // Approvals Inbox item id: lets sw.js answer an Approve/Deny action // click directly (POST /api/approvals/:id/answer) with no tab open. - approvalId: typeof data.approvalId === 'string' ? data.approvalId : undefined, + // Gated on the opt-in setting together with the action buttons. + approvalId: approvalsEnabled && typeof data.approvalId === 'string' ? data.approvalId : undefined, urgency: template.urgency, - actions: template.actions, + actions: approvalsEnabled ? template.actions : undefined, }); for (const sub of subscriptions) { diff --git a/test/push-payload-host-title.test.ts b/test/push-payload-host-title.test.ts index b5c39d85..3bf8c176 100644 --- a/test/push-payload-host-title.test.ts +++ b/test/push-payload-host-title.test.ts @@ -76,11 +76,11 @@ describe('push payload hostTitle (Web Push hostname plumbing)', () => { setVapidDetails.mockClear(); }); - it('includes hostTitle = codeman: in the payload', () => { + it('includes hostTitle = codeman: in the payload', async () => { const server = makeServerWithHost('laptop'); - ( + await ( server as unknown as { - sendPushNotifications: (e: string, d: Record) => void; + sendPushNotifications: (e: string, d: Record) => Promise; } ).sendPushNotifications('hook:idle_prompt', { sessionId: 's-1', @@ -93,11 +93,11 @@ describe('push payload hostTitle (Web Push hostname plumbing)', () => { expect(payload.title).toBe('Waiting for Input'); }); - it('falls back to os.hostname() when --title-hostname is not provided', () => { + it('falls back to os.hostname() when --title-hostname is not provided', async () => { const server = makeServerWithHost(''); // empty -> constructor uses getHostname() - ( + await ( server as unknown as { - sendPushNotifications: (e: string, d: Record) => void; + sendPushNotifications: (e: string, d: Record) => Promise; } ).sendPushNotifications('hook:permission_prompt', { sessionId: 's-2', @@ -111,18 +111,18 @@ describe('push payload hostTitle (Web Push hostname plumbing)', () => { expect(payload.title).toBe('Permission Required'); }); - it('different WebServer instances ship distinct hostTitles', () => { + it('different WebServer instances ship distinct hostTitles', async () => { const a = makeServerWithHost('host-a'); const b = makeServerWithHost('host-b'); - ( + await ( a as unknown as { - sendPushNotifications: (e: string, d: Record) => void; + sendPushNotifications: (e: string, d: Record) => Promise; } ).sendPushNotifications('hook:stop', { sessionId: 's-a', sessionName: 'A' }); - ( + await ( b as unknown as { - sendPushNotifications: (e: string, d: Record) => void; + sendPushNotifications: (e: string, d: Record) => Promise; } ).sendPushNotifications('hook:stop', { sessionId: 's-b', sessionName: 'B' }); @@ -164,3 +164,66 @@ describe('service worker displayTitle composition (mirrors sw.js)', () => { expect(computeSwDisplayTitle({})).toBe('Codeman'); }); }); + +// ─── Approvals Inbox gating ────────────────────────────────────────────── +// The Approve/Deny action buttons answer through the Approvals Inbox, so the +// payload ships them (and the approvalId they act on) only when the OPT-IN +// `approvalsInboxEnabled` setting is on. Pre-inbox these buttons rendered and +// did nothing; with the feature off they must not render at all. + +interface ApprovalAwarePayload extends PushPayload { + approvalId?: string; +} + +function setSettings(server: WebServer, settings: Record): void { + (server as unknown as { readSettings: () => Promise> }).readSettings = async () => settings; +} + +async function sendPermissionPush(server: WebServer): Promise { + await ( + server as unknown as { + sendPushNotifications: (e: string, d: Record) => Promise; + } + ).sendPushNotifications('hook:permission_prompt', { + sessionId: 's-gate', + sessionName: 'sess', + tool_name: 'Bash', + approvalId: 's-gate:1', + }); + return lastPayload() as ApprovalAwarePayload; +} + +describe('push payload Approvals Inbox gating', () => { + beforeEach(() => { + sendNotification.mockClear(); + }); + + it('strips actions and approvalId when the setting is off (the default)', async () => { + const server = makeServerWithHost('gate-off'); + setSettings(server, {}); + const payload = await sendPermissionPush(server); + expect(payload.actions).toBeUndefined(); + expect(payload.approvalId).toBeUndefined(); + // The notification itself still goes out; only the inbox parts are gated. + expect(payload.title).toBe('Permission Required'); + }); + + it('ships Approve/Deny actions and the approvalId when the setting is on', async () => { + const server = makeServerWithHost('gate-on'); + setSettings(server, { approvalsInboxEnabled: true }); + const payload = await sendPermissionPush(server); + expect(payload.actions).toEqual([ + { action: 'approve', title: 'Approve' }, + { action: 'deny', title: 'Deny' }, + ]); + expect(payload.approvalId).toBe('s-gate:1'); + }); + + it('an explicit false behaves like the default (only true enables)', async () => { + const server = makeServerWithHost('gate-false'); + setSettings(server, { approvalsInboxEnabled: false }); + const payload = await sendPermissionPush(server); + expect(payload.actions).toBeUndefined(); + expect(payload.approvalId).toBeUndefined(); + }); +});