diff --git a/packages/desktop/src/common/types/platform/acpTypes.ts b/packages/desktop/src/common/types/platform/acpTypes.ts index fb9f525294d..e7cf72ef63e 100644 --- a/packages/desktop/src/common/types/platform/acpTypes.ts +++ b/packages/desktop/src/common/types/platform/acpTypes.ts @@ -15,7 +15,17 @@ import type { TConversationRuntimeSummary } from '@/common/config/storage'; export interface CustomAgentAdvancedOverrides { yolo_id?: string; native_skills_dirs?: string[]; - behavior_policy?: { supports_side_question?: boolean }; + behavior_policy?: { + supports_side_question?: boolean; + /** + * Gates team-mode eligibility for this agent. Seeded `false` for custom + * agents and read by the backend, so a custom ACP agent that genuinely + * speaks MCP stdio cannot join a team unless this is set. The backend + * accepts it on `/api/agents/custom`; it was simply unreachable from the + * editor, which dropped the key while parsing the JSON panel. + */ + supports_team?: boolean; + }; description?: string; } diff --git a/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx b/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx index 6d17544a84a..c7c53abc9cc 100644 --- a/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx +++ b/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx @@ -5,6 +5,7 @@ */ import type { CustomAgentAdvancedOverrides } from '@/common/types/platform/acpTypes'; +import { buildAdvancedJson, parseAdvancedOverrides } from './advancedOverrides'; import type { AgentMetadata, ManagedAgent } from '@/renderer/utils/model/agentTypes'; import { acpConversation, dialog, fs } from '@/common/adapter/ipcBridge'; import { useAssistantList } from '@/renderer/hooks/assistant'; @@ -258,23 +259,15 @@ const InlineAgentEditor: React.FC = ({ agent, onSave, on } }, [t]); - // Canonical empty shape shown when the user has not filled anything yet. - // Keep keys in sync with CustomAgentAdvancedOverrides. - const buildJsonFromAdvanced = useCallback((advancedVal: CustomAgentAdvancedOverrides) => { - const skeleton: CustomAgentAdvancedOverrides = { - yolo_id: advancedVal.yolo_id ?? '', - native_skills_dirs: advancedVal.native_skills_dirs ?? [], - behavior_policy: advancedVal.behavior_policy ?? { supports_side_question: false }, - description: advancedVal.description ?? '', - }; - return JSON.stringify(skeleton, null, 2); - }, []); - + // Serialization lives in `advancedOverrides.ts` so the round-trip is unit-testable — a key + // silently dropped while parsing is indistinguishable, for the user, from one that was saved. + // Called directly rather than wrapped in `useCallback`: it is a module-level pure function, so it + // is already stable across renders. useEffect(() => { if (!isJsonEditingRef.current) { - setJsonInput(buildJsonFromAdvanced(advanced)); + setJsonInput(buildAdvancedJson(advanced)); } - }, [advanced, buildJsonFromAdvanced]); + }, [advanced]); useEffect(() => { setTestStatus('idle'); @@ -305,28 +298,14 @@ const InlineAgentEditor: React.FC = ({ agent, onSave, on isJsonEditingRef.current = true; if (jsonEditTimerRef.current) clearTimeout(jsonEditTimerRef.current); setJsonInput(value); - try { - const parsed: unknown = JSON.parse(value); - setJsonError(''); - if (parsed && typeof parsed === 'object') { - const next: CustomAgentAdvancedOverrides = {}; - const p = parsed as Record; - if (typeof p.yolo_id === 'string' && p.yolo_id.trim()) next.yolo_id = p.yolo_id; - if (Array.isArray(p.native_skills_dirs)) { - const dirs = p.native_skills_dirs.filter((x): x is string => typeof x === 'string'); - if (dirs.length > 0) next.native_skills_dirs = dirs; - } - if (p.behavior_policy && typeof p.behavior_policy === 'object') { - const bp = p.behavior_policy as Record; - if (typeof bp.supports_side_question === 'boolean') { - next.behavior_policy = { supports_side_question: bp.supports_side_question }; - } - } - if (typeof p.description === 'string' && p.description.trim()) next.description = p.description; - setAdvanced(next); - } - } catch { + const result = parseAdvancedOverrides(value); + // `ignored` (valid JSON but not an object) clears the error and keeps the previous bag — + // behaviour preserved verbatim from the inline version this replaced. + if (result.kind === 'invalid') { setJsonError('Invalid JSON'); + } else { + setJsonError(''); + if (result.kind === 'ok') setAdvanced(result.value); } jsonEditTimerRef.current = setTimeout(() => { isJsonEditingRef.current = false; diff --git a/packages/desktop/src/renderer/pages/settings/AgentSettings/advancedOverrides.ts b/packages/desktop/src/renderer/pages/settings/AgentSettings/advancedOverrides.ts new file mode 100644 index 00000000000..4eb68e5fdfb --- /dev/null +++ b/packages/desktop/src/renderer/pages/settings/AgentSettings/advancedOverrides.ts @@ -0,0 +1,93 @@ +/** + * @license + * Copyright 2025 AionUi (aionui.com) + * SPDX-License-Identifier: Apache-2.0 + * + * Serialization for the custom agent editor's advanced JSON panel. + * + * Extracted from `InlineAgentEditor` so the round-trip can be unit-tested: the panel accepts + * free-form JSON but only keeps a known set of keys, and a key silently dropped here is + * indistinguishable — to the user — from one that was saved. + */ +import type { CustomAgentAdvancedOverrides } from '@/common/types/platform/acpTypes'; + +type BehaviorPolicy = NonNullable; + +/** + * Canonical shape shown when the user has not filled anything yet. + * + * Every supported key appears, including the booleans at `false`: the panel is the only place these + * are documented, so an absent key reads as "unsupported" rather than "unset". + */ +export function buildAdvancedJson(advanced: CustomAgentAdvancedOverrides): string { + const skeleton: CustomAgentAdvancedOverrides = { + yolo_id: advanced.yolo_id ?? '', + native_skills_dirs: advanced.native_skills_dirs ?? [], + behavior_policy: advanced.behavior_policy ?? { supports_side_question: false, supports_team: false }, + description: advanced.description ?? '', + }; + return JSON.stringify(skeleton, null, 2); +} + +/** + * Outcome of parsing the panel. The three cases are distinct on purpose: + * - `invalid` — not JSON at all; the editor shows "Invalid JSON"; + * - `ignored` — valid JSON but not an object (`"s"`, `null`, a number); the editor clears the + * error and keeps the previous bag. Preserved verbatim from the original inline + * logic. Note an array is NOT ignored: `typeof [] === 'object'`, so it has always + * parsed as an object with no known keys, i.e. it empties the bag; + * - `ok` — a usable override bag. + */ +export type AdvancedParseResult = + | { kind: 'ignored' } + | { kind: 'invalid' } + | { kind: 'ok'; value: CustomAgentAdvancedOverrides }; + +/** + * Parse the panel's contents into the override bag. + * + * Unknown keys are ignored on purpose (the bag maps onto specific backend columns), and empty + * values are omitted so an untouched panel does not send a payload of blanks. + */ +export function parseAdvancedOverrides(value: string): AdvancedParseResult { + let parsed: unknown; + try { + parsed = JSON.parse(value); + } catch { + return { kind: 'invalid' }; + } + if (!parsed || typeof parsed !== 'object') return { kind: 'ignored' }; + + const p = parsed as Record; + const next: CustomAgentAdvancedOverrides = {}; + + if (typeof p.yolo_id === 'string' && p.yolo_id.trim()) next.yolo_id = p.yolo_id; + + if (Array.isArray(p.native_skills_dirs)) { + const dirs = p.native_skills_dirs.filter((x): x is string => typeof x === 'string'); + if (dirs.length > 0) next.native_skills_dirs = dirs; + } + + if (p.behavior_policy && typeof p.behavior_policy === 'object') { + const policy = parseBehaviorPolicy(p.behavior_policy as Record); + if (policy) next.behavior_policy = policy; + } + + if (typeof p.description === 'string' && p.description.trim()) next.description = p.description; + + return { kind: 'ok', value: next }; +} + +/** + * Known policy flags, accumulated rather than assigned as a fresh object per key: the previous + * inline shape kept only `supports_side_question`, so `supports_team` was discarded even though the + * backend reads it and `/api/agents/custom` accepts it. + * + * `undefined` when no known flag is present, so an unrecognised policy does not send an empty object. + */ +function parseBehaviorPolicy(bp: Record): BehaviorPolicy | undefined { + const policy: BehaviorPolicy = {}; + if (typeof bp.supports_side_question === 'boolean') policy.supports_side_question = bp.supports_side_question; + if (typeof bp.supports_team === 'boolean') policy.supports_team = bp.supports_team; + return Object.keys(policy).length > 0 ? policy : undefined; +} diff --git a/packages/desktop/src/renderer/utils/model/agentTypes.ts b/packages/desktop/src/renderer/utils/model/agentTypes.ts index 9c329907ba8..6aa7a51acd4 100644 --- a/packages/desktop/src/renderer/utils/model/agentTypes.ts +++ b/packages/desktop/src/renderer/utils/model/agentTypes.ts @@ -65,6 +65,13 @@ export type AgentEnvEntry = { */ export type BehaviorPolicy = { supports_side_question?: boolean; + /** + * Gates team-mode eligibility. Persisted by the backend and seeded `false` + * for custom agents, so a custom ACP agent stays unselectable in a team + * until it is set — even when the agent advertises + * `agentCapabilities.mcpCapabilities.stdio: true` at init. + */ + supports_team?: boolean; }; /** diff --git a/tests/unit/settings/advancedOverrides.test.ts b/tests/unit/settings/advancedOverrides.test.ts new file mode 100644 index 00000000000..0a68cd5a2c3 --- /dev/null +++ b/tests/unit/settings/advancedOverrides.test.ts @@ -0,0 +1,99 @@ +import type { CustomAgentAdvancedOverrides } from '@/common/types/platform/acpTypes'; +import { buildAdvancedJson, parseAdvancedOverrides } from '@/renderer/pages/settings/AgentSettings/advancedOverrides'; +import { describe, expect, it } from 'vitest'; + +/** The override bag for a parse that succeeded, or the outcome kind otherwise. */ +function bagOf(json: string): CustomAgentAdvancedOverrides | 'ignored' | 'invalid' { + const result = parseAdvancedOverrides(json); + return result.kind === 'ok' ? result.value : result.kind; +} + +describe('parseAdvancedOverrides', () => { + // Regression: the panel accepted this JSON, showed no error, and dropped the key on save — so a + // custom ACP agent could never be made team-selectable from the UI. + it('keeps behavior_policy.supports_team', () => { + expect(bagOf('{"behavior_policy":{"supports_team":true}}')).toEqual({ + behavior_policy: { supports_team: true }, + }); + }); + + it('keeps both policy flags without either clobbering the other', () => { + expect(bagOf('{"behavior_policy":{"supports_side_question":true,"supports_team":true}}')).toEqual({ + behavior_policy: { supports_side_question: true, supports_team: true }, + }); + }); + + it('keeps supports_side_question alone (unchanged behaviour)', () => { + expect(bagOf('{"behavior_policy":{"supports_side_question":true}}')).toEqual({ + behavior_policy: { supports_side_question: true }, + }); + }); + + it('preserves false, which is a meaningful value and not an absence', () => { + expect(bagOf('{"behavior_policy":{"supports_team":false}}')).toEqual({ + behavior_policy: { supports_team: false }, + }); + }); + + it('ignores non-boolean flags rather than coercing them', () => { + expect(bagOf('{"behavior_policy":{"supports_team":"yes"}}')).toEqual({}); + }); + + it('omits behavior_policy entirely when no known flag is present', () => { + expect(bagOf('{"behavior_policy":{"unknown_flag":true}}')).toEqual({}); + }); + + it('keeps the other override keys', () => { + expect(bagOf('{"yolo_id":"y","native_skills_dirs":["/a"],"description":"d"}')).toEqual({ + description: 'd', + native_skills_dirs: ['/a'], + yolo_id: 'y', + }); + }); + + it('drops blank strings and empty arrays so an untouched panel sends nothing', () => { + expect(bagOf('{"yolo_id":" ","native_skills_dirs":[],"description":""}')).toEqual({}); + }); + + it('filters non-string entries out of native_skills_dirs', () => { + expect(bagOf('{"native_skills_dirs":["/a",1,null]}')).toEqual({ native_skills_dirs: ['/a'] }); + }); + + it('reports invalid JSON so the editor can surface it', () => { + expect(bagOf('{oops')).toBe('invalid'); + }); + + // Valid JSON that is neither object nor array clears the error and keeps the previous bag — + // behaviour preserved verbatim from the inline logic this replaced, hence a kind of its own. + it('reports non-object JSON as ignored, not invalid', () => { + expect(bagOf('"str"')).toBe('ignored'); + expect(bagOf('null')).toBe('ignored'); + }); + + // An array passes `typeof x === 'object'`, so it has always been read as an object with no known + // keys — i.e. it clears the bag. Documented rather than changed: this PR fixes a dropped key, not + // the panel's tolerance for odd input. + it('treats an array as an empty bag, as before', () => { + expect(bagOf('[]')).toEqual({}); + }); +}); + +describe('buildAdvancedJson', () => { + it('advertises supports_team in the skeleton — the panel is its only documentation', () => { + const skeleton = JSON.parse(buildAdvancedJson({})) as CustomAgentAdvancedOverrides; + expect(skeleton.behavior_policy).toEqual({ supports_side_question: false, supports_team: false }); + }); + + // The panel is a serialize → edit → parse loop; a key surviving the skeleton but not the parse + // (the bug this suite guards) is precisely what the user cannot see. + it('round-trips a policy through build then parse', () => { + const advanced = { behavior_policy: { supports_side_question: true, supports_team: true } }; + expect(bagOf(buildAdvancedJson(advanced))).toEqual(advanced); + }); + + it('round-trips the empty skeleton to the default policy', () => { + expect(bagOf(buildAdvancedJson({}))).toEqual({ + behavior_policy: { supports_side_question: false, supports_team: false }, + }); + }); +});