fix: address review feedback — defer driver bridging, check write capability, update diagram

1. Move driver bridging to AFTER filesystem metadata loading in
   MetadataPlugin.start() to prevent system metadata from being
   persisted into the database on every cold boot.
2. Gate register()/unregister() persistence on both protocol
   ('datasource:') AND contract.capabilities.write to respect
   the loader's declared write capability.
3. Update README architecture diagram to say "datasource-backed
   storage" instead of "planned" for DatabaseLoader consistency.
4. Added 3 new tests: read-only datasource loader filtering for
   register/unregister, and driver bridging ordering verification.

Agent-Logs-Url: https://github.com/objectstack-ai/framework/sessions/856d4345-f537-4680-9c7e-e0ddd81cfa30

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>

Author: Copilot

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                          │

packages/metadata/src/metadata-manager.ts

@@ -163,11 +163,11 @@ export class MetadataManager implements IMetadataService {
     }
     this.registry.get(type)!.set(name, data);
 
-    // Persist only to database-backed loaders (protocol 'datasource:')
+    // 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 && loader.contract.protocol === 'datasource:') {
+      if (loader.save && loader.contract.protocol === 'datasource:' && loader.contract.capabilities.write) {
         await loader.save(type, name, data);
       }
     }
@@ -235,9 +235,9 @@ export class MetadataManager implements IMetadataService {
       }
     }
 
-    // Delete only from database-backed loaders (protocol 'datasource:')
+    // Delete only from database-backed loaders that declare write capability
     for (const loader of this.loaders.values()) {
-      if (loader.contract.protocol !== 'datasource:') continue;
+      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);

packages/metadata/src/metadata.test.ts

@@ -278,6 +278,23 @@ describe('MetadataManager', () => {
       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' });
@@ -328,6 +345,26 @@ describe('MetadataManager', () => {
       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' });
@@ -515,6 +552,36 @@ describe('MetadataPlugin', () => {
     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 });

packages/metadata/src/plugin.ts

@@ -69,26 +69,6 @@ export class MetadataPlugin implements Plugin {
     }
 
     start = async (ctx: PluginContext) => {
-        // Bridge database driver from kernel service registry to MetadataManager.
-        // DriverPlugin registers drivers as 'driver.{name}' services during init().
-        // We look them up here (start phase) to enable DatabaseLoader persistence.
-        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,
-            });
-        }
-
         ctx.logger.info('Loading metadata from file system...');
 
         // Use the type registry to discover metadata types (sorted by loadOrder)
@@ -122,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,
+            });
+        }
     }
 }