From bb7e5d41088a84831ddfb3dc0cc054339d941b0e Mon Sep 17 00:00:00 2001 From: David Kaya Date: Fri, 27 Mar 2026 21:50:01 +0100 Subject: [PATCH] fix: preserve editable pattern graphs Add mode-aware graph mutation helpers for pattern topology so backend code can add and remove agents without forcing a full graph rebuild. Also preserve an existing pattern graph in savePattern instead of re-syncing it from defaults, and cover the new behavior with shared and main-process tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/main/AryxAppService.ts | 2 +- src/shared/domain/pattern.ts | 223 ++++++++++++++++++++++ tests/main/appServicePattern.test.ts | 103 ++++++++++ tests/shared/pattern.test.ts | 269 +++++++++++++++++++++++++++ 4 files changed, 596 insertions(+), 1 deletion(-) create mode 100644 tests/main/appServicePattern.test.ts diff --git a/src/main/AryxAppService.ts b/src/main/AryxAppService.ts index 494c1a8..210472f 100644 --- a/src/main/AryxAppService.ts +++ b/src/main/AryxAppService.ts @@ -345,7 +345,7 @@ export class AryxAppService extends EventEmitter { async savePattern(pattern: PatternDefinition): Promise { const workspace = await this.loadWorkspace(); const knownApprovalToolNames = await this.listKnownApprovalToolNames(workspace); - const synchronizedPattern = syncPatternGraph(pattern); + const synchronizedPattern = pattern.graph ? pattern : syncPatternGraph(pattern); const issues = validatePatternDefinition( synchronizedPattern, knownApprovalToolNames, diff --git a/src/shared/domain/pattern.ts b/src/shared/domain/pattern.ts index 75c0ce1..44467c1 100644 --- a/src/shared/domain/pattern.ts +++ b/src/shared/domain/pattern.ts @@ -311,6 +311,229 @@ export function createDefaultPatternGraph( } } +function sortAgentNodesByOrder(nodes: ReadonlyArray): PatternGraphNode[] { + return nodes + .filter((node) => node.kind === 'agent') + .slice() + .sort((left, right) => + (left.order ?? Number.MAX_SAFE_INTEGER) - (right.order ?? Number.MAX_SAFE_INTEGER) + || left.id.localeCompare(right.id)); +} + +function renumberAgentOrders(nodes: ReadonlyArray): PatternGraphNode[] { + const orders = new Map(sortAgentNodesByOrder(nodes).map((node, index) => [node.id, index])); + return nodes.map((node) => + node.kind === 'agent' + ? { ...node, order: orders.get(node.id) ?? node.order ?? 0 } + : node); +} + +function getNextAgentNodePosition(graph: PatternGraph): PatternGraphPosition { + const existingAgentNodes = getAgentNodes(graph); + const maxY = existingAgentNodes.reduce((max, node) => Math.max(max, node.position.y), 0); + const avgX = existingAgentNodes.length > 0 + ? Math.round(existingAgentNodes.reduce((sum, node) => sum + node.position.x, 0) / existingAgentNodes.length) + : 400; + + return { + x: avgX, + y: maxY + 120, + }; +} + +function addUniqueEdge( + edges: ReadonlyArray, + source: string | undefined, + target: string | undefined, +): PatternGraphEdge[] { + if (!source || !target || edges.some((edge) => edge.source === source && edge.target === target)) { + return [...edges]; + } + + return [...edges, createEdge(source, target)]; +} + +function removeEdgesConnectedToNode( + edges: ReadonlyArray, + nodeId: string, +): PatternGraphEdge[] { + return edges.filter((edge) => edge.source !== nodeId && edge.target !== nodeId); +} + +function findAgentNode(graph: PatternGraph, agentId: string): PatternGraphNode | undefined { + return graph.nodes.find((node) => node.kind === 'agent' && node.agentId === agentId); +} + +function resolveEntryAgentNodeId(graph: PatternGraph): string | undefined { + const { outgoing } = buildAdjacency(graph); + const agentNodeIds = new Set(getAgentNodes(graph).map((node) => node.id)); + const directEntry = (outgoing.get(SYSTEM_NODE_IDS.userInput) ?? []) + .map((edge) => edge.target) + .find((nodeId) => agentNodeIds.has(nodeId)); + + return directEntry ?? sortAgentNodesByOrder(graph.nodes)[0]?.id; +} + +function hasAgentToAgentRoute(graph: PatternGraph): boolean { + const agentNodeIds = new Set(getAgentNodes(graph).map((node) => node.id)); + return graph.edges.some((edge) => + agentNodeIds.has(edge.source) + && agentNodeIds.has(edge.target) + && edge.source !== edge.target); +} + +export function addAgentToGraph( + graph: PatternGraph, + mode: OrchestrationMode, + agent: PatternAgentDefinition, +): PatternGraph { + if (mode === 'single') { + throw new Error('Single-agent chat requires exactly one agent.'); + } + + if (findAgentNode(graph, agent.id)) { + return graph; + } + + const existingAgentNodes = sortAgentNodesByOrder(graph.nodes); + const newNode = createAgentNode( + agent, + existingAgentNodes.length, + getNextAgentNodePosition(graph), + ); + let nextEdges = [...graph.edges]; + + switch (mode) { + case 'sequential': + case 'magentic': { + const outputNode = getNodeByKind(graph, 'user-output'); + const inputNode = getNodeByKind(graph, 'user-input'); + const { incoming } = buildAdjacency(graph); + const previousNodeId = (outputNode ? incoming.get(outputNode.id)?.[0]?.source : undefined) + ?? existingAgentNodes[existingAgentNodes.length - 1]?.id + ?? inputNode?.id; + + if (outputNode && previousNodeId) { + nextEdges = nextEdges.filter((edge) => !(edge.source === previousNodeId && edge.target === outputNode.id)); + nextEdges = addUniqueEdge(nextEdges, previousNodeId, newNode.id); + nextEdges = addUniqueEdge(nextEdges, newNode.id, outputNode.id); + } + break; + } + case 'concurrent': { + const distributorNode = getNodeByKind(graph, 'distributor'); + const collectorNode = getNodeByKind(graph, 'collector'); + nextEdges = addUniqueEdge(nextEdges, distributorNode?.id, newNode.id); + nextEdges = addUniqueEdge(nextEdges, newNode.id, collectorNode?.id); + break; + } + case 'handoff': { + const inputNode = getNodeByKind(graph, 'user-input'); + const outputNode = getNodeByKind(graph, 'user-output'); + const entryNodeId = resolveEntryAgentNodeId(graph); + + if (entryNodeId) { + nextEdges = addUniqueEdge(nextEdges, entryNodeId, newNode.id); + nextEdges = addUniqueEdge(nextEdges, newNode.id, entryNodeId); + } else { + nextEdges = addUniqueEdge(nextEdges, inputNode?.id, newNode.id); + } + + nextEdges = addUniqueEdge(nextEdges, newNode.id, outputNode?.id); + break; + } + case 'group-chat': { + const orchestratorNode = getNodeByKind(graph, 'orchestrator'); + nextEdges = addUniqueEdge(nextEdges, orchestratorNode?.id, newNode.id); + nextEdges = addUniqueEdge(nextEdges, newNode.id, orchestratorNode?.id); + break; + } + default: + break; + } + + return { + nodes: renumberAgentOrders([...graph.nodes, newNode]), + edges: nextEdges, + }; +} + +export function removeAgentFromGraph( + graph: PatternGraph, + mode: OrchestrationMode, + agentId: string, +): PatternGraph { + const nodeToRemove = findAgentNode(graph, agentId); + if (!nodeToRemove) { + return graph; + } + + const originalAgentNodes = sortAgentNodesByOrder(graph.nodes); + if (mode === 'single' && originalAgentNodes.length <= 1) { + throw new Error('Single-agent chat requires exactly one agent.'); + } + + const { incoming, outgoing } = buildAdjacency(graph); + const removedWasEntry = (outgoing.get(SYSTEM_NODE_IDS.userInput) ?? []).some( + (edge) => edge.target === nodeToRemove.id, + ); + const remainingNodes = graph.nodes.filter((node) => node.id !== nodeToRemove.id); + let nextEdges = removeEdgesConnectedToNode(graph.edges, nodeToRemove.id); + + switch (mode) { + case 'sequential': + case 'magentic': { + const predecessorId = incoming.get(nodeToRemove.id)?.[0]?.source; + const successorId = outgoing.get(nodeToRemove.id)?.[0]?.target; + nextEdges = addUniqueEdge( + nextEdges, + predecessorId && predecessorId !== nodeToRemove.id ? predecessorId : undefined, + successorId && successorId !== nodeToRemove.id ? successorId : undefined, + ); + break; + } + case 'handoff': { + const remainingGraph: PatternGraph = { nodes: remainingNodes, edges: nextEdges }; + const remainingAgentNodes = sortAgentNodesByOrder(remainingNodes); + const outputNode = getNodeByKind(remainingGraph, 'user-output'); + const entryNodeId = resolveEntryAgentNodeId(remainingGraph) ?? remainingAgentNodes[0]?.id; + + if (entryNodeId) { + const { outgoing: remainingOutgoing, incoming: remainingIncoming } = buildAdjacency(remainingGraph); + if ((remainingOutgoing.get(SYSTEM_NODE_IDS.userInput)?.length ?? 0) === 0) { + nextEdges = addUniqueEdge(nextEdges, SYSTEM_NODE_IDS.userInput, entryNodeId); + } + + if (removedWasEntry || (remainingIncoming.get(outputNode?.id ?? '')?.length ?? 0) === 0) { + nextEdges = addUniqueEdge(nextEdges, entryNodeId, outputNode?.id); + } + + if (remainingAgentNodes.length > 1 && (removedWasEntry || !hasAgentToAgentRoute(remainingGraph))) { + for (const specialistNode of remainingAgentNodes) { + if (specialistNode.id === entryNodeId) { + continue; + } + + nextEdges = addUniqueEdge(nextEdges, entryNodeId, specialistNode.id); + nextEdges = addUniqueEdge(nextEdges, specialistNode.id, entryNodeId); + nextEdges = addUniqueEdge(nextEdges, specialistNode.id, outputNode?.id); + } + } + } + break; + } + case 'concurrent': + case 'group-chat': + default: + break; + } + + return { + nodes: renumberAgentOrders(remainingNodes), + edges: nextEdges, + }; +} + export function resolvePatternGraph(pattern: PatternDefinition): PatternGraph { return pattern.graph ?? createDefaultPatternGraph(pattern); } diff --git a/tests/main/appServicePattern.test.ts b/tests/main/appServicePattern.test.ts new file mode 100644 index 0000000..aa16c40 --- /dev/null +++ b/tests/main/appServicePattern.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, mock, test } from 'bun:test'; + +import type { PatternDefinition } from '@shared/domain/pattern'; +import { resolvePatternGraph } from '@shared/domain/pattern'; +import { createWorkspaceSeed, type WorkspaceState } from '@shared/domain/workspace'; + +const TIMESTAMP = '2026-03-27T00:00:00.000Z'; + +mock.module('electron', () => { + const electronMock = { + app: { + isPackaged: false, + getAppPath: () => 'C:\\workspace\\personal\\repositories\\aryx', + getPath: () => 'C:\\workspace\\personal\\repositories\\aryx\\tests\\fixtures', + }, + dialog: { + showOpenDialog: async () => ({ canceled: true, filePaths: [] }), + }, + shell: { + openPath: async () => '', + }, + }; + + return { + ...electronMock, + default: electronMock, + }; +}); + +mock.module('keytar', () => ({ + default: { + getPassword: async () => null, + setPassword: async () => undefined, + deletePassword: async () => false, + }, +})); + +const { AryxAppService } = await import('@main/AryxAppService'); + +function createService( + workspace: WorkspaceState, + options?: { knownApprovalToolNames?: string[] }, +): InstanceType { + const service = new AryxAppService(); + const internals = service as unknown as Record; + internals.loadWorkspace = async () => workspace; + internals.persistAndBroadcast = async (nextWorkspace: WorkspaceState) => nextWorkspace; + internals.listKnownApprovalToolNames = async () => options?.knownApprovalToolNames ?? ['read', 'write', 'shell']; + + return service; +} + +function requirePattern(workspace: WorkspaceState, mode: PatternDefinition['mode']): PatternDefinition { + const pattern = workspace.patterns.find((candidate) => candidate.mode === mode); + if (!pattern) { + throw new Error(`Expected workspace seed to include a ${mode} pattern.`); + } + + return pattern; +} + +describe('AryxAppService savePattern', () => { + test('preserves a provided custom graph instead of re-syncing it', async () => { + const workspace = createWorkspaceSeed(); + const pattern = requirePattern(workspace, 'sequential'); + const baseGraph = resolvePatternGraph(pattern); + const customGraph = { + ...baseGraph, + nodes: baseGraph.nodes.map((node, index) => ({ + ...node, + position: { + x: node.position.x + 37 + index, + y: node.position.y + 19 + index * 7, + }, + })), + }; + const service = createService(workspace); + + const result = await service.savePattern({ + ...pattern, + graph: customGraph, + updatedAt: TIMESTAMP, + }); + + const savedPattern = requirePattern(result, 'sequential'); + expect(savedPattern.graph).toEqual(customGraph); + }); + + test('still seeds a default graph when the pattern graph is missing', async () => { + const workspace = createWorkspaceSeed(); + const pattern = requirePattern(workspace, 'group-chat'); + const service = createService(workspace); + + const result = await service.savePattern({ + ...pattern, + graph: undefined, + updatedAt: TIMESTAMP, + }); + + const savedPattern = requirePattern(result, 'group-chat'); + expect(savedPattern.graph).toEqual(resolvePatternGraph(pattern)); + }); +}); diff --git a/tests/shared/pattern.test.ts b/tests/shared/pattern.test.ts index 336eaa9..5220b8c 100644 --- a/tests/shared/pattern.test.ts +++ b/tests/shared/pattern.test.ts @@ -1,14 +1,28 @@ import { describe, expect, test } from 'bun:test'; import { + addAgentToGraph, createBuiltinPatterns, + removeAgentFromGraph, resolvePatternGraph, syncPatternGraph, + type PatternAgentDefinition, validatePatternDefinition, } from '@shared/domain/pattern'; const BUILTIN_TIMESTAMP = '2026-03-22T00:00:00.000Z'; +function createAgent(id: string, name = `Agent ${id}`): PatternAgentDefinition { + return { + id, + name, + description: `${name} description`, + instructions: `${name} instructions`, + model: 'gpt-5.4', + reasoningEffort: 'medium', + }; +} + describe('pattern validation', () => { test('builtin patterns are valid except explicitly unavailable modes', () => { const patterns = createBuiltinPatterns(BUILTIN_TIMESTAMP); @@ -242,6 +256,261 @@ describe('pattern validation', () => { ); }); + test('addAgentToGraph appends a new sequential agent before user output', () => { + const sequential = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'sequential', + ); + + expect(sequential).toBeDefined(); + + const updatedGraph = addAgentToGraph( + resolvePatternGraph(sequential!), + sequential!.mode, + createAgent('agent-sequential-final', 'Final Reviewer'), + ); + + expect(updatedGraph.nodes.filter((node) => node.kind === 'agent')).toHaveLength(4); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-sequential-reviewer', + target: 'agent-node-agent-sequential-final', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-sequential-final', + target: 'system-user-output', + }), + ); + expect(updatedGraph.edges).not.toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-sequential-reviewer', + target: 'system-user-output', + }), + ); + }); + + test('addAgentToGraph wires concurrent agents between the distributor and collector', () => { + const concurrent = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'concurrent', + ); + + expect(concurrent).toBeDefined(); + + const updatedGraph = addAgentToGraph( + resolvePatternGraph(concurrent!), + concurrent!.mode, + createAgent('agent-concurrent-final', 'Final Implementer'), + ); + + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'system-distributor', + target: 'agent-node-agent-concurrent-final', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-concurrent-final', + target: 'system-collector', + }), + ); + }); + + test('addAgentToGraph wires handoff specialists to the entry agent and output', () => { + const handoff = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'handoff', + ); + + expect(handoff).toBeDefined(); + + const updatedGraph = addAgentToGraph( + resolvePatternGraph(handoff!), + handoff!.mode, + createAgent('agent-handoff-docs', 'Docs Specialist'), + ); + + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-handoff-triage', + target: 'agent-node-agent-handoff-docs', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-handoff-docs', + target: 'agent-node-agent-handoff-triage', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-handoff-docs', + target: 'system-user-output', + }), + ); + }); + + test('addAgentToGraph wires group-chat agents to the orchestrator', () => { + const groupChat = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'group-chat', + ); + + expect(groupChat).toBeDefined(); + + const updatedGraph = addAgentToGraph( + resolvePatternGraph(groupChat!), + groupChat!.mode, + createAgent('agent-group-editor', 'Editor'), + ); + + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'system-orchestrator', + target: 'agent-node-agent-group-editor', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-group-editor', + target: 'system-orchestrator', + }), + ); + }); + + test('addAgentToGraph rejects additions in single-agent mode', () => { + const single = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'single', + ); + + expect(single).toBeDefined(); + expect(() => + addAgentToGraph(resolvePatternGraph(single!), single!.mode, createAgent('agent-extra', 'Extra Agent'))) + .toThrow('Single-agent chat requires exactly one agent.'); + }); + + test('removeAgentFromGraph stitches linear gaps and re-numbers remaining agent orders', () => { + const sequential = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'sequential', + ); + + expect(sequential).toBeDefined(); + + const updatedGraph = removeAgentFromGraph( + resolvePatternGraph(sequential!), + sequential!.mode, + 'agent-sequential-builder', + ); + + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-sequential-analyst', + target: 'agent-node-agent-sequential-reviewer', + }), + ); + expect(updatedGraph.edges).not.toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-sequential-analyst', + target: 'agent-node-agent-sequential-builder', + }), + ); + expect(updatedGraph.edges).not.toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-sequential-builder', + target: 'agent-node-agent-sequential-reviewer', + }), + ); + expect( + updatedGraph.nodes + .filter((node) => node.kind === 'agent') + .map((node) => node.order), + ).toEqual([0, 1]); + }); + + test('removeAgentFromGraph cleans up concurrent fan-out and fan-in edges', () => { + const concurrent = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'concurrent', + ); + + expect(concurrent).toBeDefined(); + + const updatedGraph = removeAgentFromGraph( + resolvePatternGraph(concurrent!), + concurrent!.mode, + 'agent-concurrent-product', + ); + + expect(updatedGraph.nodes.some((node) => node.agentId === 'agent-concurrent-product')).toBe(false); + expect(updatedGraph.edges.some((edge) => edge.target === 'agent-node-agent-concurrent-product')).toBe(false); + expect(updatedGraph.edges.some((edge) => edge.source === 'agent-node-agent-concurrent-product')).toBe(false); + }); + + test('removeAgentFromGraph rewires a removed handoff entry agent to the next specialist', () => { + const handoff = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'handoff', + ); + + expect(handoff).toBeDefined(); + + const updatedGraph = removeAgentFromGraph( + resolvePatternGraph(handoff!), + handoff!.mode, + 'agent-handoff-triage', + ); + + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'system-user-input', + target: 'agent-node-agent-handoff-ux', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-handoff-ux', + target: 'system-user-output', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-handoff-ux', + target: 'agent-node-agent-handoff-runtime', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-handoff-runtime', + target: 'agent-node-agent-handoff-ux', + }), + ); + }); + + test('removeAgentFromGraph preserves orchestrator routes for the remaining group-chat agents', () => { + const groupChat = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( + (pattern) => pattern.mode === 'group-chat', + ); + + expect(groupChat).toBeDefined(); + + const updatedGraph = removeAgentFromGraph( + resolvePatternGraph(groupChat!), + groupChat!.mode, + 'agent-group-reviewer', + ); + + expect(updatedGraph.nodes.some((node) => node.agentId === 'agent-group-reviewer')).toBe(false); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'system-orchestrator', + target: 'agent-node-agent-group-writer', + }), + ); + expect(updatedGraph.edges).toContainEqual( + expect.objectContaining({ + source: 'agent-node-agent-group-writer', + target: 'system-orchestrator', + }), + ); + }); + test('graph validation rejects branched sequential topology', () => { const sequential = createBuiltinPatterns(BUILTIN_TIMESTAMP).find( (pattern) => pattern.mode === 'sequential',