Skip to content

Commit 4bf079c

Browse files
fix(plugin-auth): os migrate apply's account-issuer pre-flight names each colliding row's stored issuer (#25056)
Fixes #25030 Clause-②: yes (widening) ## What was wrong When the `sys_account.issuer` retirement pre-flight finds a `(provider_id, account_id)` key held by more than one row, `os migrate apply --allow-destructive` refuses to drop `sys_account.issuer` and prints the pre-flight's report. That report labelled every colliding row's issuer `(none)`. `os migrate account-issuer` printed the stored values for the same database. The issuers are what an operator needs to decide which row survives. Cause: `probeAccountIdentityCollisions` (`packages/plugins/plugin-auth/src/account-identity-preflight.ts`) read through whatever `find` it was handed. `apply`'s gate handed it the `objectql` engine, whose read verbs answer declared columns only, and the shipped `sys_account` no longer declares `issuer`. `account-issuer.ts` handed it a reader of its own over the driver (`getDriverForObject`). So the two doors ran one probe over two readers, and the readers disagreed. ### Reproduced first, at `d68aab6fde`, on the built CLI The fixture is a SQLite copy. The driver built `sys_account` from today's object. Then the legacy `issuer` column was added and the `(provider_id, account_id)` unique was dropped. Two rows, `acc_dup` and `acc_two`, share `credential / usr_25030`, with stored issuers `https://idp-a.example` and `https://idp-b.example`. The app config declares `requires: ['auth']`, so `apply` composes the auth family. | Door (exit) | Report at `d68aab6fde` | At this branch | |:--|:--|:--| | `os migrate apply --allow-destructive --yes` (1) | `credential / usr_25030 — 2 rows [acc_dup, acc_two], issuers [(none)], users [usr_25030]` | `… issuers [https://idp-a.example, https://idp-b.example], users [usr_25030]` | | `os migrate apply … --json` (1) | `preflight.collisions[0].issuers: ["(none)"]` | `["https://idp-a.example","https://idp-b.example"]` | | `os migrate account-issuer` (1) | `… issuers [https://idp-a.example, https://idp-b.example] …` | unchanged | ## The fix: one reader, owned by plugin-auth - **The probe reads storage itself.** When its source offers the storage door (`getDriverForObject`, the `objectql` engine both CLI doors pass), `probeAccountIdentityCollisions` reads `sys_account` through the driver the engine routes that name to, and only through it. A missing driver refuses; the probe never falls back to the engine's `find`. The query is the one `account-issuer.ts` used to send: same columns, same keyset walk, no driver options. `apply`'s gate (`utils/apply-drop-gates.ts`) is untouched and now reports the stored issuers. - **`os migrate account-issuer` adopts it.** The command passes its engine to the probe and deletes its own reader. Its missing-table reading (a database with no `sys_account` table reads as no rows) moves into the probe as an opt-in, `onTableAbsent`. A caller that passes it gets no rows and is told once. A caller that does not pass it is refused, as for any other failed read. No other `packages/cli` file is touched. - **A source that offers `find` alone is still read as handed, as before.** No production caller does this any more. It is kept because `packages/cli/test/migrate-drop-gate-remedies.pin.test.ts` drives the real probe through a `find`-only fake engine, and that file is outside this card's fence. The open question in the dev report covers retiring that path. - **Nothing else moves.** `sys_account` does not re-declare `issuer`, and nothing under `packages/spec` or `packages/objectql` changes. ## Unchanged, measured **Public door, before and after.** The CLI was built at `d68aab6fde` and again at this branch (`turbo run build --filter='@objectstack/cli...'`). `account-issuer` and `apply --allow-destructive --yes`, human and `--json`, ran on five databases: issuer column with a collision, no issuer column with a collision, issuer column and clean, no `sys_account` table, and no database file. That is 20 runs per build. Their stdout and stderr were compared after normalising timestamps and timings. - Exit codes are identical in all 20. - 38 of the 40 streams are byte-identical. The two that differ are `apply`'s report on the issuer-column collision database, human and `--json`, and only on the issuers line shown above. - With `preflight.collisions[].issuers` removed, the `--json` document equals the old one: the same `message: refused_account_issuer_collisions`, `ok`, keys, row ids, users and counts. - `account-issuer` is byte-identical on all five databases, human and `--json`, including the no-table line and the no-file run. These comparisons were measured twice: at `37077ac369`, an earlier strict-contract cut before the merge of `origin/main`, and at `010afefb70`, the merged head that this PR carries. Both builds compare against the same `d68aab6fde` baseline, with the same 20 exit codes and the same 38 identical streams. The card's own database was re-run at `010afefb70` with the result in the table above. **Pins**, in a new file: `packages/plugins/plugin-auth/src/account-identity-preflight-stored-issuer.test.ts`. They run on a real `SqlDriver` over a SQLite file that is built independently of the code under test. Each database is read three ways: - through `apply`'s composition, with `sys_account` registered as it ships; - through `account-issuer`'s composition, with `sys_account` unregistered; - through a reference: the engine-`find` read the probe made before this change. What they pin: - **Stored issuers are named.** The colliding rows' stored issuers appear in the report and in its text. A premise check shows the engine read of the same database gives `(none)`. - **One reader.** Both compositions give the same report and the same text. - **The verdict is unchanged.** Every field except `issuers` equals the reference, and the refusal (`RESOURCE_CONFLICT` / 409) carries the same message. This also holds over a 1,200-row walk with the colliding rows on its first and last page. A clean table stays clean. - **No `issuer` column.** A database without the column gives exactly the reference report (every row `(none)`) on both compositions. - **Missing table.** With `onTableAbsent`, the probe reads no rows and tells the caller once. Without it, the probe refuses. When the table is present, the caller is never told it is absent. - **No driver.** If the storage door yields no driver, the probe refuses and never calls the source's own `find`. **Ablation**, one leg per pinned behaviour, on the committed fix through `scripts/ablation-replace.mjs`. In every leg the anchor went from 1 hit to 0, the blob changed, and the run went red at the assertion of interest. Every restore was `git checkout HEAD -- PATH`, proven by the blob equalling HEAD's and an empty `git diff HEAD`. The final `git status` was clean. The subject resolves through a relative `src/` import, so there is no `dist/` leg. Every leg ran at `010afefb70`. | Leg | Mutation | Red, and where | |:--|:--|:--| | L1 | the engine's `find` wins again whenever the source has one (the defect) | 7/10. The stored-issuer pin fails at its `toEqual` (issuers `(none)`). The one-reader pin, the multi-page pin, the no-column pin and two missing-table pins fail because `account-issuer`'s composition is refused `OBJECT_NOT_FOUND`. The no-driver pin fails because the source's `find` is read. The verdict and clean-table pins stay green, as they should. | | L2 | a reader that refuses rows without a stored `issuer` | 5/10. The no-column pin fails at its own probe call. The other four fail at their engine-read reference, which carries no `issuer` by construction. | | L3 | collisions grouped by `issuer` too | 4/10. The verdict pin fails at `after.ok` (expected false, got true). The stored-issuer, one-reader and multi-page pins fail too. Before this fix the mutation was invisible, because `issuer` always read as undefined. | | L4 | the `onTableAbsent` call removed | 1/10: "a caller that asks … is told once" (`told` 0) | | L5 | a missing table softened without the opt-in | 1/10: "a caller that does not ask is refused" | | L6 | no driver falls back to the source's `find` | 1/10: the no-driver pin | ## Verification Every run went through `scripts/pm/os-verify-lock.sh`, with each exit captured before any pipe. Merged head `010afefb70`; the only later commit is changeset text, `652462c694`. - **Build closures:** `pnpm --filter '@objectstack/plugin-auth^...' build` exit 0. `turbo run build --filter='@objectstack/cli...'` exit 0 (61/61 tasks). - **New pins:** `pnpm --filter @objectstack/plugin-auth exec vitest run --maxWorkers=2 src/account-identity-preflight-stored-issuer.test.ts`, 10 passed. - **`pnpm --filter @objectstack/plugin-auth test`:** `Test Files 176 passed (176)`, `Tests 3348 passed | 10 skipped (3358)`. - **`pnpm --filter @objectstack/plugin-auth typecheck`:** exit 0, `check:test-typecheck: OK`. Debt is unchanged at 10 files / 88 errors. `tsc -p tsconfig.test.json --listFiles` lists the new test file once. - **`pnpm --filter @objectstack/cli typecheck`:** exit 0, `check:test-typecheck: OK`. - **CLI unit tier:** `pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2`, `Test Files 319 passed (319)`, `Tests 4730 passed (4730)`. - An earlier strict-contract cut of this branch reddened four cases in `test/migrate-drop-gate-remedies.pin.test.ts`. That file's fake engine offers `find` alone. Those reds are why the probe still reads a handed `find` source. - **CLI integration:** `src/commands/migrate/apply.account-issuer-retirement.integration.test.ts` was run locally because `account-issuer`'s read moved: `Tests 4 passed (4)`. The rest of the integration tier is declared to CI. - **Gates:** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` at `652462c694` derived 72 commands. - 70 exited 0. - `check:dual-build-cjs-loads` and `check:i18n-coverage` exited 3 (PREREQUISITE NOT MET: untouched packages have no `dist/`). After a full `turbo run build` (76/76 tasks, exit 0), both exited 0 on the same head: `check:dual-build-cjs-loads — 112 published require entry point(s) across 69 package(s) load`, and `check-i18n-coverage: OK (14 config(s), 621 baselined untranslated string(s), none new)`. - `--ran` over the first record: `72 derived famil(ies) accounted for — 70 run, 2 NOT-MEASURED`. Over the record with the two re-runs: `✓ dispatch-gates --ran: 72 derived famil(ies) accounted for — 72 run, 0 NOT-MEASURED (a DERIVED zero — all 72 recorded an exit code and none of them is 3).` - Every family CI runs is read from this PR's check-runs, not re-run here. ## Acceptance notes - **Published surface.** `@objectstack/plugin-auth` gains: - two exported types, `AccountTableStorage` (`getDriverForObject`) and `AccountTableDriver`; - one option, `AccountIdentityPreflightOptions.onTableAbsent`. `probeAccountIdentityCollisions` now takes `AccountTableStorage | AccountIdentityReadEngine`, so every earlier caller still type-checks. The dispatch's `Clause-②: no` is kept as given. The dev report asks whether these additions are "public surface" under the clause. - **Unreachable wording delta in `account-issuer`.** If the booted stack had no `objectql` service at all, the refusal used to read `no driver serves sys_account on this stack`. It now reads `no readable ObjectQL engine`. Both are refusals with exit 1. `bootSchemaStack` always composes `objectql`, so this path is not reachable today. Where an engine is present but has no driver for the table, the old wording is unchanged. - **Stale measurement row.** `find-envelope-limb-removal.test.ts` row B9 still measures an engine `find` of `sys_account` as "the exact read" of this site. The site now reads the driver. `SqlDriver.find` declares an array of rows as its answer, and the probe still refuses a non-array. The row stays green; it is outside the file surface, so it is left as is. - **Merged `origin/main` before opening.** The merge (`eddffc3390`) was clean. The incoming commits touch `packages/spec` translations, `packages/cli/src/utils/i18n-extract.ts` and its tests, `core`, `lint` and `rest`, and none of them touch this diff's files. Everything under Verification was re-run on the merged head. --- **Seat's append** (`domain:services` seat 1, from the dev's patch-round report on #25030; the dev writes its PR body once). The seat moved the `Clause-②:` line above from `no` to `yes (widening)`, per the claim amendment `6308624635`. - Patch round 1 (`dffec4ba41`), changeset only: `@objectstack/plugin-auth` moves from patch to minor under `Clause-②: yes (widening)`. One bullet names the widened surface: `AccountTableStorage`, `AccountTableDriver`, `AccountIdentityPreflightOptions.onTableAbsent`, and the probe's `AccountTableStorage | AccountIdentityReadEngine` source. `@objectstack/cli` stays patch. - Re-run at `dffec4ba41`: `check-changeset-no-major` exit 0 and `check-empty-changeset` exit 0. `git diff --stat 652462c HEAD` touches only that changeset (+3/-2). --- _Generated by [Claude Code](https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 72518d2 commit 4bf079c

4 files changed

Lines changed: 495 additions & 63 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/plugin-auth': minor
3+
'@objectstack/cli': patch
4+
---
5+
6+
fix(plugin-auth): `os migrate apply`'s `sys_account.issuer` pre-flight report names each colliding row's stored issuer instead of `(none)`
7+
8+
Clause-②: yes (widening)
9+
10+
`probeAccountIdentityCollisions` read `sys_account` through the engine's `find`. The engine answers declared columns only, and the shipped `sys_account` no longer declares `issuer`, so the report `os migrate apply --allow-destructive` prints when it refuses the `sys_account.issuer` drop labelled every colliding row's issuer `(none)`, in the human report and in the `--json` document's `preflight.collisions[].issuers` alike. `os migrate account-issuer` read the same database through the driver and printed the stored values. The issuers are what an operator needs to decide which colliding row survives.
11+
12+
- **`@objectstack/plugin-auth`.** When its source offers the storage door (`getDriverForObject`, as the `objectql` engine both CLI doors pass does), the probe reads `sys_account` through the driver the engine routes that name to, and through nothing else: a missing driver is refused, never answered from the engine's `find`. A source that offers `find` alone is still read as handed. A new option, `onTableAbsent`, reads a missing `sys_account` table as no rows and reports that it did. Without it, a missing table is refused like any other failed read.
13+
- **Widened public surface (`@objectstack/plugin-auth`).** The package entry re-exports the pre-flight module, so it now publishes two new types, `AccountTableStorage` (`getDriverForObject`) and `AccountTableDriver`, and a new option, `AccountIdentityPreflightOptions.onTableAbsent`. `probeAccountIdentityCollisions` now accepts `AccountTableStorage | AccountIdentityReadEngine` as its source. Every earlier caller still type-checks, and nothing narrows.
14+
- **`@objectstack/cli`.** `os migrate account-issuer` reads through the probe's reader instead of its own, so it and `os migrate apply` read one table one way. Its output is unchanged.
15+
16+
**Unchanged.** The collision verdict (`ok`, the colliding keys, row ids, users, counts), the `apply` refusal and its exit code. A database whose `sys_account` has no `issuer` column reports each row as `(none)`, as before.

‎packages/cli/src/commands/migrate/account-issuer.ts‎

Lines changed: 21 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import { Command, Flags } from '@oclif/core';
44
import chalk from 'chalk';
5-
import { isMissingTableError } from '@objectstack/types';
65
import {
76
printHeader,
87
printSuccess,
@@ -17,14 +16,6 @@ import {
1716
} from '../../utils/format.js';
1817
import { bootSchemaStack } from '../../utils/schema-migrate.js';
1918

20-
/** The table this pre-flight inventories. Never written. */
21-
const SYS_ACCOUNT = 'sys_account';
22-
23-
/** The driver read this pre-flight issues: `find` only, read-only by construction. */
24-
interface AccountTableDriver {
25-
find(object: string, query: Record<string, unknown>): Promise<unknown>;
26-
}
27-
2819
/**
2920
* `os migrate account-issuer` — the PLAN leg of the `sys_account.issuer`
3021
* retirement (#17440).
@@ -134,51 +125,34 @@ export default class MigrateAccountIssuer extends Command {
134125
const engine = (stack.kernel as { getService?: (n: string) => unknown }).getService?.call(
135126
stack.kernel,
136127
'objectql',
137-
) as { getDriverForObject?: (object: string) => AccountTableDriver | undefined } | undefined;
128+
) as Parameters<typeof probeAccountIdentityCollisions>[0];
138129

139130
if (!flags.json) printStep('Scanning sys_account…');
140131

141-
// The pre-flight reads the PHYSICAL `sys_account` table through the
142-
// driver the engine routes that name to, not through the engine's
143-
// `find`. This boot composes no `AuthPlugin`, so `sys_account` is not a
144-
// registered object here, and the engine refuses a name its registry
145-
// does not resolve (`OBJECT_NOT_FOUND`) before any driver is asked. That
146-
// refusal is a fact about this boot's composition, never about the
147-
// database: ⛔ reading it as "no rows" would report a table full of
148-
// accounts as a clean pre-flight and authorise the drop. The table is
149-
// read in its legacy shape, `issuer` included, which is the very column
150-
// the registered schema no longer declares, so the driver (the path the
151-
// engine leaves to host code) is the reader this inventory needs.
132+
// [#25030] The pre-flight reads the PHYSICAL `sys_account` table through
133+
// the probe's own reader, `@objectstack/plugin-auth`'s: the driver the
134+
// engine routes that name to, never the engine's `find`. It is the same
135+
// reader `os migrate apply`'s drop gate runs, so the two reports cannot
136+
// disagree about a row's stored `issuer`. This boot composes no
137+
// `AuthPlugin`, so `sys_account` is not a registered object here, and the
138+
// engine's `find` refuses a name its registry does not resolve
139+
// (`OBJECT_NOT_FOUND`) before any driver is asked. ⛔ Reading that
140+
// refusal as "no rows" would report a table full of accounts as a clean
141+
// pre-flight and authorise the drop.
152142
//
153143
// [#21552] A database with no `sys_account` table holds no account, so no
154-
// two rows collide: the probe reads it as no rows. The read is not
155-
// avoided, measured: `sys_account` is not a registered object on this
156-
// boot, so the held-back sync never lists it, and
157-
// `stack.tableAbsent('sys_account')` answers false on every database.
158-
// The driver's missing-table refusal is therefore recognised, with the
159-
// shared predicate and for this command's own table only. ⛔ No other
160-
// refused read is softened: it still throws the probe's refusal below
161-
// and is never read as clean.
144+
// two rows collide: the probe reads it as no rows, and only because this
145+
// command asks it to and says so below. The read is not avoided,
146+
// measured: `sys_account` is not a registered object on this boot, so the
147+
// held-back sync never lists it, and `stack.tableAbsent('sys_account')`
148+
// answers false on every database. ⛔ No other refused read is softened:
149+
// it still throws the probe's refusal and is never read as clean.
162150
let noAccountTable = false;
163-
const readView: Parameters<typeof probeAccountIdentityCollisions>[0] = {
164-
find: async (object, query) => {
165-
// No driver is not an empty table: the probe turns this into its
166-
// refusal, never into a clean report.
167-
const driver = engine?.getDriverForObject?.(object);
168-
if (!driver || typeof driver.find !== 'function') {
169-
throw new Error(`no driver serves ${object} on this stack`);
170-
}
171-
try {
172-
return await driver.find(object, query);
173-
} catch (error) {
174-
if (object !== SYS_ACCOUNT || !isMissingTableError(error, object)) throw error;
175-
noAccountTable = true;
176-
return [];
177-
}
178-
},
179-
};
180-
const report = await probeAccountIdentityCollisions(readView, {
151+
const report = await probeAccountIdentityCollisions(engine, {
181152
...(flags['max-records'] != null ? { max: flags['max-records'] } : {}),
153+
onTableAbsent: () => {
154+
noAccountTable = true;
155+
},
182156
});
183157
const noAccountTableLine = noAccountTable
184158
? 'sys_account has no table in this database yet, so no account is stored in it and it was read as no rows.'

0 commit comments

Comments
 (0)