Skip to content

Commit d18bedd

Browse files
os-justinclaude
andauthored
fix(cli): canonicalise os migrate duplicates holder createdAt to ISO-8601 UTC (#13999) (#14252)
`DuplicateHolder.createdAt` is declared `string | null`, but the mapper built it with `String(row.created_at)`. `created_at` is a builtin audit column, so no presentation runs on the holder probe's raw-SQL path: Postgres and MySQL materialise a JS `Date` and the operator read a `Date.toString()` rendering with their local zone baked in, whole seconds and no `Z`, while SQLite printed canonical ISO-Z. One instant, two spellings, chosen by the dialect. Canonicalised at the mapper, following the `occurredAt` form already in packages/metadata-protocol/src/protocol.ts. No driver file is touched and no tolerant fallback is added. Claude-Session: https://claude.ai/code/session_015YPiiDdw96RGS25WLctCQP Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent e764507 commit d18bedd

3 files changed

Lines changed: 377 additions & 2 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(cli): `os migrate duplicates` reports every holder's `createdAt` as canonical ISO-8601 UTC on every dialect (#13999)
6+
7+
`DuplicateHolder.createdAt` is declared `string | null`, and the holder mapper
8+
built it with `String(row.created_at)`. `created_at` is a **builtin audit
9+
column** — not in `datetimeFields`, and `SqlDriver#formatOutput` repairs it only
10+
inside its `if (this.isSqlite)` arm — and the holder probe reads through the
11+
raw-SQL seam, so no presentation runs on this path at all. The dialect therefore
12+
decided what the operator saw.
13+
14+
On **Postgres and MySQL**, `created_at` materialises as a JS `Date`, so
15+
`String()` ran `Date.prototype.toString`:
16+
17+
```
18+
Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)
19+
```
20+
21+
where the same command against **SQLite** printed `2026-08-30T10:19:25.947Z`.
22+
One instant, two spellings, chosen by the dialect: the operator's local zone
23+
baked in, whole seconds instead of milliseconds, no `Z`, and not
24+
`Date.parse`-safe for anything consuming this command's JSON.
25+
26+
## What changes for a consumer
27+
28+
`duplicates[].holders[].createdAt` in the `os migrate duplicates` JSON now
29+
carries canonical ISO-8601 UTC (`…Z`, milliseconds) on **every** dialect. On
30+
SQLite the value is byte-identical to what it was — that side was already
31+
canonical, which is why every existing pin on this command was green through the
32+
defect. On Postgres and MySQL the value changes from the `Date.toString()`
33+
rendering to the ISO spelling the field has always declared; a consumer that was
34+
parsing the old rendering was parsing a zone-dependent, millisecond-lossy string.
35+
36+
Unchanged on purpose: a holder whose object carries no `created_at` column still
37+
reports `createdAt: null` (the probe's `withCreatedAt: false` retry), and a
38+
`Date` carrying no time value keeps its verbatim rendering rather than throwing
39+
`RangeError` out of a read-only report.
40+
41+
## Where the repair lands, and where it deliberately does not
42+
43+
At the **mapper**. The CLI is a leaf consumer with a declared `string | null`, so
44+
it is the side that owes the canonical spelling; the form follows the
45+
`occurredAt` mapper already in `packages/metadata-protocol/src/protocol.ts`.
46+
47+
Not on the producer side: giving the driver one presented shape per dialect at
48+
the read door would repair this site for free, but it reverses a deliberate
49+
driver decision (`SqlDriver.withPostgresCalendarDayAsText`) and is a maintainer
50+
call on the #13973 census as a whole. Zero driver files are touched here. And not
51+
a `??` fallback — per #13973's standing prohibition the question is which side
52+
owes the canonical spelling, and a tolerant fallback answers it by hiding it.
Lines changed: 269 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,269 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#13999] `os migrate duplicates` reports ONE `createdAt` spelling, on every dialect.
5+
*
6+
* ## The defect, and why every existing pin was green through it
7+
*
8+
* `DuplicateHolder.createdAt` is declared `string | null`, and the mapper built
9+
* it with `String(row.created_at)`. `created_at` is a BUILTIN audit column — not
10+
* in `datetimeFields`, and `SqlDriver#formatOutput` repairs it only inside its
11+
* `if (this.isSqlite)` arm — and the holder probe reads through the raw-SQL seam,
12+
* so no presentation runs on this path at all. The dialect therefore decides what
13+
* arrives:
14+
*
15+
* - **Postgres / MySQL** materialise a JS `Date`, so `String()` ran
16+
* `Date.prototype.toString`: `Sun Aug 30 2026 18:19:25 GMT+0800 (China
17+
* Standard Time)` — the operator's local zone baked in, whole seconds instead
18+
* of milliseconds, no `Z`, and not `Date.parse`-safe for anything consuming
19+
* this command's JSON.
20+
* - **SQLite** and its siblings hand back canonical ISO-8601 UTC text, which
21+
* `String()` passes through untouched.
22+
*
23+
* Every pin this command already has drives SQLite (`duplicates.contract.test.ts`
24+
* asserts the holder document against a real better-sqlite3 fixture), which is
25+
* exactly the side that was already correct. That is why this file exists and why
26+
* its whole point is to DISTINGUISH the two dialects rather than re-assert one:
27+
* a test exercising only SQLite proves nothing about the defect.
28+
*
29+
* ## Which half rests on which evidence
30+
*
31+
* §A2 is a real dialect measurement — a live better-sqlite3 database, the real
32+
* probes, the real collector. §A1 is the other side, and no runner here hosts a
33+
* Postgres or a MySQL, so it drives the materialisation those dialects produce
34+
* through a hand-built seam double. That `Date` is not this file's claim to make:
35+
* it is the fact pinned, against live servers, in
36+
* `packages/drivers/driver-sql/src/sql-driver-13567-audit-stamp-materialisation.test.ts`
37+
* (read for this card, deliberately neither duplicated nor edited here — and the
38+
* layering runs the same way `@objectstack/metadata-protocol`'s own OCC suite is
39+
* argued: the consumer's seam is measured with the producer's measured value).
40+
*
41+
* §A3 is the assertion that actually states the contract — the two legs agree —
42+
* and §A4 keeps the whole file non-vacuous by measuring what the removed
43+
* expression really produced.
44+
*
45+
* ⛔ Not a `??` fallback and not a driver change: `withPostgresCalendarDayAsText`
46+
* is a deliberate driver decision and is untouched. The CLI is a leaf consumer
47+
* with a declared `string | null`, so the canonical spelling is owed here.
48+
*/
49+
50+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
51+
import { mkdtempSync, mkdirSync, rmSync } from 'node:fs';
52+
import { tmpdir } from 'node:os';
53+
import { join } from 'node:path';
54+
import { SqlDriver } from '@objectstack/driver-sql';
55+
import {
56+
normalizeRows,
57+
GLOBAL_TENANT,
58+
ORGANIZATION_FIELD,
59+
SEQUENCES_TABLE,
60+
type SeedTenancyExec,
61+
} from '@objectstack/metadata-protocol';
62+
import {
63+
canonicalHolderCreatedAt,
64+
collectDuplicateIdentifierReport,
65+
type DuplicateHolder,
66+
} from './duplicates.js';
67+
68+
/** The instant from the #13567 production report, kept verbatim. */
69+
const GLOBAL_INSTANT = '2026-08-30T10:19:25.947Z';
70+
/** A second instant, so the mapper is measured per row rather than against a constant. */
71+
const ORG_INSTANT = '2026-02-01T00:00:00.001Z';
72+
73+
/**
74+
* The zone the incident was observed in, FORCED rather than required.
75+
*
76+
* Test Core runs at UTC and `Temporal Conformance` at `America/New_York`, so the
77+
* operator-facing symptom in §A4 would otherwise be spelled differently on every
78+
* runner. Forcing it also makes §A1 mean what it says: the canonical spelling it
79+
* asserts is produced while the process is demonstrably NOT at UTC.
80+
*/
81+
const INCIDENT_ZONE = 'Asia/Shanghai';
82+
83+
/**
84+
* Run `body` with the process pinned to `tz`, then restore.
85+
*
86+
* Restoring rather than assuming matters because vitest reuses a worker across
87+
* files — a leaked `TZ` would silently re-zone whatever runs next in this
88+
* process. (A sibling copy lives in the driver-sql pin above; a zone-scoping
89+
* utility is not a guard that could weaken in one copy and nowhere else, so the
90+
* two are deliberately independent rather than shared across a package boundary.)
91+
*/
92+
async function underProcessZone<T>(tz: string, body: () => Promise<T> | T): Promise<T> {
93+
const previous = process.env.TZ;
94+
process.env.TZ = tz;
95+
try {
96+
return await body();
97+
} finally {
98+
if (previous === undefined) delete process.env.TZ;
99+
else process.env.TZ = previous;
100+
}
101+
}
102+
103+
/** The registry view the booted stack hands the command. */
104+
const CASE_ONLY = [{ name: 'crm_case', fields: { case_number: { type: 'autonumber' } } }];
105+
const CASE_AND_TICKET = [
106+
...CASE_ONLY,
107+
// No `created_at`: an object that opted out of system fields still has to
108+
// produce holders, with a null timestamp rather than a failed probe.
109+
{ name: 'crm_ticket', fields: { ticket_number: { type: 'autonumber' } } },
110+
];
111+
112+
const collect = (exec: SeedTenancyExec, objects: unknown[]) =>
113+
collectDuplicateIdentifierReport({
114+
exec,
115+
normalize: normalizeRows,
116+
objects,
117+
database: 'fixture',
118+
globalTenant: GLOBAL_TENANT,
119+
organizationField: ORGANIZATION_FIELD,
120+
sequencesTable: SEQUENCES_TABLE,
121+
client: 'better-sqlite3',
122+
now: () => new Date(GLOBAL_INSTANT),
123+
// This file measures the holder mapper and nothing else; the pre-flight
124+
// section has its own pins over its own fixtures in the contract test.
125+
runtimeIndexPreflight: [],
126+
});
127+
128+
const holdersOf = (duplicates: Array<{ holders: DuplicateHolder[] }>): DuplicateHolder[] =>
129+
duplicates.flatMap((d) => d.holders);
130+
131+
// ── §A1's seam: the value the live dialects put on the wire ─────────────────
132+
133+
/**
134+
* A raw-SQL seam that answers the holder probe with `created_at` materialised the
135+
* way Postgres and MySQL materialise it — a JS `Date`.
136+
*
137+
* Dispatches on the probes' own aliases: `AS holder_id` is unique to the holder
138+
* statement, `AS dup_value` is then the duplicate statement, and the counter
139+
* table simply does not exist in this fixture (a `__global__` counter beside an
140+
* organization-scoped one is #8928's live CONDITION, a different section of the
141+
* report and not this card's).
142+
*/
143+
function liveDialectSeam(globalStamp: unknown, orgStamp: unknown): SeedTenancyExec {
144+
return async (sql: string) => {
145+
if (sql.includes('AS holder_id')) {
146+
return [
147+
{ holder_id: 's1', dup_value: 'CASE-00001', organization: null, created_at: globalStamp },
148+
{ holder_id: 'a1', dup_value: 'CASE-00001', organization: 'org_x', created_at: orgStamp },
149+
];
150+
}
151+
if (sql.includes('AS dup_value')) {
152+
return [{ dup_value: 'CASE-00001', holder_count: 2, partition_count: 2 }];
153+
}
154+
throw new Error(`no such table: ${SEQUENCES_TABLE}`);
155+
};
156+
}
157+
158+
// ── §A2's fixture: a real SQLite database, the real probes ──────────────────
159+
160+
let dir: string;
161+
let driver: SqlDriver;
162+
let sqliteExec: SeedTenancyExec;
163+
164+
beforeAll(async () => {
165+
dir = mkdtempSync(join(tmpdir(), 'os-13999-'));
166+
mkdirSync(join(dir, 'data'), { recursive: true });
167+
driver = new SqlDriver({
168+
client: 'better-sqlite3',
169+
connection: { filename: join(dir, 'data', 'app.db') },
170+
useNullAsDefault: true,
171+
});
172+
// The same wrapper `resolveSeedTenancyExec` builds around a driver exposing
173+
// `execute(sql, params)`.
174+
sqliteExec = (sql: string, params?: unknown[]) => driver.execute(sql, (params ?? []) as any[]);
175+
const k = (driver as any).knex;
176+
177+
await k.schema.createTable('crm_case', (t: any) => {
178+
t.string('id').primary();
179+
t.timestamp('created_at');
180+
t.string('organization_id');
181+
t.string('case_number');
182+
});
183+
await k('crm_case').insert([
184+
{ id: 's1', created_at: GLOBAL_INSTANT, organization_id: null, case_number: 'CASE-00001' },
185+
{ id: 'a1', created_at: ORG_INSTANT, organization_id: 'org_x', case_number: 'CASE-00001' },
186+
]);
187+
188+
await k.schema.createTable('crm_ticket', (t: any) => {
189+
t.string('id').primary();
190+
t.string('organization_id');
191+
t.string('ticket_number');
192+
});
193+
await k('crm_ticket').insert([
194+
{ id: 't1', organization_id: null, ticket_number: 'TKT-1' },
195+
{ id: 't2', organization_id: 'org_x', ticket_number: 'TKT-1' },
196+
]);
197+
});
198+
199+
afterAll(async () => {
200+
try { await driver.disconnect(); } catch { /* already down */ }
201+
try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ }
202+
});
203+
204+
describe('#13999 §A — one instant, two dialect materialisations, one reported spelling', () => {
205+
it('§A1 Postgres/MySQL hand a JS `Date`; the report carries canonical ISO-Z', async () => {
206+
const produced = await underProcessZone(INCIDENT_ZONE, () =>
207+
collect(liveDialectSeam(new Date(GLOBAL_INSTANT), new Date(ORG_INSTANT)), CASE_ONLY),
208+
);
209+
expect(holdersOf(produced.duplicates)).toEqual([
210+
{ id: 's1', organization: null, partition: GLOBAL_TENANT, createdAt: GLOBAL_INSTANT },
211+
{ id: 'a1', organization: 'org_x', partition: 'org_x', createdAt: ORG_INSTANT },
212+
]);
213+
});
214+
215+
it('§A2 SQLite hands canonical ISO-Z text; a real database, unchanged through the mapper', async () => {
216+
const produced = await underProcessZone(INCIDENT_ZONE, () => collect(sqliteExec, CASE_ONLY));
217+
expect(holdersOf(produced.duplicates)).toEqual([
218+
{ id: 's1', organization: null, partition: GLOBAL_TENANT, createdAt: GLOBAL_INSTANT },
219+
{ id: 'a1', organization: 'org_x', partition: 'org_x', createdAt: ORG_INSTANT },
220+
]);
221+
});
222+
223+
it('§A3 the two dialects agree — the operator reads one document, not two', async () => {
224+
const live = await collect(liveDialectSeam(new Date(GLOBAL_INSTANT), new Date(ORG_INSTANT)), CASE_ONLY);
225+
const sqlite = await collect(sqliteExec, CASE_ONLY);
226+
expect(holdersOf(live.duplicates)).toEqual(holdersOf(sqlite.duplicates));
227+
// And what they agree ON is machine-readable, which is the point of the
228+
// command's JSON: every spelling re-parses to the instant it came from.
229+
for (const holder of holdersOf(live.duplicates)) {
230+
expect(new Date(holder.createdAt as string).toISOString()).toBe(holder.createdAt);
231+
}
232+
});
233+
234+
it('§A4 non-vacuity: `String(row.created_at)` really did produce a different document', async () => {
235+
// What the removed expression shipped on the production default driver.
236+
const spelled = await underProcessZone(INCIDENT_ZONE, () => String(new Date(GLOBAL_INSTANT)));
237+
expect(spelled.startsWith('Sun Aug 30 2026 18:19:25 GMT+0800')).toBe(true);
238+
expect(spelled).not.toBe(GLOBAL_INSTANT);
239+
// Whole seconds: the milliseconds are not merely re-spelled, they are gone,
240+
// so this was lossy and not only unsightly.
241+
expect(new Date(spelled).toISOString()).toBe('2026-08-30T10:19:25.000Z');
242+
// And the SQLite side of the same run was already canonical — which is how
243+
// the split survived: one dialect's output was never wrong.
244+
expect(String(GLOBAL_INSTANT)).toBe(GLOBAL_INSTANT);
245+
});
246+
});
247+
248+
describe('#13999 §B — the arms that were not broken', () => {
249+
it('§B1 an object with no `created_at` column still reports holders, with `createdAt: null`', async () => {
250+
const produced = await collect(sqliteExec, CASE_AND_TICKET);
251+
const tickets = produced.duplicates.filter((d) => d.object === 'crm_ticket');
252+
expect(tickets).toHaveLength(1);
253+
expect(tickets[0].holders.map((h) => h.createdAt)).toEqual([null, null]);
254+
// Through the real retry, not a shortcut: the `withCreatedAt: true` probe
255+
// fails on this table and the collector re-asks without the column.
256+
expect(produced.skipped.some((s) => s.object === 'crm_ticket')).toBe(false);
257+
});
258+
259+
it('§B2 a `Date` carrying no time value keeps its verbatim rendering instead of throwing', () => {
260+
// `mysql2` hands one back for a zero date, and `toISOString()` throws on it.
261+
// A non-instant has no canonical spelling; a spelling defect in a report must
262+
// not become a crashed migration command.
263+
const invalid = new Date(Number.NaN);
264+
expect(() => invalid.toISOString()).toThrow(RangeError);
265+
expect(canonicalHolderCreatedAt(invalid)).toBe(String(invalid));
266+
expect(canonicalHolderCreatedAt(null)).toBeNull();
267+
expect(canonicalHolderCreatedAt(undefined)).toBeNull();
268+
});
269+
});

0 commit comments

Comments
 (0)