From 041cab71db2e77b89c568039bee9de01837ce4f1 Mon Sep 17 00:00:00 2001 From: Arnaud Palin Sainte Agathe Date: Mon, 24 Aug 2026 18:12:19 +0200 Subject: [PATCH 1/3] fix(agent-editor): stop dropping behavior_policy.supports_team MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The custom agent editor's advanced panel is a free-form JSON editor, but `handleJsonChange` whitelists the keys it keeps. For `behavior_policy` it only preserved `supports_side_question`, so typing { "behavior_policy": { "supports_team": true } } looked accepted and was silently discarded on save — the field never reached `/api/agents/custom`, which does accept it. That flag gates team-mode eligibility and is seeded `false` for custom agents, so a custom ACP agent could not be made team-selectable through the UI at all. The only remaining routes are the HTTP API (unreachable from outside the app since business endpoints require a session) or editing aioncore's SQLite directly. - parse `supports_team` alongside `supports_side_question`, building the policy object incrementally so neither key clobbers the other; - surface both keys in the skeleton JSON so the panel is self-documenting — `supports_team` was otherwise undiscoverable; - declare the field in `CustomAgentAdvancedOverrides` and `BehaviorPolicy`, where it had been dropped while the backend still reads it. No behaviour change for agents that do not set the flag. --- .../desktop/src/common/types/platform/acpTypes.ts | 12 +++++++++++- .../settings/AgentSettings/InlineAgentEditor.tsx | 15 +++++++++++++-- .../src/renderer/utils/model/agentTypes.ts | 7 +++++++ 3 files changed, 31 insertions(+), 3 deletions(-) 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..18cdcc76486 100644 --- a/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx +++ b/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx @@ -264,7 +264,10 @@ const InlineAgentEditor: React.FC = ({ agent, onSave, on const skeleton: CustomAgentAdvancedOverrides = { yolo_id: advancedVal.yolo_id ?? '', native_skills_dirs: advancedVal.native_skills_dirs ?? [], - behavior_policy: advancedVal.behavior_policy ?? { supports_side_question: false }, + // Both keys are shown in the skeleton so the panel is self-documenting: + // `supports_team` is otherwise undiscoverable, yet required for a custom + // ACP agent to be selectable in a team. + behavior_policy: advancedVal.behavior_policy ?? { supports_side_question: false, supports_team: false }, description: advancedVal.description ?? '', }; return JSON.stringify(skeleton, null, 2); @@ -318,9 +321,17 @@ const InlineAgentEditor: React.FC = ({ agent, onSave, on } if (p.behavior_policy && typeof p.behavior_policy === 'object') { const bp = p.behavior_policy as Record; + // Build incrementally: assigning a fresh object per key would drop the + // sibling flag. `supports_team` was previously discarded here, so a user + // typing it into the JSON panel saw it silently vanish on save. + const policy: NonNullable = {}; if (typeof bp.supports_side_question === 'boolean') { - next.behavior_policy = { supports_side_question: bp.supports_side_question }; + policy.supports_side_question = bp.supports_side_question; } + if (typeof bp.supports_team === 'boolean') { + policy.supports_team = bp.supports_team; + } + if (Object.keys(policy).length > 0) next.behavior_policy = policy; } if (typeof p.description === 'string' && p.description.trim()) next.description = p.description; setAdvanced(next); 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; }; /** From 01936471bddd639ccdc3dfdcb5bc9374ba0d5630 Mon Sep 17 00:00:00 2001 From: Arnaud Palin Sainte Agathe Date: Tue, 25 Aug 2026 07:25:36 +0200 Subject: [PATCH 2/3] test(agent-editor): extract the advanced JSON round-trip and cover it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit, which fixed the dropped key without a regression test — the parsing lived in an inline `useCallback`, so it was not reachable from a unit test. - move the serialize/parse pair to `advancedOverrides.ts` (a .ts module, so the node-environment test does not pull React); - cover it in `tests/unit/settings/`: `supports_team` survives, both policy flags coexist, non-booleans are ignored, blanks are dropped, and the build → parse round-trip preserves what the panel shows. Behaviour is preserved, including two quirks now pinned by tests rather than left implicit: - valid JSON that is not an object (`"s"`, `null`) clears the error and keeps the previous bag, so the result is a three-way outcome (`ok` / `ignored` / `invalid`) rather than a nullable value; - an array parses as an empty bag, because `typeof [] === 'object'` has always let it through. Documented, not changed: this PR fixes a dropped key, not the panel's tolerance for odd input. Writing the tests caught the second point — the first draft asserted arrays were ignored, which would have been a silent behaviour change. --- .../AgentSettings/InlineAgentEditor.tsx | 54 +++------- .../AgentSettings/advancedOverrides.ts | 93 +++++++++++++++++ tests/unit/settings/advancedOverrides.test.ts | 99 +++++++++++++++++++ 3 files changed, 203 insertions(+), 43 deletions(-) create mode 100644 packages/desktop/src/renderer/pages/settings/AgentSettings/advancedOverrides.ts create mode 100644 tests/unit/settings/advancedOverrides.test.ts diff --git a/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx b/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx index 18cdcc76486..73ae7f08556 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,20 +259,9 @@ 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 ?? [], - // Both keys are shown in the skeleton so the panel is self-documenting: - // `supports_team` is otherwise undiscoverable, yet required for a custom - // ACP agent to be selectable in a team. - behavior_policy: advancedVal.behavior_policy ?? { supports_side_question: false, supports_team: 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. + const buildJsonFromAdvanced = useCallback((advancedVal: CustomAgentAdvancedOverrides) => buildAdvancedJson(advancedVal), []); useEffect(() => { if (!isJsonEditingRef.current) { @@ -308,36 +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; - // Build incrementally: assigning a fresh object per key would drop the - // sibling flag. `supports_team` was previously discarded here, so a user - // typing it into the JSON panel saw it silently vanish on save. - const policy: NonNullable = {}; - 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; - } - if (Object.keys(policy).length > 0) next.behavior_policy = policy; - } - 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/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 }, + }); + }); +}); From c8acef23a8e9d9a49319d1cc6d0a120123b8bb2c Mon Sep 17 00:00:00 2001 From: Arnaud Palin Sainte Agathe Date: Tue, 25 Aug 2026 07:41:29 +0200 Subject: [PATCH 3/3] style(agent-editor): oxfmt + drop the redundant useCallback wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI feedback: - Oxfmt reformatted `InlineAgentEditor.tsx` (one over-long line). Applied with the pinned oxfmt 0.41.0 so the hook is a no-op now. - Codecov flagged the patch: the remaining uncovered lines are the thin component-side binding, not the logic. `buildJsonFromAdvanced` was a `useCallback` wrapper around a module-level pure function — already stable across renders, so the indirection bought nothing and only added untestable lines. Calling `buildAdvancedJson` directly removes it and simplifies the effect's dependency array. oxlint reports 0 warnings / 0 errors on both new files. --- .../pages/settings/AgentSettings/InlineAgentEditor.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx b/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx index 73ae7f08556..c7c53abc9cc 100644 --- a/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx +++ b/packages/desktop/src/renderer/pages/settings/AgentSettings/InlineAgentEditor.tsx @@ -261,13 +261,13 @@ const InlineAgentEditor: React.FC = ({ agent, onSave, on // 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. - const buildJsonFromAdvanced = useCallback((advancedVal: CustomAgentAdvancedOverrides) => buildAdvancedJson(advancedVal), []); - + // 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');