Fix all PR reviewer feedback: schema, rollback, diff, cleanup, routes, tests, docs

Agent-Logs-Url: https://github.com/objectstack-ai/framework/sessions/b9843a05-b0ee-4a50-bf70-18097fedf0b8

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

Author: Copilot

CHANGELOG.md

@@ -20,7 +20,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
   This aligns ObjectStack with enterprise platforms like Salesforce Setup Audit Trail and
   ServiceNow Update Sets. See `docs/METADATA_HISTORY.md` for detailed usage.
-  ([Phase 4a: Metadata Versioning & History](https://github.com/objectstack-ai/framework/issues/XXXX))
 
 - **CLI: Remote API Commands** - Added 12 new CLI commands for interacting with remote ObjectStack servers:
   - **Authentication**: `os auth login`, `os auth logout`, `os auth whoami`

docs/METADATA_HISTORY.md

@@ -202,7 +202,7 @@ The diff result includes:
 
 ## Performance Considerations
 
-- History records are written asynchronously and failures don't block main operations
+- History records are written synchronously as part of each save operation, ensuring consistency between metadata state and the history timeline
 - Checksum deduplication prevents storing identical versions
 - Indexes optimize common query patterns
 - Automatic cleanup prevents unbounded growth

packages/metadata/src/loaders/database-loader.ts

@@ -113,9 +113,10 @@ export class DatabaseLoader implements MetadataLoader {
         name: this.historyTableName,
       });
       this.historySchemaReady = true;
-    } catch {
-      // If syncSchema fails (e.g. table already exists), mark ready and continue
-      this.historySchemaReady = true;
+    } catch (error) {
+      // Log the error; historySchemaReady remains false so the next operation retries.
+      // If the error is a benign "already exists" the next attempt will also succeed.
+      console.error('Failed to ensure history schema, will retry on next operation:', error);
     }
   }
 
@@ -378,6 +379,112 @@ export class DatabaseLoader implements MetadataLoader {
     }
   }
 
+  /**
+   * Fetch a single history snapshot by (type, name, version).
+   * Returns null when the record does not exist.
+   */
+  async getHistoryRecord(
+    type: string,
+    name: string,
+    version: number
+  ): Promise<MetadataHistoryRecord | null> {
+    if (!this.trackHistory) return null;
+
+    await this.ensureHistorySchema();
+
+    // Resolve the parent metadata record ID
+    const metadataRow = await this.driver.findOne(this.tableName, {
+      object: this.tableName,
+      where: this.baseFilter(type, name),
+    });
+    if (!metadataRow) return null;
+
+    const filter: Record<string, unknown> = {
+      metadata_id: metadataRow.id,
+      version,
+    };
+    if (this.tenantId) {
+      filter.tenant_id = this.tenantId;
+    }
+
+    const row = await this.driver.findOne(this.historyTableName, {
+      object: this.historyTableName,
+      where: filter,
+    });
+    if (!row) return null;
+
+    return {
+      id: row.id as string,
+      metadataId: row.metadata_id as string,
+      name: row.name as string,
+      type: row.type as string,
+      version: row.version as number,
+      operationType: row.operation_type as MetadataHistoryRecord['operationType'],
+      metadata: typeof row.metadata === 'string' ? JSON.parse(row.metadata as string) : row.metadata,
+      checksum: row.checksum as string,
+      previousChecksum: row.previous_checksum as string | undefined,
+      changeNote: row.change_note as string | undefined,
+      tenantId: row.tenant_id as string | undefined,
+      recordedBy: row.recorded_by as string | undefined,
+      recordedAt: row.recorded_at as string,
+    };
+  }
+
+  /**
+   * Perform a rollback: persist `restoredData` as the new current state and record a
+   * single 'revert' history entry (instead of the usual 'update' entry that `save()`
+   * would produce). This avoids the duplicate-version problem that arises when
+   * `register()` → `save()` writes an 'update' entry followed by an additional
+   * 'revert' entry for the same version number.
+   */
+  async registerRollback(
+    type: string,
+    name: string,
+    restoredData: unknown,
+    targetVersion: number,
+    changeNote?: string,
+    recordedBy?: string
+  ): Promise<void> {
+    await this.ensureSchema();
+
+    const now = new Date().toISOString();
+    const metadataJson = JSON.stringify(restoredData);
+    const newChecksum = await calculateChecksum(restoredData);
+
+    const existing = await this.driver.findOne(this.tableName, {
+      object: this.tableName,
+      where: this.baseFilter(type, name),
+    });
+
+    if (!existing) {
+      throw new Error(`Metadata ${type}/${name} not found for rollback`);
+    }
+
+    const previousChecksum = existing.checksum as string | undefined;
+    const newVersion = ((existing.version as number) ?? 0) + 1;
+
+    await this.driver.update(this.tableName, existing.id as string, {
+      metadata: metadataJson,
+      version: newVersion,
+      checksum: newChecksum,
+      updated_at: now,
+      state: 'active',
+    });
+
+    // Write exactly one 'revert' history entry (not an 'update' entry)
+    await this.createHistoryRecord(
+      existing.id as string,
+      type,
+      name,
+      newVersion,
+      restoredData,
+      'revert',
+      previousChecksum,
+      changeNote ?? `Rolled back to version ${targetVersion}`,
+      recordedBy
+    );
+  }
+
   async save(
     type: string,
     name: string,
@@ -399,9 +506,19 @@ export class DatabaseLoader implements MetadataLoader {
       });
 
       if (existing) {
+        // Skip update if the content is identical (prevents phantom version bumps)
+        const previousChecksum = existing.checksum as string | undefined;
+        if (newChecksum === previousChecksum) {
+          return {
+            success: true,
+            path: `datasource://${this.tableName}/${type}/${name}`,
+            size: metadataJson.length,
+            saveTime: Date.now() - startTime,
+          };
+        }
+
         // Update existing record
         const version = ((existing.version as number) ?? 0) + 1;
-        const previousChecksum = existing.checksum as string | undefined;
 
         await this.driver.update(this.tableName, existing.id as string, {
           metadata: metadataJson,

packages/metadata/src/metadata-history.test.ts

@@ -1,8 +1,8 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { describe, it, expect, beforeEach } from 'vitest';
-import { MetadataManager } from '../metadata-manager.js';
-import { DatabaseLoader } from '../loaders/database-loader.js';
+import { MetadataManager } from './metadata-manager';
+import { DatabaseLoader } from './loaders/database-loader';
 import { MemoryDriver } from '@objectstack/driver-memory';
 
 describe('Metadata History', () => {

packages/metadata/src/metadata-manager.ts

@@ -1272,23 +1272,28 @@ export class MetadataManager implements IMetadataService {
 
     // Convert rows to MetadataHistoryRecord format
     const includeMetadata = options?.includeMetadata !== false;
-    const historyResult = records.map((row: Record<string, unknown>) => ({
-      id: row.id as string,
-      metadataId: row.metadata_id as string,
-      name: row.name as string,
-      type: row.type as string,
-      version: row.version as number,
-      operationType: row.operation_type as 'create' | 'update' | 'publish' | 'revert' | 'delete',
-      metadata: includeMetadata
-        ? (typeof row.metadata === 'string' ? JSON.parse(row.metadata as string) : row.metadata)
-        : undefined,
-      checksum: row.checksum as string,
-      previousChecksum: row.previous_checksum as string | undefined,
-      changeNote: row.change_note as string | undefined,
-      tenantId: row.tenant_id as string | undefined,
-      recordedBy: row.recorded_by as string | undefined,
-      recordedAt: row.recorded_at as string,
-    }));
+    const historyResult = records.map((row: Record<string, unknown>) => {
+      const parsedMetadata =
+        typeof row.metadata === 'string'
+          ? JSON.parse(row.metadata as string)
+          : (row.metadata as Record<string, unknown> | null | undefined);
+
+      return {
+        id: row.id as string,
+        metadataId: row.metadata_id as string,
+        name: row.name as string,
+        type: row.type as string,
+        version: row.version as number,
+        operationType: row.operation_type as 'create' | 'update' | 'publish' | 'revert' | 'delete',
+        metadata: includeMetadata ? parsedMetadata : null,
+        checksum: row.checksum as string,
+        previousChecksum: row.previous_checksum as string | undefined,
+        changeNote: row.change_note as string | undefined,
+        tenantId: row.tenant_id as string | undefined,
+        recordedBy: row.recorded_by as string | undefined,
+        recordedAt: row.recorded_at as string,
+      };
+    });
 
     return {
       records: historyResult,
@@ -1315,9 +1320,8 @@ export class MetadataManager implements IMetadataService {
       throw new Error('Rollback requires a database loader to be configured');
     }
 
-    // Get the target version from history
-    const history = await this.getHistory(type, name, { limit: 1000, includeMetadata: true });
-    const targetVersion = history.records.find(r => r.version === version);
+    // Fetch the target version snapshot directly from the history table
+    const targetVersion = await dbLoader.getHistoryRecord(type, name, version);
 
     if (!targetVersion) {
       throw new Error(`Version ${version} not found in history for ${type}/${name}`);
@@ -1327,41 +1331,17 @@ export class MetadataManager implements IMetadataService {
       throw new Error(`Version ${version} metadata snapshot not available`);
     }
 
-    // Restore the metadata
+    // Restore the metadata using the dedicated rollback path so that a single
+    // 'revert' history entry is written (instead of a conflicting 'update' entry)
     const restoredMetadata = targetVersion.metadata;
-
-    // Register the restored version
-    await this.register(type, name, restoredMetadata);
-
-    // Create a history record for the rollback operation
-    const driver = (dbLoader as any).driver as IDataDriver;
-    const tableName = (dbLoader as any).tableName as string;
-    const tenantId = (dbLoader as any).tenantId as string | undefined;
-
-    const filter: Record<string, unknown> = { type, name };
-    if (tenantId) {
-      filter.tenant_id = tenantId;
-    }
-
-    const metadataRecord = await driver.findOne(tableName, {
-      object: tableName,
-      where: filter,
-    });
-
-    if (metadataRecord) {
-      const currentVersion = (metadataRecord.version as number) ?? 1;
-      await (dbLoader as any).createHistoryRecord(
-        metadataRecord.id as string,
-        type,
-        name,
-        currentVersion,
-        restoredMetadata,
-        'revert',
-        metadataRecord.checksum as string | undefined,
-        options?.changeNote ?? `Rolled back to version ${version}`,
-        options?.recordedBy
-      );
-    }
+    await dbLoader.registerRollback(
+      type,
+      name,
+      restoredMetadata,
+      version,
+      options?.changeNote,
+      options?.recordedBy
+    );
 
     return restoredMetadata;
   }
@@ -1381,10 +1361,9 @@ export class MetadataManager implements IMetadataService {
       throw new Error('Diff requires a database loader to be configured');
     }
 
-    // Get both versions from history
-    const history = await this.getHistory(type, name, { limit: 1000, includeMetadata: true });
-    const v1 = history.records.find(r => r.version === version1);
-    const v2 = history.records.find(r => r.version === version2);
+    // Fetch the two version snapshots directly from the history table
+    const v1 = await dbLoader.getHistoryRecord(type, name, version1);
+    const v2 = await dbLoader.getHistoryRecord(type, name, version2);
 
     if (!v1) {
       throw new Error(`Version ${version1} not found in history for ${type}/${name}`);

packages/metadata/src/routes/history-routes.ts

@@ -47,8 +47,20 @@ export function registerMetadataHistoryRoutes(
     try {
       const options: any = {};
 
-      if (query.limit) options.limit = parseInt(query.limit, 10);
-      if (query.offset) options.offset = parseInt(query.offset, 10);
+      if (query.limit !== undefined) {
+        const limit = parseInt(query.limit, 10);
+        if (!Number.isFinite(limit) || limit < 1) {
+          return c.json({ success: false, error: 'limit must be a positive integer' }, 400);
+        }
+        options.limit = limit;
+      }
+      if (query.offset !== undefined) {
+        const offset = parseInt(query.offset, 10);
+        if (!Number.isFinite(offset) || offset < 0) {
+          return c.json({ success: false, error: 'offset must be a non-negative integer' }, 400);
+        }
+        options.offset = offset;
+      }
       if (query.since) options.since = query.since;
       if (query.until) options.until = query.until;
       if (query.operationType) options.operationType = query.operationType;