fix(ci): resolve remaining E2E instability root causes

linonetwo · linonetwo · commit faf84b2b9762 · 2026-06-01T19:28:37.000+08:00
- crossWindowSync: replace watch-fs marker wait with file content polling
  step to correctly verify browser-originated syncer writes
- workspace DnD: hybrid rect (dnd-kit top + live DOM height) prevents stale
  cached heights from misclassifying center drops as reorder-after
- agent cleanup: mergeWithDefaultAgent on read, partial-update undefined filter,
  deleteAgentDef temp-only guard with AgentInstanceService lifecycle cleanup,
  ref-based unmount-only cleanup in CreateNewAgentContent
- add unit tests for default merge semantics and deleteAgentDef temp guard
diff --git a/features/crossWindowSync.feature b/features/crossWindowSync.feature
@@ -22,7 +22,7 @@ Feature: Cross-Window Synchronization
     # Edit Index tiddler in window A via TW syncer (this triggers save to server → disk)
     When I execute TiddlyWiki code in browser view: "$tw.wiki.addTiddler(new $tw.Tiddler({title: 'Index', text: 'CrossWindowSyncTestContent123'}))"
     # Wait for the syncer to save the tiddler to disk via server, so window B sees it
-    Then I wait for tiddler "Index" to be updated by watch-fs
+    Then tiddler "Index" in workspace "wiki" should be saved to disk with text "CrossWindowSyncTestContent123"
     
     # Open workspace in a new window (window B)
     When I open workspace "wiki" in a new window
diff --git a/features/stepDefinitions/wiki.ts b/features/stepDefinitions/wiki.ts
@@ -756,6 +756,43 @@ Then('I wait for tiddler {string} to be deleted by watch-fs', async function(thi
   );
 });
 
+/**
+ * Poll disk until a tiddler .tid file exists and contains expected text.
+ * Replaces watch-fs log-marker waits for browser-originated writes, which are
+ * self-writes through the syncer and correctly suppressed by watch-fs echo prevention.
+ */
+Then('tiddler {string} in workspace {string} should be saved to disk with text {string}', async function(
+  this: ApplicationWorld,
+  tiddlerTitle: string,
+  workspaceName: string,
+  expectedText: string,
+) {
+  const wikiPath = getWikiTestWikiPath(this);
+  const tiddlerPath = path.join(wikiPath, 'tiddlers', `${tiddlerTitle}.tid`);
+
+  try {
+    await backOff(
+      async () => {
+        if (!await fs.pathExists(tiddlerPath)) {
+          throw new Error(`Tiddler file not found yet: ${tiddlerPath}`);
+        }
+        const content = await fs.readFile(tiddlerPath, 'utf-8');
+        if (!content.includes(expectedText)) {
+          throw new Error(`Tiddler file does not contain expected text yet: ${tiddlerPath}`);
+        }
+      },
+      BACKOFF_OPTIONS,
+    );
+  } catch {
+    const exists = await fs.pathExists(tiddlerPath);
+    const content = exists ? await fs.readFile(tiddlerPath, 'utf-8') : '(file does not exist)';
+    throw new Error(
+      `Tiddler "${tiddlerTitle}" was not saved to disk with text "${expectedText}" in workspace "${workspaceName}". ` +
+        `Path: ${tiddlerPath}\nFile content:\n${content}`,
+    );
+  }
+});
+
 // File manipulation step definitions
 
 When('I create file {string} with content:', async function(this: ApplicationWorld, filePath: string, content: string) {
diff --git a/src/pages/Agent/TabContent/TabTypes/CreateNewAgentContent.tsx b/src/pages/Agent/TabContent/TabTypes/CreateNewAgentContent.tsx
@@ -7,7 +7,7 @@ import type { AgentDefinition } from '@services/agentDefinition/interface';
 import { AgentFrameworkConfig } from '@services/agentInstance/promptConcat/promptConcatSchema';
 import useDebouncedCallback from 'beautiful-react-hooks/useDebouncedCallback';
 import { nanoid } from 'nanoid';
-import React, { useCallback, useEffect, useState } from 'react';
+import React, { useCallback, useEffect, useRef, useState } from 'react';
 import { useTranslation } from 'react-i18next';
 import { TemplateSearch } from '../../components/Search/TemplateSearch';
 import { useTabStore } from '../../store/tabStore';
@@ -63,6 +63,16 @@ export const CreateNewAgentContent: React.FC<CreateNewAgentContentProps> = ({ ta
   const [previewAgentId, setPreviewAgentId] = useState<string | null>(null);
   const [isLoading, setIsLoading] = useState(false);
   const [promptSchema, setPromptSchema] = useState<RJSFSchema | null>(null);
+  const temporaryAgentDefinitionIdReference = useRef<string | null>(tab.agentDefId ?? null);
+  const previewAgentIdReference = useRef<string | null>(null);
+
+  useEffect(() => {
+    temporaryAgentDefinitionIdReference.current = temporaryAgentDefinition?.id ?? tab.agentDefId ?? null;
+  }, [tab.agentDefId, temporaryAgentDefinition?.id]);
+
+  useEffect(() => {
+    previewAgentIdReference.current = previewAgentId;
+  }, [previewAgentId]);
 
   // Restore state from backend when component mounts
   useEffect(() => {
@@ -173,26 +183,30 @@ export const CreateNewAgentContent: React.FC<CreateNewAgentContentProps> = ({ ta
   // Cleanup when component unmounts or tab closes
   useEffect(() => {
     return () => {
-      // Cleanup temporary agent definition and preview agent when tab closes
+      // Cleanup temporary agent definition and preview agent when tab closes.
+      // Delete preview agent instance BEFORE definition to avoid FK constraint failures.
       const cleanup = async () => {
-        if (temporaryAgentDefinition?.id && temporaryAgentDefinition.id.startsWith('temp-')) {
+        const currentPreviewAgentId = previewAgentIdReference.current;
+        const currentTemporaryAgentDefinitionId = temporaryAgentDefinitionIdReference.current;
+
+        if (currentPreviewAgentId) {
           try {
-            await window.service.agentDefinition.deleteAgentDef(temporaryAgentDefinition.id);
+            await window.service.agentInstance.deleteAgent(currentPreviewAgentId);
           } catch (error) {
-            console.error('Failed to cleanup temporary agent definition:', error);
+            console.error('Failed to cleanup preview agent:', error);
           }
         }
-        if (previewAgentId) {
+        if (currentTemporaryAgentDefinitionId?.startsWith('temp-')) {
           try {
-            await window.service.agentInstance.deleteAgent(previewAgentId);
+            await window.service.agentDefinition.deleteAgentDef(currentTemporaryAgentDefinitionId);
           } catch (error) {
-            console.error('Failed to cleanup preview agent:', error);
+            console.error('Failed to cleanup temporary agent definition:', error);
           }
         }
       };
       void cleanup();
     };
-  }, [temporaryAgentDefinition?.id, previewAgentId]);
+  }, []);
 
   const handleNext = async () => {
     if (currentStep < STEPS.length - 1) {
diff --git a/src/pages/Main/WorkspaceIconAndSelector/SortableWorkspaceSelectorList.tsx b/src/pages/Main/WorkspaceIconAndSelector/SortableWorkspaceSelectorList.tsx
@@ -833,13 +833,17 @@ export function SortableWorkspaceSelectorList({ workspacesList, showSideBarText,
       const canGroup = !isSameGroup && isGroupableWorkspace(activeWorkspace) && isGroupableWorkspace(resolvedOverWorkspace);
       let intent: TDragIntent;
 
+      const liveRect = _getLiveSortableRect(resolvedOverId, overRect);
+      // Keep dnd-kit's coordinate frame for pointer-vs-top math, but refresh
+      // height from the DOM so stale cached heights do not skew zone boundaries.
+      const intentRect = { top: overRect.top, height: liveRect?.height ?? overRect.height };
       const reorderIntent = getReorderIntentFromPointer({
         pointerY: referenceY,
-        rect: overRect,
+        rect: intentRect,
       });
-      const relativeY = Math.min(Math.max(referenceY - overRect.top, 0), overRect.height);
-      const beforeBoundary = overRect.height / 3;
-      const afterBoundary = overRect.height - beforeBoundary;
+      const relativeY = Math.min(Math.max(referenceY - intentRect.top, 0), intentRect.height);
+      const beforeBoundary = intentRect.height / 3;
+      const afterBoundary = intentRect.height - beforeBoundary;
 
       if (relativeY > beforeBoundary && relativeY < afterBoundary && canGroup) {
         intent = 'group';
diff --git a/src/services/agentDefinition/__tests__/index.test.ts b/src/services/agentDefinition/__tests__/index.test.ts
@@ -4,7 +4,7 @@ import defaultAgents from '@services/agentInstance/agentFrameworks/taskAgents.js
 import type { IAgentInstanceService } from '@services/agentInstance/interface';
 import { container } from '@services/container';
 import type { IDatabaseService } from '@services/database/interface';
-import { AgentDefinitionEntity } from '@services/database/schema/agent';
+import { AgentDefinitionEntity, AgentInstanceEntity } from '@services/database/schema/agent';
 import serviceIdentifier from '@services/serviceIdentifier';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
@@ -99,13 +99,14 @@ describe('AgentDefinitionService getAgentDefs integration', () => {
     expect(typeof exampleAgent!.agentFrameworkConfig).toBe('object');
   });
 
-  it('should return only database data without fallback to defaultAgents', async () => {
+  it('should merge default agent fields when database stores only user overrides', async () => {
     // Get the real database repository that the service uses
     const realDatabaseService = container.get<IDatabaseService>(serviceIdentifier.Database);
     const realDataSource = await realDatabaseService.getDatabase('agent');
     const agentDefRepo = realDataSource.getRepository(AgentDefinitionEntity);
 
-    // Save only minimal record (id only) to test new behavior
+    // Save only minimal record (id only). Nullable fields mean "inherit from
+    // the built-in default agent", while the raw DB row stays minimal.
     const example = (defaultAgents as unknown as AgentDefinition[])[0];
     await agentDefRepo.save({
       id: example.id,
@@ -115,15 +116,14 @@ describe('AgentDefinitionService getAgentDefs integration', () => {
 
     const found = defs.find(d => d.id === example.id);
     expect(found).toBeDefined();
-    // With new behavior, only id should be present, other fields should be undefined or empty
     expect(found!.id).toBe(example.id);
-    expect(found!.agentFrameworkID).toBeUndefined();
-    expect(found!.name).toBeUndefined();
-    expect(found!.description).toBeUndefined();
-    expect(found!.avatarUrl).toBeUndefined();
-    expect(found!.agentFrameworkConfig).toEqual({});
-    expect(found!.aiApiConfig).toBeUndefined();
-    expect(found!.agentTools).toBeUndefined();
+    expect(found!.agentFrameworkID).toBe(example.agentFrameworkID);
+    expect(found!.name).toBe(example.name);
+    expect(found!.description).toBe(example.description);
+    expect(found!.avatarUrl).toBe(example.avatarUrl);
+    expect(found!.agentFrameworkConfig).toEqual(example.agentFrameworkConfig);
+    expect(found!.aiApiConfig).toEqual(example.aiApiConfig);
+    expect(found!.agentTools).toEqual(example.agentTools);
   });
 
   it('should have only id field populated when directly querying database entity for build-in agent', async () => {
@@ -188,4 +188,56 @@ describe('AgentDefinitionService getAgentDefs integration', () => {
     const templates = await agentDefinitionService.getAgentTemplates();
     expect(templates.length).toBe((defaultAgents as unknown as AgentDefinition[]).length);
   });
+
+  it('should reject deleteAgentDef for non-temporary IDs', async () => {
+    await expect(agentDefinitionService.deleteAgentDef('real-agent-id')).rejects.toThrow(
+      'Refusing to delete non-temporary agent definition via cleanup path',
+    );
+  });
+
+  it('should delete temporary agent definition and dependent instances', async () => {
+    const realDatabaseService = container.get<IDatabaseService>(serviceIdentifier.Database);
+    const realDataSource = await realDatabaseService.getDatabase('agent');
+    const agentDefRepo = realDataSource.getRepository(AgentDefinitionEntity);
+    const instanceRepo = realDataSource.getRepository(AgentInstanceEntity);
+
+    // Create a temp agent definition
+    const tempId = 'temp-test-agent';
+    await agentDefRepo.save({
+      id: tempId,
+      name: 'Temp Agent',
+      agentFrameworkID: 'test-framework',
+    });
+
+    // Create a dependent instance
+    const instanceId = 'test-instance';
+    await instanceRepo.save({
+      id: instanceId,
+      agentDefId: tempId,
+      name: 'Test Instance',
+      status: { state: 'completed' },
+    });
+
+    // Mock agentInstanceService.deleteAgent to perform DB deletion without full lifecycle teardown
+    const serviceWithPrivate = agentDefinitionService as unknown as {
+      agentInstanceService: { deleteAgent: (...args: unknown[]) => Promise<unknown> };
+    };
+    const deleteAgentMock = vi.fn().mockImplementation(async (instanceId: string) => {
+      await instanceRepo.delete(instanceId);
+    });
+    serviceWithPrivate.agentInstanceService.deleteAgent = deleteAgentMock;
+
+    await agentDefinitionService.deleteAgentDef(tempId);
+
+    // Verify instance service was called for cleanup
+    expect(deleteAgentMock).toHaveBeenCalledWith(instanceId);
+
+    // Verify definition is removed
+    const remainingDef = await agentDefRepo.findOne({ where: { id: tempId } });
+    expect(remainingDef).toBeNull();
+
+    // Verify instance is removed
+    const remainingInstance = await instanceRepo.findOne({ where: { id: instanceId } });
+    expect(remainingInstance).toBeNull();
+  });
 });
diff --git a/src/services/agentDefinition/index.ts b/src/services/agentDefinition/index.ts
@@ -8,12 +8,34 @@ import defaultAgents from '@services/agentInstance/agentFrameworks/taskAgents.js
 import type { IAgentInstanceService } from '@services/agentInstance/interface';
 import { container } from '@services/container';
 import type { IDatabaseService } from '@services/database/interface';
-import { AgentDefinitionEntity } from '@services/database/schema/agent';
+import { AgentDefinitionEntity, AgentInstanceEntity, ScheduledTaskEntity } from '@services/database/schema/agent';
 import { logger } from '@services/libs/log';
 import serviceIdentifier from '@services/serviceIdentifier';
 import { getWikiAgentTemplates } from './getAgentDefinitionTemplatesFromWikis';
 import type { AgentDefinition, IAgentDefinitionService } from './interface';
 
+const defaultAgentsList = defaultAgents as AgentDefinition[];
+
+function mergeTextOverride(value: string | null | undefined, fallback: string | undefined): string | undefined {
+  return value?.trim() ? value : fallback;
+}
+
+function mergeWithDefaultAgent(entity: AgentDefinitionEntity): AgentDefinition {
+  const defaultAgent = defaultAgentsList.find(agent => agent.id === entity.id);
+
+  return {
+    id: entity.id,
+    name: mergeTextOverride(entity.name, defaultAgent?.name),
+    description: mergeTextOverride(entity.description, defaultAgent?.description),
+    avatarUrl: mergeTextOverride(entity.avatarUrl, defaultAgent?.avatarUrl),
+    agentFrameworkID: mergeTextOverride(entity.agentFrameworkID, defaultAgent?.agentFrameworkID),
+    agentFrameworkConfig: entity.agentFrameworkConfig ?? defaultAgent?.agentFrameworkConfig ?? {},
+    aiApiConfig: entity.aiApiConfig ?? defaultAgent?.aiApiConfig,
+    agentTools: entity.agentTools ?? defaultAgent?.agentTools,
+    heartbeat: entity.heartbeat ?? defaultAgent?.heartbeat,
+  };
+}
+
 @injectable()
 export class AgentDefinitionService implements IAgentDefinitionService {
   @inject(serviceIdentifier.Database)
@@ -70,7 +92,6 @@ export class AgentDefinitionService implements IAgentDefinitionService {
       const existingCount = await this.agentDefRepository.count();
       if (existingCount === 0) {
         logger.info('Agent database is empty, initializing with default agents');
-        const defaultAgentsList = defaultAgents as AgentDefinition[];
         // Create agent definition entities with complete data from taskAgents.json
         const agentDefinitionEntities = defaultAgentsList.map(defaultAgent =>
           this.agentDefRepository!.create({
@@ -144,7 +165,10 @@ export class AgentDefinitionService implements IAgentDefinitionService {
         throw new Error(`Agent definition not found: ${agent.id}`);
       }
 
-      const pickedProperties = pick(agent, ['name', 'description', 'avatarUrl', 'agentFrameworkID', 'agentFrameworkConfig', 'aiApiConfig', 'heartbeat']);
+      const pickedProperties = Object.fromEntries(
+        Object.entries(pick(agent, ['name', 'description', 'avatarUrl', 'agentFrameworkID', 'agentFrameworkConfig', 'aiApiConfig', 'heartbeat']))
+          .filter(([, value]) => value !== undefined),
+      );
       Object.assign(existingAgent, pickedProperties);
 
       await this.agentDefRepository!.save(existingAgent);
@@ -167,17 +191,7 @@ export class AgentDefinitionService implements IAgentDefinitionService {
       const agentDefsFromDB = await this.agentDefRepository!.find();
 
       // Convert entities to agent definitions
-      const agentDefs: AgentDefinition[] = agentDefsFromDB.map(entity => ({
-        id: entity.id,
-        name: entity.name || undefined,
-        description: entity.description || undefined,
-        avatarUrl: entity.avatarUrl || undefined,
-        agentFrameworkID: entity.agentFrameworkID || undefined,
-        agentFrameworkConfig: entity.agentFrameworkConfig || {},
-        aiApiConfig: entity.aiApiConfig || undefined,
-        agentTools: entity.agentTools || undefined,
-        heartbeat: entity.heartbeat || undefined,
-      }));
+      const agentDefs: AgentDefinition[] = agentDefsFromDB.map(mergeWithDefaultAgent);
 
       return agentDefs;
     } catch (error) {
@@ -209,32 +223,45 @@ export class AgentDefinitionService implements IAgentDefinitionService {
       }
 
       // Convert entity to agent definition
-      const agentDefinition: AgentDefinition = {
-        id: entity.id,
-        name: entity.name || undefined,
-        description: entity.description || undefined,
-        avatarUrl: entity.avatarUrl || undefined,
-        agentFrameworkID: entity.agentFrameworkID || undefined,
-        agentFrameworkConfig: entity.agentFrameworkConfig || {},
-        aiApiConfig: entity.aiApiConfig || undefined,
-        agentTools: entity.agentTools || undefined,
-        heartbeat: entity.heartbeat || undefined,
-      };
-
-      return agentDefinition;
+      return mergeWithDefaultAgent(entity);
     } catch (error) {
       logger.error(`Failed to get agent definition: ${error as Error}`);
       throw error;
     }
   }
 
-  // Delete agent definition and all associated instances
-  // Note: This will delegate instance deletion to AgentInstanceService
+  // Delete agent definition and all associated instances.
+  // Deletes dependent agent instances first to avoid FK constraint failures.
   public async deleteAgentDef(id: string): Promise<void> {
     this.ensureRepositories();
 
+    if (!id.startsWith('temp-')) {
+      throw new Error(`Refusing to delete non-temporary agent definition via cleanup path: ${id}`);
+    }
+
     try {
-      // Delete the agent definition - instances will be handled by cleanup processes
+      // Delete dependent agent instances before the definition. Use AgentInstanceService
+      // so runtime resources (heartbeats, alarms, scheduled tasks, MCP clients, and
+      // subscriptions) are cleaned up together with database rows.
+      const instanceRepo = this.dataSource!.getRepository(AgentInstanceEntity);
+      const scheduledTaskRepo = this.dataSource!.getRepository(ScheduledTaskEntity);
+      const agentInstanceService = container.get<IAgentInstanceService>(serviceIdentifier.AgentInstance);
+
+      const dependentInstances = await instanceRepo.find({
+        where: { agentDefId: id },
+      });
+
+      for (const instance of dependentInstances) {
+        await agentInstanceService.deleteAgent(instance.id);
+      }
+
+      await scheduledTaskRepo.delete({ agentDefinitionId: id });
+
+      if (dependentInstances.length > 0) {
+        logger.info(`Cleaned up ${dependentInstances.length} dependent agent instances before deleting definition: ${id}`);
+      }
+
+      // Now safe to delete the agent definition
       await this.agentDefRepository!.delete(id);
       logger.info(`Deleted agent definition: ${id}`);
     } catch (error) {
@@ -248,7 +275,6 @@ export class AgentDefinitionService implements IAgentDefinitionService {
       const templates: AgentDefinition[] = [];
 
       // Add default agents from JSON
-      const defaultAgentsList = defaultAgents as AgentDefinition[];
       templates.push(...defaultAgentsList);
 
       // Get templates from active main workspaces
