mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-20 09:09:35 +02:00
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).
This commit is contained in:
1 parent
aa3730aace
commit
f5e59283f9
9 files changed
+119
-24
No files matched your search
@@ -85,6 +85,13 @@ export interface ApprovalItem {
|
|||||||
* answer endpoint accepts; absent → only approve('1')/deny(Esc) are allowed.
|
* answer endpoint accepts; absent → only approve('1')/deny(Esc) are allowed.
|
||||||
*/
|
*/
|
||||||
options?: ApprovalOption[];
|
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 {
|
export interface ApprovalResolvedInfo {
|
||||||
@@ -108,6 +115,11 @@ interface NotePromptArgs {
|
|||||||
* opens ALREADY acknowledged: see `notePrompt()`.
|
* opens ALREADY acknowledged: see `notePrompt()`.
|
||||||
*/
|
*/
|
||||||
watching?: string | null;
|
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. */
|
/** Returns the raw (ANSI-bearing) pane frame, or null when unavailable. */
|
||||||
capture?: () => string | null;
|
capture?: () => string | null;
|
||||||
}
|
}
|
||||||
@@ -261,6 +273,7 @@ export class ApprovalInbox {
|
|||||||
toolSummary: args.toolSummary,
|
toolSummary: args.toolSummary,
|
||||||
message: args.message,
|
message: args.message,
|
||||||
cwd: args.cwd,
|
cwd: args.cwd,
|
||||||
|
...(args.answerInTerminal ? { answerInTerminal: true } : {}),
|
||||||
};
|
};
|
||||||
if (args.kind === 'idle' && args.watching) {
|
if (args.kind === 'idle' && args.watching) {
|
||||||
item.acknowledgedAt = item.createdAt;
|
item.acknowledgedAt = item.createdAt;
|
||||||
|
|||||||
@@ -261,6 +261,11 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
`onkeydown="if(event.key==='Enter')app.answerApprovalIdleText('${id}')">` +
|
`onkeydown="if(event.key==='Enter')app.answerApprovalIdleText('${id}')">` +
|
||||||
`<button class="approval-btn approval-btn-primary" onclick="app.answerApprovalIdleText('${id}')">Send</button>` +
|
`<button class="approval-btn approval-btn-primary" onclick="app.answerApprovalIdleText('${id}')">Send</button>` +
|
||||||
`</div>`;
|
`</div>`;
|
||||||
|
} 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 =
|
||||||
|
`<span class="approval-terminal-hint" data-i18n-skip>Answer in the terminal</span>`;
|
||||||
} else if (item.options && item.options.length) {
|
} else if (item.options && item.options.length) {
|
||||||
actions = item.options
|
actions = item.options
|
||||||
.map(
|
.map(
|
||||||
|
|||||||
@@ -794,7 +794,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
const wrap = document.createElement('div');
|
const wrap = document.createElement('div');
|
||||||
wrap.className = 'mobile-overview-row-wrap';
|
wrap.className = 'mobile-overview-row-wrap';
|
||||||
wrap.appendChild(item);
|
wrap.appendChild(item);
|
||||||
wrap.appendChild(this._buildMobileOverviewApprovalStrip(approval));
|
wrap.appendChild(this._buildMobileOverviewApprovalStrip(approval, row));
|
||||||
return wrap;
|
return wrap;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -949,7 +949,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
},
|
},
|
||||||
|
|
||||||
/** Compact answer buttons for a NEEDS YOU row: parsed options, else Approve/Deny. */
|
/** Compact answer buttons for a NEEDS YOU row: parsed options, else Approve/Deny. */
|
||||||
_buildMobileOverviewApprovalStrip(approval) {
|
_buildMobileOverviewApprovalStrip(approval, row) {
|
||||||
const strip = document.createElement('div');
|
const strip = document.createElement('div');
|
||||||
strip.className = 'mobile-overview-approval-strip';
|
strip.className = 'mobile-overview-approval-strip';
|
||||||
strip.setAttribute('data-i18n-skip', '');
|
strip.setAttribute('data-i18n-skip', '');
|
||||||
@@ -964,6 +964,15 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
});
|
});
|
||||||
strip.appendChild(btn);
|
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) {
|
if (approval.options && approval.options.length) {
|
||||||
for (const o of approval.options) {
|
for (const o of approval.options) {
|
||||||
const label = o.label.length > 24 ? o.label.slice(0, 24) + '…' : o.label;
|
const label = o.label.length > 24 ? o.label.slice(0, 24) + '…' : o.label;
|
||||||
|
|||||||
@@ -102,14 +102,12 @@ export function registerApprovalRoutes(app: FastifyInstance, ctx: SessionPort):
|
|||||||
if (!hooksAvailableForMode(session.mode, sessionHookOptions(session))) {
|
if (!hooksAvailableForMode(session.mode, sessionHookOptions(session))) {
|
||||||
return createErrorResponse(ApiErrorCode.CONFLICT, 'Session mode cannot have pending approvals');
|
return createErrorResponse(ApiErrorCode.CONFLICT, 'Session mode cannot have pending approvals');
|
||||||
}
|
}
|
||||||
// A dsh approval is an ALERT, not an answerable card: the dialog belongs to
|
// Alert-only items (supervised-hook CLIs such as DeepSeek Harness): the
|
||||||
// a third-party TUI whose keystroke contract Codeman has not measured, the
|
// dialog belongs to a third-party TUI whose keystroke contract Codeman has
|
||||||
// Claude-shaped option parser never reads options off its frames, and
|
// not measured, so '1'/Esc would be a blind keystroke into a foreign
|
||||||
// verifyStillAnswerable() can therefore never be conclusive for it — so the
|
// composer. The item still raises the red alert and clears on the harness's
|
||||||
// '1'/Esc below would be a blind keystroke into a foreign composer. The item
|
// own working/stop reports; answering happens in the terminal.
|
||||||
// still raises the red alert and clears on the harness's own working/stop
|
if (item.answerInTerminal) {
|
||||||
// reports; answering happens in the terminal.
|
|
||||||
if (session.mode === 'deepseek') {
|
|
||||||
return createErrorResponse(
|
return createErrorResponse(
|
||||||
ApiErrorCode.INVALID_INPUT,
|
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.'
|
'DeepSeek Harness approvals must be answered in the terminal: the dialog belongs to a third-party TUI whose keystrokes Codeman cannot verify.'
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ import { sanitizeHookData, parseBody } from '../route-helpers.js';
|
|||||||
import { persistDockerCaseClaudeSessionId } from '../../docker-hosts.js';
|
import { persistDockerCaseClaudeSessionId } from '../../docker-hosts.js';
|
||||||
import { getDataDir } from '../../config/instance.js';
|
import { getDataDir } from '../../config/instance.js';
|
||||||
import { sessionWaits, hooksAvailableForMode, sessionHookOptions } from '../session-wait-registry.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 { approvalInbox, type ApprovalKind } from '../approval-inbox.js';
|
||||||
import type { SessionPort, EventPort, RespawnPort, ConfigPort, InfraPort } from '../ports/index.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,
|
toolSummary: typeof toolSummary === 'string' ? toolSummary : undefined,
|
||||||
message: typeof safeData.message === 'string' ? safeData.message : undefined,
|
message: typeof safeData.message === 'string' ? safeData.message : undefined,
|
||||||
cwd: typeof safeData.cwd === 'string' ? safeData.cwd : 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
|
// 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
|
// session that is watching its own work opens acknowledged, so it never arms an
|
||||||
// alert nobody can answer; notePrompt() carries the whole reasoning.
|
// alert nobody can answer; notePrompt() carries the whole reasoning.
|
||||||
@@ -244,7 +249,9 @@ export function registerHookEventRoutes(
|
|||||||
sessionId,
|
sessionId,
|
||||||
sessionName,
|
sessionName,
|
||||||
...safeData,
|
...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 }),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+5
-1
@@ -2742,9 +2742,13 @@ export class WebServer extends EventEmitter {
|
|||||||
// Approvals Inbox item id: lets sw.js answer an Approve/Deny action
|
// Approvals Inbox item id: lets sw.js answer an Approve/Deny action
|
||||||
// click directly (POST /api/approvals/:id/answer) with no tab open.
|
// click directly (POST /api/approvals/:id/answer) with no tab open.
|
||||||
// Gated on the opt-in setting together with the action buttons.
|
// 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,
|
approvalId: approvalsEnabled && typeof data.approvalId === 'string' ? data.approvalId : undefined,
|
||||||
urgency: template.urgency,
|
urgency: template.urgency,
|
||||||
actions: approvalsEnabled ? template.actions : undefined,
|
actions:
|
||||||
|
approvalsEnabled && typeof data.approvalId === 'string' ? template.actions : undefined,
|
||||||
});
|
});
|
||||||
|
|
||||||
for (const sub of subscriptions) {
|
for (const sub of subscriptions) {
|
||||||
|
|||||||
@@ -124,10 +124,6 @@ const ALLOWED_BRANCHES: Record<string, Allowance> = {
|
|||||||
1,
|
1,
|
||||||
'an error message explaining why THIS mode in particular will never deliver a stop signal'
|
'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 === 'claude'": allow(1, 'cron launch handling, not yet generalised'),
|
||||||
"cron/cron-service.ts::mode === 'shell'": 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(
|
"web/routes/session-routes.ts::mode === 'claude'": allow(
|
||||||
@@ -169,14 +165,6 @@ const ALLOWED_BRANCHES: Record<string, Allowance> = {
|
|||||||
),
|
),
|
||||||
"tui/tui-render.ts::mode !== 'claude'": allow(1, 'cosmetic: suppress the mode badge for the default mode'),
|
"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 <Mode>Config plumbing, same category as the `===` entries above.
|
// Legacy <Mode>Config plumbing, same category as the `===` entries above.
|
||||||
"web/routes/session-routes.ts::mode !== 'omp'": allow(
|
"web/routes/session-routes.ts::mode !== 'omp'": allow(
|
||||||
|
|||||||
@@ -219,6 +219,27 @@ describe('push payload Approvals Inbox gating', () => {
|
|||||||
expect(payload.approvalId).toBe('s-gate:1');
|
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<string, unknown>) => Promise<void>;
|
||||||
|
}
|
||||||
|
).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 () => {
|
it('an explicit false behaves like the default (only true enables)', async () => {
|
||||||
const server = makeServerWithHost('gate-false');
|
const server = makeServerWithHost('gate-false');
|
||||||
setSettings(server, { approvalsInboxEnabled: false });
|
setSettings(server, { approvalsInboxEnabled: false });
|
||||||
|
|||||||
@@ -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', () => {
|
describe('approval routes: multi-user scoping', () => {
|
||||||
const saved: Record<string, string | undefined> = {};
|
const saved: Record<string, string | undefined> = {};
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user