From 5767d234441776a7051e8c9818bb74b253922650 Mon Sep 17 00:00:00 2001 From: guantw Date: Tue, 8 Sep 2026 11:25:34 +0800 Subject: [PATCH 1/2] fix(acp): remove automatic rejection from permission settings Limit ACP permission choices to ask and auto approve across the settings views. Normalize legacy reject_once values to ask when saving through settings, remove obsolete translations, and cover supported and legacy permission configurations. --- .../components/AcpAgentsConfig.test.tsx | 45 +++++++++++++++++++ .../config/components/AcpAgentsConfig.tsx | 3 +- .../locales/en-US/settings/acp-agents.json | 3 +- .../locales/zh-CN/settings/acp-agents.json | 3 +- .../locales/zh-TW/settings/acp-agents.json | 3 +- 5 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx index 407490e178..f145a1c1ac 100644 --- a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx +++ b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx @@ -300,6 +300,51 @@ describe('AcpAgentsConfig', () => { vi.clearAllMocks(); }); + it.each([ + ['ask', 'ask'], + ['allow_once', 'allow_once'], + ['reject_once', 'ask'], + ])('loads and saves permission mode %s as %s with only supported choices', async (storedMode, expectedMode) => { + loadJsonConfigMock.mockResolvedValue(JSON.stringify({ + acpClients: { + opencode: { command: 'opencode', args: ['acp'], permissionMode: storedMode }, + }, + })); + + await act(async () => { + root.render(); + }); + + const permissionSelect = container.querySelector( + '[data-openbitfun-part="confirmation"] select', + ); + expect(permissionSelect).not.toBeNull(); + expect(Array.from(permissionSelect!.options).map(option => option.value)).toEqual([ + 'ask', 'allow_once', + ]); + expect(permissionSelect!.value).toBe(expectedMode); + expect(saveJsonConfigMock).not.toHaveBeenCalled(); + + await openView(container, 'views.json'); + const editor = container.querySelector('textarea')!; + await act(async () => { + Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, 'value')?.set + ?.call(editor, `${editor.value}\n`); + editor.dispatchEvent(new Event('input', { bubbles: true })); + }); + const saveButton = Array.from(container.querySelectorAll('button')) + .find(button => button.textContent === 'actions.saveJson'); + expect(saveButton?.disabled).toBe(false); + await act(async () => { + saveButton!.click(); + }); + + const savedConfig = JSON.parse(saveJsonConfigMock.mock.calls[0][0]); + expect(savedConfig.acpClients.opencode).toMatchObject({ + command: 'opencode', args: ['acp'], permissionMode: expectedMode, + }); + }); + it('probes requirements when opened and does not treat missing probe data as invalid config', async () => { await act(async () => { root.render(); diff --git a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx index 0c55a13930..d8f015aaf7 100644 --- a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx @@ -253,7 +253,7 @@ function normalizeEnvObject(value: unknown): Record { } function normalizePermissionMode(value: unknown): AcpClientPermissionMode { - return value === 'allow_once' || value === 'reject_once' ? value : 'ask'; + return value === 'allow_once' ? value : 'ask'; } function normalizeSubagentConfig(value: unknown): AcpClientSubagentConfig { @@ -1056,7 +1056,6 @@ const AcpAgentsConfig: React.FC = ({ const permissionOptions = useMemo(() => [ { value: 'ask', label: t('permissionMode.ask') }, { value: 'allow_once', label: t('permissionMode.allowOnce') }, - { value: 'reject_once', label: t('permissionMode.rejectOnce') }, ], [t]); const registryFilterOptions = useMemo(() => [ diff --git a/src/web-ui/src/locales/en-US/settings/acp-agents.json b/src/web-ui/src/locales/en-US/settings/acp-agents.json index 3da7026d04..ad71bd55f4 100644 --- a/src/web-ui/src/locales/en-US/settings/acp-agents.json +++ b/src/web-ui/src/locales/en-US/settings/acp-agents.json @@ -111,8 +111,7 @@ }, "permissionMode": { "ask": "Ask", - "allowOnce": "Auto approve", - "rejectOnce": "Auto reject" + "allowOnce": "Auto approve" }, "requirements": { "tool": "CLI", diff --git a/src/web-ui/src/locales/zh-CN/settings/acp-agents.json b/src/web-ui/src/locales/zh-CN/settings/acp-agents.json index 14acd8e20d..f6e5e06998 100644 --- a/src/web-ui/src/locales/zh-CN/settings/acp-agents.json +++ b/src/web-ui/src/locales/zh-CN/settings/acp-agents.json @@ -111,8 +111,7 @@ }, "permissionMode": { "ask": "询问", - "allowOnce": "自动通过", - "rejectOnce": "自动拒绝" + "allowOnce": "自动通过" }, "requirements": { "tool": "CLI", diff --git a/src/web-ui/src/locales/zh-TW/settings/acp-agents.json b/src/web-ui/src/locales/zh-TW/settings/acp-agents.json index f851e2427e..b43d00bceb 100644 --- a/src/web-ui/src/locales/zh-TW/settings/acp-agents.json +++ b/src/web-ui/src/locales/zh-TW/settings/acp-agents.json @@ -111,8 +111,7 @@ }, "permissionMode": { "ask": "詢問", - "allowOnce": "自動通過", - "rejectOnce": "自動拒絕" + "allowOnce": "自動通過" }, "requirements": { "tool": "CLI", From 2ad01b8ca0dfcc11bd8c577c0f78db0d69a8cbe6 Mon Sep 17 00:00:00 2001 From: guantw Date: Tue, 8 Sep 2026 11:59:30 +0800 Subject: [PATCH 2/2] fix(acp): make legacy permission migration explicitly saveable Track persisted automatic-rejection settings separately from edited values and show a warning with a Save and apply action across ACP settings views. Keep the action available after a failed save and clear it only after successful persistence. Exercise legacy configuration saving, unchanged Ask selection, cross-view actions, and retry behavior with the real Select component. --- .../components/AcpAgentsConfig.test.tsx | 111 ++++++++++++++---- .../config/components/AcpAgentsConfig.tsx | 34 +++++- .../locales/en-US/settings/acp-agents.json | 4 +- .../locales/zh-CN/settings/acp-agents.json | 4 +- .../locales/zh-TW/settings/acp-agents.json | 4 +- 5 files changed, 126 insertions(+), 31 deletions(-) diff --git a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx index f145a1c1ac..f3805351ab 100644 --- a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx +++ b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx @@ -97,21 +97,6 @@ vi.mock('@openbitfun/ui', async importOriginal => ({ onChange?: React.ChangeEventHandler; placeholder?: string; }) => , - Select: ({ - value, - onChange, - options, - }: { - value?: string; - onChange?: (value: string) => void; - options?: Array<{ value: string; label: string }>; - }) => ( - - ), TabGroup: ({ items, onValueChange, @@ -244,6 +229,19 @@ async function openView(container: HTMLElement, label: string): Promise { }); } +async function selectPermission(container: HTMLElement, value: string): Promise { + const trigger = container.querySelector( + '[data-openbitfun-part="confirmation"] button[role="combobox"]', + ); + expect(trigger).not.toBeNull(); + await act(async () => trigger!.click()); + const options = Array.from(document.querySelectorAll('[role="option"]')); + expect(options.map(option => option.dataset.value)).toEqual(['ask', 'allow_once']); + const selected = options.find(option => option.dataset.value === value); + expect(selected).toBeTruthy(); + await act(async () => selected!.click()); +} + describe('AcpAgentsConfig', () => { let container: HTMLDivElement; let root: Root; @@ -303,7 +301,6 @@ describe('AcpAgentsConfig', () => { it.each([ ['ask', 'ask'], ['allow_once', 'allow_once'], - ['reject_once', 'ask'], ])('loads and saves permission mode %s as %s with only supported choices', async (storedMode, expectedMode) => { loadJsonConfigMock.mockResolvedValue(JSON.stringify({ acpClients: { @@ -315,14 +312,8 @@ describe('AcpAgentsConfig', () => { root.render(); }); - const permissionSelect = container.querySelector( - '[data-openbitfun-part="confirmation"] select', - ); - expect(permissionSelect).not.toBeNull(); - expect(Array.from(permissionSelect!.options).map(option => option.value)).toEqual([ - 'ask', 'allow_once', - ]); - expect(permissionSelect!.value).toBe(expectedMode); + expect(container.textContent).not.toContain('permissionMode.legacyRejectWarning'); + await selectPermission(container, expectedMode); expect(saveJsonConfigMock).not.toHaveBeenCalled(); await openView(container, 'views.json'); @@ -345,6 +336,78 @@ describe('AcpAgentsConfig', () => { }); }); + it.each([ + ['local', true], + ['ssh', true], + ['json', true], + ['local', false], + ])('explicitly applies a legacy permission from %s (wrapped config: %s)', async (view, wrapped) => { + const legacyClient = { + command: 'opencode', args: ['acp'], env: { ACP_TEST: 'preserved' }, permissionMode: 'reject_once', + }; + const otherClient = { command: 'custom-agent', args: [], permissionMode: 'allow_once' }; + const acpClients = { opencode: legacyClient, custom: otherClient }; + loadJsonConfigMock.mockResolvedValue(JSON.stringify(wrapped ? { acpClients } : acpClients)); + saveJsonConfigMock.mockImplementation(async (rawConfig: string) => { + loadJsonConfigMock.mockResolvedValue(rawConfig); + window.dispatchEvent(new Event('openbitfun:acp-clients-changed')); + }); + + await act(async () => root.render()); + expect(container.querySelector('[data-openbitfun-part="confirmation"]')?.textContent) + .toContain('permissionMode.ask'); + expect(container.querySelector('[role="alert"]')?.textContent) + .toContain('permissionMode.legacyRejectWarning'); + expect(saveJsonConfigMock).not.toHaveBeenCalled(); + + // The real Select does not emit a change when Ask is already selected. + await selectPermission(container, 'ask'); + expect(Array.from(container.querySelectorAll('button')) + .find(button => button.textContent === 'actions.save')).toBeUndefined(); + if (view !== 'local') await openView(container, `views.${view}`); + expect(saveJsonConfigMock).not.toHaveBeenCalled(); + + const applyButton = Array.from(container.querySelectorAll('button')) + .find(button => button.textContent === 'permissionMode.saveAndApply'); + expect(applyButton?.disabled).toBe(false); + await act(async () => applyButton!.click()); + + expect(saveJsonConfigMock).toHaveBeenCalledTimes(1); + const savedConfig = JSON.parse(saveJsonConfigMock.mock.calls[0][0]); + expect(savedConfig.acpClients.opencode).toMatchObject({ ...legacyClient, permissionMode: 'ask' }); + expect(savedConfig.acpClients.custom).toMatchObject(otherClient); + expect(container.textContent).not.toContain('permissionMode.legacyRejectWarning'); + expect(container.textContent).not.toContain('permissionMode.saveAndApply'); + + await act(async () => { + window.dispatchEvent(new Event('openbitfun:acp-clients-changed')); + }); + expect(container.textContent).not.toContain('permissionMode.legacyRejectWarning'); + expect(saveJsonConfigMock).toHaveBeenCalledTimes(1); + }); + + it('keeps the migration action after a failed save and applies the current selection on retry', async () => { + loadJsonConfigMock.mockResolvedValue(JSON.stringify({ + acpClients: { opencode: { command: 'opencode', permissionMode: 'reject_once' } }, + })); + saveJsonConfigMock.mockRejectedValueOnce(new Error('Save failed')); + await act(async () => root.render()); + await selectPermission(container, 'allow_once'); + const applyButton = () => Array.from(container.querySelectorAll('button')) + .find(button => button.textContent === 'permissionMode.saveAndApply'); + + await act(async () => applyButton()!.click()); + expect(notifyErrorMock).toHaveBeenCalledWith('Save failed', expect.anything()); + expect(container.textContent).toContain('permissionMode.legacyRejectWarning'); + expect(applyButton()?.disabled).toBe(false); + + await act(async () => applyButton()!.click()); + expect(saveJsonConfigMock).toHaveBeenCalledTimes(2); + expect(JSON.parse(saveJsonConfigMock.mock.calls[1][0]).acpClients.opencode.permissionMode) + .toBe('allow_once'); + expect(container.textContent).not.toContain('permissionMode.legacyRejectWarning'); + }); + it('probes requirements when opened and does not treat missing probe data as invalid config', async () => { await act(async () => { root.render(); diff --git a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx index d8f015aaf7..50a89e4718 100644 --- a/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.tsx @@ -1,4 +1,5 @@ import { OverflowText, + Alert, Button, ConfirmDialog, Icon, @@ -210,7 +211,10 @@ function defaultConfigForPreset(preset: AcpClientPreset): AcpClientConfig { }; } -function normalizeConfigValue(value: unknown): AcpClientConfigFile { +function normalizeConfigValue(value: unknown): { + config: AcpClientConfigFile; + hasLegacyPermissionModes: boolean; +} { const candidate = value && typeof value === 'object' ? value as Record : {}; const rawClients = ( candidate.acpClients && typeof candidate.acpClients === 'object' && !Array.isArray(candidate.acpClients) @@ -219,6 +223,7 @@ function normalizeConfigValue(value: unknown): AcpClientConfigFile { : candidate; const acpClients: Record = {}; + let hasLegacyPermissionModes = false; for (const [id, rawConfig] of Object.entries(rawClients)) { if (!rawConfig || typeof rawConfig !== 'object' || Array.isArray(rawConfig)) { continue; @@ -230,6 +235,7 @@ function normalizeConfigValue(value: unknown): AcpClientConfigFile { continue; } + hasLegacyPermissionModes ||= item.permissionMode === 'reject_once'; acpClients[id] = { name: typeof item.name === 'string' ? item.name : undefined, command, @@ -242,7 +248,7 @@ function normalizeConfigValue(value: unknown): AcpClientConfigFile { }; } - return { acpClients }; + return { config: { acpClients }, hasLegacyPermissionModes }; } function normalizeEnvObject(value: unknown): Record { @@ -483,6 +489,7 @@ const AcpAgentsConfig: React.FC = ({ const [loadFailed, setLoadFailed] = useState(false); const [saving, setSaving] = useState(false); const [dirty, setDirty] = useState(false); + const [pendingPermissionMigration, setPendingPermissionMigration] = useState(false); const [jsonConfig, setJsonConfig] = useState(''); const [jsonBaseline, setJsonBaseline] = useState(formatConfig({ acpClients: {} })); const [jsonDirty, setJsonDirty] = useState(false); @@ -713,8 +720,9 @@ const AcpAgentsConfig: React.FC = ({ log.warn('Failed to load saved SSH connections for ACP remote overrides', error); return [] as SavedConnection[]; }); - const parsed = normalizeConfigValue(JSON.parse(rawConfig || '{}')); + const { config: parsed, hasLegacyPermissionModes } = normalizeConfigValue(JSON.parse(rawConfig || '{}')); setConfig(parsed); + setPendingPermissionMigration(hasLegacyPermissionModes); const formattedConfig = formatConfig(parsed); setJsonConfig(formattedConfig); setJsonBaseline(formattedConfig); @@ -959,6 +967,7 @@ const AcpAgentsConfig: React.FC = ({ setJsonBaseline(formattedConfig); setDirty(false); setJsonDirty(false); + setPendingPermissionMigration(false); await refreshRequirementProbes({ force: true, notifyOnError: false }); loadedRemoteProbeIdsRef.current.clear(); setRemoteProbeRefreshNonce(prev => prev + 1); @@ -1010,7 +1019,7 @@ const AcpAgentsConfig: React.FC = ({ const saveJsonConfig = async (): Promise => { try { - const parsed = normalizeConfigValue(JSON.parse(jsonConfig)); + const { config: parsed } = normalizeConfigValue(JSON.parse(jsonConfig)); const saved = await saveConfig(parsed, { mergeEnvDrafts: false }); if (!saved) return false; setConfig(parsed); @@ -1347,6 +1356,23 @@ const AcpAgentsConfig: React.FC = ({ onValueChange={handleViewChange} value={activeView} /> + {pendingPermissionMigration && ( + { void (activeView === 'json' ? saveJsonConfig() : saveConfig()); }} + > + {t('permissionMode.saveAndApply')} + + )} + /> + )} {activeView === 'json' && ( )} diff --git a/src/web-ui/src/locales/en-US/settings/acp-agents.json b/src/web-ui/src/locales/en-US/settings/acp-agents.json index ad71bd55f4..e63bd2651b 100644 --- a/src/web-ui/src/locales/en-US/settings/acp-agents.json +++ b/src/web-ui/src/locales/en-US/settings/acp-agents.json @@ -111,7 +111,9 @@ }, "permissionMode": { "ask": "Ask", - "allowOnce": "Auto approve" + "allowOnce": "Auto approve", + "legacyRejectWarning": "Some agents still have Auto reject saved. The editor replaces it with Ask. Save and apply your current choices to make them take effect; until then, those agents will continue to reject requests automatically.", + "saveAndApply": "Save and apply" }, "requirements": { "tool": "CLI", diff --git a/src/web-ui/src/locales/zh-CN/settings/acp-agents.json b/src/web-ui/src/locales/zh-CN/settings/acp-agents.json index f6e5e06998..a8ec3edae5 100644 --- a/src/web-ui/src/locales/zh-CN/settings/acp-agents.json +++ b/src/web-ui/src/locales/zh-CN/settings/acp-agents.json @@ -111,7 +111,9 @@ }, "permissionMode": { "ask": "询问", - "allowOnce": "自动通过" + "allowOnce": "自动通过", + "legacyRejectWarning": "部分 Agent 的已保存设置仍为“自动拒绝”，编辑器已将其替换为“询问”。请保存并应用当前选择；保存前，这些 Agent 仍会自动拒绝请求。", + "saveAndApply": "保存并应用" }, "requirements": { "tool": "CLI", diff --git a/src/web-ui/src/locales/zh-TW/settings/acp-agents.json b/src/web-ui/src/locales/zh-TW/settings/acp-agents.json index b43d00bceb..4e0d876407 100644 --- a/src/web-ui/src/locales/zh-TW/settings/acp-agents.json +++ b/src/web-ui/src/locales/zh-TW/settings/acp-agents.json @@ -111,7 +111,9 @@ }, "permissionMode": { "ask": "詢問", - "allowOnce": "自動通過" + "allowOnce": "自動通過", + "legacyRejectWarning": "部分 Agent 的已儲存設定仍為「自動拒絕」，編輯器已將其替換為「詢問」。請儲存並套用目前選擇；儲存前，這些 Agent 仍會自動拒絕請求。", + "saveAndApply": "儲存並套用" }, "requirements": { "tool": "CLI",