From f5e59283f94fd4bb78f2748a5d2cb0b76b57ef80 Mon Sep 17 00:00:00 2001 From: DDT <141296842+Hugo-DDT@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:16:16 +0800 Subject: [PATCH] fix(approvals): treat DeepSeek permission prompts as terminal-only alerts Supervised-hook CLIs (DeepSeek Harness) raise Approvals Inbox items whose answer route refuses keystrokes, but push notifications and the mobile/desktop UI still offered Approve/Deny. Mark those items answerInTerminal, omit approvalId (and therefore push actions) for supervised hooks, and show "Answer in the terminal" / Open session instead of broken buttons. Fixes #598 (functional part; docs gaps left for a follow-up). --- src/web/approval-inbox.ts | 13 ++++++ src/web/public/approvals-ui.js | 5 +++ src/web/public/mobile-overview.js | 13 +++++- src/web/routes/approval-routes.ts | 14 +++---- src/web/routes/hook-event-routes.ts | 9 +++- src/web/server.ts | 6 ++- test/cli-registry-no-id-branching.test.ts | 12 ------ test/push-payload-host-title.test.ts | 21 ++++++++++ test/routes/approval-routes.test.ts | 50 +++++++++++++++++++++++ 9 files changed, 119 insertions(+), 24 deletions(-) diff --git a/src/web/approval-inbox.ts b/src/web/approval-inbox.ts index 4330f42c..8326e500 100644 --- a/src/web/approval-inbox.ts +++ b/src/web/approval-inbox.ts @@ -85,6 +85,13 @@ export interface ApprovalItem { * answer endpoint accepts; absent → only approve('1')/deny(Esc) are allowed. */ options?: ApprovalOption[]; + /** + * When true the dialog belongs to a third-party TUI whose keystrokes Codeman + * cannot verify (today: DeepSeek Harness / `hooks: 'supervised'`). The item + * is an ALERT only: the answer route refuses keystrokes, push omits Approve/ + * Deny actions, and the UI offers "Open session" instead of answer buttons. + */ + answerInTerminal?: boolean; } export interface ApprovalResolvedInfo { @@ -108,6 +115,11 @@ interface NotePromptArgs { * opens ALREADY acknowledged: see `notePrompt()`. */ watching?: string | null; + /** + * Alert-only item: answer in the terminal, not via Approve/Deny. See + * `ApprovalItem.answerInTerminal`. + */ + answerInTerminal?: boolean; /** Returns the raw (ANSI-bearing) pane frame, or null when unavailable. */ capture?: () => string | null; } @@ -261,6 +273,7 @@ export class ApprovalInbox { toolSummary: args.toolSummary, message: args.message, cwd: args.cwd, + ...(args.answerInTerminal ? { answerInTerminal: true } : {}), }; if (args.kind === 'idle' && args.watching) { item.acknowledgedAt = item.createdAt; diff --git a/src/web/public/approvals-ui.js b/src/web/public/approvals-ui.js index a51a82f1..6769b00b 100644 --- a/src/web/public/approvals-ui.js +++ b/src/web/public/approvals-ui.js @@ -261,6 +261,11 @@ Object.assign(CodemanApp.prototype, { `onkeydown="if(event.key==='Enter')app.answerApprovalIdleText('${id}')">` + `` + ``; + } else if (item.answerInTerminal) { + // Supervised-hook CLIs (DeepSeek Harness): keystrokes are unverifiable, so + // Approve/Deny would hit the answer route and fail. Offer only Open session. + actions = + `Answer in the terminal`; } else if (item.options && item.options.length) { actions = item.options .map( diff --git a/src/web/public/mobile-overview.js b/src/web/public/mobile-overview.js index 764697a6..cb313fc0 100644 --- a/src/web/public/mobile-overview.js +++ b/src/web/public/mobile-overview.js @@ -794,7 +794,7 @@ Object.assign(CodemanApp.prototype, { const wrap = document.createElement('div'); wrap.className = 'mobile-overview-row-wrap'; wrap.appendChild(item); - wrap.appendChild(this._buildMobileOverviewApprovalStrip(approval)); + wrap.appendChild(this._buildMobileOverviewApprovalStrip(approval, row)); return wrap; } @@ -949,7 +949,7 @@ Object.assign(CodemanApp.prototype, { }, /** Compact answer buttons for a NEEDS YOU row: parsed options, else Approve/Deny. */ - _buildMobileOverviewApprovalStrip(approval) { + _buildMobileOverviewApprovalStrip(approval, row) { const strip = document.createElement('div'); strip.className = 'mobile-overview-approval-strip'; strip.setAttribute('data-i18n-skip', ''); @@ -964,6 +964,15 @@ Object.assign(CodemanApp.prototype, { }); strip.appendChild(btn); }; + // Alert-only (DeepSeek Harness / supervised hooks): open the session instead + // of Approve/Deny that the answer route would refuse. + if (approval.answerInTerminal) { + addBtn('Answer in terminal', 'primary', () => { + if (typeof this.selectSession === 'function') this.selectSession(approval.sessionId); + else if (typeof this.openApprovalSession === 'function') this.openApprovalSession(approval.id); + }); + return strip; + } if (approval.options && approval.options.length) { for (const o of approval.options) { const label = o.label.length > 24 ? o.label.slice(0, 24) + '…' : o.label; diff --git a/src/web/routes/approval-routes.ts b/src/web/routes/approval-routes.ts index 4f1b127d..283e1a80 100644 --- a/src/web/routes/approval-routes.ts +++ b/src/web/routes/approval-routes.ts @@ -102,14 +102,12 @@ export function registerApprovalRoutes(app: FastifyInstance, ctx: SessionPort): if (!hooksAvailableForMode(session.mode, sessionHookOptions(session))) { return createErrorResponse(ApiErrorCode.CONFLICT, 'Session mode cannot have pending approvals'); } - // A dsh approval is an ALERT, not an answerable card: the dialog belongs to - // a third-party TUI whose keystroke contract Codeman has not measured, the - // Claude-shaped option parser never reads options off its frames, and - // verifyStillAnswerable() can therefore never be conclusive for it — so the - // '1'/Esc below would be a blind keystroke into a foreign composer. The item - // still raises the red alert and clears on the harness's own working/stop - // reports; answering happens in the terminal. - if (session.mode === 'deepseek') { + // Alert-only items (supervised-hook CLIs such as DeepSeek Harness): the + // dialog belongs to a third-party TUI whose keystroke contract Codeman has + // not measured, so '1'/Esc would be a blind keystroke into a foreign + // composer. The item still raises the red alert and clears on the harness's + // own working/stop reports; answering happens in the terminal. + if (item.answerInTerminal) { return createErrorResponse( ApiErrorCode.INVALID_INPUT, 'DeepSeek Harness approvals must be answered in the terminal: the dialog belongs to a third-party TUI whose keystrokes Codeman cannot verify.' diff --git a/src/web/routes/hook-event-routes.ts b/src/web/routes/hook-event-routes.ts index e4d8cd65..2cc594d7 100644 --- a/src/web/routes/hook-event-routes.ts +++ b/src/web/routes/hook-event-routes.ts @@ -14,6 +14,7 @@ import { sanitizeHookData, parseBody } from '../route-helpers.js'; import { persistDockerCaseClaudeSessionId } from '../../docker-hosts.js'; import { getDataDir } from '../../config/instance.js'; import { sessionWaits, hooksAvailableForMode, sessionHookOptions } from '../session-wait-registry.js'; +import { getCli } from '../../config/cli-registry/registry.js'; import { approvalInbox, type ApprovalKind } from '../approval-inbox.js'; import type { SessionPort, EventPort, RespawnPort, ConfigPort, InfraPort } from '../ports/index.js'; @@ -200,6 +201,10 @@ export function registerHookEventRoutes( toolSummary: typeof toolSummary === 'string' ? toolSummary : undefined, message: typeof safeData.message === 'string' ? safeData.message : undefined, cwd: typeof safeData.cwd === 'string' ? safeData.cwd : undefined, + // Supervised-hook CLIs (DeepSeek Harness): dialog keystrokes are unverifiable, + // so the item is an alert — answer in the terminal. Capability-gated, not by + // mode id, so a future supervised CLI inherits the same shape. + answerInTerminal: getCli(session.mode)?.capabilities.hooks === 'supervised', // What the pane says is still running in the background. An idle prompt from a // session that is watching its own work opens acknowledged, so it never arms an // alert nobody can answer; notePrompt() carries the whole reasoning. @@ -244,7 +249,9 @@ export function registerHookEventRoutes( sessionId, sessionName, ...safeData, - ...(approvalId && session?.mode !== 'deepseek' && { approvalId }), + // Supervised-hook CLIs answer in the terminal: omit approvalId so push + // cannot offer Approve/Deny that the answer route would refuse. + ...(approvalId && session && getCli(session.mode)?.capabilities.hooks !== 'supervised' && { approvalId }), }); } diff --git a/src/web/server.ts b/src/web/server.ts index bb159e16..28fabda1 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -2742,9 +2742,13 @@ export class WebServer extends EventEmitter { // Approvals Inbox item id: lets sw.js answer an Approve/Deny action // click directly (POST /api/approvals/:id/answer) with no tab open. // Gated on the opt-in setting together with the action buttons. + // Approve/Deny only when we also have an approvalId to answer with. + // Supervised-hook CLIs (DeepSeek) omit approvalId upstream so this also + // strips their action buttons — buttons without an id can never answer. approvalId: approvalsEnabled && typeof data.approvalId === 'string' ? data.approvalId : undefined, urgency: template.urgency, - actions: approvalsEnabled ? template.actions : undefined, + actions: + approvalsEnabled && typeof data.approvalId === 'string' ? template.actions : undefined, }); for (const sub of subscriptions) { diff --git a/test/cli-registry-no-id-branching.test.ts b/test/cli-registry-no-id-branching.test.ts index e35faeca..355a3bec 100644 --- a/test/cli-registry-no-id-branching.test.ts +++ b/test/cli-registry-no-id-branching.test.ts @@ -124,10 +124,6 @@ const ALLOWED_BRANCHES: Record = { 1, 'an error message explaining why THIS mode in particular will never deliver a stop signal' ), - "web/routes/approval-routes.ts::mode === 'deepseek'": allow( - 1, - 'the DeepSeek status bridge is the only non-claude source of approval items' - ), "cron/cron-service.ts::mode === 'claude'": allow(1, 'cron launch handling, not yet generalised'), "cron/cron-service.ts::mode === 'shell'": allow(1, 'cron launch handling, not yet generalised'), "web/routes/session-routes.ts::mode === 'claude'": allow( @@ -169,14 +165,6 @@ const ALLOWED_BRANCHES: Record = { ), "tui/tui-render.ts::mode !== 'claude'": allow(1, 'cosmetic: suppress the mode badge for the default mode'), - // Push approve/deny BUTTONS are withheld for dsh because the answer route refuses - // keystrokes for its dialogs (third-party TUI, unmeasured contract) — a button whose - // answer would be refused is worse than none. Arguably wants an "answerable dialogs" - // capability; deliberately not invented here. - "web/routes/hook-event-routes.ts::mode !== 'deepseek'": allow( - 1, - 'push buttons withheld where the answer route refuses keystrokes' - ), // Legacy Config plumbing, same category as the `===` entries above. "web/routes/session-routes.ts::mode !== 'omp'": allow( diff --git a/test/push-payload-host-title.test.ts b/test/push-payload-host-title.test.ts index 3bf8c176..84c048b5 100644 --- a/test/push-payload-host-title.test.ts +++ b/test/push-payload-host-title.test.ts @@ -219,6 +219,27 @@ describe('push payload Approvals Inbox gating', () => { expect(payload.approvalId).toBe('s-gate:1'); }); + + it('ships no Approve/Deny actions when the inbox is on but approvalId is absent', async () => { + // Supervised-hook CLIs omit approvalId upstream so buttons cannot answer. + const server = makeServerWithHost('gate-no-id'); + setSettings(server, { approvalsInboxEnabled: true }); + await ( + server as unknown as { + sendPushNotifications: (e: string, d: Record) => Promise; + } + ).sendPushNotifications('hook:permission_prompt', { + sessionId: 's-gate', + sessionName: 'sess', + tool_name: 'Bash', + // deliberately no approvalId + }); + const payload = lastPayload() as ApprovalAwarePayload; + expect(payload.actions).toBeUndefined(); + expect(payload.approvalId).toBeUndefined(); + expect(payload.title).toBe('Permission Required'); + }); + it('an explicit false behaves like the default (only true enables)', async () => { const server = makeServerWithHost('gate-false'); setSettings(server, { approvalsInboxEnabled: false }); diff --git a/test/routes/approval-routes.test.ts b/test/routes/approval-routes.test.ts index 439459a0..2e3acb89 100644 --- a/test/routes/approval-routes.test.ts +++ b/test/routes/approval-routes.test.ts @@ -455,6 +455,56 @@ describe('approval routes', () => { }); }); + +describe('approval routes: supervised-hook (DeepSeek) alert-only items', () => { + let harness: RouteTestHarness; + let session: MockSession; + + beforeEach(async () => { + harness = await createEnvelopeHarness(); + session = harness.ctx.sessions.get(SESSION_ID)!; + session.mode = 'deepseek'; + session.terminalBuffer = 'some dsh dialog frame'; + }); + + afterEach(async () => { + for (const item of approvalInbox.listPending()) { + approvalInbox.resolveForSession(item.sessionId, 'dismissed'); + } + approvalInbox.onPending = approvalInbox.onUpdated = approvalInbox.onResolved = undefined; + await harness.app.close(); + }); + + it('creates an answerInTerminal inbox item and omits approvalId from push', async () => { + await postHook(harness, 'permission_prompt', { tool_name: 'Bash' }); + const approvals = await listApprovals(harness); + expect(approvals).toHaveLength(1); + expect(approvals[0]).toMatchObject({ + sessionId: SESSION_ID, + kind: 'permission', + answerInTerminal: true, + }); + const push = harness.ctx.sendPushNotifications.mock.calls.find((c) => c[0] === 'hook:permission_prompt'); + expect(push?.[1]).toBeDefined(); + expect(push?.[1]).not.toHaveProperty('approvalId'); + }); + + it('refuses Approve/Deny with a terminal-only error', async () => { + await postHook(harness, 'permission_prompt', {}); + const [item] = await listApprovals(harness); + const res = await harness.app.inject({ + method: 'POST', + url: `/api/approvals/${item.id}/answer`, + payload: { action: 'approve' }, + }); + expect(res.statusCode).toBe(400); + expect(res.json().error).toMatch(/answered in the terminal/i); + expect(session.writeBuffer).toEqual([]); + // Item stays pending — it is an alert, not a failed answer. + expect(await listApprovals(harness)).toHaveLength(1); + }); +}); + describe('approval routes: multi-user scoping', () => { const saved: Record = {};