Merge pull request #1089 from objectstack-ai/copilot/fix-metadata-manager-driver-issue

hotlong · web-flow · commit bebd18c9c6ff · 2026-04-08T18:32:33.000+08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `ObjectDef`/`FieldDef` type definitions in `data-tools.ts` are removed.
 
 ### Fixed
+- **MetadataPlugin: Driver bridging for database-backed persistence** — `MetadataPlugin.start()`
+  now discovers registered driver services (`driver.*`) from the kernel service registry and
+  calls `manager.setDatabaseDriver()` to enable `DatabaseLoader`. Previously, no code bridged
+  the kernel's database driver to the `MetadataManager`, leaving `DatabaseLoader` unconfigured
+  and metadata persistence limited to the filesystem only.
+- **MetadataManager: register() no longer writes to FilesystemLoader** — `register()` now
+  persists metadata only to `datasource:` protocol loaders (i.e. `DatabaseLoader`), skipping
+  `file:` protocol loaders (`FilesystemLoader`). Previously, `register()` broadcast writes to
+  all loaders indiscriminately, causing crashes in read-only environments (e.g. serverless,
+  containerized deployments) when `FilesystemLoader.save()` attempted to write to disk.
+  The same protocol filter is applied to `unregister()` for consistency.
 - **Agent Chat: Vercel SSE Data Stream support** — The agent chat endpoint
   (`/api/v1/ai/agents/:agentName/chat`) now returns Vercel AI SDK v6 UI Message Stream Protocol
   (SSE) by default, matching the general chat endpoint behaviour. Previously, the agent chat route
diff --git a/packages/metadata/README.md b/packages/metadata/README.md
@@ -28,7 +28,7 @@ It implements the **`IMetadataService`** contract from `@objectstack/spec` and a
 │  │ (files)      │  │   (HTTP)     │  │   (test)          │  │
 │  └─────────────┘  └──────────────┘  └───────────────────┘  │
 │  ┌──────────────────────────────────────────────────────┐   │
-│  │ DatabaseLoader (planned — datasource-backed storage) │   │
+│  │ DatabaseLoader (datasource-backed storage)          │   │
 │  └──────────────────────────────────────────────────────┘   │
 ├─────────────────────────────────────────────────────────────┤
 │                    Serializer Layer                          │
@@ -61,7 +61,7 @@ Loaders are pluggable data sources that know how to read/write metadata from dif
 | `FilesystemLoader`  | `file:`        | ✅   | ✅    | ✅    | Implemented  |
 | `MemoryLoader`      | `memory:`      | ✅   | ✅    | ❌    | Implemented  |
 | `RemoteLoader`      | `http:`        | ✅   | ✅    | ❌    | Implemented  |
-| `DatabaseLoader`    | `datasource:`  | ✅   | ✅    | ✅    | Planned      |
+| `DatabaseLoader`    | `datasource:`  | ✅   | ✅    | ❌    | Implemented  |
 
 ### 3. Serializers
 
diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts
@@ -153,17 +153,21 @@ export class MetadataManager implements IMetadataService {
 
   /**
    * Register/save a metadata item by type
-   * Stores in-memory registry and persists to writable loaders (if configured)
+   * Stores in-memory registry and persists to database-backed loaders only.
+   * FilesystemLoader (protocol 'file:') is read-only for static metadata and
+   * should not be written to during runtime registration.
    */
   async register(type: string, name: string, data: unknown): Promise<void> {
     if (!this.registry.has(type)) {
       this.registry.set(type, new Map());
     }
     this.registry.get(type)!.set(name, data);
 
-    // Persist to writable loaders (e.g., DatabaseLoader for history tracking)
+    // Persist only to database-backed loaders that declare write capability.
+    // FilesystemLoader is read-only at runtime — writing to it can crash in
+    // read-only environments (e.g. serverless, containerized deployments).
     for (const loader of this.loaders.values()) {
-      if (loader.save) {
+      if (loader.save && loader.contract.protocol === 'datasource:' && loader.contract.capabilities.write) {
         await loader.save(type, name, data);
       }
     }
@@ -218,7 +222,8 @@ export class MetadataManager implements IMetadataService {
   }
 
   /**
-   * Unregister/remove a metadata item by type and name
+   * Unregister/remove a metadata item by type and name.
+   * Deletes from database-backed loaders only (same rationale as register()).
    */
   async unregister(type: string, name: string): Promise<void> {
     // Remove from in-memory registry
@@ -230,9 +235,9 @@ export class MetadataManager implements IMetadataService {
       }
     }
 
-    // Also delete from all loaders that support deletion
+    // Delete only from database-backed loaders that declare write capability
     for (const loader of this.loaders.values()) {
-      // Check if the loader has a delete method
+      if (loader.contract.protocol !== 'datasource:' || !loader.contract.capabilities.write) continue;
       if (typeof (loader as any).delete === 'function') {
         try {
           await (loader as any).delete(type, name);
diff --git a/packages/metadata/src/metadata.test.ts b/packages/metadata/src/metadata.test.ts
@@ -226,6 +226,155 @@ describe('MetadataManager', () => {
     });
   });
 
+  describe('register — loader protocol filtering', () => {
+    it('should persist to datasource: protocol loaders', async () => {
+      const dbLoader: MetadataLoader = {
+        contract: { name: 'database', protocol: 'datasource:' as const, capabilities: { read: true, write: true, watch: false, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+      };
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [dbLoader] });
+      await m.register('object', 'account', { name: 'account' });
+
+      expect(dbLoader.save).toHaveBeenCalledWith('object', 'account', { name: 'account' });
+    });
+
+    it('should NOT persist to file: protocol loaders', async () => {
+      const fsLoader: MetadataLoader = {
+        contract: { name: 'filesystem', protocol: 'file:' as const, capabilities: { read: true, write: true, watch: true, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+      };
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [fsLoader] });
+      await m.register('object', 'account', { name: 'account' });
+
+      expect(fsLoader.save).not.toHaveBeenCalled();
+    });
+
+    it('should NOT persist to memory: protocol loaders', async () => {
+      const memLoader: MetadataLoader = {
+        contract: { name: 'memory', protocol: 'memory:' as const, capabilities: { read: true, write: true, watch: false, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+      };
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [memLoader] });
+      await m.register('object', 'account', { name: 'account' });
+
+      expect(memLoader.save).not.toHaveBeenCalled();
+    });
+
+    it('should NOT persist to datasource: protocol loaders with write: false', async () => {
+      const readOnlyDbLoader: MetadataLoader = {
+        contract: { name: 'database-ro', protocol: 'datasource:' as const, capabilities: { read: true, write: false, watch: false, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+      };
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [readOnlyDbLoader] });
+      await m.register('object', 'account', { name: 'account' });
+
+      expect(readOnlyDbLoader.save).not.toHaveBeenCalled();
+    });
+
+    it('should still store in in-memory registry regardless of loaders', async () => {
+      const m = new MetadataManager({ formats: ['json'], loaders: [] });
+      await m.register('object', 'account', { name: 'account' });
+
+      const result = await m.get('object', 'account');
+      expect(result).toEqual({ name: 'account' });
+    });
+  });
+
+  describe('unregister — loader protocol filtering', () => {
+    it('should delete from datasource: protocol loaders', async () => {
+      const deleteFn = vi.fn();
+      const dbLoader: MetadataLoader = {
+        contract: { name: 'database', protocol: 'datasource:' as const, capabilities: { read: true, write: true, watch: false, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+        delete: deleteFn,
+      } as any;
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [dbLoader] });
+      await m.register('object', 'account', { name: 'account' });
+      await m.unregister('object', 'account');
+
+      expect(deleteFn).toHaveBeenCalledWith('object', 'account');
+    });
+
+    it('should NOT delete from file: protocol loaders', async () => {
+      const deleteFn = vi.fn();
+      const fsLoader: MetadataLoader = {
+        contract: { name: 'filesystem', protocol: 'file:' as const, capabilities: { read: true, write: true, watch: true, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+        delete: deleteFn,
+      } as any;
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [fsLoader] });
+      await m.register('object', 'account', { name: 'account' });
+      await m.unregister('object', 'account');
+
+      expect(deleteFn).not.toHaveBeenCalled();
+    });
+
+    it('should NOT delete from datasource: protocol loaders with write: false', async () => {
+      const deleteFn = vi.fn();
+      const readOnlyDbLoader: MetadataLoader = {
+        contract: { name: 'database-ro', protocol: 'datasource:' as const, capabilities: { read: true, write: false, watch: false, list: true } },
+        load: vi.fn().mockResolvedValue({ data: null }),
+        loadMany: vi.fn().mockResolvedValue([]),
+        exists: vi.fn().mockResolvedValue(false),
+        stat: vi.fn().mockResolvedValue(null),
+        list: vi.fn().mockResolvedValue([]),
+        save: vi.fn().mockResolvedValue({ success: true }),
+        delete: deleteFn,
+      } as any;
+
+      const m = new MetadataManager({ formats: ['json'], loaders: [readOnlyDbLoader] });
+      await m.register('object', 'account', { name: 'account' });
+      await m.unregister('object', 'account');
+
+      expect(deleteFn).not.toHaveBeenCalled();
+    });
+
+    it('should remove from in-memory registry', async () => {
+      const m = new MetadataManager({ formats: ['json'], loaders: [] });
+      await m.register('object', 'account', { name: 'account' });
+      expect(await m.get('object', 'account')).toEqual({ name: 'account' });
+
+      await m.unregister('object', 'account');
+      expect(await m.get('object', 'account')).toBeUndefined();
+    });
+  });
+
   describe('registerLoader', () => {
     it('should register a new loader', async () => {
       const newLoader = new MemoryLoader();
@@ -340,6 +489,7 @@ describe('MetadataPlugin', () => {
       registerLoader = vi.fn();
       stopWatching = vi.fn();
       setTypeRegistry = vi.fn();
+      setDatabaseDriver = vi.fn();
       register = vi.fn();
     };
     return { NodeMetadataManager: MockNodeMetadataManager };
@@ -382,6 +532,83 @@ describe('MetadataPlugin', () => {
     // start should call logger.info at least once
     expect(ctx.logger.info).toHaveBeenCalled();
   });
+
+  it('should bridge driver service to MetadataManager in start()', async () => {
+    const { MetadataPlugin } = await import('./plugin.js');
+    const plugin = new MetadataPlugin({ rootDir: '/tmp/test', watch: false });
+
+    const mockDriver = { name: 'mock-driver', find: vi.fn(), create: vi.fn() };
+    const services = new Map<string, any>();
+    services.set('driver.mock-driver', mockDriver);
+
+    const ctx = createMockPluginContext();
+    ctx.getServices = vi.fn().mockReturnValue(services);
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+
+    // Verify setDatabaseDriver was called on the manager with the driver
+    const manager = (plugin as any).manager;
+    expect(manager.setDatabaseDriver).toHaveBeenCalledWith(mockDriver);
+  });
+
+  it('should bridge driver AFTER filesystem metadata loading', async () => {
+    const { MetadataPlugin } = await import('./plugin.js');
+    const plugin = new MetadataPlugin({ rootDir: '/tmp/test', watch: false });
+
+    const callOrder: string[] = [];
+    const mockDriver = { name: 'mock-driver', find: vi.fn(), create: vi.fn() };
+    const services = new Map<string, any>();
+    services.set('driver.mock-driver', mockDriver);
+
+    const manager = (plugin as any).manager;
+    manager.loadMany = vi.fn().mockImplementation(async () => {
+      callOrder.push('loadMany');
+      return [];
+    });
+    manager.setDatabaseDriver = vi.fn().mockImplementation(() => {
+      callOrder.push('setDatabaseDriver');
+    });
+
+    const ctx = createMockPluginContext();
+    ctx.getServices = vi.fn().mockReturnValue(services);
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+
+    // setDatabaseDriver must be called after all loadMany calls
+    const lastLoad = callOrder.lastIndexOf('loadMany');
+    const driverIdx = callOrder.indexOf('setDatabaseDriver');
+    expect(driverIdx).toBeGreaterThan(lastLoad);
+  });
+
+  it('should not fail when no driver service is available', async () => {
+    const { MetadataPlugin } = await import('./plugin.js');
+    const plugin = new MetadataPlugin({ rootDir: '/tmp/test', watch: false });
+
+    const ctx = createMockPluginContext();
+    ctx.getServices = vi.fn().mockReturnValue(new Map());
+
+    await plugin.init(ctx);
+    // Should not throw
+    await expect(plugin.start(ctx)).resolves.not.toThrow();
+
+    // setDatabaseDriver should not have been called
+    const manager = (plugin as any).manager;
+    expect(manager.setDatabaseDriver).not.toHaveBeenCalled();
+  });
+
+  it('should gracefully handle getServices errors', async () => {
+    const { MetadataPlugin } = await import('./plugin.js');
+    const plugin = new MetadataPlugin({ rootDir: '/tmp/test', watch: false });
+
+    const ctx = createMockPluginContext();
+    ctx.getServices = vi.fn().mockImplementation(() => { throw new Error('services unavailable'); });
+
+    await plugin.init(ctx);
+    // Should not throw even when getServices fails
+    await expect(plugin.start(ctx)).resolves.not.toThrow();
+  });
 });
 
 // ---------- Helpers ----------
diff --git a/packages/metadata/src/plugin.ts b/packages/metadata/src/plugin.ts
@@ -102,5 +102,27 @@ export class MetadataPlugin implements Plugin {
             totalItems: totalLoaded,
             registeredTypes: sortedTypes.length,
         });
+
+        // Bridge database driver from kernel service registry to MetadataManager.
+        // DriverPlugin registers drivers as 'driver.{name}' services during init().
+        // This runs AFTER filesystem loading so that system metadata populated via
+        // register() above is stored in-memory only, without being persisted to the
+        // database (which would create noisy DB state/history on every cold boot).
+        try {
+            const services = ctx.getServices();
+            for (const [serviceName, service] of services) {
+                if (serviceName.startsWith('driver.') && service) {
+                    ctx.logger.info('[MetadataPlugin] Bridging driver to MetadataManager for database-backed persistence', {
+                        driverService: serviceName,
+                    });
+                    this.manager.setDatabaseDriver(service);
+                    break; // Use the first discovered driver — typically only one driver is registered per deployment
+                }
+            }
+        } catch (e: any) {
+            ctx.logger.debug('[MetadataPlugin] No driver service found — database metadata persistence not available', {
+                error: e.message,
+            });
+        }
     }
 }
