fix(metadata)!: sys_metadata_history.recorded_by 存 NULL 而非哨兵字符串 'system' (#4556)

.changeset/recorded-by-nullable-lookup.md

@@ -0,0 +1,71 @@
+---
+"@objectstack/metadata-core": major
+"@objectstack/metadata-protocol": major
+"@objectstack/cli": minor
+---
+
+fix(metadata)!: `sys_metadata_history.recorded_by` stores NULL, not the sentinel string `'system'` (#4556)
+
+`recorded_by` is declared `Field.lookup('sys_user', { readonly: true })` — a
+foreign key. The write path filled it with `actor ?? 'system'`, so every
+metadata write without a caller actor (boot sync, migration, an internal call)
+stored the **string** `'system'` in a column whose declared type says "the id
+of a `sys_user` row". No such row exists, and `SystemUserId.SYSTEM`
+(`'usr_system'`) is not auto-provisioned on the current runtime either, so the
+value resolved to nothing under any reading. Any consumer that read the field
+by its declaration — `expand`, an owner column in a report, an audit timeline
+showing "who changed this" — got an id that could not be dereferenced.
+
+It had already cost twice. #4441 had to exempt every `readonly` field from the
+write-path referential-integrity check, because otherwise ordinary metadata
+authoring (package create / publish / clone) was rejected. #4551's
+dangling-reference audit had to skip the same set for the same reason. The
+field ended up the platform's only reference column that is neither enforced
+nor audited.
+
+**The fix is on the write path, not the declaration.** `recorded_by` stays a
+`lookup('sys_user')`; an actor-less write now stores `NULL`, and `NULL` means
+"system-initiated (boot sync, migration, scheduled job)" — the standard
+expression of "no link", and already what this column's `set_null` delete
+behaviour means. No magic system-user account (a row that can never sign in yet
+holds an identity is a new security surface), and no `actor_kind` companion
+column.
+
+**Breaking — the repository contract is now explicitly nullable.**
+
+| Surface | Before | After |
+|:--|:--|:--|
+| `PutOptions.actor`, `DeleteOptions.actor` | `string` | `string \| null` (still **required**) |
+| `MetadataEvent.actor` | `string` | `string \| null` |
+| `MetadataItem.authoredBy` | `string` | `string \| null` |
+
+`actor` stays required rather than becoming optional on purpose: every call
+site must state which of the two it is, so a forgotten actor cannot silently
+become a fake foreign key. Migrating a caller:
+
+- **Writers** — passing a real identity: unchanged. Passing `'system'`, `''`,
+  or a label to satisfy the type: pass `null` instead.
+- **Readers** — `event.actor` and `item.authoredBy` can be `null`. Handle it at
+  the point of display (`actor ?? 'System'` in a UI string is fine — the fix is
+  that the *stored* value no longer lies, not that no label may ever be shown).
+
+Two read paths also stopped inventing a value: `SysMetadataRepository.history()`
+and `getByHash()` rendered an absent actor as the string `'unknown'`, which is
+indistinguishable from a real user id to anything that resolves the field. They
+now surface `null`.
+
+**Existing rows: `os migrate recorded-by`.** The stored `'system'` values are
+rewritten to `NULL` by a new command, which runs the conversion through the
+ADR-0119 D2 migration journal (chunk-atomic, resumable via `os migrate resume`).
+It is a dry run by default and safe to re-run — it selects only rows still
+holding the sentinel, so a second `--apply` converts nothing.
+
+The rewrite is **semantically equivalent, not a reinterpretation**: this column
+has only ever held that one sentinel, written by exactly one expression
+(`actor ?? 'system'`), and both spellings mean "no actor" — only `NULL` is
+expressible in the declared type.
+
+Deliberately unchanged: `sys_metadata_audit.actor` is a `text` column whose
+declaration already says "user id, system id, or `'system'`", so its `'system'`
+default is honest and stays. The #4441 `readonly` narrowing and the #4551 audit
+skip also stay — see the PR for why they are still correct.

packages/cli/src/commands/migrate/recorded-by.test.ts

@@ -0,0 +1,43 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `os migrate recorded-by` command shape (#4556).
+ *
+ * The conversion itself is proven against the real journal runner in
+ * `@objectstack/metadata-protocol`'s `migrations/recorded-by-sentinel.test.ts`.
+ * What is pinned here is the thing a unit test of the plan cannot see: that
+ * this subcommand honours #2186 — a bare `os migrate <topic>` must never
+ * mutate the database by surprise, so writing is opt-in behind `--apply`.
+ */
+
+import { describe, it, expect } from 'vitest';
+import MigrateRecordedBy from './recorded-by.js';
+import {
+  RECORDED_BY_SENTINEL,
+  RECORDED_BY_SENTINEL_PLAN_ID,
+  createRecordedBySentinelPlan,
+} from '@objectstack/metadata-protocol';
+
+describe('os migrate recorded-by', () => {
+  it('is a dry run by default — --apply is opt-in (#2186)', () => {
+    expect(MigrateRecordedBy.flags.apply.default).toBe(false);
+  });
+
+  it('requires explicit confirmation to write — --yes is opt-in', () => {
+    expect(MigrateRecordedBy.flags.yes.default).toBe(false);
+  });
+
+  it('describes what it converts, so `os migrate --help` is self-explanatory', () => {
+    expect(MigrateRecordedBy.description).toContain('recorded_by');
+    expect(MigrateRecordedBy.description).toContain('NULL');
+  });
+
+  it('drives the plan the metadata package owns — no second copy of the conversion', () => {
+    const plan = createRecordedBySentinelPlan();
+    expect(plan.id).toBe(RECORDED_BY_SENTINEL_PLAN_ID);
+    // A rediscovered run goes FORWARD: unwinding would put the fake foreign
+    // key back, which is the thing this plan exists to remove.
+    expect(plan.onCrash).toBe('resume');
+    expect(RECORDED_BY_SENTINEL).toBe('system');
+  });
+});

packages/cli/src/commands/migrate/recorded-by.ts

@@ -0,0 +1,198 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { Command, Flags } from '@oclif/core';
+import chalk from 'chalk';
+import { createInterface } from 'node:readline';
+import {
+  runMigrationJournal,
+  MigrationJournalRefusal,
+  type MigrationPlanProvider,
+} from '@objectstack/core';
+import {
+  createRecordedBySentinelPlan,
+  findSentinelHistoryRows,
+  RECORDED_BY_SENTINEL,
+  RECORDED_BY_SENTINEL_PLAN_ID,
+} from '@objectstack/metadata-protocol';
+import type { IObjectQLEngine } from '@objectstack/spec/contracts';
+import {
+  printHeader,
+  printSuccess,
+  printWarning,
+  printError,
+  printInfo,
+  printStep,
+  createTimer,
+  emitJson,
+} from '../../utils/format.js';
+import { bootSchemaStack } from '../../utils/schema-migrate.js';
+import { buildDataMigrationPlugins } from '../../utils/data-migration-plugins.js';
+
+async function confirm(question: string): Promise<boolean> {
+  if (!process.stdin.isTTY) return false; // non-interactive → require --yes
+  const rl = createInterface({ input: process.stdin, output: process.stdout });
+  try {
+    const answer: string = await new Promise((resolve) => rl.question(question, resolve));
+    return /^y(es)?$/i.test(answer.trim());
+  } finally {
+    rl.close();
+  }
+}
+
+/**
+ * `os migrate recorded-by` — rewrite the `'system'` sentinel in
+ * `sys_metadata_history.recorded_by` to `NULL` (#4556).
+ *
+ * `recorded_by` is a `lookup('sys_user')` that used to receive the STRING
+ * `'system'` on every actor-less metadata write. That string is not any
+ * user's id, so the column declared a foreign key and stored something no
+ * join could resolve. The runtime no longer writes it (the write path stores
+ * `NULL`); this command converts the rows already on disk.
+ *
+ * The conversion is semantically equivalent, not a reinterpretation: the
+ * column has only ever held that one sentinel, written by one expression, and
+ * both spellings mean "no actor" — only `NULL` is expressible in the declared
+ * type.
+ *
+ * Dry run by default, like every other `os migrate` subcommand (#2186): a
+ * bare invocation reports how many rows still carry the sentinel and writes
+ * nothing. `--apply` runs the conversion through the ADR-0119 D2 migration
+ * journal, so each chunk is one transaction and an interrupted run is
+ * recoverable via `os migrate resume`.
+ *
+ * Safe to re-run: the plan selects only rows still holding the sentinel, so a
+ * second `--apply` finds none and commits zero chunks.
+ */
+export default class MigrateRecordedBy extends Command {
+  static override description =
+    "Rewrite the legacy 'system' sentinel in sys_metadata_history.recorded_by to NULL (#4556). " +
+    'Dry-run by default; --apply runs the conversion through the migration journal.';
+
+  static override examples = [
+    '$ os migrate recorded-by',
+    '$ os migrate recorded-by --json',
+    '$ os migrate recorded-by --apply --yes',
+  ];
+
+  static override flags = {
+    'database-url': Flags.string({
+      description: 'Database URL to inspect (defaults to $OS_DATABASE_URL / the project DB)',
+      env: 'OS_DATABASE_URL',
+    }),
+    apply: Flags.boolean({ description: 'Perform the conversion (default is a read-only report)', default: false }),
+    yes: Flags.boolean({ char: 'y', description: 'Skip the confirmation prompt', default: false }),
+    'chunk-size': Flags.integer({ description: 'Rows per journal chunk', default: 200 }),
+    json: Flags.boolean({ description: 'Machine-readable output', default: false }),
+  };
+
+  async run(): Promise<void> {
+    const { flags } = await this.parse(MigrateRecordedBy);
+    const timer = createTimer();
+
+    if (!flags.json) printHeader('Migrate · recorded-by sentinel → NULL');
+    if (!flags.json) printStep(flags.apply ? 'Booting data stack…' : 'Booting data stack (read-only)…');
+
+    let stack;
+    try {
+      stack = await bootSchemaStack({
+        databaseUrl: flags['database-url'],
+        extraPlugins: await buildDataMigrationPlugins(),
+      });
+    } catch (error: any) {
+      if (flags.json) { await emitJson({ error: error.message }, 0, { compact: true }); this.exit(1); }
+      printError(error.message || String(error));
+      this.exit(1);
+      return;
+    }
+
+    try {
+      const engine: IObjectQLEngine = stack.kernel.getService('objectql');
+      if (typeof engine?.find !== 'function') {
+        throw new Error('No ObjectQL engine on this stack — cannot read sys_metadata_history.');
+      }
+
+      const plan = createRecordedBySentinelPlan({ chunkSize: flags['chunk-size'] });
+
+      // Register the plan so an interrupted run is resumable in THIS process
+      // too — `os migrate resume` looks plans up by id, and a run whose plan
+      // nothing registers is reported unresumable.
+      try {
+        const plans = stack.kernel.getService('migration-plans') as MigrationPlanProvider;
+        plans?.register?.(plan);
+      } catch { /* no registry composed — resume reports it, this run still works */ }
+
+      const pending = await findSentinelHistoryRows(engine);
+
+      // ── dry run (default): read-only ─────────────────────────────────
+      if (!flags.apply) {
+        if (flags.json) {
+          await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, sentinel: RECORDED_BY_SENTINEL, pending: pending.length, applied: false }, timer.elapsed());
+          return;
+        }
+        if (pending.length === 0) {
+          printSuccess(`No sys_metadata_history row holds the '${RECORDED_BY_SENTINEL}' sentinel — nothing to convert.`);
+          return;
+        }
+        printWarning(`${pending.length} sys_metadata_history row(s) hold recorded_by = '${RECORDED_BY_SENTINEL}'.`);
+        printInfo("Nothing was changed. Re-run with --apply to rewrite them to NULL.");
+        return;
+      }
+
+      // ── apply ────────────────────────────────────────────────────────
+      if (pending.length === 0) {
+        const msg = `No sys_metadata_history row holds the '${RECORDED_BY_SENTINEL}' sentinel — nothing to convert.`;
+        if (flags.json) { await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, pending: 0, applied: true, status: 'completed', chunksCommitted: 0 }, timer.elapsed()); return; }
+        printSuccess(msg);
+        return;
+      }
+
+      if (!flags.yes) {
+        const summary = `Rewrite recorded_by '${RECORDED_BY_SENTINEL}' → NULL on ${pending.length} row(s)`;
+        if (flags.json || !process.stdin.isTTY) {
+          if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+          printWarning(`Confirmation required: ${summary}. Re-run with --yes.`);
+          this.exit(1);
+          return;
+        }
+        const ok = await confirm(chalk.bold(`\n${summary}? [y/N] `));
+        if (!ok) { printInfo('Aborted — nothing changed.'); return; }
+      }
+
+      if (!flags.json) printStep('Converting…');
+      const result = await runMigrationJournal(engine, plan);
+
+      if (flags.json) {
+        await emitJson({ ...result, pending: pending.length, applied: true, error: result.error ? String(result.error) : undefined }, timer.elapsed());
+        this.exit(result.status === 'completed' ? 0 : 1);
+        return;
+      }
+
+      if (result.status === 'completed') {
+        printSuccess(
+          `Converted ${pending.length} row(s) — run '${result.runId}', ${result.chunksCommitted}/${result.chunksTotal} chunk(s) committed.`,
+        );
+      } else if (result.status === 'compensated') {
+        printWarning(
+          `Run '${result.runId}' failed and was unwound — ${result.chunksCompensated} chunk(s) compensated. ` +
+            `The sentinel rows are back as they were; nothing is half-converted.`,
+        );
+        this.exit(1);
+      } else {
+        printError(
+          `Run '${result.runId}' FAILED and its compensation did not finish. ` +
+            `Inspect sys_migration_journal for run '${result.runId}' — this needs a decision, not a retry.`,
+        );
+        this.exit(1);
+      }
+    } catch (error: any) {
+      const msg = error instanceof MigrationJournalRefusal
+        ? `Refused (${error.code}): ${error.message}`
+        : (error?.message || String(error));
+      if (flags.json) { await emitJson({ error: msg }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+      printError(msg);
+      this.exit(1);
+    } finally {
+      try { await stack.shutdown?.(); } catch { /* best effort */ }
+    }
+  }
+}

packages/cli/src/index.ts

@@ -24,6 +24,9 @@ export { default as MigrateApplyCommand } from './commands/migrate/apply.js';
 // ADR-0119 D2 (#4617): act on a run the journal says was interrupted. Boot
 // discovers (MigrationRecoveryPlugin); this acts, under operator intent.
 export { default as MigrateResumeCommand } from './commands/migrate/resume.js';
+// #4556: rewrite the legacy `'system'` sentinel in
+// `sys_metadata_history.recorded_by` to NULL, through the same journal.
+export { default as MigrateRecordedByCommand } from './commands/migrate/recorded-by.js';
 
 // ─── Environments topic subcommands ─────────────────────────────────
 export { default as EnvironmentsListCommand } from './commands/environments/list.js';

packages/metadata-core/src/objects/sys-metadata-history.object.ts

@@ -152,11 +152,23 @@ export const SysMetadataHistoryObject = ObjectSchema.create({
       description: 'Organization for multi-tenant isolation.',
     }),
 
-    /** User who made this change (= MetadataEvent.actor). */
+    /**
+     * User who made this change (= MetadataEvent.actor).
+     *
+     * NULL when the write had no human actor — a system-initiated write
+     * (boot metadata sync, a data migration, a scheduled job). #4556: this
+     * column used to receive the sentinel STRING `'system'`, which is not
+     * any `sys_user` id, so a lookup column declared as a foreign key held
+     * a value no join could ever resolve. NULL is the standard expression
+     * of "no link" and is what `deleteBehavior: 'set_null'` already means
+     * here, so the declared type and the stored value now agree.
+     */
     recorded_by: Field.lookup('sys_user', {
       label: 'Recorded By',
       required: false,
       readonly: true,
+      description:
+        'User who made this change. NULL = system-initiated (boot sync, migration, scheduled job) — never a sentinel string.',
     }),
 
     /** When was this version recorded */