diff --git a/.changeset/commit-timeline-instant-order.md b/.changeset/commit-timeline-instant-order.md new file mode 100644 index 0000000000..f694258e6e --- /dev/null +++ b/.changeset/commit-timeline-instant-order.md @@ -0,0 +1,38 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): order the ADR-0067 commit timeline by INSTANT, so `rollbackToPackageCommit` stops planning off the weekday name (#13995) + +`created_at` is an engine-injected audit column: it is not in `datetimeFields`, +and `SqlDriver#formatOutput` repairs it only inside `if (this.isSqlite)`. So the +live SQL dialects hand it out of the record read door as a JS `Date` while the +SQLite family hands out canonical ISO-Z text. Both ADR-0067 commit-timeline +consumers compared `String(created_at)` — and `String(aDate)` is +`"Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)"`, whose LEADING token +is the weekday NAME. Lexicographic order over those strings is +`Fri < Mon < Sat < Sun < Thu < Tue < Wed`: unrelated to chronology, stable +across the whole set, and therefore wrong on every run and wrong the same way — +there was never an "it worked once" to warn anyone. + +- `listCommits` returned the package timeline in weekday-name order while + claiming newest-first. Its own comment stated the assumption in as many words + ("sort by the ISO timestamp") and it was false on the production default + driver. +- `rollbackToPackageCommit` both CONSUMED that ordering and re-derived the same + comparison itself, so neither site could correct the other. On Postgres and + MySQL it reverted `apply` commits OLDER than the target and skipped the newer + ones it exists to undo — a destructive operation planning off a wrong + predicate. + +Both sites now compare canonical absolute instants, through a sibling of the +`canonicalVersionInstant` helper #13382 landed one seam over in this same file +for the OCC `updated_at` comparison. The canonicalisation is reused; the +ordering is new, because `versionTokensAgree` answers equality between two +client-supplied version tokens and an ordering question needs `<`/`>`. When +either side does not denote an instant the two are compared verbatim exactly as +before, so the only verdicts that change are the pairs that denote one. + +On SQLite and the memory driver both sides were already canonical ISO-Z text and +lexicographic order equalled chronological order, so nothing changes there — +which is why every test these sites had stayed green through the defect. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index a514a33271..f6d1f58cdf 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -112,7 +112,7 @@ that silently does not happen. | 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10787` | | 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10949` | | 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9680` | -| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1664` | +| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1736` | | 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9717`, `readonly-strict-errors.ts:66` | | 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5705` | | 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3574`, `:3584`, `:3611` | diff --git a/packages/metadata-protocol/src/protocol.commit-timeline-instant-order.test.ts b/packages/metadata-protocol/src/protocol.commit-timeline-instant-order.test.ts new file mode 100644 index 0000000000..736716d74a --- /dev/null +++ b/packages/metadata-protocol/src/protocol.commit-timeline-instant-order.test.ts @@ -0,0 +1,243 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#13995] The ADR-0067 package commit timeline sorted by WEEKDAY NAME on the +// production default drivers, and `rollbackToPackageCommit` planned its reverts +// off the same comparison. +// +// --------------------------------------------------------------------------- +// The defect +// --------------------------------------------------------------------------- +// `created_at` is an engine-injected audit column: it is not in `datetimeFields` +// and `SqlDriver#formatOutput` repairs it only inside `if (this.isSqlite)`, so +// the live SQL dialects hand it out of the record read door as a JS `Date` while +// the SQLite family hands out canonical ISO-Z text. Pinned one layer down by +// `packages/drivers/driver-sql/src/sql-driver-13567-audit-stamp-materialisation.test.ts`. +// +// Both timeline consumers in `protocol.ts` compared `String(created_at)`: +// +// listCommits mapped.sort(… String(b.createdAt).localeCompare(…)) +// rollbackToPackageCommit all.filter(c => String(c.createdAt) > targetCreatedAt) +// +// `String(aDate)` is `"Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)"` +// — the LEADING token is the weekday name — so lexicographic order over those +// strings is `Fri < Mon < Sat < Sun < Thu < Tue < Wed`, unrelated to chronology. +// The order is stable across the whole set, so the failure is systematic and +// identical on every run: there is never an "it worked once" to warn anyone. +// +// The two sites reinforce rather than backstop each other. `listCommits` returns +// a mis-ordered timeline while claiming newest-first, and +// `rollbackToPackageCommit` both CONSUMES that ordering and re-derives the same +// comparison itself — so it reverts `apply` commits OLDER than the target and +// skips the newer ones it exists to undo. That is a destructive operation +// planning off a wrong predicate. +// +// --------------------------------------------------------------------------- +// Why the fixture is FOUR consecutive days, and why that makes this pin +// timezone-independent +// --------------------------------------------------------------------------- +// `String(aDate)` renders the weekday in the PROCESS timezone, so which weekday +// name each instant carries depends on `TZ`. Rather than pin `TZ` (which Node +// caches per platform) the fixture is chosen so the old comparison is wrong in +// EVERY alignment: map each weekday to its rank in the lexicographic order +// (`Fri`=0, `Mon`=1, `Sat`=2, `Sun`=3, `Thu`=4, `Tue`=5, `Wed`=6) and read off +// the seven windows of four consecutive weekdays — +// +// Sun Mon Tue Wed -> 3 1 5 6 Thu Fri Sat Sun -> 4 0 2 3 +// Mon Tue Wed Thu -> 1 5 6 4 Fri Sat Sun Mon -> 0 2 3 1 +// Tue Wed Thu Fri -> 5 6 4 0 Sat Sun Mon Tue -> 2 3 1 5 +// Wed Thu Fri Sat -> 6 4 0 2 +// +// — not one of the seven is monotonic, so no timezone can make the old sort +// agree with chronology. (Three consecutive days is NOT enough: `Mon Tue Wed` +// and `Fri Sat Sun` are both increasing.) The same table settles the planner: +// with the target at the second day, the old predicate selects `{1,3,4}`, +// `{3}`, `{}`, `{1}`, `{1,3,4}`, `{3}`, `{4}` across the seven alignments and +// never the correct `{3,4}`. Every instant is 24h apart at 12:00Z, far enough +// from local midnight that no offset or DST step can collapse two onto one +// local day. `assertTheFixtureDiscriminates` below asserts the property +// mechanically rather than trusting this comment. +// +// --------------------------------------------------------------------------- +// Why the `Date` is hand-made rather than read off a driver +// --------------------------------------------------------------------------- +// `@objectstack/metadata-protocol` has no driver dependency and must not grow +// one — the layering runs the other way. This is the same split +// `sql-driver-13567-audit-stamp-materialisation.test.ts` documents for the OCC +// seam: the driver package pins WHAT the dialects materialise, and the consumer +// package pins that it is correct FOR that shape. +// +// --------------------------------------------------------------------------- +// Reverse verification, direction predicted BEFORE running +// --------------------------------------------------------------------------- +// Ordinary red, and separated by site so the ablation says which one broke: +// +// * Restore `String(b.createdAt ?? '').localeCompare(String(a.createdAt ?? ''))` +// in `listCommits` -> the two ordering cases go red, the planner case stays +// GREEN (a `filter` preserves order but the selected SET does not depend on +// it), and the ISO-text cases stay green. +// * Restore `String(c.createdAt ?? '') > String(target.created_at ?? '')` in +// `rollbackToPackageCommit` -> the planner case goes red on the SET, and the +// ordering cases stay green. +// +// The ISO-text half stays green in both directions on purpose: it is what shows +// the repair is the `Date` shape and not a blanket rewrite of the comparison. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; +import { assertEngineFindOnePredicate, type EngineFindOneQueryInput } from '@objectstack/metadata-core'; + +/** Four consecutive days plus one more, at 12:00Z. See the header for why. */ +const DAY_1 = '2026-08-30T12:00:00.000Z'; +const DAY_2 = '2026-08-31T12:00:00.000Z'; +const DAY_3 = '2026-09-01T12:00:00.000Z'; +const DAY_4 = '2026-09-02T12:00:00.000Z'; +const DAY_5 = '2026-09-03T12:00:00.000Z'; + +/** A registry with nothing in it — the commit store is the only source here. */ +function emptyRegistry() { + return { + getObject: () => undefined, + getItem: () => undefined, + listItems: () => [], + applyNavContributions: (x: any) => x, + isPackageDisabled: () => false, + getObjectOwner: () => undefined, + }; +} + +/** One `sys_metadata_commit` row, in the driver's snake_case wire shape. */ +function commitRow(id: string, createdAt: Date | string, operation = 'apply') { + return { + id, + package_id: 'pkg_crm', + organization_id: null, + operation, + message: `commit ${id}`, + actor: 'alice', + item_count: 1, + items: JSON.stringify([{ type: 'object', name: 'acct', existedBefore: true, prevVersion: 3 }]), + created_at: createdAt, + }; +} + +/** + * The five commits, in the shape ONE dialect family hands them over. + * + * `stamp` is the whole difference between the two families: `Date` is what + * Postgres and MySQL materialise for this column, the ISO-Z string is what the + * SQLite family and memory return. Rows are handed over oldest-first on purpose + * — the sort, not the driver, is what must make the timeline newest-first. + */ +function timeline(stamp: (iso: string) => Date | string) { + return [ + commitRow('cmt_d1', stamp(DAY_1)), + commitRow('cmt_d2', stamp(DAY_2)), + commitRow('cmt_d3', stamp(DAY_3)), + commitRow('cmt_d4', stamp(DAY_4)), + commitRow('cmt_d5', stamp(DAY_5), 'revert'), + ]; +} + +const asDate = (iso: string) => new Date(iso); +const asIsoText = (iso: string) => iso; + +/** An engine that answers both commit-store reads out of `rows`. */ +function engineWithCommits(rows: any[]) { + return { + registry: emptyRegistry(), + find: vi.fn(async () => rows), + findOne: vi.fn(async (object: string, query?: EngineFindOneQueryInput) => { + assertEngineFindOnePredicate(object, query); + const id = (query as any)?.where?.id; + return rows.find((r) => r.id === id) ?? null; + }), + } as any; +} + +/** + * A protocol whose `revertCommit` only RECORDS what it was handed. + * + * The defect under test is which commits `rollbackToPackageCommit` SELECTS, not + * what reverting one does; stubbing the per-commit revert keeps the assertion on + * the plan and off `revertCommit`'s own (separately pinned) machinery. + */ +function protocolWithPlanRecorder(rows: any[]) { + const protocol = new ObjectStackProtocolImplementation(engineWithCommits(rows)); + (protocol as any).revertCommit = async () => ({ + success: true, + revertedCount: 1, + failedCount: 0, + reverted: [{ type: 'object', name: 'acct', action: 'restored' }], + failed: [], + }); + return protocol; +} + +describe('[#13995] the commit timeline orders by INSTANT, not by the weekday name', () => { + it('the fixture discriminates: the old `String(...)` compare disagrees with chronology', () => { + // The positive control for everything below. If this ever passes, the + // `Date` cases stop being able to catch the defect and the pin is + // vacuous — which is exactly the state the OCC seam was in. + const stamps = [DAY_1, DAY_2, DAY_3, DAY_4].map(asDate); + const byOldStringCompare = [...stamps] + .sort((a, b) => String(b).localeCompare(String(a))) + .map((d) => d.toISOString()); + const byChronology = [...stamps] + .sort((a, b) => b.getTime() - a.getTime()) + .map((d) => d.toISOString()); + + expect(byOldStringCompare).not.toEqual(byChronology); + }); + + describe('the `Date`-materialising dialects (Postgres, MySQL)', () => { + it('listCommits returns the timeline newest-first', async () => { + const p = new ObjectStackProtocolImplementation(engineWithCommits(timeline(asDate))); + + const commits = await p.listCommits({ packageId: 'pkg_crm' }); + + expect(commits.map((c) => c.id)).toEqual([ + 'cmt_d5', 'cmt_d4', 'cmt_d3', 'cmt_d2', 'cmt_d1', + ]); + }); + + it('rollbackToPackageCommit reverts exactly the `apply` commits NEWER than the target', async () => { + const p = protocolWithPlanRecorder(timeline(asDate)); + + const result = await p.rollbackToPackageCommit({ commitId: 'cmt_d2' }); + + // Asserted as a SET: membership is decided by this site's predicate + // alone, so this case stays green if only `listCommits`' sort is + // reverted and red if only this site's is. + expect([...result.revertedCommits].sort()).toEqual(['cmt_d3', 'cmt_d4']); + // The target itself and everything older than it are untouched, and + // the `revert` commit is skipped — its effect is already captured by + // re-reverting the apply it undid. + expect(result.revertedCommits).not.toContain('cmt_d1'); + expect(result.revertedCommits).not.toContain('cmt_d2'); + expect(result.revertedCommits).not.toContain('cmt_d5'); + expect(result.success).toBe(true); + }); + }); + + describe('the ISO-text dialects (the SQLite family, memory) are unchanged', () => { + it('listCommits still returns the timeline newest-first', async () => { + const p = new ObjectStackProtocolImplementation(engineWithCommits(timeline(asIsoText))); + + const commits = await p.listCommits({ packageId: 'pkg_crm' }); + + expect(commits.map((c) => c.id)).toEqual([ + 'cmt_d5', 'cmt_d4', 'cmt_d3', 'cmt_d2', 'cmt_d1', + ]); + expect(commits[0]!.createdAt).toBe(DAY_5); + }); + + it('rollbackToPackageCommit still reverts exactly the `apply` commits newer than the target', async () => { + const p = protocolWithPlanRecorder(timeline(asIsoText)); + + const result = await p.rollbackToPackageCommit({ commitId: 'cmt_d2' }); + + expect([...result.revertedCommits].sort()).toEqual(['cmt_d3', 'cmt_d4']); + expect(result.success).toBe(true); + }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 91cde66b45..5ce4ef8afe 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1598,6 +1598,78 @@ function versionTokensAgree(current: NormalisedVersion, expected: NormalisedVers return current.token === expected.token; } +/** The verbatim spelling the ADR-0067 timeline compared before #13995 - `String(v ?? '')`. */ +function auditInstantToken(value: unknown): string { + return value === null || value === undefined ? '' : String(value); +} + +/** + * Epoch milliseconds for a DRIVER-stamped audit column, or null when the value + * does not denote an instant. + * + * The input domain is {@link canonicalVersionInstant}'s, and it is delegated to + * rather than re-spelled: that domain (`Date` / epoch `number` / canonical ISO + * text / opaque) was measured off the drivers for #13382 and is the same set a + * `created_at` arrives in. Its canonical answer round-trips through `Date.parse` + * exactly, and it has already bounded the value to a finite, `toISOString`-able + * time, so the number below is always finite. + */ +function auditInstantMs(value: unknown): number | null { + if (value === null || value === undefined) return null; + const canonical = canonicalVersionInstant(value, auditInstantToken(value).trim()); + return canonical === null ? null : Date.parse(canonical); +} + +/** + * Compare two engine-stamped audit instants, OLDEST-FIRST (`< 0` when `a` is + * older than `b`) - the shape both a `sort` comparator and a "strictly newer + * than" filter need. + * + * ## Why this exists (#13995) + * + * `created_at` is an engine-injected audit column: it is not in `datetimeFields`, + * and `SqlDriver#formatOutput` repairs it only inside `if (this.isSqlite)`, so + * the live dialects hand it out of the record read door as a JS `Date` and the + * SQLite family as canonical ISO-Z text (pinned by driver-sql's + * `sql-driver-13567-audit-stamp-materialisation.test.ts`). `String(aDate)` is + * `"Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)"`, whose LEADING + * token is the weekday NAME - so a lexicographic compare over those strings + * orders `Fri < Mon < Sat < Sun < Thu < Tue < Wed`, which has nothing to do with + * chronology. It is stable across the whole set, so the failure is systematic + * rather than intermittent: there is never an "it worked once" to warn anyone. + * Both ADR-0067 commit-timeline consumers compared exactly that way, and one of + * them - {@link ObjectStackProtocolImplementation.rollbackToPackageCommit} - + * chooses the set of commits to UNDO with it. + * + * ## Why a sibling of {@link canonicalVersionInstant} rather than a reuse of + * {@link versionTokensAgree} + * + * The canonicalisation IS reused (see {@link auditInstantMs}). What is not is + * `versionTokensAgree`: it answers EQUALITY between two client-facing version + * TOKENS, and ordering needs `<`/`>`, not `===`. It also takes the pre-tidied + * {@link NormalisedVersion} pair an HTTP `If-Match` header produces - RFC-7232 + * quote stripping, the "no token supplied" opt-out - none of which applies to a + * column a driver stamped and no client ever sends. + * + * The opaque limb is deliberately the one `versionTokensAgree` documents: when + * either side does not denote an instant, the two are compared VERBATIM, exactly + * as this seam did before. So the only verdicts that change are pairs that denote + * instants - the defect - and nothing a host stamps into the column starts + * ordering differently. This is not a tolerant fallback over a mis-spelled key + * (#13973's standing prohibition): no alternative spelling is accepted anywhere + * here, and an unparseable value is not repaired, only left where it was. + */ +function compareAuditInstants(a: unknown, b: unknown): number { + const aMs = auditInstantMs(a); + const bMs = auditInstantMs(b); + if (aMs !== null && bMs !== null) { + return aMs < bMs ? -1 : aMs > bMs ? 1 : 0; + } + const aToken = auditInstantToken(a); + const bToken = auditInstantToken(b); + return aToken < bToken ? -1 : aToken > bToken ? 1 : 0; +} + // Lifecycle columns the engine always owns; the clone path drops them by NAME // so the insert re-stamps fresh values instead of copying the source's. Mirrors // record-validator's SKIP_FIELDS (system-injected, never author-supplied). @@ -18351,8 +18423,17 @@ export class ObjectStackProtocolImplementation implements ...(r.created_at ? { createdAt: r.created_at } : {}), })); // Newest-first; tolerate drivers that don't order by returning - // insertion order, then sort by the ISO timestamp. - mapped.sort((a, b) => String(b.createdAt ?? '').localeCompare(String(a.createdAt ?? ''))); + // insertion order, then sort by the audit instant. + // + // [#13995] The comparison is on INSTANTS, not on the raw column's + // `String()` spelling. + // The comment here used to say "sort by the ISO timestamp" - an + // assumption written down and never verified, and false on the + // production default driver: Postgres and MySQL materialise this + // column as a JS `Date`, whose string spelling LEADS WITH THE WEEKDAY + // NAME, so this timeline was ordered `Fri < Mon < Sat < ...` while + // claiming newest-first. See {@link compareAuditInstants}. + mapped.sort((a, b) => compareAuditInstants(b.createdAt, a.createdAt)); return mapped; } catch (error) { // [#5980] Benign (the table has not been provisioned) falls through; @@ -18933,9 +19014,19 @@ export class ObjectStackProtocolImplementation implements // listCommits is newest-first; revert every `apply` commit strictly newer // than the target (by created_at). Revert commits are skipped (their // effect is already captured by re-reverting the apply they undid). - const targetCreatedAt = String(target.created_at ?? ''); + // + // [#13995] "Strictly newer" is an INSTANT comparison. This site both + // CONSUMES {@link listCommits}' ordering and re-derives the same + // comparison itself, so neither site could correct the other: with + // the raw column's `String()` spelling over a `Date`-materialising driver + // the predicate + // ranked by weekday NAME, and this rollback then reverted `apply` commits + // OLDER than the target while skipping the newer ones it exists to undo - + // a destructive operation planning off a wrong predicate, on every run and + // in the same way. See {@link compareAuditInstants}. + const targetCreatedAt = target.created_at; const toRevert = all.filter( - (c) => String(c.createdAt ?? '') > targetCreatedAt && c.operation === 'apply', + (c) => compareAuditInstants(c.createdAt, targetCreatedAt) > 0 && c.operation === 'apply', ); const revertedCommits: string[] = []; const failed: Array<{ commitId: string; error: string }> = []; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3d2d1bad3b..940d3040e0 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -366,6 +366,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/metadata-protocol/src/protocol.commit-timeline-instant-order.test.ts", + "verb": "findOne", + "pinned": 1 + }, { "file": "packages/metadata-protocol/src/protocol.container-issue-descent.test.ts", "verb": "delete",