fix(#208): checkpoint code-review fixes — offline DROP suppression + own-source parity + dedup

Addresses the /code-review high findings on the TS reference (Tasks 4-6):

- [1, correctness] The default OFFLINE `meta migrate` generate path (planOffline)
  never threaded the @Unmanaged name set, so an @Unmanaged table captured into the
  snapshot by a `baseline --from-db` was proposed for DROP. planOffline now threads
  collectUnmanagedNames(args.metadata) into its diff internally (parity with the
  online + verify paths). Regression test added.

- [own-source parity] expected-schema's @Unmanaged writable-source detection now uses
  ownChildren() to match collectUnmanagedNames — so the "skip the descriptor" half and
  the "suppress the drop" half agree exactly. This converts a theoretical split-brain
  (a table not created yet still proposed for destructive drop, for an @Unmanaged source
  inherited via extends) into consistent treat-as-managed. collectUnmanagedNames now
  takes MetaData (not MetaRoot) so planOffline can call it without a cast.

- [6, cleanup] The DDL-ownership classification block was duplicated verbatim across the
  projection and write-through loops in build-projection-views. Factored into a shared
  classifyReadOnlySource() returning skip | sql | derive, so the ownership rules can
  never diverge between the two host kinds (the emit tail was already shared).

Deferred/intended (documented in Task 11, NOT bugs): an @SQL view's dependsOn is
derived from extends-anchors only (D7 / §11 YAGNI — the @dependsOn escape is deferred
until an adopter hits an undeclared-dependency alter); an @Unmanaged view is not
drop/recreated around a managed dependency's column ALTER (the tool has no body for an
external view — it cannot recreate it) and an FK into an @Unmanaged table has no ordering
guarantee vs the external tool (both documented adopter caveats, spec §7).

migrate-ts 588 pass; codegen-ts 120 projection; cli 375; view-lifecycle-pg 16/16;
typechecks clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NGQ7oSuNcjhsMHWwZzhBwr

Author: dmealing

server/typescript/packages/codegen-ts/src/projection/build-projection-views.ts

@@ -93,44 +93,21 @@ export function buildProjectionViews(
 
   const out: ExpectedView[] = [];
   for (const projection of root.objects().filter(isProjection)) {
-    // Only PLAIN-VIEW projections produce managed CREATE VIEW DDL. The other
-    // read-only kinds must be skipped, not fed to extractViewSpec:
-    //   - storedProc / tableFunction (FR-015) are CALLABLES, not views. They are
-    //     base-less (no extends-bound identity), so extractViewSpec THROWS for
-    //     them — and the CLI calls this function unconditionally, so one proc
-    //     projection used to crash `meta migrate` outright.
-    //   - materializedView cannot be managed by the migrate pipeline today:
-    //     there is no CREATE MATERIALIZED VIEW emit, and PG introspection cannot
-    //     even see matviews (information_schema.views excludes them), so a
-    //     "managed" matview would re-propose create-view on every run and the
-    //     apply would collide with the existing object. Worse, feeding it
-    //     through here silently created a PLAIN view under the matview's name.
-    //     Matviews are hand-managed, like the documented custom-SQL-view
-    //     exception: migrate neither creates nor drops them.
-    //   - a STANDALONE read-model — a plain view projection declaring its own
-    //     columns, with no origin.* children — is hand-authored SQL too. Codegen
-    //     already treats it that way on purpose (see projection-decl.ts: "lets a
-    //     standalone read-only view-entity — explicit columns, no `extends` —
-    //     generate its read model … standalone views hand-author their SQL").
-    //     The SCHEMA path never got that memo: extractViewSpec THREW on it
-    //     ("cannot derive the base entity"), and because the CLI calls this
-    //     function unconditionally, ONE such projection aborted `meta migrate`
-    //     for the ENTIRE model — every other entity included. Same crash class as
-    //     the proc projections above. Skip it instead.
-    // ADR-0039: own — mirrors isProjection/viewName's own-source classification.
-    const readOnlySource = projection.ownChildren().find(
-      (c): c is MetaSource => c instanceof MetaSource && c.isReadOnly(),
-    );
-    if (readOnlySource === undefined) continue;
-    // #208 suppression rule (§6) — classify DDL ownership BEFORE viewIsDerived, so an
-    // escape-valve view carrying extends-bound identity/fields (pure shape / row
+    // #208 §6 — classify DDL ownership BEFORE viewIsDerived (see classifyReadOnlySource),
+    // so an escape-valve view carrying extends-bound identity/fields (pure shape / row
     // identity) is never mis-synthesized into a wrong base-table passthrough SELECT.
-    if (readOnlySource.isUnmanaged) continue;                 // external: Flyway/hand-migration owns it (§7)
-    if (readOnlySource.sqlBody !== undefined) {               // supplied: the author owns the body
-      emitSqlView(projection, readOnlySource, root, joinTables, out);
+    const cls = classifyReadOnlySource(projection);
+    if (cls.kind === "skip") continue;
+    if (cls.kind === "sql") {
+      emitSqlView(projection, cls.source, root, joinTables, out);
       continue;
     }
-    if (readOnlySource.effectiveKind !== SOURCE_KIND_VIEW) continue;
+    // A plain-view projection with no extends anchor and no origin.* is a STANDALONE
+    // read-model that hand-authors its own SQL — viewIsDerived returns false and we skip
+    // it (feeding it to extractViewSpec throws "cannot derive the base entity", and the
+    // CLI calls this unconditionally, so ONE such projection would abort `meta migrate`
+    // for the whole model). Codegen already treats this shape as intended in
+    // projection-decl.ts ("standalone views hand-author their SQL").
     if (!viewIsDerived(projection)) continue;
     emitViewFor(projection, root, joinTables, dialect, columnNamingStrategy, out);
   }
@@ -145,23 +122,55 @@ export function buildProjectionViews(
   // `view` read source synthesizes CREATE VIEW DDL — a matview/proc/tableFunction
   // read source is hand-managed, exactly as for projections above.
   for (const entity of root.objects().filter(isWriteThrough)) {
-    const readOnlySource = entity.ownChildren().find(
-      (c): c is MetaSource => c instanceof MetaSource && c.isReadOnly(),
-    );
-    if (readOnlySource === undefined) continue;
-    // #208 suppression rule (§6) — same DDL-ownership classification as the projection
-    // loop above: an @unmanaged replica view is skipped, an @sql one is verbatim.
-    if (readOnlySource.isUnmanaged) continue;
-    if (readOnlySource.sqlBody !== undefined) {
-      emitSqlView(entity, readOnlySource, root, joinTables, out);
+    // #208 §6 — same DDL-ownership classification as the projection loop. Unlike a
+    // projection there is no viewIsDerived gate: a write-through entity IS the base and
+    // always has a derived view to emit (extractViewSpec detects the write-through host).
+    const cls = classifyReadOnlySource(entity);
+    if (cls.kind === "skip") continue;
+    if (cls.kind === "sql") {
+      emitSqlView(entity, cls.source, root, joinTables, out);
       continue;
     }
-    if (readOnlySource.effectiveKind !== SOURCE_KIND_VIEW) continue;
     emitViewFor(entity, root, joinTables, dialect, columnNamingStrategy, out);
   }
   return out;
 }
 
+/**
+ * #208 §6 — classify a host's read-only source by DDL OWNERSHIP, BEFORE any derivation
+ * decision. Shared by the projection and write-through loops so the ownership rules can
+ * never diverge between the two host kinds (the emit TAIL is already shared via emitViewFor).
+ *
+ *   - no read-only source, OR @unmanaged (external), OR a non-`view` read-only kind →
+ *     "skip". The non-view read-only kinds are hand-managed and MUST NOT reach
+ *     extractViewSpec:
+ *       · storedProc / tableFunction (FR-015) are CALLABLES, base-less → extractViewSpec
+ *         throws (and the CLI calls this unconditionally, so one proc crashed migrate);
+ *       · materializedView has no CREATE-MATVIEW emit and is invisible to
+ *         information_schema.views, so a "managed" matview re-proposes create every run;
+ *       · @unmanaged: Flyway / a hand-migration owns the DDL (§7).
+ *   - @sql read source → "sql": the author owns a verbatim body (emitSqlView).
+ *   - a plain `view` read source → "derive": a tool-synthesized body (the caller applies
+ *     its own viewIsDerived / standalone-read-model gate).
+ *
+ * ADR-0039: own — mirrors isProjection/viewName's own-source classification.
+ */
+type ReadOnlySourceClass =
+  | { kind: "skip" }
+  | { kind: "sql"; source: MetaSource }
+  | { kind: "derive"; source: MetaSource };
+
+function classifyReadOnlySource(host: MetaObject): ReadOnlySourceClass {
+  const source = host.ownChildren().find(
+    (c): c is MetaSource => c instanceof MetaSource && c.isReadOnly(),
+  );
+  if (source === undefined) return { kind: "skip" };
+  if (source.isUnmanaged) return { kind: "skip" }; // external — Flyway/hand-migration owns it
+  if (source.sqlBody !== undefined) return { kind: "sql", source }; // author-supplied body
+  if (source.effectiveKind !== SOURCE_KIND_VIEW) return { kind: "skip" }; // matview/proc/tableFunction
+  return { kind: "derive", source };
+}
+
 /** Extract + emit one host's (projection or write-through entity) view body and
  *  push it onto `out`. The single tail shared by both walks in buildProjectionViews. */
 function emitViewFor(

server/typescript/packages/migrate-ts/src/expected-schema.ts

@@ -153,8 +153,11 @@ export function buildExpectedSchema(
     if (hasReadOnlySource && !hasWritableSource) continue;
     const tableName = resolveTableName(child);
     // #208 §7 — an @unmanaged writable (table) source: emit no descriptor, but keep the
-    // entity in entityToTable so an inbound FK resolves the physical name.
-    const writableSource = child.children().find(
+    // entity in entityToTable so an inbound FK resolves the physical name. OWN-source
+    // detection (not resolving) to match collectUnmanagedNames, so the skip here and the
+    // act-side drop-suppression there agree exactly — no split-brain where a table is not
+    // created yet is still proposed for drop.
+    const writableSource = child.ownChildren().find(
       (c): c is MetaSource => c instanceof MetaSource && c.isWritable(),
     );
     if (writableSource?.isUnmanaged) {

server/typescript/packages/migrate-ts/src/snapshot/plan.ts

@@ -2,6 +2,7 @@
 import type { ColumnNamingStrategy, MetaData } from "@metaobjectsdev/metadata";
 import { buildExpectedSchema } from "../expected-schema.js";
 import { diff, type DiffArgs } from "../diff/index.js";
+import { collectUnmanagedNames } from "../unmanaged.js";
 import type { Dialect, DiffResult, SchemaSnapshot } from "../types.js";
 import type { ExpectedViewInput } from "../expected-schema.js";
 
@@ -37,6 +38,10 @@ export async function planOffline(args: PlanOfflineArgs): Promise<PlanOfflineRes
     expected: nextSnapshot,
     actual: args.snapshot,
     dialect: args.dialect,
+    // #208 §7 — exclude declared-@unmanaged objects from the actual (snapshot) side too,
+    // so the OFFLINE generate path never proposes DROP for an external table that a
+    // `baseline --from-db` captured into the snapshot (parity with the online/verify paths).
+    unmanagedNames: collectUnmanagedNames(args.metadata),
     ...(args.allow ? { allow: args.allow } : {}),
     ...(args.onAmbiguous ? { onAmbiguous: args.onAmbiguous } : {}),
     ...(args.ignoreTables ? { ignoreTables: args.ignoreTables } : {}),

server/typescript/packages/migrate-ts/src/unmanaged.ts

@@ -11,23 +11,28 @@ import {
   MetaSource,
   resolveTableSchema,
   DEFAULT_DB_SCHEMA_POSTGRES,
-  type MetaRoot,
+  TYPE_OBJECT,
+  type MetaData,
 } from "@metaobjectsdev/metadata";
 
 /**
  * The qualified physical names (`schema.name`, schema defaulting to Postgres `public`)
  * of every DB object declared `@unmanaged`. The format matches `tableIdentity` /
  * `viewIdentity` in the diff, so one set silences both drop-table and drop-view.
  *
- * Walks each object's OWN sources: `@unmanaged` is per-source (ADR-0007 — physical
- * concerns live on the source child), so a write-through entity can mark only its
- * read-view source `@unmanaged` while its table stays managed. Own-source iteration
- * also keeps `source.physicalName`'s owner-fallback resolving against the declaring
- * object rather than an `extends` super.
+ * Walks each object's OWN sources — the SAME own-source basis buildExpectedSchema Pass 1
+ * uses to skip an `@unmanaged` table's descriptor, so the two halves of the diff agree
+ * (an object skipped from `expected` is also excluded from `actual`): no split-brain
+ * where a table is not created yet is still proposed for drop. `@unmanaged` is
+ * per-source (ADR-0007 — physical concerns live on the source child), so a write-through
+ * entity can mark only its read-view source `@unmanaged` while its table stays managed.
+ * Own-source iteration also keeps `source.physicalName`'s owner-fallback resolving
+ * against the declaring object rather than an `extends` super.
  */
-export function collectUnmanagedNames(root: MetaRoot): string[] {
+export function collectUnmanagedNames(root: MetaData): string[] {
   const out: string[] = [];
-  for (const obj of root.objects()) {
+  for (const obj of root.children()) {
+    if (obj.type !== TYPE_OBJECT) continue;
     for (const src of obj.ownChildren()) {
       if (!(src instanceof MetaSource) || !src.isUnmanaged) continue;
       const schema = resolveTableSchema(obj) ?? DEFAULT_DB_SCHEMA_POSTGRES;

server/typescript/packages/migrate-ts/test/snapshot/plan.test.ts

@@ -46,6 +46,45 @@ describe("planOffline", () => {
   });
 });
 
+describe("planOffline — #208 @unmanaged (offline DROP suppression)", () => {
+  const legacyMeta = (unmanaged: boolean) =>
+    JSON.stringify({
+      "metadata.root": {
+        children: [
+          {
+            "object.entity": {
+              name: "Legacy",
+              children: [
+                { "field.long": { name: "id" } },
+                {
+                  "source.rdb": {
+                    name: "src",
+                    "@table": "legacy_accounts",
+                    ...(unmanaged ? { "@unmanaged": true } : {}),
+                  },
+                },
+                { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } },
+              ],
+            },
+          },
+        ],
+      },
+    });
+
+  test("does NOT propose drop-table for an @unmanaged table captured in the snapshot (a from-db baseline)", async () => {
+    // A `baseline --from-db` captured the Flyway-owned table into the committed snapshot...
+    const snapshot = baselineFromMetadata(await loadJson(legacyMeta(false)), "postgres");
+    expect(snapshot.tables.map((t) => t.name)).toContain("legacy_accounts");
+
+    // ...and the metadata now declares that table @unmanaged. The offline generate path
+    // must stay silent for it — not propose DROP for an externally-owned table.
+    const metadata = await loadJson(legacyMeta(true));
+    const { diff } = await planOffline({ metadata, dialect: "postgres", snapshot });
+    expect(diff.changes.some((c) => c.kind === "drop-table" && c.table === "legacy_accounts")).toBe(false);
+    expect(diff.changes).toHaveLength(0);
+  });
+});
+
 describe("baselineFromMetadata", () => {
   test("equals buildExpectedSchema for the dialect", async () => {
     const metadata = await loadJson(META);