From d9366442ace212886ee3b372187233469b4ae40d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 11:44:16 +0000 Subject: [PATCH] fix(app-shell): narrow the signed-in cast so id/name/email are required `buildExpressionUser` reads its input through a cast that declared `id`, `name` and `email` optional, while the signed-in branch forwards exactly those three raw. The declaration was therefore WIDER than the contract it mirrors -- `useAuth().user` is `@object-ui/auth`'s `AuthUser`, which extends the spec's `AuthUser` (`id: string; email: string; name: string`) -- and it declared a session missing any of them to be a legitimate input that would answer `{ id: undefined, ... }`, the present-and-always-undefined shape objectui#5424 removed `roles` from this object for. Declarative only: no production producer can supply such a session, so no runtime behaviour moves and none was made to move. The rejected shape was a consumer-side `id: u.id ?? null` fallback. No fault-handling path moved. `expressionUser.sessionContract.types.test.ts` drives `tsc` over the real declarations and carries its own discrimination leg against the pre-fix optionality. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .changeset/6551-signedin-user-id-required.md | 42 +++ ...pressionUser.sessionContract.types.test.ts | 344 ++++++++++++++++++ .../app-shell/src/providers/expressionUser.ts | 53 ++- 3 files changed, 435 insertions(+), 4 deletions(-) create mode 100644 .changeset/6551-signedin-user-id-required.md create mode 100644 packages/app-shell/src/providers/expressionUser.sessionContract.types.test.ts diff --git a/.changeset/6551-signedin-user-id-required.md b/.changeset/6551-signedin-user-id-required.md new file mode 100644 index 0000000000..236bd0641c --- /dev/null +++ b/.changeset/6551-signedin-user-id-required.md @@ -0,0 +1,42 @@ +--- +'@object-ui/app-shell': patch +--- + +`buildExpressionUser`'s signed-in cast declares `id`, `name` and `email` REQUIRED, so it is +no longer wider than the contract it mirrors (objectui#6551). + +The normaliser reads its input through a cast, and that cast wrote all three keys optional +(`{ id?: string; name?: string; email?: string; … }`) while the signed-in branch forwards +exactly those three RAW. So the declaration said `buildExpressionUser({ name: 'B', email: +'b@c.d' })` was a legitimate input, and the code answered `{ id: undefined, … }` for it — +present-and-always-`undefined`, which is the shape objectui#5424 removed `roles` from this +same object for ("the shape that teaches the wrong thing") and the one objectui#6534 +refused for the anonymous branch, one key over. The three keys BELOW them already defended +with `??`; the asymmetry sat inside one object literal. + +The contract disagreed with the cast. Every production input is `useAuth().user`, typed as +`@object-ui/auth`'s `AuthUser`, which extends the spec's `AuthUser` +(`@objectstack/spec/contracts`): `id: string; email: string; name: string`, with only +`positions` and `tenantId` optional. `name` and `email` are narrowed alongside `id` because +that same interface declares them required too — the same answer from the same authority, +not a widened scope. `role` stays optional (it is `@object-ui/auth`'s display-only +addition, not a spec key), and the index signature stays (better-auth projects an app's +custom user columns onto this object, and it is how `isPlatformAdmin` / `positions` are +read). + +NOTHING REACHABLE CHANGES, and deliberately so. The only production producer is a +better-auth principal that always carries `id`, which is why this was graded a latent shape +hazard rather than a bug, and why the fix moved a declaration and no runtime behaviour: the +defect was that the cast LIED about the contract. `id: u.id ?? null` was the rejected shape +(triage, 2026-08-26) — a lenient default in the consumer is what AGENTS.md #0.1 forbids and +what objectui#6534 shipped a scope fence against, and it silently equates "signed in, no +id" with "signed out". A producer without an `id` is wrong at the producer. + +Because `id?: string` and `id: string` produce byte-identical output for every input a +producer can supply, no runtime assertion can pin this; the new +`expressionUser.sessionContract.types.test.ts` drives `tsc` itself over the real +declarations and carries its own discrimination leg — the same cases compiled a second time +against the pre-fix optionality, with the five that flip named by index. + +No fault-handling path moved. Fail-open on a predicate that DOES fault stays deliberate +policy (objectui#6443 / #6487 / #6445). diff --git a/packages/app-shell/src/providers/expressionUser.sessionContract.types.test.ts b/packages/app-shell/src/providers/expressionUser.sessionContract.types.test.ts new file mode 100644 index 0000000000..e09ebe96bf --- /dev/null +++ b/packages/app-shell/src/providers/expressionUser.sessionContract.types.test.ts @@ -0,0 +1,344 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Type-level pin for objectui#6551 — the cast `buildExpressionUser` reads its + * input through is not WIDER than the contract it mirrors. + * + * ## What the defect was, and why only the compiler can see it + * + * The normaliser's signed-in branch forwards `id` / `name` / `email` RAW, out + * of a cast that declared all three optional: + * + * const u = user as { id?: string; name?: string; email?: string; … } + * + * So the declaration said a session with no `id` was a legitimate input, and + * the code answered `{ id: undefined, … }` for it — present-and-always- + * `undefined`, the exact shape objectui#5424 removed `roles` from this object + * for and objectui#6534 refused for the anonymous branch one key over. The + * contract it mirrors disagrees: `useAuth().user` is `@object-ui/auth`'s + * `AuthUser`, which extends the spec's `AuthUser` — `id: string; + * email: string; name: string`, with only `positions` / `tenantId` optional. + * + * NOTHING REACHABLE TODAY HITS IT. The only production input is a better-auth + * principal that always carries `id`, which is why this card was graded a + * latent shape hazard and why the fix moved a DECLARATION and no runtime + * behaviour. That is also why no runtime assertion can pin it: `id?: string` + * and `id: string` produce byte-identical output for every input a producer + * can actually supply. The compiler is the only instrument that sees the + * difference, so this file drives `tsc` itself — same mechanism as + * `core/src/utils/__tests__/freeze-schema.types.test.ts` and + * `core/src/actions/__tests__/actionKeys.types.test.ts` (vitest's `typecheck` + * mode is off in `vitest.config.mts`; turning it on is a shared-config change + * owned by objectui#3181, not by this card). + * + * NO BUILD ARTIFACT SITS BETWEEN THE EDIT AND THESE ASSERTIONS. The preamble + * imports `../expressionUser` by relative path, so the program reads the + * SOURCE file — and that module is a LEAF that imports nothing, so there is no + * workspace `dist` anywhere on the path that could go stale and degrade a type + * to `any` (the hazard `actionKeys.types.test.ts` documents). `AuthUser` comes + * from the PUBLISHED `@objectstack/spec` typings in `node_modules`, and the + * preamble-integrity case below fails loudly if either import stops resolving, + * rather than letting a silent `any` turn a rejection case green. + * + * ## Discrimination is built in, not promised + * + * Every case is compiled a SECOND time with one thing changed: the type + * redeclared with its PRE-FIX optionality, verbatim from `main` at + * `0235ce7c1`, everything else still the real module. `EXPECTED_FLIPS` names by + * index exactly which cases that reverts. A case outside that set is green on + * BOTH legs and is stated as such rather than quietly counted as evidence — + * three of the cases below are in that position deliberately, because "the + * narrowing did not overshoot" is a claim that needs its own controls. + * + * ## Fenced boundary + * + * `id: u.id ?? null` is the REJECTED shape (triage ruling, 2026-08-26): a + * lenient default in the consumer is what AGENTS.md #0.1 forbids and what + * objectui#6534 shipped a scope fence against. The last case in the runtime + * section is that fence made mechanical — it reds if a fallback is smuggled in + * later. Changing it is re-deciding the card, not editing a test. + */ +import { describe, it, expect } from 'vitest'; +import ts from 'typescript'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +import { buildExpressionUser } from './expressionUser'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const MODULE_IMPORT = join(HERE, 'expressionUser').replace(/\\/g, '/'); + +/** One statement, and whether `tsc` is required to reject it. */ +interface Case { + readonly what: string; + readonly code: string; + readonly rejected: boolean; +} + +/** + * One line each — the diagnostics are mapped back to cases BY LINE NUMBER, so + * a two-line case would silently mis-attribute its own error to its neighbour. + */ +const CASES: readonly Case[] = [ + // ── The defect, in type form ────────────────────────────────────────────── + // Case 0 is the card's own literal: `buildExpressionUser({ name: 'B', + // email: 'b@c.d' })` was a DECLARED-legitimate input that answered + // `{ id: undefined, … }`. + { + what: 'a session with no `id` is not a signed-in session', + rejected: true, + code: `const a: ExpressionUserSession = { name: 'B', email: 'b@c.d' }; void a;`, + }, + { + what: 'a session with no `name` is not a signed-in session', + rejected: true, + code: `const b: ExpressionUserSession = { id: 'u_1', email: 'b@c.d' }; void b;`, + }, + { + what: 'a session with no `email` is not a signed-in session', + rejected: true, + code: `const c: ExpressionUserSession = { id: 'u_1', name: 'B' }; void c;`, + }, + { + // The present-and-undefined shape ITSELF, written out. The optional cast + // accepted this spelling too, which is how the defect reached the object + // literal below `u` unaltered. + what: 'an explicitly `undefined` `id` is not a signed-in session either', + rejected: true, + code: `const d: ExpressionUserSession = { id: undefined, name: 'B', email: 'b@c.d' }; void d;`, + }, + { + // Rejected on BOTH legs — see EXPECTED_FLIPS. It is here because `null` is + // the ANONYMOUS branch's answer, and "signed in with no id" must not be + // spellable as "signed out" on the way IN. That was rejected shape 1 in the + // ruling, and the type refuses it independently of this card's change. + what: 'a `null` `id` is not a signed-in session — the anonymous answer is not an input', + rejected: true, + code: `const e: ExpressionUserSession = { id: null, name: 'B', email: 'b@c.d' }; void e;`, + }, + { + // The spec-derived half: this one does not restate the three key names, so + // it follows `@objectstack/spec` if the principal's required set ever + // moves. An INCOMPLETE principal is not an input. + what: 'a Partial of the spec principal is not a signed-in session', + rejected: true, + code: `const f: ExpressionUserSession = { ...partialPrincipal }; void f;`, + }, + + // ── The narrowing did not overshoot ─────────────────────────────────────── + // Green on both legs. Without these, "reds when the cast widens" would be + // satisfied by a type that rejects everything, which is the other way to be + // wider-or-narrower than the contract. + { + what: 'a COMPLETE spec principal is a signed-in session', + rejected: false, + code: `const g: ExpressionUserSession = { ...principal }; void g;`, + }, + { + what: 'the minimum complete principal types — `role` and `positions` stay optional', + rejected: false, + code: `const h: ExpressionUserSession = { id: 'u_1', name: 'B', email: 'b@c.d' }; void h;`, + }, + { + // The index signature is load-bearing and must survive the narrowing: + // better-auth projects an app's custom user columns onto this object, and + // it is also the route by which `isPlatformAdmin` and `positions` are read. + what: 'better-auth custom columns are still absorbed by the index signature', + rejected: false, + code: `const i: ExpressionUserSession = { id: 'u_1', name: 'Ada', email: 'a@e.d', role: 'user', positions: ['user'], isPlatformAdmin: true, department__c: 'sales' }; void i;`, + }, + { + // The honest limit of this change, pinned so no reader has to infer it: the + // PARAMETER is `unknown`, so narrowing the CAST does not make any producer + // loud at compile time. It makes the module stop declaring the broken input + // legitimate. Widening the parameter is a different question and a + // different card. + what: 'the exported function still accepts an unchecked input — the cast is not a parameter check', + rejected: false, + code: `buildExpressionUser({ name: 'B', email: 'b@c.d' });`, + }, +]; + +/** Diagnostics from one compile: how many landed in the preamble, and which case lines erred. */ +interface Run { + readonly headerErrors: number; + readonly lines: ReadonlySet; +} + +function compile(preamble: string): Run { + const header = `${preamble}\n`; + const body = CASES.map((c) => c.code).join('\n'); + const source = `${header}${body}\nexport {};\n`; + const headerLines = header.split('\n').length - 1; + + const VIRTUAL = join(HERE, '__expressionUserSessionContract.virtual.ts').replace(/\\/g, '/'); + const options: ts.CompilerOptions = { + strict: true, + skipLibCheck: true, + noEmit: true, + moduleResolution: ts.ModuleResolutionKind.Bundler, + module: ts.ModuleKind.ESNext, + target: ts.ScriptTarget.ESNext, + }; + const host = ts.createCompilerHost(options); + const getSourceFile = host.getSourceFile.bind(host); + host.getSourceFile = (fileName, languageVersion, ...rest) => + fileName === VIRTUAL + ? ts.createSourceFile(fileName, source, languageVersion, true) + : getSourceFile(fileName, languageVersion, ...rest); + const fileExists = host.fileExists.bind(host); + host.fileExists = (fileName) => (fileName === VIRTUAL ? true : fileExists(fileName)); + const readFile = host.readFile.bind(host); + host.readFile = (fileName) => (fileName === VIRTUAL ? source : readFile(fileName)); + + const program = ts.createProgram([VIRTUAL], options, host); + const sf = program.getSourceFile(VIRTUAL); + if (!sf) throw new Error('virtual source file was not added to the program'); + + const lines = new Set(); + let headerErrors = 0; + const diagnostics = [...program.getSemanticDiagnostics(sf), ...program.getSyntacticDiagnostics(sf)]; + for (const d of diagnostics) { + if (d.start == null) continue; + const line = sf.getLineAndCharacterOfPosition(d.start).line; + if (line < headerLines) headerErrors++; + else lines.add(line - headerLines); + } + return { headerErrors, lines }; +} + +/** + * The fixtures both legs share. `principal` and `partialPrincipal` are the only + * things tying these cases to `@objectstack/spec` rather than to a local + * restatement of it. + */ +const FIXTURES = + `declare const principal: AuthUser;\n` + `declare const partialPrincipal: Partial;`; + +const REAL_PREAMBLE = + `import { buildExpressionUser } from '${MODULE_IMPORT}';\n` + + `import type { ExpressionUserSession } from '${MODULE_IMPORT}';\n` + + `import type { AuthUser } from '@objectstack/spec/contracts';\n` + + FIXTURES; + +/** + * The same program with ONE thing changed: `ExpressionUserSession` redeclared + * with the optionality it carried before this card — copied verbatim from the + * cast on `main` at `0235ce7c1`. The module, the spec principal and every case + * are otherwise identical, so a flip here can only be caused by that + * optionality. + */ +const REVERTED_PREAMBLE = + `import { buildExpressionUser } from '${MODULE_IMPORT}';\n` + + `import type { AuthUser } from '@objectstack/spec/contracts';\n` + + `type ExpressionUserSession = { id?: string; name?: string; email?: string; role?: string; [key: string]: unknown };\n` + + FIXTURES; + +// Module scope on purpose (see the header of `actionKeys.types.test.ts`): the +// compiler cost lands in the import phase rather than under a hook timeout. +const againstReal = compile(REAL_PREAMBLE); +const againstReverted = compile(REVERTED_PREAMBLE); + +describe('objectui#6551 — the signed-in cast is no wider than the contract it mirrors', () => { + it('resolves the real declarations — no diagnostic lands in either preamble', () => { + // The guard against a SILENT `any`. If `../expressionUser` or + // `@objectstack/spec/contracts` stopped resolving, `ExpressionUserSession` + // and `AuthUser` would degrade and every rejection case below would go + // green for a reason that has nothing to do with this card. + expect({ real: againstReal.headerErrors, reverted: againstReverted.headerErrors }).toEqual({ + real: 0, + reverted: 0, + }); + }); + + for (const [i, c] of CASES.entries()) { + it(c.what, () => { + expect({ case: c.what, rejected: againstReal.lines.has(i) }).toEqual({ + case: c.what, + rejected: c.rejected, + }); + }); + } +}); + +/** + * Indices the PRE-FIX optionality turns green again. Named explicitly so the + * file records which assertions actually discriminate — the rest are green on + * both legs and prove nothing about this change on their own. + * + * 0,1,2 — the three keys the spec principal declares required. + * 3 — the present-and-`undefined` spelling itself. + * 4 — NOT here: `id: null` is refused by `id?: string` too, so it does + * not move. It pins that the anonymous answer is not an input, + * which is a different claim from this card's. + * 5 — the spec-derived incomplete principal. + * 6-9 — the overshoot controls and the `unknown`-parameter limit; green on + * both legs by construction. + */ +const EXPECTED_FLIPS = [0, 1, 2, 3, 5]; + +describe('discrimination: the same cases against the pre-fix optionality', () => { + it('re-widening the cast is exactly what turns the rejection cases green', () => { + const flipped = CASES.map((_, i) => i).filter( + (i) => againstReverted.lines.has(i) !== againstReal.lines.has(i), + ); + expect(flipped).toEqual(EXPECTED_FLIPS); + }); + + it('every flipped case is REJECTED now and was ACCEPTED before', () => { + for (const i of EXPECTED_FLIPS) { + expect({ i, now: againstReal.lines.has(i), before: againstReverted.lines.has(i) }).toEqual({ + i, + now: true, + before: false, + }); + } + }); +}); + +/** + * The runtime half. Both of these pass on `main` at `0235ce7c1` too — the + * runtime was never the defect and this card deliberately did not move it — + * and they are here so the mismatch cannot be "fixed" later by moving the + * runtime instead of the producer. + */ +describe('objectui#6551 — the runtime the narrowed declaration now describes', () => { + it('still answers the real principal unchanged', () => { + expect( + buildExpressionUser({ id: 'u_1', name: 'Ada', email: 'a@e.d', positions: ['user'] }), + ).toStrictEqual({ + id: 'u_1', + name: 'Ada', + email: 'a@e.d', + role: 'user', + isPlatformAdmin: false, + positions: ['user'], + }); + }); + + it('adds no consumer-side fallback for the input the type now refuses', () => { + // THE FENCE, MECHANISED. Rejected shape 1 was `id: u.id ?? null`, which + // would make this read `null` — a lenient default in the consumer + // (AGENTS.md #0.1), and one that silently equates "signed in, no id" with + // "signed out". No such producer exists, so the honest answer is that the + // input never arrives; the type is where that is now said, and this asserts + // nothing was quietly added below it. Re-deciding this is a card, not a + // test edit. + const built = buildExpressionUser({ name: 'B', email: 'b@c.d' } as unknown); + + expect(built.id).toBeUndefined(); + expect(built.id).not.toBeNull(); + // The keys that DO carry a declared default still carry it — the asymmetry + // this card was filed about is between these and the three above, and it is + // closed by narrowing the three, not by lowering these. + expect(built.role).toBe('user'); + expect(built.isPlatformAdmin).toBe(false); + expect(built.positions).toEqual([]); + }); +}); diff --git a/packages/app-shell/src/providers/expressionUser.ts b/packages/app-shell/src/providers/expressionUser.ts index d23ec20f75..b150bb9852 100644 --- a/packages/app-shell/src/providers/expressionUser.ts +++ b/packages/app-shell/src/providers/expressionUser.ts @@ -71,11 +71,56 @@ * predicates ever reach that policy. None of them touches what the policy does * once one has. */ +/** + * The signed-in session principal this normaliser MIRRORS — the declared shape + * of what its production callers hand it (objectui#6551). + * + * `id`, `name` and `email` are REQUIRED because the contract they mirror + * declares them required. Every production input is `useAuth().user`, whose + * type is `@object-ui/auth`'s `AuthUser`; that interface EXTENDS the spec's + * `AuthUser` (`@objectstack/spec/contracts`), which declares + * `id: string; email: string; name: string` and makes only `positions` and + * `tenantId` optional. This cast used to write all three as `id?` / `name?` / + * `email?`, which is WIDER than the contract it mirrors: it declared a session + * missing any of them to be a legitimate input, while the signed-in branch + * below forwards those three keys RAW. So the declaration said + * `buildExpressionUser({ name: 'B', email: 'b@c.d' })` was fine and the code + * answered `{ id: undefined, … }` — present-and-always-`undefined`, which is + * exactly the shape objectui#5424 removed `roles` from this same object for + * ("the shape that teaches the wrong thing") and the one objectui#6534 refused + * for the anonymous branch, one key over. + * + * The narrowing is DECLARATIVE, and deliberately only that. Nothing reachable + * today hands this function a session without `id` — the better-auth principal + * above always carries one — so no runtime behaviour moves here and none was + * made to move: the defect was that the declaration LIED about the contract, + * not that a user could reach it. What refuses the widening from coming back is + * the compiler, via `expressionUser.sessionContract.types.test.ts`. + * + * ⛔ Not a fallback. `id: u.id ?? null` was the rejected shape (triage ruling, + * 2026-08-26): it puts a lenient default in the CONSUMER, which AGENTS.md #0.1 + * forbids and which objectui#6534 shipped a scope fence against, and it + * silently equates "signed in, no id" with "signed out". A producer with no + * `id` is wrong AT THE PRODUCER, and the type is where that is now said. + * + * The index signature stays: better-auth projects an app's custom user columns + * onto this object and no local type can enumerate them — the same reason + * `@object-ui/auth`'s own `AuthUser` carries one, and the route by which + * `isPlatformAdmin` and `positions` are read below. `role` stays OPTIONAL: it + * is not a spec key at all, it is the display-only field `@object-ui/auth` + * adds, so `?? 'user'` below is its declared default rather than a fallback + * around a broken producer. + */ +export type ExpressionUserSession = { + id: string; + name: string; + email: string; + role?: string; + [key: string]: unknown; +}; + export function buildExpressionUser(user: unknown): Record { - const u = user as - | { id?: string; name?: string; email?: string; role?: string; [key: string]: unknown } - | null - | undefined; + const u = user as ExpressionUserSession | null | undefined; if (!u) { return { // `null`, not absent — objectui#6534. An ABSENT key is not `false`: it