From ca13d75cc139bbe0bb2829609f9d2fdb4e7b2576 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 00:30:06 +0000 Subject: [PATCH] fix(service-storage): accept the driver's `Date` for `created_at` in the stranded-orphan inventory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `inventoryStrandedFileOrphans` projects `created_at` out of the `sys_file` read door and then tested it with `typeof row.created_at === 'string'`. `created_at` is a BUILTIN audit column, so no declared-field coercion reaches it and `SqlDriver#formatOutput` repairs the audit columns only inside its `if (this.isSqlite)` arm: that door hands the value back as canonical ISO-Z text on SQLite and as a JS `Date` on Postgres and MySQL, the production default drivers. The guard was therefore false for every row on both live dialects and the projected field was silently discarded from every sample. Accept both shapes at the consumer, in the form `@objectstack/metadata-protocol` already carries for `occurred_at`. Normalising at the driver's read door instead would reverse the deliberate `withPostgresCalendarDayAsText` decision that a `timestamptz` is an instant, so the consumer owes the canonical spelling. The terminal arm stays `undefined` rather than `String(value)`: this field is optional where `occurredAt` is not, and stringifying a null or an Invalid Date would spell `"undefined"` / `"Invalid Date"` into the position an operator reads a timestamp from. The sibling `key` / `name` guards are untouched — those are text columns on every dialect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- ...nded-orphan-inventory-createdat-dialect.md | 32 +++++ .../src/stranded-orphan-inventory.test.ts | 135 ++++++++++++++++++ .../src/stranded-orphan-inventory.ts | 53 ++++++- 3 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 .changeset/stranded-orphan-inventory-createdat-dialect.md diff --git a/.changeset/stranded-orphan-inventory-createdat-dialect.md b/.changeset/stranded-orphan-inventory-createdat-dialect.md new file mode 100644 index 0000000000..6a88d9968a --- /dev/null +++ b/.changeset/stranded-orphan-inventory-createdat-dialect.md @@ -0,0 +1,32 @@ +--- +"@objectstack/service-storage": patch +--- + +fix(service-storage): report `createdAt` on every stranded-orphan sample, not only on SQLite (#13996) + +`inventoryStrandedFileOrphans` projects `created_at` out of the `sys_file` read +door and then tested it with `typeof row.created_at === 'string'`. `created_at` +is a BUILTIN audit column — it is not in `datetimeFields`, and +`SqlDriver#formatOutput` repairs the audit columns only inside its +`if (this.isSqlite)` arm — so that door hands the value back as canonical ISO-Z +text on SQLite and as a JS `Date` on Postgres and MySQL, the production default +drivers (pinned per dialect in driver-sql's +`sql-driver-13567-audit-stamp-materialisation.test.ts`). + +The guard was therefore `false` for **every** row on both live dialects: a field +explicitly asked for from the driver was silently discarded, and every sample in +an operator's stranded-orphan report carried `createdAt: undefined` there while +looking correct on the SQLite the suite runs on. + +The consumer now accepts both shapes and reports the canonical ISO-Z spelling — +the repair `@objectstack/metadata-protocol` already carries for `occurred_at`. +Normalising at the driver's read door instead would reverse the deliberate +`withPostgresCalendarDayAsText` decision that a `timestamptz` is an instant, so +the consumer owes the spelling. + +No exported shape changes: `StrandedOrphanSample.createdAt` stays +`string | undefined`. An ISO string is still passed through byte-for-byte, and +an absent, null or unparseable stamp still reports `undefined` — never the +literal `"Invalid Date"` or `"undefined"` in the position an operator reads a +timestamp from. The sibling `key` / `name` guards are untouched: those are text +columns on every dialect, and only the timestamp straddles the divergence. diff --git a/packages/services/service-storage/src/stranded-orphan-inventory.test.ts b/packages/services/service-storage/src/stranded-orphan-inventory.test.ts index fbd1a70401..95d9539972 100644 --- a/packages/services/service-storage/src/stranded-orphan-inventory.test.ts +++ b/packages/services/service-storage/src/stranded-orphan-inventory.test.ts @@ -505,3 +505,138 @@ describe("[#10950] the sweep cannot nominate a stranded orphan — the card's pr expect(report.stranded).toBe(1); }); }); +// ── [#13996] `createdAt` across the dialect divergence ────────────────────── + +/** + * What `samples[].createdAt` is, per runtime shape of `created_at`. + * + * ## Why this file could not have caught the defect before + * + * `created_at` is a BUILTIN audit column, so no declared-field coercion + * reaches it and `SqlDriver#formatOutput` repairs it only inside its + * `if (this.isSqlite)` arm. The record read door therefore hands it back as + * canonical ISO-Z TEXT on SQLite and as a JS `Date` on Postgres and MySQL — + * pinned at that door, per dialect, in driver-sql's + * `sql-driver-13567-audit-stamp-materialisation.test.ts` (§B0/§B1). + * + * ⚠️ This repo's default test backend is SQLite, and every fixture above + * spells `created_at` as an ISO STRING — the one shape the old + * `typeof row.created_at === 'string'` guard accepted. So the guard dropped + * the field for every row on both production default drivers while every pin + * here stayed green. The discriminating input is a `Date`, and until this + * block nothing in this file produced one. + * + * ## What each case is worth as evidence + * + * - POSITIVE is the only case that changes verdict with the repair: red + * before it (`undefined`), green after. + * - CONTROL is a declared REGRESSION CONTROL and ⛔ NOT ablation evidence: + * it is green in both directions by construction, because the SQLite shape + * already worked. It exists to pin that the repair added an accepted shape + * and changed nothing about the one that already round-tripped. + * - REVERSE CONTROL guards the `String(…)` trap: the arm that makes the + * `occurred_at` shape work for a non-optional field would spell the literal + * `"undefined"` / `"Invalid Date"` into an operator's report here. + */ +describe('[#13996] `samples[].createdAt` — the driver-dependent runtime type of `created_at`', () => { + /** Canonical audit-timestamp text: what SQLite stores and `toISOString()` emits. */ + const ISO_Z = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/; + /** Sub-second digits are load-bearing — `String(Date)` drops exactly these. */ + const INSTANT = '2026-08-30T10:19:25.947Z'; + + it('POSITIVE — a JS `Date` (the Postgres/MySQL shape) is reported as canonical ISO-Z text', async () => { + const engine = inventoryEngine({ + files: [strandedRow('os13996_pg', { created_at: new Date(INSTANT) })], + }); + + const report = await inventoryStrandedFileOrphans(engine); + + expect(report.stranded).toBe(1); + expect(report.samples).toHaveLength(1); + const sample = report.samples[0]; + // The whole defect, in one assertion: this was `undefined` for EVERY row + // on both live dialects, for a field the walk explicitly projects. + expect( + sample.createdAt, + 'the driver handed `created_at` out as a Date and the sample dropped it', + ).toBe(INSTANT); + expect(typeof sample.createdAt).toBe('string'); + expect(sample.createdAt).toMatch(ISO_Z); + }); + + it('POSITIVE — it is the `toISOString()` spelling, not `String(Date)`: the milliseconds survive', async () => { + const value = new Date(INSTANT); + expect(value.getMilliseconds(), 'the fixture is vacuous without sub-second digits').toBe(947); + + const report = await inventoryStrandedFileOrphans( + inventoryEngine({ files: [strandedRow('os13996_ms', { created_at: value })] }), + ); + const spelled = report.samples[0].createdAt as string; + + // `String(Date)` renders whole seconds in the PROCESS zone (§A2 of the + // driver pin). Naming the instant exactly is what separates the two. + expect(Date.parse(spelled), 'the reported stamp names the row instant').toBe(value.getTime()); + expect(spelled).not.toBe(String(value)); + expect(Date.parse(String(value))).toBe(value.getTime() - value.getMilliseconds()); + }); + + it('CONTROL (⛔ not ablation evidence — green both directions) — an ISO string is passed through byte-for-byte', async () => { + // The SQLite shape, which already worked. Asserted as the WHOLE sample so + // a change to any neighbouring field would show up here too. + const report = await inventoryStrandedFileOrphans( + inventoryEngine({ files: [strandedRow('os13996_sqlite')] }), + ); + + expect(report.samples).toEqual([ + { + fileId: 'os13996_sqlite', + key: 'attachments/os13996_sqlite.bin', + name: 'os13996_sqlite.bin', + size: 1024, + createdAt: '2026-01-01T00:00:00.000Z', + }, + ]); + }); + + it('CONTROL — passthrough is TOTAL over strings: a non-canonical stamp is not re-parsed or re-spelled', async () => { + // Today any string reaches the report unchanged, including a naive-UTC + // spelling. The repair adds an accepted shape; it must not quietly start + // validating or normalising the shape that already round-tripped. + for (const text of ['2026-01-01 00:00:00', 'not-a-timestamp', '']) { + const report = await inventoryStrandedFileOrphans( + inventoryEngine({ files: [strandedRow('os13996_text', { created_at: text })] }), + ); + expect(report.samples[0].createdAt, `passthrough of ${JSON.stringify(text)}`).toBe(text); + } + }); + + it('REVERSE CONTROL — an absent, null or unusable stamp stays `undefined`, never a spelled-out one', async () => { + const absent = strandedRow('os13996_absent'); + delete (absent as Record).created_at; + + const cases: Array<[string, Record]> = [ + ['absent', absent], + ['null', strandedRow('os13996_null', { created_at: null })], + // `instanceof Date` is TRUE for this one, and `toISOString()` THROWS on + // it — the case that would take the whole inventory down. + ['Invalid Date', strandedRow('os13996_nat', { created_at: new Date('not a date') })], + // Epoch millis: a shape no dialect produces here, kept `undefined` + // rather than stringified into a timestamp position. + ['epoch millis', strandedRow('os13996_num', { created_at: Date.parse(INSTANT) })], + ]; + + for (const [label, row] of cases) { + const report = await inventoryStrandedFileOrphans(inventoryEngine({ files: [row] })); + + expect(report.stranded, `${label}: the row is still inventoried`).toBe(1); + const sample = report.samples[0]; + expect(sample.createdAt, `${label}: an absent stamp must stay absent`).toBeUndefined(); + // Spelled out explicitly: these two strings are what a bare `String(…)` + // terminal arm would put where an operator reads a timestamp. + expect(sample.createdAt).not.toBe('Invalid Date'); + expect(sample.createdAt).not.toBe('undefined'); + // …and dropping the stamp must not drop the row's identity with it. + expect(sample.fileId, `${label}: fileId`).toBe(row.id); + } + }); +}); diff --git a/packages/services/service-storage/src/stranded-orphan-inventory.ts b/packages/services/service-storage/src/stranded-orphan-inventory.ts index 791b997152..e9837b32ba 100644 --- a/packages/services/service-storage/src/stranded-orphan-inventory.ts +++ b/packages/services/service-storage/src/stranded-orphan-inventory.ts @@ -168,6 +168,57 @@ function usableSize(value: unknown): number | undefined { return n; } +/** + * `created_at` as canonical ISO-8601-Z text, whichever shape the driver handed + * it out AS — and `undefined` when the row carries no usable stamp. + * + * ## Why a `typeof v === 'string'` test alone is wrong here + * + * `created_at` is a BUILTIN audit column: it is not in `datetimeFields`, so no + * declared-field coercion reaches it, and `SqlDriver#formatOutput` repairs the + * audit columns only inside its `if (this.isSqlite)` arm. So the read door the + * walk below goes through hands this value back as canonical ISO-Z TEXT on + * SQLite and as a JS `Date` on Postgres and MySQL — the production default + * drivers. Pinned per dialect, at that door, in driver-sql's + * `sql-driver-13567-audit-stamp-materialisation.test.ts` (§B1). + * + * A bare string test therefore answers FALSE for EVERY row on the live + * dialects: a field the walk explicitly projects (`fields: [… 'created_at']`) + * was asked for from the driver and then silently discarded, so every sample + * in the operator's report carried `createdAt: undefined` there while looking + * correct on the SQLite the tests run. The sibling guards on `key` and `name` + * are NOT this — those are text columns on every dialect. Only the timestamp + * straddles the divergence, which is why reading the code did not show it. + * + * ## Why the consumer owes the canonical spelling + * + * Normalising at the driver's read door instead would reverse the deliberate + * `withPostgresCalendarDayAsText` decision — that a `timestamptz` IS an + * instant and a `Date` is the right materialisation for it. So the repair is + * the one `@objectstack/metadata-protocol` already carries for `occurred_at`: + * accept both shapes where the value is consumed. + * + * ⛔ NOT `row.created_at ?? undefined`. That reads as fixed and is worse: it + * puts a raw `Date` into a field declared `string | undefined`, trading a + * dropped field for a wrong type. + * + * ⛔ And the terminal arm is `undefined`, not `String(value)`. This field is + * optional where `occurredAt` is not, and `String()` over a null or an Invalid + * Date spells the literal `"undefined"` / `"Invalid Date"` into an operator's + * report in the position a timestamp is read from — a stamp that is absent + * must stay absent. + */ +function usableCreatedAt(value: unknown): string | undefined { + if (typeof value === 'string') return value; + if (value instanceof Date) { + // An Invalid Date IS `instanceof Date`, and `toISOString()` THROWS on it + // (RangeError) rather than returning something odd. One unparseable stamp + // must not take down a read-only inventory of the whole `sys_file` table. + return Number.isNaN(value.getTime()) ? undefined : value.toISOString(); + } + return undefined; +} + /** * Count the `sys_file` rows that the forward-only fixes (#10171, #10240) would * have tombstoned had they existed when the rows were orphaned, and that the @@ -264,7 +315,7 @@ export async function inventoryStrandedFileOrphans( key: typeof row.key === 'string' ? row.key : undefined, name: typeof row.name === 'string' ? row.name : undefined, size, - createdAt: typeof row.created_at === 'string' ? row.created_at : undefined, + createdAt: usableCreatedAt(row.created_at), }); } }