Merge pull request #2883 from guantw/fix/acp-remove-auto-reject

fix(acp): remove automatic rejection from permission settings

Author: wgqqqqq

src/web-ui/src/infrastructure/config/components/AcpAgentsConfig.test.tsx

@@ -97,21 +97,6 @@ vi.mock('@openbitfun/ui', async importOriginal => ({
     onChange?: React.ChangeEventHandler<HTMLInputElement>;
     placeholder?: string;
   }) => <input value={value} onChange={onChange} placeholder={placeholder} />,
-  Select: ({
-    value,
-    onChange,
-    options,
-  }: {
-    value?: string;
-    onChange?: (value: string) => void;
-    options?: Array<{ value: string; label: string }>;
-  }) => (
-    <select value={value} onChange={(event) => onChange?.(event.target.value)}>
-      {(options ?? []).map((option) => (
-        <option key={option.value} value={option.value}>{option.label}</option>
-      ))}
-    </select>
-  ),
   TabGroup: ({
     items,
     onValueChange,
@@ -244,6 +229,19 @@ async function openView(container: HTMLElement, label: string): Promise<void> {
   });
 }
 
+async function selectPermission(container: HTMLElement, value: string): Promise<void> {
+  const trigger = container.querySelector<HTMLButtonElement>(
+    '[data-openbitfun-part="confirmation"] button[role="combobox"]',
+  );
+  expect(trigger).not.toBeNull();
+  await act(async () => trigger!.click());
+  const options = Array.from(document.querySelectorAll<HTMLElement>('[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;
@@ -300,6 +298,116 @@ describe('AcpAgentsConfig', () => {
     vi.clearAllMocks();
   });
 
+  it.each([
+    ['ask', 'ask'],
+    ['allow_once', 'allow_once'],
+  ])('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(<AcpAgentsConfig />);
+    });
+
+    expect(container.textContent).not.toContain('permissionMode.legacyRejectWarning');
+    await selectPermission(container, expectedMode);
+    expect(saveJsonConfigMock).not.toHaveBeenCalled();
+
+    await openView(container, 'views.json');
+    const editor = container.querySelector<HTMLTextAreaElement>('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.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(<AcpAgentsConfig />));
+    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(<AcpAgentsConfig />));
+    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(<AcpAgentsConfig />);

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<string, unknown> : {};
   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<string, AcpClientConfig> = {};
+  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<string, string> {
@@ -253,7 +259,7 @@ function normalizeEnvObject(value: unknown): Record<string, string> {
 }
 
 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 {
@@ -483,6 +489,7 @@ const AcpAgentsConfig: React.FC<AcpAgentsConfigProps> = ({
   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<AcpAgentsConfigProps> = ({
         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<AcpAgentsConfigProps> = ({
       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<AcpAgentsConfigProps> = ({
 
   const saveJsonConfig = async (): Promise<boolean> => {
     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);
@@ -1056,7 +1065,6 @@ const AcpAgentsConfig: React.FC<AcpAgentsConfigProps> = ({
   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(() => [
@@ -1348,6 +1356,23 @@ const AcpAgentsConfig: React.FC<AcpAgentsConfigProps> = ({
           onValueChange={handleViewChange}
           value={activeView}
         />
+        {pendingPermissionMigration && (
+          <Alert
+            tone="warning"
+            message={t('permissionMode.legacyRejectWarning')}
+            description={(
+              <Button
+                variant="fill"
+                size="sm"
+                disabled={saving}
+                loading={saving}
+                onClick={() => { void (activeView === 'json' ? saveJsonConfig() : saveConfig()); }}
+              >
+                {t('permissionMode.saveAndApply')}
+              </Button>
+            )}
+          />
+        )}
         {activeView === 'json' && (
           <ConfigMessage message={{ type: 'warning', text: t('security.secretWarning') }} />
         )}

src/web-ui/src/locales/en-US/settings/acp-agents.json

@@ -112,7 +112,8 @@
   "permissionMode": {
     "ask": "Ask",
     "allowOnce": "Auto approve",
-    "rejectOnce": "Auto reject"
+    "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",

src/web-ui/src/locales/zh-CN/settings/acp-agents.json

@@ -112,7 +112,8 @@
   "permissionMode": {
     "ask": "询问",
     "allowOnce": "自动通过",
-    "rejectOnce": "自动拒绝"
+    "legacyRejectWarning": "部分 Agent 的已保存设置仍为“自动拒绝”，编辑器已将其替换为“询问”。请保存并应用当前选择；保存前，这些 Agent 仍会自动拒绝请求。",
+    "saveAndApply": "保存并应用"
   },
   "requirements": {
     "tool": "CLI",

src/web-ui/src/locales/zh-TW/settings/acp-agents.json

@@ -112,7 +112,8 @@
   "permissionMode": {
     "ask": "詢問",
     "allowOnce": "自動通過",
-    "rejectOnce": "自動拒絕"
+    "legacyRejectWarning": "部分 Agent 的已儲存設定仍為「自動拒絕」，編輯器已將其替換為「詢問」。請儲存並套用目前選擇；儲存前，這些 Agent 仍會自動拒絕請求。",
+    "saveAndApply": "儲存並套用"
   },
   "requirements": {
     "tool": "CLI",