From 5a577ea68bfc665f01a31f6ea0c9e71153e172c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 01:19:03 +0000 Subject: [PATCH 1/2] fix(metadata)!: store NULL in sys_metadata_history.recorded_by, not the 'system' sentinel (#4556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `recorded_by` is declared `Field.lookup('sys_user', { readonly: true })` — a foreign key — while the write path filled it with `actor ?? 'system'`. Every actor-less metadata write therefore stored the STRING `'system'` in a column whose declared type says "an id of a sys_user row", and no such row exists. Declared != actual, at the data layer. Per the maintainer's 2026-08-02 ruling, the fix is on the WRITE path, not the declaration: `recorded_by` stays a lookup, an actor-less write stores NULL, and NULL means "system-initiated (boot sync, migration, scheduled job)". No magic system-user account, no actor-kind companion column. - `sys-metadata-history.object.ts`: `recorded_by` carries a `description` stating the NULL semantics, so the declaration says what the column holds. - `metadata-core/types.ts`: `PutOptions.actor` / `DeleteOptions.actor` widen to `string | null` and stay REQUIRED, so every call site must say which of the two it is; `MetadataEvent.actor` and `MetadataItem.authoredBy` become nullable. - `sys-metadata-repository.ts`: both history writes store `opts.actor ?? null`; the three read paths surface `null` instead of inventing `'unknown'`; `close()`'s synthetic drain event carries no actor. - `protocol.ts`: the five `?? 'system'` sites that flow into `recorded_by` (save / publish / revert-commit / rollback / delete) pass `null`. The three that do NOT are left alone: `sys_metadata_audit.actor` is a `text` column whose declaration already admits `'system'`, and `PublishMaterializer.actor` is a plugin callback argument that reaches no lookup column. - New `os migrate recorded-by` rewrites stored `'system'` to NULL through the ADR-0119 D2 migration journal (chunk-atomic, resumable, dry run by default, idempotent on re-run). The plan itself lives with the code that wrote the sentinel, in `metadata-protocol/src/migrations/`. Tests: repository-level NULL round-trip, the plan under the real journal runner (idempotence, chunking, compensation, transaction binding), and an end-to-end protocol suite against a real ObjectQL engine where `recorded_by` is declared as the real readonly lookup — which also pins that create/publish/delete authoring still passes the #4441 integrity check. Deliberately NOT touched: #4441's `readonly` narrowing in `objectql/engine.ts` and #4551's audit skip. See the PR description. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- .changeset/recorded-by-nullable-lookup.md | 71 ++++++ .../src/commands/migrate/recorded-by.test.ts | 43 ++++ .../cli/src/commands/migrate/recorded-by.ts | 198 +++++++++++++++ packages/cli/src/index.ts | 3 + .../objects/sys-metadata-history.object.ts | 14 +- packages/metadata-core/src/types.ts | 27 +- packages/metadata-protocol/src/index.ts | 11 + .../migrations/recorded-by-sentinel.test.ts | 230 +++++++++++++++++ .../src/migrations/recorded-by-sentinel.ts | 156 ++++++++++++ packages/metadata-protocol/src/protocol.ts | 18 +- ...ys-metadata-repository.recorded-by.test.ts | 176 +++++++++++++ .../src/sys-metadata-repository.ts | 29 ++- .../src/protocol-recorded-by-null.test.ts | 235 ++++++++++++++++++ 13 files changed, 1192 insertions(+), 19 deletions(-) create mode 100644 .changeset/recorded-by-nullable-lookup.md create mode 100644 packages/cli/src/commands/migrate/recorded-by.test.ts create mode 100644 packages/cli/src/commands/migrate/recorded-by.ts create mode 100644 packages/metadata-protocol/src/migrations/recorded-by-sentinel.test.ts create mode 100644 packages/metadata-protocol/src/migrations/recorded-by-sentinel.ts create mode 100644 packages/metadata-protocol/src/sys-metadata-repository.recorded-by.test.ts create mode 100644 packages/objectql/src/protocol-recorded-by-null.test.ts diff --git a/.changeset/recorded-by-nullable-lookup.md b/.changeset/recorded-by-nullable-lookup.md new file mode 100644 index 0000000000..90a17d5c75 --- /dev/null +++ b/.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. diff --git a/packages/cli/src/commands/migrate/recorded-by.test.ts b/packages/cli/src/commands/migrate/recorded-by.test.ts new file mode 100644 index 0000000000..ec41fbf49d --- /dev/null +++ b/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 ` 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'); + }); +}); diff --git a/packages/cli/src/commands/migrate/recorded-by.ts b/packages/cli/src/commands/migrate/recorded-by.ts new file mode 100644 index 0000000000..c53573cb4f --- /dev/null +++ b/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 { + 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 { + 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 */ } + } + } +} diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 10842574f7..0d68437078 100644 --- a/packages/cli/src/index.ts +++ b/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'; diff --git a/packages/metadata-core/src/objects/sys-metadata-history.object.ts b/packages/metadata-core/src/objects/sys-metadata-history.object.ts index e1ae807fde..29baceb4b8 100644 --- a/packages/metadata-core/src/objects/sys-metadata-history.object.ts +++ b/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 */ diff --git a/packages/metadata-core/src/types.ts b/packages/metadata-core/src/types.ts index eae2a56ec6..2b9d9d426d 100644 --- a/packages/metadata-core/src/types.ts +++ b/packages/metadata-core/src/types.ts @@ -99,7 +99,7 @@ export const MetadataItemSchema = z.object({ body: z.record(z.string(), z.unknown()).describe('Canonical Zod-normalised spec'), hash: z.string().regex(/^sha256:[0-9a-f]{64}$/).describe('sha256(canonicalize(body))'), parentHash: z.string().nullable().describe('Hash this version was derived from; null for first version'), - authoredBy: z.string().describe('Identity of the writer (user id, "cli", "ai:claude", …)'), + authoredBy: z.string().nullable().describe('Identity of the writer (user id, "cli", "ai:claude", …); null = system-initiated, no actor'), authoredAt: z.string().describe('ISO-8601 timestamp'), message: z.string().optional().describe('Optional commit message'), seq: z.number().int().nonnegative().describe('Sequence number this write produced in the org log'), @@ -136,7 +136,13 @@ export const MetadataEventSchema = z.object({ */ version: z.number().int().positive().optional(), previousName: z.string().optional().describe('Set on op="rename"'), - actor: z.string(), + /** + * Who wrote this. `null` = system-initiated (boot sync, migration, + * scheduled job) — see `PutOptions.actor` (#4556). Never a sentinel + * string: consumers that resolve this against `sys_user` must be able to + * tell "nobody" from "a user id", and only `null` says the former. + */ + actor: z.string().nullable(), message: z.string().optional(), ts: z.string(), source: z.string().describe('Origin label: "fs", "studio", "rest", "ai", "git-import", …'), @@ -170,8 +176,18 @@ export interface PutOptions { * absence". A mismatch throws ConflictError. */ parentVersion: string | null; - /** Identity of the writer; mirrored to MetadataEvent.actor. */ - actor: string; + /** + * Identity of the writer; mirrored to MetadataEvent.actor and stored in + * `sys_metadata_history.recorded_by` (a `lookup('sys_user')`). + * + * **Required but nullable, deliberately** (#4556). Pass `null` — never a + * label like `'system'` — when the write has no human actor: a boot + * metadata sync, a data migration, a scheduled job. Keeping the property + * required rather than optional forces every call site to state which of + * the two it is, so a forgotten actor cannot silently become a fake + * foreign key in a lookup column. + */ + actor: string | null; /** Optional human-readable commit message. */ message?: string; /** Optional label for the change log "source" column. */ @@ -198,7 +214,8 @@ export interface PutResult { export interface DeleteOptions { parentVersion: string; - actor: string; + /** Identity of the writer; `null` = system-initiated. See {@link PutOptions.actor}. */ + actor: string | null; message?: string; source?: string; /** Two-tier authorization intent; defaults to `override-artifact`. */ diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 390e00ce11..3bc534e7aa 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -34,6 +34,17 @@ export type { MetadataDiagnostics } from './metadata-diagnostics.js'; export type { MetadataHostEngine } from './host-engine.js'; +// #4556 — the `sys_metadata_history.recorded_by` sentinel → NULL conversion, +// as an ADR-0119 D2 migration plan. Run by `os migrate recorded-by`. +export { + createRecordedBySentinelPlan, + findSentinelHistoryRows, + METADATA_HISTORY_OBJECT, + RECORDED_BY_SENTINEL, + RECORDED_BY_SENTINEL_PLAN_ID, +} from './migrations/recorded-by-sentinel.js'; +export type { SentinelHistoryRow } from './migrations/recorded-by-sentinel.js'; + export { SeedLoaderService } from './seed-loader.js'; export { runBuildProbes } from './build-probes.js'; export type { diff --git a/packages/metadata-protocol/src/migrations/recorded-by-sentinel.test.ts b/packages/metadata-protocol/src/migrations/recorded-by-sentinel.test.ts new file mode 100644 index 0000000000..16dcd308c4 --- /dev/null +++ b/packages/metadata-protocol/src/migrations/recorded-by-sentinel.test.ts @@ -0,0 +1,230 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4556 — the `'system'` → `NULL` conversion, under the ADR-0119 D2 journal. + * + * The bar is not "rows changed". It is: + * + * 1. only sentinel rows are touched (a real user id is never nulled); + * 2. the run leaves a journal that says what it did; + * 3. **re-running is a no-op** — the operator who is unsure whether it ran + * must be able to just run it again, which is the property a data + * migration is most often trusted with and least often given; + * 4. writes carry the chunk's transaction context, which is also what + * carries `isSystem` — without it ObjectQL strips the write to + * `recorded_by` (a `readonly` field) and the migration silently does + * nothing while reporting success. + * + * The fake engine implements REAL rollback for the same reason the runner's + * own suite does: without it, every assertion here would also pass against a + * plan that never opened a transaction. + */ + +import { describe, it, expect } from 'vitest'; +import { runMigrationJournal, readRunJournal } from '@objectstack/core'; +import { + createRecordedBySentinelPlan, + findSentinelHistoryRows, + METADATA_HISTORY_OBJECT, + RECORDED_BY_SENTINEL, + RECORDED_BY_SENTINEL_PLAN_ID, +} from './recorded-by-sentinel.js'; + +interface FakeRow { [k: string]: unknown } + +class FakeEngine { + tables = new Map(); + /** Every context handed to `update`, so a test can prove transaction binding. */ + updateContexts: unknown[] = []; + private txDepth = 0; + private snapshot: Map | null = null; + /** Set to a chunk index to make that chunk's first write throw. */ + failOnUpdateOfId: string | null = null; + + private rows(name: string): FakeRow[] { + if (!this.tables.has(name)) this.tables.set(name, []); + return this.tables.get(name)!; + } + + seedHistory(rows: FakeRow[]): void { + this.tables.set(METADATA_HISTORY_OBJECT, rows.map((r) => ({ ...r }))); + } + + history(): FakeRow[] { return this.rows(METADATA_HISTORY_OBJECT); } + + async insert(objectName: string, data: FakeRow): Promise { + const row = { ...data }; + if (objectName === 'sys_migration_journal') { + const dup = this.rows(objectName).find((r) => r.run_id === row.run_id && r.seq === row.seq); + if (dup) throw new Error(`duplicate journal key (${String(row.run_id)}, ${String(row.seq)})`); + } + this.rows(objectName).push(row); + return row; + } + + async find(objectName: string, query?: { where?: Record }): Promise { + const where = query?.where ?? {}; + return this.rows(objectName).filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)); + } + + async findOne(objectName: string, query?: { where?: Record }): Promise { + return (await this.find(objectName, query))[0] ?? null; + } + + async update( + objectName: string, + data: FakeRow, + options?: { where?: Record; context?: unknown }, + ): Promise { + this.updateContexts.push(options?.context); + const id = options?.where?.id; + if (this.failOnUpdateOfId !== null && id === this.failOnUpdateOfId) { + throw new Error(`simulated driver failure on ${String(id)}`); + } + const row = this.rows(objectName).find((r) => r.id === id); + if (!row) throw new Error(`not found: ${objectName}/${String(id)}`); + Object.assign(row, data); + return row; + } + + async delete(): Promise { return { deleted: 0 }; } + async count(): Promise { return 0; } + async aggregate(): Promise { return []; } + getObject(name: string): unknown { return { name }; } + getDefaultDriverName(): string { return 'fake'; } + getDriverByName(): unknown { return { beginTransaction: () => {}, commit: () => {}, rollback: () => {} }; } + + async transaction(cb: (trxCtx: unknown) => Promise, baseContext?: unknown): Promise { + if (this.txDepth > 0) return cb({ ...(baseContext as object), __tx: true }); + this.txDepth++; + this.snapshot = new Map([...this.tables].map(([k, v]) => [k, v.map((r) => ({ ...r }))])); + try { + const out = await cb({ ...(baseContext as object), __tx: true }); + this.snapshot = null; + return out; + } catch (err) { + this.tables = this.snapshot!; // real rollback + this.snapshot = null; + throw err; + } finally { + this.txDepth--; + } + } +} + +const asEngine = (e: FakeEngine) => e as unknown as Parameters[0]; + +/** Three sentinel rows, one real-actor row, one already-null row. */ +function seedMixed(engine: FakeEngine): void { + engine.seedHistory([ + { id: 'h1', recorded_by: RECORDED_BY_SENTINEL, operation_type: 'create' }, + { id: 'h2', recorded_by: 'usr_alice', operation_type: 'update' }, + { id: 'h3', recorded_by: RECORDED_BY_SENTINEL, operation_type: 'update' }, + { id: 'h4', recorded_by: null, operation_type: 'delete' }, + { id: 'h5', recorded_by: RECORDED_BY_SENTINEL, operation_type: 'publish' }, + ]); +} + +const byId = (engine: FakeEngine, id: string) => engine.history().find((r) => r.id === id)!; + +describe('#4556 migration — sentinel → NULL', () => { + it('rewrites every sentinel row to NULL and leaves real actors alone', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + + const result = await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan()); + + expect(result.status).toBe('completed'); + expect(result.chunksTotal).toBe(1); + expect(result.chunksCommitted).toBe(1); + + expect(byId(engine, 'h1').recorded_by).toBeNull(); + expect(byId(engine, 'h3').recorded_by).toBeNull(); + expect(byId(engine, 'h5').recorded_by).toBeNull(); + // Untouched: a real user id is data, not a sentinel. + expect(byId(engine, 'h2').recorded_by).toBe('usr_alice'); + expect(byId(engine, 'h4').recorded_by).toBeNull(); + }); + + it('writes to sys_metadata_history join the chunk transaction (and so carry isSystem)', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + + await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan()); + + expect(engine.updateContexts.length).toBe(3); + for (const ctx of engine.updateContexts) { + // `__tx` proves the write ran inside the chunk's transaction; `isSystem` + // is what the runner seeds the base context with, and is what stops + // ObjectQL stripping the write to this `readonly` column. + expect(ctx).toMatchObject({ __tx: true, isSystem: true }); + } + }); + + it('leaves a journal that names the run: started → chunk_started → chunk_done → done', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + + const result = await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan()); + const events = await readRunJournal(asEngine(engine), result.runId); + + expect(events.map((e) => e.kind)).toEqual(['run_started', 'chunk_started', 'chunk_done', 'run_done']); + const started = events[0]!; + expect(String(started.detail)).toContain(RECORDED_BY_SENTINEL_PLAN_ID); + }); + + it('is idempotent — a second run finds nothing, commits nothing, changes nothing', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + + const first = await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan()); + expect(first.chunksCommitted).toBe(1); + const afterFirst = engine.history().map((r) => ({ id: r.id, recorded_by: r.recorded_by })); + + // Nothing left to select — the plan's load() is the idempotence guard. + expect(await findSentinelHistoryRows(asEngine(engine))).toEqual([]); + + const second = await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan()); + expect(second.status).toBe('completed'); + expect(second.chunksTotal).toBe(0); + expect(second.chunksCommitted).toBe(0); + expect(second.runId).not.toBe(first.runId); + + expect(engine.history().map((r) => ({ id: r.id, recorded_by: r.recorded_by }))).toEqual(afterFirst); + }); + + it('chunks the work — a chunkSize of 2 over 3 sentinel rows runs two chunks', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + + const result = await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan({ chunkSize: 2 })); + + expect(result.chunksTotal).toBe(2); + expect(result.chunksCommitted).toBe(2); + expect(engine.history().filter((r) => r.recorded_by === RECORDED_BY_SENTINEL)).toEqual([]); + }); + + it('a failing chunk unwinds the committed ones — no half-converted audit log', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + // Chunk 0 converts h1+h3; chunk 1 blows up on h5. + engine.failOnUpdateOfId = 'h5'; + + const result = await runMigrationJournal(asEngine(engine), createRecordedBySentinelPlan({ chunkSize: 2 })); + + expect(result.status).toBe('compensated'); + expect(result.chunksCompensated).toBe(1); + // Compensation restored the sentinel the first chunk had removed: the log + // is back to exactly one pre-run state, not a mixture of two. + expect(byId(engine, 'h1').recorded_by).toBe(RECORDED_BY_SENTINEL); + expect(byId(engine, 'h3').recorded_by).toBe(RECORDED_BY_SENTINEL); + expect(byId(engine, 'h5').recorded_by).toBe(RECORDED_BY_SENTINEL); + expect(byId(engine, 'h2').recorded_by).toBe('usr_alice'); + }); + + it('findSentinelHistoryRows selects only sentinel rows, by id', async () => { + const engine = new FakeEngine(); + seedMixed(engine); + expect(await findSentinelHistoryRows(asEngine(engine))).toEqual([{ id: 'h1' }, { id: 'h3' }, { id: 'h5' }]); + }); +}); diff --git a/packages/metadata-protocol/src/migrations/recorded-by-sentinel.ts b/packages/metadata-protocol/src/migrations/recorded-by-sentinel.ts new file mode 100644 index 0000000000..fc4cadcead --- /dev/null +++ b/packages/metadata-protocol/src/migrations/recorded-by-sentinel.ts @@ -0,0 +1,156 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `sys_metadata_history.recorded_by`: sentinel `'system'` → `NULL` (#4556). + * + * ## What was wrong + * + * `recorded_by` is declared `Field.lookup('sys_user', { readonly: true })` — + * a foreign key. The write path filled it with `actor ?? 'system'`, so every + * actor-less metadata write (boot sync, migration, scheduled job) stored the + * STRING `'system'` in a column whose declared type says "an 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 could not resolve under any reading. + * + * That is `declared ≠ actual` at the data layer, and it had already cost + * twice: #4441 had to exempt every `readonly` field from the write-path + * referential-integrity check to stop ordinary package create/publish/clone + * being rejected, and #4551's dangling-reference audit had to skip the same + * set. The field ended up the platform's only reference column that is + * neither enforced nor audited. + * + * The maintainer's ruling (2026-08-02) fixed the WRITE path rather than + * loosening the declared type: `recorded_by` stays a `lookup('sys_user')`, + * an actor-less write stores `NULL`, and `NULL` means "system-initiated". + * No Salesforce-style magic system-user row (a record that can never log in + * yet holds an identity is a new security surface), and no `actor_kind` + * companion column. + * + * ## What this plan does + * + * Rewrites the rows already on disk. The column has only ever held one + * sentinel — `'system'`, written by exactly one expression, `actor ?? + * 'system'` — so `'system'` → `NULL` is SEMANTICALLY EQUIVALENT, not a + * reinterpretation: both spellings mean "no actor", and only the second one + * is expressible in the declared type. + * + * ## Why it rides the journal rather than a boot-time backfill + * + * ADR-0119 D2 (#4617). A backfill that runs as a side effect of a process + * starting is invisible when it half-finishes, and this one rewrites an + * append-only audit log — the one table where "it partly ran" must never be + * a guess. Under the journal each chunk is one transaction, `chunk_done` is + * written INSIDE it, and an interrupted run stays exactly as recoverable as + * it was at the moment it died. + * + * ## Idempotency + * + * `forward` sets a constant (`NULL`) on rows addressed by primary key, so a + * redelivered chunk (`attempt > 1`, outcome UNKNOWN) reaches the same state + * as a first delivery — the recheck the runner's at-least-once contract asks + * for is satisfied by construction rather than by a lookup. `load()` selects + * only rows still holding the sentinel, so a second RUN of the whole plan + * finds nothing and commits zero chunks. + */ + +import type { MigrationPlan, MigrationPlanStep, MigrationChunkContext } from '@objectstack/core'; +import type { IObjectQLEngine } from '@objectstack/spec/contracts'; +import type { EngineUpdateOptions } from '@objectstack/spec/data'; + +/** + * `MigrationChunkContext.context` is declared `unknown` — the runner hands + * back whatever the driver's `transaction()` produced and has no business + * asserting its shape. The engine's write options declare the execution + * envelope. This narrows once, at the one place that threads the two + * together, rather than at each call. + */ +type EngineExecutionContext = EngineUpdateOptions['context']; + +/** The table this plan rewrites. Matches `SysMetadataRepository.historyTable`. */ +export const METADATA_HISTORY_OBJECT = 'sys_metadata_history'; + +/** + * The one sentinel this column has ever held. + * + * Named rather than inlined so the plan, its compensation and its tests all + * refer to the same literal — a compensation that restores a DIFFERENT string + * than forward removed would silently rewrite history. + */ +export const RECORDED_BY_SENTINEL = 'system'; + +/** Stable plan id — recorded in the journal and looked up by `os migrate resume`. */ +export const RECORDED_BY_SENTINEL_PLAN_ID = 'metadata.recorded-by-sentinel-to-null'; + +/** Minimal row shape the plan needs: the primary key, nothing else. */ +export interface SentinelHistoryRow { + readonly id: string; +} + +/** + * Read the rows still holding the sentinel. + * + * Exported so a dry run can report the exact same set the apply run would + * rewrite, rather than a second query that could disagree with it. + */ +export async function findSentinelHistoryRows( + engine: IObjectQLEngine, +): Promise { + const rows = (await engine.find( + METADATA_HISTORY_OBJECT, + { where: { recorded_by: RECORDED_BY_SENTINEL }, fields: ['id'] }, + { context: { isSystem: true } }, + )) as Array<{ id?: unknown }> | null | undefined; + return (rows ?? []) + .filter((r): r is { id: string } => typeof r?.id === 'string') + .map((r) => ({ id: r.id })); +} + +/** + * Set `recorded_by` on one chunk of rows. + * + * `context: ctx.context` is the transaction-bound context, so the writes join + * the chunk's transaction instead of committing beside it — AND it carries + * `isSystem`, which is what lets a `readonly` column be written at all + * (`ObjectQL.update` strips caller-supplied writes to `readonly` fields for + * every non-system caller; a migration is the platform, not a caller). + */ +async function setRecordedBy( + rows: readonly SentinelHistoryRow[], + ctx: MigrationChunkContext, + engine: IObjectQLEngine, + value: string | null, +): Promise { + for (const row of rows) { + await engine.update( + METADATA_HISTORY_OBJECT, + { recorded_by: value }, + { where: { id: row.id }, context: ctx.context as EngineExecutionContext }, + ); + } +} + +/** + * The `'system'` → `NULL` plan. + * + * `onCrash: 'resume'` — a rediscovered run goes FORWARD. Unwinding would + * re-introduce the fake foreign key this plan exists to remove, so "finish + * the job" is the safe direction here and "put it back" is not. A + * `compensate` is still declared, because an in-run failure always unwinds + * (the runner is alive to do it) and a partially-rewritten audit log is + * nobody's intent. + */ +export function createRecordedBySentinelPlan(opts: { chunkSize?: number } = {}): MigrationPlan { + const step: MigrationPlanStep = { + name: 'sys_metadata_history.recorded_by: sentinel → null', + load: (engine) => findSentinelHistoryRows(engine), + forward: (rows, ctx, engine) => setRecordedBy(rows, ctx, engine, null), + compensate: (rows, ctx, engine) => setRecordedBy(rows, ctx, engine, RECORDED_BY_SENTINEL), + }; + return { + id: RECORDED_BY_SENTINEL_PLAN_ID, + steps: [step], + onCrash: 'resume', + ...(opts.chunkSize !== undefined ? { chunkSize: opts.chunkSize } : {}), + }; +} diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index a9cf6c9d64..52d8988f51 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -6825,7 +6825,10 @@ export class ObjectStackProtocolImplementation implements try { const result = await repo.put(ref, request.item, { parentVersion, - actor: request.actor ?? 'system', + // #4556 — `actor` lands in `sys_metadata_history.recorded_by`, + // a lookup('sys_user'). No caller actor → NULL, never the + // sentinel string 'system' (which resolves to no user row). + actor: request.actor ?? null, source: writeSource, intent, state: mode === 'draft' ? 'draft' : 'active', @@ -7462,7 +7465,8 @@ export class ObjectStackProtocolImplementation implements } as Parameters[0]; try { const result = await repo.promoteDraft(ref, { - actor: request.actor ?? 'system', + // #4556 — NULL, not 'system', for an actor-less publish. + actor: request.actor ?? null, source: 'protocol.publishMetaItem', ...(request.message ? { message: request.message } : {}), intent, @@ -8607,7 +8611,9 @@ export class ObjectStackProtocolImplementation implements } const items = this.parseCommitItems(row.items); const repo = this.getOverlayRepo(orgId); - const actor = request.actor ?? 'system'; + // #4556 — threaded into repo.put/delete → `recorded_by`; NULL when the + // revert carries no human actor. + const actor = request.actor ?? null; const reverted: Array<{ type: string; name: string; action: 'removed' | 'restored' }> = []; const failed: Array<{ type: string; name: string; error: string; code?: string }> = []; @@ -8787,7 +8793,8 @@ export class ObjectStackProtocolImplementation implements } as Parameters[0]; try { const result = await repo.restoreVersion(ref, request.toVersion, { - actor: request.actor ?? 'system', + // #4556 — NULL, not 'system', for an actor-less rollback. + actor: request.actor ?? null, source: 'protocol.rollbackMetaItem', ...(request.message ? { message: request.message } : {}), intent, @@ -9049,7 +9056,8 @@ export class ObjectStackProtocolImplementation implements const result = await repo.delete(ref, { parentVersion, - actor: request.actor ?? 'system', + // #4556 — NULL, not 'system', for an actor-less delete. + actor: request.actor ?? null, source: 'protocol.deleteMetaItem', intent: this.isArtifactBacked(singularTypeForRepo, request.name) ? 'override-artifact' diff --git a/packages/metadata-protocol/src/sys-metadata-repository.recorded-by.test.ts b/packages/metadata-protocol/src/sys-metadata-repository.recorded-by.test.ts new file mode 100644 index 0000000000..89d2283539 --- /dev/null +++ b/packages/metadata-protocol/src/sys-metadata-repository.recorded-by.test.ts @@ -0,0 +1,176 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4556 — `sys_metadata_history.recorded_by` stores an id or NULL, never a + * sentinel string, and the read paths surface that NULL as `null`. + * + * Two halves, and they fail for different reasons if either regresses: + * + * - **write**: an actor-less write must land SQL NULL. `undefined` is not + * good enough to assert on — a driver that drops undefined keys and one + * that writes them are indistinguishable at the row level — so the + * repository normalises to `null` and these tests pin `toBeNull()`. + * - **read**: the two read paths used to render an absent actor as the + * string `'unknown'`, which is the same declared-≠-actual defect pointed + * the other way: an audit timeline showing "changed by unknown" cannot be + * told apart from a real user id, and no consumer can resolve it. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SysMetadataRepository } from './sys-metadata-repository.js'; + +interface Row { [k: string]: unknown } + +/** + * Minimal engine fake. Deliberately stores exactly what it is handed — no + * key-dropping, no coercion — so `recorded_by: null` and a missing + * `recorded_by` are distinguishable in the assertions below. + */ +function makeFakeEngine() { + const rows = new Map(); + const historyRows: Row[] = []; + + const keyOf = (w: Record) => + `${String(w.type)}|${String(w.name)}|${String(w.organization_id ?? 'null')}|${String(w.state ?? 'active')}`; + + const findRow = (where: Record) => { + if (where.id !== undefined) { + for (const [k, r] of rows) if (r.id === where.id) return { key: k, row: r }; + return null; + } + const k = keyOf(where); + const r = rows.get(k); + return r ? { key: k, row: r } : null; + }; + + const matchesHistory = (h: Row, where: Record): boolean => + Object.entries(where).every(([k, v]) => v === undefined || h[k] === v); + + return { + rows, + historyRows, + async find(table: string, opts: { where: Record }) { + if (table === 'sys_metadata_history') return historyRows.filter((h) => matchesHistory(h, opts.where)); + return Array.from(rows.values()).filter((r) => { + if (opts.where.type && r.type !== opts.where.type) return false; + if (opts.where.organization_id !== undefined && r.organization_id !== opts.where.organization_id) return false; + if (opts.where.state && r.state !== opts.where.state) return false; + return true; + }); + }, + async findOne(table: string, opts: { where: Record }) { + if (table === 'sys_metadata_history') return historyRows.find((h) => matchesHistory(h, opts.where)) ?? null; + return findRow(opts.where)?.row ?? null; + }, + async insert(table: string, data: Record) { + if (table === 'sys_metadata_history') { + const h: Row = { ...data }; + if (!h.id) h.id = `h_${historyRows.length + 1}`; + historyRows.push(h); + return { id: h.id as string }; + } + const k = keyOf(data); + const row: Row = { id: `r_${rows.size + 1}`, ...data }; + rows.set(k, row); + return { id: row.id as string }; + }, + async update(_t: string, data: Record, opts: { where: Record }) { + const found = findRow(opts.where); + if (!found) throw new Error('not found'); + rows.set(found.key, { ...found.row, ...data }); + return { id: found.row.id as string }; + }, + async delete(_t: string, opts: { where: Record }) { + const found = findRow(opts.where); + if (!found) return { deleted: 0 }; + rows.delete(found.key); + return { deleted: 1 }; + }, + async transaction(cb: (ctx: any) => Promise): Promise { return cb(undefined); }, + }; +} + +const view = (label: string) => ({ name: 'case_grid', label, object: 'case', columns: [{ field: 'name' }] }); + +describe('#4556 — recorded_by holds a user id or NULL, never a sentinel', () => { + let engine: ReturnType; + let repo: SysMetadataRepository; + const ref = { org: 'org_alpha', type: 'view' as const, name: 'case_grid' }; + + beforeEach(() => { + engine = makeFakeEngine(); + repo = new SysMetadataRepository({ engine, organizationId: 'org_alpha', orgLabel: 'org_alpha' }); + }); + + it('put with actor: null writes SQL NULL — not "system", not any other string', async () => { + await repo.put(ref, view('A'), { parentVersion: null, actor: null }); + + expect(engine.historyRows).toHaveLength(1); + const row = engine.historyRows[0]!; + // The point of the issue: a lookup('sys_user') column must not hold a + // value that resolves to no user row. + expect(row.recorded_by).toBeNull(); + expect(typeof row.recorded_by).not.toBe('string'); + expect(row.recorded_by).not.toBe('system'); + }); + + it('put with a real actor still stores the id verbatim', async () => { + await repo.put(ref, view('A'), { parentVersion: null, actor: 'usr_alice' }); + expect(engine.historyRows[0]!.recorded_by).toBe('usr_alice'); + }); + + it('delete with actor: null writes a tombstone whose recorded_by is NULL', async () => { + const first = await repo.put(ref, view('A'), { parentVersion: null, actor: 'usr_alice' }); + await repo.delete(ref, { parentVersion: first.version, actor: null }); + + const tombstone = engine.historyRows.find((h) => h.operation_type === 'delete')!; + expect(tombstone).toBeDefined(); + expect(tombstone.recorded_by).toBeNull(); + }); + + it('history() surfaces an actor-less event as actor: null — never the string "unknown"', async () => { + await repo.put(ref, view('A'), { parentVersion: null, actor: null }); + + const events = []; + for await (const e of repo.history(ref)) events.push(e); + + expect(events).toHaveLength(1); + expect(events[0]!.actor).toBeNull(); + // 'unknown' is indistinguishable from a user id to every consumer that + // resolves this field — that is exactly what made it a lie. + expect(events[0]!.actor).not.toBe('unknown'); + }); + + it('history() still surfaces a real actor unchanged', async () => { + await repo.put(ref, view('A'), { parentVersion: null, actor: 'usr_alice' }); + const events = []; + for await (const e of repo.history(ref)) events.push(e); + expect(events[0]!.actor).toBe('usr_alice'); + }); + + it('getByHash() returns authoredBy: null for an actor-less version', async () => { + const put = await repo.put(ref, view('A'), { parentVersion: null, actor: null }); + + const item = await repo.getByHash(ref, put.version); + expect(item).not.toBeNull(); + expect(item!.authoredBy).toBeNull(); + expect(item!.authoredBy).not.toBe('unknown'); + }); + + it('getByHash() returns the real actor when there was one', async () => { + const put = await repo.put(ref, view('A'), { parentVersion: null, actor: 'usr_alice' }); + const item = await repo.getByHash(ref, put.version); + expect(item!.authoredBy).toBe('usr_alice'); + }); + + it('list() reports authoredBy: null when the overlay row carries no updated_by/created_by', async () => { + await repo.put(ref, view('A'), { parentVersion: null, actor: null }); + + const headers = []; + for await (const h of repo.list({ type: 'view' })) headers.push(h); + + expect(headers).toHaveLength(1); + expect(headers[0]!.authoredBy).toBeNull(); + expect(headers[0]!.authoredBy).not.toBe('unknown'); + }); +}); diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index d5b8c4201f..89e2fb0391 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -318,7 +318,10 @@ export class SysMetadataRepository implements MetadataRepository { body: body as Record, hash, parentHash: (row as any).previous_checksum ?? null, - authoredBy: (row as any).recorded_by ?? 'unknown', + // #4556 — a null `recorded_by` means the write had no actor. Rendering + // that as the string 'unknown' invents an identity the column never + // held, which is the same declared-≠-actual defect on the read side. + authoredBy: ((row as any).recorded_by as string | null | undefined) ?? null, authoredAt: (row as any).recorded_at ?? new Date(0).toISOString(), message: (row as any).change_note ?? undefined, seq: ((row as any).event_seq as number) ?? 0, @@ -424,7 +427,10 @@ export class SysMetadataRepository implements MetadataRepository { change_note: opts.message, source: opts.source ?? 'sys-metadata-repo', organization_id: this.organizationId, - recorded_by: opts.actor, + // #4556 — `recorded_by` is a lookup('sys_user'). A write with no + // actor stores NULL, never a sentinel string: an id that resolves + // to no row is a foreign key that lies. + recorded_by: opts.actor ?? null, recorded_at: now, }, { context: ctx }, @@ -546,7 +552,8 @@ export class SysMetadataRepository implements MetadataRepository { change_note: opts.message, source: opts.source ?? 'sys-metadata-repo', organization_id: this.organizationId, - recorded_by: opts.actor, + // #4556 — NULL, not a sentinel, when the delete had no actor. + recorded_by: opts.actor ?? null, recorded_at: now, }, { context: ctx }, @@ -596,7 +603,7 @@ export class SysMetadataRepository implements MetadataRepository { */ async promoteDraft( ref: MetaRef, - opts: { actor: string; source?: string; message?: string; intent?: MetadataWriteIntent }, + opts: { actor: string | null; source?: string; message?: string; intent?: MetadataWriteIntent }, ): Promise<{ version: string; seq: number; item: MetadataItem; packageId: string | null }> { this.assertOpen(); // Read the RAW draft row (not just the body) so the promotion can carry @@ -667,7 +674,7 @@ export class SysMetadataRepository implements MetadataRepository { async restoreVersion( ref: MetaRef, targetVersion: number, - opts: { actor: string; source?: string; message?: string; intent?: MetadataWriteIntent }, + opts: { actor: string | null; source?: string; message?: string; intent?: MetadataWriteIntent }, ): Promise<{ version: string; seq: number; item: MetadataItem }> { this.assertOpen(); const full = this.fullRef(ref); @@ -825,7 +832,10 @@ export class SysMetadataRepository implements MetadataRepository { hash: (row.checksum as string | null) ?? null, parentHash: (row.previous_checksum as string | null) ?? null, version: typeof row.version === 'number' ? row.version : undefined, - actor: (row.recorded_by as string | undefined) ?? 'unknown', + // #4556 — surface the absence, do not paper it over with a label. + // An audit timeline that must show "who changed this" needs to know + // the answer is "the platform", not a user literally named 'unknown'. + actor: (row.recorded_by as string | null | undefined) ?? null, message: (row.change_note as string | undefined) ?? undefined, ts: (row.recorded_at as string) ?? new Date(0).toISOString(), source: (row.source as string | undefined) ?? 'sys-metadata-repo', @@ -898,7 +908,8 @@ export class SysMetadataRepository implements MetadataRepository { ref: { org: '', type: 'view', name: '_close' } as MetaRef, hash: null, parentHash: null, - actor: 'system', + // #4556 — a synthetic drain event has no actor at all. + actor: null, ts: new Date().toISOString(), source: 'sys-metadata-repo-close', }); @@ -1009,7 +1020,9 @@ export class SysMetadataRepository implements MetadataRepository { body, hash, parentHash: null, - authoredBy: row.updated_by ?? row.created_by ?? 'unknown', + // #4556 — `updated_by` / `created_by` are lookup('sys_user') too; + // absent means absent, not a user called 'unknown'. + authoredBy: (row.updated_by as string | null | undefined) ?? (row.created_by as string | null | undefined) ?? null, authoredAt: row.updated_at ?? row.created_at ?? new Date().toISOString(), message: undefined, seq: this.seqCounter, diff --git a/packages/objectql/src/protocol-recorded-by-null.test.ts b/packages/objectql/src/protocol-recorded-by-null.test.ts new file mode 100644 index 0000000000..98699969a6 --- /dev/null +++ b/packages/objectql/src/protocol-recorded-by-null.test.ts @@ -0,0 +1,235 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4556 — an actor-less metadata write stores NULL in + * `sys_metadata_history.recorded_by`, against a REAL {@link ObjectQL} engine. + * + * The repository unit suite can only prove the repository forwards what it + * is given. The sentinel was born one layer up: the protocol filled + * `actor: request.actor ?? 'system'` on five write paths, so every metadata + * write with no caller actor — boot sync, migration, an unauthenticated + * internal call — put the STRING `'system'` into a column declared + * `Field.lookup('sys_user')`. This suite drives the protocol methods a real + * caller drives and reads the row that actually landed. + * + * `recorded_by` is declared here as the real thing — a `readonly` lookup to + * `sys_user` — rather than the `text` stub the older repo-path suite uses, + * because that is what makes the second half of the file meaningful: #4441 + * had to exempt `readonly` fields from the write-path referential-integrity + * check precisely because this column held a value no `sys_user` row + * matched. With the sentinel gone the ordinary authoring paths must still + * pass — that is the regression #4441 was bitten by. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ObjectQL } from './engine.js'; + +const sysUserObject = { + name: 'sys_user', + label: 'User', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; + +const sysMetadataObject = { + name: 'sys_metadata', + label: 'System Metadata', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, + name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, + organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + metadata: { name: 'metadata', label: 'Body', type: 'longtext' as const }, + checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, + state: { name: 'state', label: 'State', type: 'text' as const }, + version: { name: 'version', label: 'Version', type: 'number' as const }, + created_at: { name: 'created_at', label: 'Created', type: 'datetime' as const }, + updated_at: { name: 'updated_at', label: 'Updated', type: 'datetime' as const }, + }, +}; + +const sysMetadataHistoryObject = { + name: 'sys_metadata_history', + label: 'Metadata History', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + event_seq: { name: 'event_seq', label: 'Seq', type: 'number' as const, required: true }, + type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, + name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, + version: { name: 'version', label: 'Version', type: 'number' as const, required: true }, + operation_type: { name: 'operation_type', label: 'Op', type: 'text' as const, required: true }, + metadata: { name: 'metadata', label: 'Body', type: 'longtext' as const }, + checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, + previous_checksum: { name: 'previous_checksum', label: 'Prev Checksum', type: 'text' as const, maxLength: 71 }, + change_note: { name: 'change_note', label: 'Note', type: 'longtext' as const }, + source: { name: 'source', label: 'Source', type: 'text' as const }, + organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + // The real declaration, not a `text` stand-in — see the file header. + recorded_by: { + name: 'recorded_by', label: 'Recorded By', + type: 'lookup' as const, referenceTo: 'sys_user', readonly: true, + }, + recorded_at: { name: 'recorded_at', label: 'At', type: 'datetime' as const, required: true }, + }, +}; + +/** Minimal in-memory driver; equality-only WHERE. */ +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + if (Array.isArray(where.$and)) return where.$and.every((w: any) => matchesWhere(row, w)); + if (Array.isArray(where.$or)) return where.$or.some((w: any) => matchesWhere(row, w)); + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + const a = row[k] === undefined ? null : row[k]; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + + const driver: any = { + name: 'memory', + version: '0.0.0', + supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +const viewBody = (label: string) => ({ name: 'cases', type: 'grid', label, columns: ['id'] }); + +describe('#4556 — protocol write paths store NULL, not the sentinel string', () => { + let engine: ObjectQL; + let protocol: ObjectStackProtocolImplementation; + + const historyRows = async () => + (await engine.find('sys_metadata_history', { where: { organization_id: 'org_x' } })) as any[]; + + beforeEach(async () => { + engine = new ObjectQL(); + const { driver } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(sysUserObject as any); + engine.registry.registerObject(sysMetadataObject as any); + engine.registry.registerObject(sysMetadataHistoryObject as any); + await engine.insert('sys_user', { id: 'usr_alice', name: 'Alice' }, { context: { isSystem: true } } as any); + protocol = new ObjectStackProtocolImplementation(engine); + }); + + it('saveMetaItem with NO actor lands recorded_by = NULL', async () => { + await protocol.saveMetaItem({ + type: 'view', name: 'cases', organizationId: 'org_x', item: viewBody('A'), + }); + + const rows = await historyRows(); + expect(rows).toHaveLength(1); + // The whole issue in one assertion: the column is a lookup('sys_user'), + // so 'system' was a foreign key pointing at nothing. + expect(rows[0].recorded_by ?? null).toBeNull(); + expect(rows[0].recorded_by).not.toBe('system'); + }); + + it('saveMetaItem WITH an actor still stores that user id', async () => { + await protocol.saveMetaItem({ + type: 'view', name: 'cases', organizationId: 'org_x', item: viewBody('A'), actor: 'usr_alice', + }); + const rows = await historyRows(); + expect(rows[0].recorded_by).toBe('usr_alice'); + }); + + it('deleteMetaItem with NO actor writes a tombstone with recorded_by = NULL', async () => { + await protocol.saveMetaItem({ + type: 'view', name: 'cases', organizationId: 'org_x', item: viewBody('A'), actor: 'usr_alice', + }); + await protocol.deleteMetaItem({ type: 'view', name: 'cases', organizationId: 'org_x' }); + + const tombstone = (await historyRows()).find((h) => h.operation_type === 'delete'); + expect(tombstone).toBeDefined(); + expect(tombstone.recorded_by ?? null).toBeNull(); + expect(tombstone.recorded_by).not.toBe('system'); + }); + + it('publishMetaItem with NO actor records the publish event with recorded_by = NULL', async () => { + await protocol.saveMetaItem({ + type: 'view', name: 'cases', organizationId: 'org_x', item: viewBody('draft'), mode: 'draft', + }); + await protocol.publishMetaItem({ type: 'view', name: 'cases', organizationId: 'org_x' }); + + const publishRow = (await historyRows()).find((h) => h.operation_type === 'publish'); + expect(publishRow).toBeDefined(); + expect(publishRow.recorded_by ?? null).toBeNull(); + expect(publishRow.recorded_by).not.toBe('system'); + }); + + it('no history row on ANY path carries a value that is not a sys_user id', async () => { + // The three authoring paths #4441 was bitten by: create, publish, delete. + await protocol.saveMetaItem({ + type: 'view', name: 'a', organizationId: 'org_x', item: viewBody('a'), mode: 'draft', + }); + await protocol.publishMetaItem({ type: 'view', name: 'a', organizationId: 'org_x' }); + await protocol.saveMetaItem({ + type: 'view', name: 'b', organizationId: 'org_x', item: viewBody('b'), actor: 'usr_alice', + }); + await protocol.deleteMetaItem({ type: 'view', name: 'b', organizationId: 'org_x' }); + + const rows = await historyRows(); + expect(rows.length).toBeGreaterThan(0); + const users = new Set(['usr_alice']); + for (const row of rows) { + const v = row.recorded_by ?? null; + // Either NULL, or an id that a sys_user row actually has. Nothing else. + expect(v === null || users.has(v)).toBe(true); + } + }); +}); From f7943377e684d1a9d7f82dceab4adffd145abe6e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 01:36:42 +0000 Subject: [PATCH 2/2] chore(i18n): regenerate platform-objects bundles for the new recorded_by help text (#4556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `description` added to `sys_metadata_history.recorded_by` lands in the i18n schema as that field's `help` string, which made all four platform-objects locale bundles stale and turned `check-i18n-bundles` (in the required `TypeScript Type Check` job) red. Regenerated with `node scripts/check-i18n-bundles.mjs --write` — merge mode, so every existing translation is preserved. The diff is exactly one added `help` key per locale and nothing else; the non-English locales carry the source string, which is this tool's "awaiting translation" state, not a translation claim. Verified after: `node scripts/check-i18n-bundles.mjs` → OK (9 packages, all bundles in sync); `pnpm check:i18n-coverage` → OK (12 configs, none new). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- .../src/apps/translations/en.objects.generated.ts | 3 ++- .../src/apps/translations/es-ES.objects.generated.ts | 3 ++- .../src/apps/translations/ja-JP.objects.generated.ts | 3 ++- .../src/apps/translations/zh-CN.objects.generated.ts | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/platform-objects/src/apps/translations/en.objects.generated.ts b/packages/platform-objects/src/apps/translations/en.objects.generated.ts index 32e52f10bf..d3935d09c5 100644 --- a/packages/platform-objects/src/apps/translations/en.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/en.objects.generated.ts @@ -2746,7 +2746,8 @@ export const enObjects: NonNullable = { help: "Organization for multi-tenant isolation." }, recorded_by: { - label: "Recorded By" + label: "Recorded By", + help: "User who made this change. NULL = system-initiated (boot sync, migration, scheduled job) — never a sentinel string." }, recorded_at: { label: "Recorded At" diff --git a/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts b/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts index 6d200672ed..a4ed943ac4 100644 --- a/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts @@ -2746,7 +2746,8 @@ export const esESObjects: NonNullable = { help: "Organización para el aislamiento multi-tenant." }, recorded_by: { - label: "Registrado por" + label: "Registrado por", + help: "User who made this change. NULL = system-initiated (boot sync, migration, scheduled job) — never a sentinel string." }, recorded_at: { label: "Registrado el" diff --git a/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts b/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts index 76ab4b657b..2d0d293327 100644 --- a/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts @@ -2746,7 +2746,8 @@ export const jaJPObjects: NonNullable = { help: "マルチテナント分離のための組織。" }, recorded_by: { - label: "記録者" + label: "記録者", + help: "User who made this change. NULL = system-initiated (boot sync, migration, scheduled job) — never a sentinel string." }, recorded_at: { label: "記録日時" diff --git a/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts b/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts index 828e6fd71e..6246e12c78 100644 --- a/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts @@ -2746,7 +2746,8 @@ export const zhCNObjects: NonNullable = { help: "用于多租户隔离的组织。" }, recorded_by: { - label: "记录人" + label: "记录人", + help: "User who made this change. NULL = system-initiated (boot sync, migration, scheduled job) — never a sentinel string." }, recorded_at: { label: "记录时间"