Skip to content

Commit 4e36790

Browse files
committed
Merge remote-tracking branch 'origin/main' into claude/issue-4669-permission-backfill-strict-spec
2 parents 99db937 + 65f184b commit 4e36790

17 files changed

Lines changed: 1200 additions & 23 deletions
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
---
2+
"@objectstack/metadata-core": major
3+
"@objectstack/metadata-protocol": major
4+
"@objectstack/cli": minor
5+
---
6+
7+
fix(metadata)!: `sys_metadata_history.recorded_by` stores NULL, not the sentinel string `'system'` (#4556)
8+
9+
`recorded_by` is declared `Field.lookup('sys_user', { readonly: true })` — a
10+
foreign key. The write path filled it with `actor ?? 'system'`, so every
11+
metadata write without a caller actor (boot sync, migration, an internal call)
12+
stored the **string** `'system'` in a column whose declared type says "the id
13+
of a `sys_user` row". No such row exists, and `SystemUserId.SYSTEM`
14+
(`'usr_system'`) is not auto-provisioned on the current runtime either, so the
15+
value resolved to nothing under any reading. Any consumer that read the field
16+
by its declaration — `expand`, an owner column in a report, an audit timeline
17+
showing "who changed this" — got an id that could not be dereferenced.
18+
19+
It had already cost twice. #4441 had to exempt every `readonly` field from the
20+
write-path referential-integrity check, because otherwise ordinary metadata
21+
authoring (package create / publish / clone) was rejected. #4551's
22+
dangling-reference audit had to skip the same set for the same reason. The
23+
field ended up the platform's only reference column that is neither enforced
24+
nor audited.
25+
26+
**The fix is on the write path, not the declaration.** `recorded_by` stays a
27+
`lookup('sys_user')`; an actor-less write now stores `NULL`, and `NULL` means
28+
"system-initiated (boot sync, migration, scheduled job)" — the standard
29+
expression of "no link", and already what this column's `set_null` delete
30+
behaviour means. No magic system-user account (a row that can never sign in yet
31+
holds an identity is a new security surface), and no `actor_kind` companion
32+
column.
33+
34+
**Breaking — the repository contract is now explicitly nullable.**
35+
36+
| Surface | Before | After |
37+
|:--|:--|:--|
38+
| `PutOptions.actor`, `DeleteOptions.actor` | `string` | `string \| null` (still **required**) |
39+
| `MetadataEvent.actor` | `string` | `string \| null` |
40+
| `MetadataItem.authoredBy` | `string` | `string \| null` |
41+
42+
`actor` stays required rather than becoming optional on purpose: every call
43+
site must state which of the two it is, so a forgotten actor cannot silently
44+
become a fake foreign key. Migrating a caller:
45+
46+
- **Writers** — passing a real identity: unchanged. Passing `'system'`, `''`,
47+
or a label to satisfy the type: pass `null` instead.
48+
- **Readers**`event.actor` and `item.authoredBy` can be `null`. Handle it at
49+
the point of display (`actor ?? 'System'` in a UI string is fine — the fix is
50+
that the *stored* value no longer lies, not that no label may ever be shown).
51+
52+
Two read paths also stopped inventing a value: `SysMetadataRepository.history()`
53+
and `getByHash()` rendered an absent actor as the string `'unknown'`, which is
54+
indistinguishable from a real user id to anything that resolves the field. They
55+
now surface `null`.
56+
57+
**Existing rows: `os migrate recorded-by`.** The stored `'system'` values are
58+
rewritten to `NULL` by a new command, which runs the conversion through the
59+
ADR-0119 D2 migration journal (chunk-atomic, resumable via `os migrate resume`).
60+
It is a dry run by default and safe to re-run — it selects only rows still
61+
holding the sentinel, so a second `--apply` converts nothing.
62+
63+
The rewrite is **semantically equivalent, not a reinterpretation**: this column
64+
has only ever held that one sentinel, written by exactly one expression
65+
(`actor ?? 'system'`), and both spellings mean "no actor" — only `NULL` is
66+
expressible in the declared type.
67+
68+
Deliberately unchanged: `sys_metadata_audit.actor` is a `text` column whose
69+
declaration already says "user id, system id, or `'system'`", so its `'system'`
70+
default is honest and stays. The #4441 `readonly` narrowing and the #4551 audit
71+
skip also stay — see the PR for why they are still correct.
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `os migrate recorded-by` command shape (#4556).
5+
*
6+
* The conversion itself is proven against the real journal runner in
7+
* `@objectstack/metadata-protocol`'s `migrations/recorded-by-sentinel.test.ts`.
8+
* What is pinned here is the thing a unit test of the plan cannot see: that
9+
* this subcommand honours #2186 — a bare `os migrate <topic>` must never
10+
* mutate the database by surprise, so writing is opt-in behind `--apply`.
11+
*/
12+
13+
import { describe, it, expect } from 'vitest';
14+
import MigrateRecordedBy from './recorded-by.js';
15+
import {
16+
RECORDED_BY_SENTINEL,
17+
RECORDED_BY_SENTINEL_PLAN_ID,
18+
createRecordedBySentinelPlan,
19+
} from '@objectstack/metadata-protocol';
20+
21+
describe('os migrate recorded-by', () => {
22+
it('is a dry run by default — --apply is opt-in (#2186)', () => {
23+
expect(MigrateRecordedBy.flags.apply.default).toBe(false);
24+
});
25+
26+
it('requires explicit confirmation to write — --yes is opt-in', () => {
27+
expect(MigrateRecordedBy.flags.yes.default).toBe(false);
28+
});
29+
30+
it('describes what it converts, so `os migrate --help` is self-explanatory', () => {
31+
expect(MigrateRecordedBy.description).toContain('recorded_by');
32+
expect(MigrateRecordedBy.description).toContain('NULL');
33+
});
34+
35+
it('drives the plan the metadata package owns — no second copy of the conversion', () => {
36+
const plan = createRecordedBySentinelPlan();
37+
expect(plan.id).toBe(RECORDED_BY_SENTINEL_PLAN_ID);
38+
// A rediscovered run goes FORWARD: unwinding would put the fake foreign
39+
// key back, which is the thing this plan exists to remove.
40+
expect(plan.onCrash).toBe('resume');
41+
expect(RECORDED_BY_SENTINEL).toBe('system');
42+
});
43+
});
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { Command, Flags } from '@oclif/core';
4+
import chalk from 'chalk';
5+
import { createInterface } from 'node:readline';
6+
import {
7+
runMigrationJournal,
8+
MigrationJournalRefusal,
9+
type MigrationPlanProvider,
10+
} from '@objectstack/core';
11+
import {
12+
createRecordedBySentinelPlan,
13+
findSentinelHistoryRows,
14+
RECORDED_BY_SENTINEL,
15+
RECORDED_BY_SENTINEL_PLAN_ID,
16+
} from '@objectstack/metadata-protocol';
17+
import type { IObjectQLEngine } from '@objectstack/spec/contracts';
18+
import {
19+
printHeader,
20+
printSuccess,
21+
printWarning,
22+
printError,
23+
printInfo,
24+
printStep,
25+
createTimer,
26+
emitJson,
27+
} from '../../utils/format.js';
28+
import { bootSchemaStack } from '../../utils/schema-migrate.js';
29+
import { buildDataMigrationPlugins } from '../../utils/data-migration-plugins.js';
30+
31+
async function confirm(question: string): Promise<boolean> {
32+
if (!process.stdin.isTTY) return false; // non-interactive → require --yes
33+
const rl = createInterface({ input: process.stdin, output: process.stdout });
34+
try {
35+
const answer: string = await new Promise((resolve) => rl.question(question, resolve));
36+
return /^y(es)?$/i.test(answer.trim());
37+
} finally {
38+
rl.close();
39+
}
40+
}
41+
42+
/**
43+
* `os migrate recorded-by` — rewrite the `'system'` sentinel in
44+
* `sys_metadata_history.recorded_by` to `NULL` (#4556).
45+
*
46+
* `recorded_by` is a `lookup('sys_user')` that used to receive the STRING
47+
* `'system'` on every actor-less metadata write. That string is not any
48+
* user's id, so the column declared a foreign key and stored something no
49+
* join could resolve. The runtime no longer writes it (the write path stores
50+
* `NULL`); this command converts the rows already on disk.
51+
*
52+
* The conversion is semantically equivalent, not a reinterpretation: the
53+
* column has only ever held that one sentinel, written by one expression, and
54+
* both spellings mean "no actor" — only `NULL` is expressible in the declared
55+
* type.
56+
*
57+
* Dry run by default, like every other `os migrate` subcommand (#2186): a
58+
* bare invocation reports how many rows still carry the sentinel and writes
59+
* nothing. `--apply` runs the conversion through the ADR-0119 D2 migration
60+
* journal, so each chunk is one transaction and an interrupted run is
61+
* recoverable via `os migrate resume`.
62+
*
63+
* Safe to re-run: the plan selects only rows still holding the sentinel, so a
64+
* second `--apply` finds none and commits zero chunks.
65+
*/
66+
export default class MigrateRecordedBy extends Command {
67+
static override description =
68+
"Rewrite the legacy 'system' sentinel in sys_metadata_history.recorded_by to NULL (#4556). " +
69+
'Dry-run by default; --apply runs the conversion through the migration journal.';
70+
71+
static override examples = [
72+
'$ os migrate recorded-by',
73+
'$ os migrate recorded-by --json',
74+
'$ os migrate recorded-by --apply --yes',
75+
];
76+
77+
static override flags = {
78+
'database-url': Flags.string({
79+
description: 'Database URL to inspect (defaults to $OS_DATABASE_URL / the project DB)',
80+
env: 'OS_DATABASE_URL',
81+
}),
82+
apply: Flags.boolean({ description: 'Perform the conversion (default is a read-only report)', default: false }),
83+
yes: Flags.boolean({ char: 'y', description: 'Skip the confirmation prompt', default: false }),
84+
'chunk-size': Flags.integer({ description: 'Rows per journal chunk', default: 200 }),
85+
json: Flags.boolean({ description: 'Machine-readable output', default: false }),
86+
};
87+
88+
async run(): Promise<void> {
89+
const { flags } = await this.parse(MigrateRecordedBy);
90+
const timer = createTimer();
91+
92+
if (!flags.json) printHeader('Migrate · recorded-by sentinel → NULL');
93+
if (!flags.json) printStep(flags.apply ? 'Booting data stack…' : 'Booting data stack (read-only)…');
94+
95+
let stack;
96+
try {
97+
stack = await bootSchemaStack({
98+
databaseUrl: flags['database-url'],
99+
extraPlugins: await buildDataMigrationPlugins(),
100+
});
101+
} catch (error: any) {
102+
if (flags.json) { await emitJson({ error: error.message }, 0, { compact: true }); this.exit(1); }
103+
printError(error.message || String(error));
104+
this.exit(1);
105+
return;
106+
}
107+
108+
try {
109+
const engine: IObjectQLEngine = stack.kernel.getService('objectql');
110+
if (typeof engine?.find !== 'function') {
111+
throw new Error('No ObjectQL engine on this stack — cannot read sys_metadata_history.');
112+
}
113+
114+
const plan = createRecordedBySentinelPlan({ chunkSize: flags['chunk-size'] });
115+
116+
// Register the plan so an interrupted run is resumable in THIS process
117+
// too — `os migrate resume` looks plans up by id, and a run whose plan
118+
// nothing registers is reported unresumable.
119+
try {
120+
const plans = stack.kernel.getService('migration-plans') as MigrationPlanProvider;
121+
plans?.register?.(plan);
122+
} catch { /* no registry composed — resume reports it, this run still works */ }
123+
124+
const pending = await findSentinelHistoryRows(engine);
125+
126+
// ── dry run (default): read-only ─────────────────────────────────
127+
if (!flags.apply) {
128+
if (flags.json) {
129+
await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, sentinel: RECORDED_BY_SENTINEL, pending: pending.length, applied: false }, timer.elapsed());
130+
return;
131+
}
132+
if (pending.length === 0) {
133+
printSuccess(`No sys_metadata_history row holds the '${RECORDED_BY_SENTINEL}' sentinel — nothing to convert.`);
134+
return;
135+
}
136+
printWarning(`${pending.length} sys_metadata_history row(s) hold recorded_by = '${RECORDED_BY_SENTINEL}'.`);
137+
printInfo("Nothing was changed. Re-run with --apply to rewrite them to NULL.");
138+
return;
139+
}
140+
141+
// ── apply ────────────────────────────────────────────────────────
142+
if (pending.length === 0) {
143+
const msg = `No sys_metadata_history row holds the '${RECORDED_BY_SENTINEL}' sentinel — nothing to convert.`;
144+
if (flags.json) { await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, pending: 0, applied: true, status: 'completed', chunksCommitted: 0 }, timer.elapsed()); return; }
145+
printSuccess(msg);
146+
return;
147+
}
148+
149+
if (!flags.yes) {
150+
const summary = `Rewrite recorded_by '${RECORDED_BY_SENTINEL}' → NULL on ${pending.length} row(s)`;
151+
if (flags.json || !process.stdin.isTTY) {
152+
if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary }, timer.elapsed(), { compact: true }); this.exit(1); return; }
153+
printWarning(`Confirmation required: ${summary}. Re-run with --yes.`);
154+
this.exit(1);
155+
return;
156+
}
157+
const ok = await confirm(chalk.bold(`\n${summary}? [y/N] `));
158+
if (!ok) { printInfo('Aborted — nothing changed.'); return; }
159+
}
160+
161+
if (!flags.json) printStep('Converting…');
162+
const result = await runMigrationJournal(engine, plan);
163+
164+
if (flags.json) {
165+
await emitJson({ ...result, pending: pending.length, applied: true, error: result.error ? String(result.error) : undefined }, timer.elapsed());
166+
this.exit(result.status === 'completed' ? 0 : 1);
167+
return;
168+
}
169+
170+
if (result.status === 'completed') {
171+
printSuccess(
172+
`Converted ${pending.length} row(s) — run '${result.runId}', ${result.chunksCommitted}/${result.chunksTotal} chunk(s) committed.`,
173+
);
174+
} else if (result.status === 'compensated') {
175+
printWarning(
176+
`Run '${result.runId}' failed and was unwound — ${result.chunksCompensated} chunk(s) compensated. ` +
177+
`The sentinel rows are back as they were; nothing is half-converted.`,
178+
);
179+
this.exit(1);
180+
} else {
181+
printError(
182+
`Run '${result.runId}' FAILED and its compensation did not finish. ` +
183+
`Inspect sys_migration_journal for run '${result.runId}' — this needs a decision, not a retry.`,
184+
);
185+
this.exit(1);
186+
}
187+
} catch (error: any) {
188+
const msg = error instanceof MigrationJournalRefusal
189+
? `Refused (${error.code}): ${error.message}`
190+
: (error?.message || String(error));
191+
if (flags.json) { await emitJson({ error: msg }, timer.elapsed(), { compact: true }); this.exit(1); return; }
192+
printError(msg);
193+
this.exit(1);
194+
} finally {
195+
try { await stack.shutdown?.(); } catch { /* best effort */ }
196+
}
197+
}
198+
}

packages/cli/src/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ export { default as MigrateApplyCommand } from './commands/migrate/apply.js';
2424
// ADR-0119 D2 (#4617): act on a run the journal says was interrupted. Boot
2525
// discovers (MigrationRecoveryPlugin); this acts, under operator intent.
2626
export { default as MigrateResumeCommand } from './commands/migrate/resume.js';
27+
// #4556: rewrite the legacy `'system'` sentinel in
28+
// `sys_metadata_history.recorded_by` to NULL, through the same journal.
29+
export { default as MigrateRecordedByCommand } from './commands/migrate/recorded-by.js';
2730

2831
// ─── Environments topic subcommands ─────────────────────────────────
2932
export { default as EnvironmentsListCommand } from './commands/environments/list.js';

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,23 @@ export const SysMetadataHistoryObject = ObjectSchema.create({
152152
description: 'Organization for multi-tenant isolation.',
153153
}),
154154

155-
/** User who made this change (= MetadataEvent.actor). */
155+
/**
156+
* User who made this change (= MetadataEvent.actor).
157+
*
158+
* NULL when the write had no human actor — a system-initiated write
159+
* (boot metadata sync, a data migration, a scheduled job). #4556: this
160+
* column used to receive the sentinel STRING `'system'`, which is not
161+
* any `sys_user` id, so a lookup column declared as a foreign key held
162+
* a value no join could ever resolve. NULL is the standard expression
163+
* of "no link" and is what `deleteBehavior: 'set_null'` already means
164+
* here, so the declared type and the stored value now agree.
165+
*/
156166
recorded_by: Field.lookup('sys_user', {
157167
label: 'Recorded By',
158168
required: false,
159169
readonly: true,
170+
description:
171+
'User who made this change. NULL = system-initiated (boot sync, migration, scheduled job) — never a sentinel string.',
160172
}),
161173

162174
/** When was this version recorded */

0 commit comments

Comments
 (0)