feat: subagent selection from existing agents + UI cleanup

[Kaiohz]

Changes

Subagent selection from existing agents

• Subagents can only be added by selecting from existing agents via "Add from existing agents" dropdown
• SubAgentEditor simplified to reference-only mode: badge ref:, name read-only, description editable
• No more inline subagent creation

Agent config form cleanup

• Added "Description" input in General section
• Removed "Tools" accordion section (MCP-only now)
• Removed "Debug" toggle from General section
• Removed Tools display from agent config viewer
• Removed Debug badge from viewer

Modal overflow fix

• Added overflow-x-hidden on dialog scroll containers
• Added min-w-0 on Input base class and grid/flex children
• Inputs no longer overflow horizontally beyond modal boundaries

Other fixes

• Fixed AgentsPage is_builtin reference (field was removed from metadata)
• Added mcpApiBaseUrl to AppConfigSchema (needed for build)

Tests

• 725 unit tests pass (26 new tests for description/agent_ref/tools-removal/debug-removal/overflow)
• ESLint + Prettier clean
• Trivy: 0 new vulnerabilities (17 pre-existing HIGH in dependencies)
• QA: 18/18 E2E scenarios passed (form, viewer, overflow, edge cases)

[Kaiohz]

Code review — PR #23

Score: 7/10 — solid, well-tested refactor with good TDD discipline, but the PR is doing too much for a single change, the title is misleading, and there are a handful of dead-code/UX gaps worth fixing before merge.

---

1. PR scope vs. title (high impact)

The title says "subagent selection from existing agents + UI cleanup", but the diff also ships a full MCP Registry page (registry list/create/edit/delete, reveal endpoint, dedicated axios instance, 5 new hooks, new domain port + adapter, ~30+ new files). The first commit even rolls all of that up under a single feat commit (f8e41ed9), before the squash (28d081c6).

Suggestion: split the MCP registry work out into a separate PR. Two reasons:

• A reviewer can only meaningfully review ~300–500 LOC of new product surface at a time; this is ~3000 LOC mixed across two unrelated features.
• If the subagent refactor needs a hotfix, the registry goes with it on a revert.
• The "ci: make SonarQube non-blocking" commit is yet a third concern buried in the same PR.

The current squash + the description: "feat: subagent selection..." title help, but the title should mention the registry until the work is split.

---

2. SubAgent refactor — good

• SubAgentEditor correctly drops the inline creation surface (instructions/model/tools/skills editors) and becomes a read-only reference card (name derived from agent_ref, only description editable). That's the right direction — subagents should resolve at runtime from the referenced agent.
• AgentConfigForm.addFromExistingAgent correctly filters out self-reference (agent.name !== currentAgentName) and de-duplicates already-added refs. Nice touch.
• Badge "ref:" on the viewer is a good visual cue.
• The index prop is now dead in the interface — it's still typed but no longer destructured (and no longer used in JSX):

  interface SubAgentEditorProps {
    value: SubAgentConfig;
    onChange: (value: SubAgentConfig) => void;
    onRemove: () => void;
    index: number;   // ← not destructured, not used
  }

  Tests still pass index={0} harmlessly. Fix: drop it from the interface (and from the tests).

---

3. MCP Registry page — UX gap

McpRegistryPage.openEdit calls mcpRegistryApi.reveal(server.name) on every click, unconditionally and on every keystroke of nothing (just opening the dialog). For a registry of N servers, the list view masks secrets and reveal returns them in plaintext. A few concerns:

• No prefetch/cache. If a user opens → close → re-open the same server, you re-call /reveal. Consider keeping a revealCache: Map<name, RegisteredMcpServer> invalidated on useUpdateMcpServer mutation success.
• On reveal failure, you fall back to the masked server and open the dialog. The form fields are bound to editServer (masked) — users will think the field "doesn't show anything" and submit empty auth_token / headers, silently wiping the secret on save. This is a data-loss footgun. Consider: don't open the dialog at all on failure, or show a clear "Could not load secrets — try again" state.
• The "Add from registry" picker in McpServersAccordionItem.addFromRegistry also calls /reveal and re-uses the secrets as the new local McpServerConfig. That means any per-project override of those secrets is silently dropped on the way in (the local copy will track the registry value going forward). Worth a "Detach from registry?" affordance, or at least a docstring.

---

4. Minor code-quality nits

• AgentConfigViewer.tsx removes the Tools section from the viewer but AgentConfig.tools is still in the schema and parsed. A user with an old YAML containing tools: [...] will load fine, but the viewer silently hides them. Either drop tools from the schema/interface (matches the "MCP-only now" PR description) or keep showing them with a deprecation note.
• AgentConfigForm.tsx:151 — addFromExistingAgent does a linear find on every add. Trivial at the scale of agents a single human manages, but a Map keyed by name built once in a useMemo would be cleaner.
• McpServersAccordionItem inlines a second useMcpRegistry call. It's cheap (React Query dedupes), but co-locating the picker with the registry list page or lifting it to a provider would be more consistent.
• JsonBlock.tsx swallows clipboard errors silently (/* ignore */). On http:// (non-secure) or denied permission, the user clicks "Copy" and gets zero feedback. At least a toast.error on failure.
• agentConfigSchema.ts — description: z.string().nullable().optional() is duplicated 1:1 with system_prompt/system_prompt_file. Consider .extend(...) or a nullableOptionalString helper, the same way you presumably do for the other fields (e.g. instructions, model).
• appConfig.ts — mcpApiBaseUrl was added as a build-time required field with .or(z.literal("")).optional().default(""). That's a soft default; consider whether the McpRegistry page should refuse to render when the URL is empty (better UX than letting axios hit "" and produce a confusing 404).
• Continue-on-error: true on SonarQube is fine pragmatically, but please file a follow-up to bring the scan back to a blocking status once the server is stable. Silently swallowing CI signals accumulates tech debt.

---

5. Tests

• 26 new unit tests, all passing. Good TDD coverage of: description field, agent_ref plumbing, tools/debug removal, overflow classes. The negative tests (queryByText(/^tools$/i) etc.) are explicit about what should not render — nice.
• E2E scenarios 18/18 pass per the description.
• One small test smell: SubAgentEditor.test.tsx still passes index={0} to a component whose interface no longer uses it. If you keep index in the interface "for future use", leave a comment; otherwise remove it from the props + tests.

---

TL;DR

• Architecture direction: ✅ The subagent-as-ref model is the right call.
• MCP registry is a great addition but doesn't belong in this PR. Split it out.
• Fix the data-loss footgun in openEdit (reveal failure → masked fallback → save wipes secret) before merging.
• Remove the dead index prop.
• Either remove tools from the schema or keep showing it in the viewer.

Verdict: approve-on-condition — split the registry out, fix the reveal-failure UX, and remove the dead index prop. Then ship.

[Kaiohz]

Code review — feat: subagent selection from existing agents + UI cleanup

Clean PR, 88 fichiers / +2982 / -349 / 6 commits, CI ✅, mergeable ✅, 6 commits (squash recommended). Build solide, tests complets (26 nouveaux, 725 au total), architecture hexagonale respectée.

👍 Points forts

1. Architecture hexagonale exemplaire sur la nouvelle feature MCP registry : McpRegistryPort (domain) → mcpRegistryApi (infra) → 5 hooks (application) → composants. Boundaries nettes, faible couplage.
2. Tests exhaustifs et bien découpés : un fichier de test par couche (entity, schema, hook, API, dialog, page), fixtures dédiées, hoisting propre. Les 26 nouveaux tests ciblent les vrais chemins de code (external/openapi switching, validation, reveal, encodeURI, invalidation query key, baseURL fallback).
3. Masked vs revealed : la distinction auth_token: null (list/get) vs auth_token: "secret" (reveal) est bien documentée dans RegisteredMcpServer JSDoc, et le test supports a revealed entry where secrets are present verrouille le contrat.
4. Axios interceptor fix (status propagation dans l'Error) débloque storeApi.isNotFound après le unwrap du response. Patch minimal et bien testé (axiosInstance.test.ts + storeApi.test.ts).
5. Modal overflow fix propre : min-w-0 au niveau Input + overflow-x-hidden sur le scroll container. Les nouveaux tests input.test.tsx et la modification de McpServerEditor montrent que le fix est appliqué partout.
6. Subagent picker filtre correctement l'agent courant (anti self-ref) et les subagents déjà ajoutés. Bonne garde UX.
7. README mis à jour pour refléter les changements UI (Description, Tools removed, Debug removed, subagent ref-only) — souvent oublié.
8. Store-file-previews invalidation propagée à tous les hooks (create/delete/put) corrige un bug de cache stale post-mutation.
9. Squash-ready : 6 commits propres, le dernier revert/fix est acceptable mais prêt pour squash.

⚠️ Concerns / suggestions

1. SubAgentEditor — agent_ref undefined :

   • id={sub-name-${value.agent_ref}} → ID sub-name-undefined si jamais un subagent legacy sans agent_ref arrive (data migration incomplète, ou code appelant qui passe un objet invalide).
   • <Badge>ref: {value.agent_ref}</Badge> rend ref: undefined en texte — laid.
   • Le SubAgentEditor est désormais un composant purement "ref-mode" mais reste générique sur le type. Considérer un guard en haut du composant : if (!value.agent_ref) return null; (ou throw en dev), ou un type discriminant SubAgentConfig & { mode: "ref" }. Aujourd'hui, le useFieldArray.append({...EMPTY_SUBAGENT, agent_ref: ...}) est la seule voie d'entrée grâce à la PR, mais rien dans le composant ne le documente.

2. addFromRegistry — error swallowing :
   Dans AgentConfigForm :

   const addFromRegistry = useCallback(
     async (name: string) => {
       const revealed = await mcpRegistryApi.reveal(name);  // <-- si reject, l'erreur remonte non capturée
       mcpServersArray.append(serverConfig);
     },
     ...
   );

   Comparer avec McpRegistryPage.openEdit qui wrappe dans try/catch + toast.error. Une reveal qui échoue (réseau, 500) fait juste crasher silencieusement la Select.onValueChange. Recommandation : try/catch + toast.error(extractApiMessage(err)) pour symétrie.

3. mcpAxiosInstance.cachedMcpBaseURL — cache forever :

   let cachedMcpBaseURL: string | null = null;
   mcpApiClient.interceptors.request.use(async (config) => {
     if (!cachedMcpBaseURL) { ... cachedMcpBaseURL = appConfig.mcpApiBaseUrl || ... }
     config.baseURL = cachedMcpBaseURL;
   });

   Le baseURL est mis en cache une fois pour toute la lifetime de l'app. Acceptable en SPA (config chargée au boot) mais aucun commentaire ne le dit. Soit documenter, soit exposer un setMcpApiBaseUrl(url) si un hot-reload est envisagé.

4. storeApi.isNotFound — double chemin :

   if ("status" in error && (error as { status?: number }).status === 404) return true;
   if ("response" in error) {
     const status = (error as { response?: { status?: number } }).response?.status;
     return status === 404;
   }

   Les deux branches correspondent à : (a) erreur déjà wrappée par l'interceptor, (b) erreur axios brute (avant passage par l'interceptor — donc depuis un test ou un mock qui bypasse). Le test returns null when the raw axios error has response.status 404 couvre explicitement (b). C'est correct mais fragile : si un futur code appelle apiClient.get directement sans passer par l'interceptor (ex. un test), la branche (a) échouera. Une alternative : forcer systématiquement l'interceptor à toujours transformer (un seul format d'erreur). Aujourd'hui, on accepte les deux — c'est documenté implicitement par le commit 99539278 fix: store file 404 status lost by axios interceptor unwrap mais pas dans le code lui-même.

5. McpServerCard — server.url ?? "—" :
   Le displayedUrl est openapi_url ?? url ?? "—". Si un serveur openapi n'a pas openapi_url (données corrompues), on tombe sur server.url qui peut être "" (empty string pour openapi), donc l'affichage devient —. C'est OK fonctionnellement, mais le rendu d'un tiret dans une card pour un cas marginal peut induire en erreur. Suggestion : afficher un placeholder explicite "No URL configured" plutôt que —.

6. CreateMcpServerDialog — resetForm ne reset pas isOpenapi via sourceType :

   function resetForm() {
     setSourceType("external");  // OK
     setName("");
     setUrl("");
     setOpenapiUrl("");
     ...
   }

   Le setSourceType("external") est correct mais désactivé en mode edit (disabled={isEdit}), donc si l'utilisateur ouvre en mode edit (openapi) puis ferme puis rouvre en create, le state React est-il réinitialisé ? En réalité, le composant n'est pas démonté entre edit→close (le open={createDialogOpen} est contrôlé par la page parente). Donc le state persiste. Mais le resetForm n'est appelé qu'à handleClose, donc OK. À vérifier : si l'edit passe de openapi → external → create (nouveau), est-ce que sourceType reset bien ? En lisant le flow, oui (le state est dans le composant, et l'editServer change déclenche un useState initial). Mais c'est subtil — un test de régression "switch type in edit, close, reopen in create → defaults to external" serait utile.

7. MCP API : validate ne suit pas le pattern CRUD :

   async validate(input: McpServerInput): Promise<McpServerValidationResult> {
     const response = await mcpApiClient.post<McpServerValidationResult>(`${BASE}/validate`, input);

   L'endpoint est /api/v1/mcp/servers/validate — un POST sur ce path pourrait entrer en conflit avec /api/v1/mcp/servers/{name} si jamais name="validate" est créé. Risque faible (les noms sont validés côté UI), mais à garder en tête. Le test posts the input to POST /api/v1/mcp/servers/validate ne le couvre pas explicitement (pas de test de régression "POST /api/v1/mcp/servers/validate ne capture pas l'input comme un create avec name='validate'").

8. Pas de test E2E pour le Picker : les tests unitaires de AgentConfigForm couvrent "expand Subagents → option appears → click → ref badge shows". Mais pas le scénario "user selects the same agent twice → second select is absent from picker" (filtrage des subagents déjà ajoutés). Le test clicking an existing agent in the picker adds a subagent ne re-teste pas après un 1er add. Suggestion : un test "add same agent twice → second attempt is filtered out of options".

💡 Nitpicks

• axiosInstance.ts : wrapped.status = error.response.status — TS pourrait se plaindre (Error n'a pas de status par défaut). Le cast as Error & { status?: number } est OK, mais un petit commentaire "augmented Error with status for downstream consumers" aiderait.
• mcpApiClient.interceptors.response.use ne wrappe pas avec status non plus — incohérence entre apiClient et mcpApiClient : le 1er préserve status, le 2nd non. Si demain un consumer de MCP veut distinguer 404 vs 500, il devra refaire la même gymnastique.
• McpServerInput.headers est Record<string, string> mais l'UI permet de saisir des valeurs vides (vide → clé conservée avec valeur ""). Le backend peut le rejeter mais aucun test ne couvre ce cas. Suggestion : trim ou filter les paires vides avant submit.
• Le data-od-id="json-block" sur JsonBlock est un test-hook qui pollue un peu le DOM de prod — à confirmer que c'est la convention du projet (les autres composants l'utilisent : mcp-source-type, mcp-server-card-xxx, agent-count...). Si oui, c'est OK.

📊 Score : 8/10

Code de très bonne qualité, PR bien structurée, tests au rendez-vous. Les 8 points ci-dessus sont surtout de la robustesse/defensive programming, pas de bugs bloquants. Squash les 6 commits en 1, merge après avoir tranché sur les concerns #1, #2, #5.

✅ Ready to merge après :

• Décision sur le comportement SubAgentEditor quand agent_ref est absent (concern #1)
• Harmonisation du error handling addFromRegistry avec openEdit (concern #2)
• (Optionnel) Test de régression sur le filtrage anti-doublon du picker (concern #8)

Squash des commits + merge. Beau boulot 👏