diff --git a/.changeset/hook-context-session-positions-preserve-audit.md b/.changeset/hook-context-session-positions-preserve-audit.md new file mode 100644 index 0000000000..1d6095f47c --- /dev/null +++ b/.changeset/hook-context-session-positions-preserve-audit.md @@ -0,0 +1,53 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec): `HookContext.session` declares `positions` and `preserveAudit` (#5605) + +Two keys the engine has been **producing** all along, that consumers have been +**reading** and the docs have been **teaching**, were missing from the contract: +`HookContextSchema.session` declared `userId` / `actor` / `organizationId` / +`accessToken` / `isSystem` / `skipTriggers` / `skipAutomations` and nothing else. +Both are now declared, per the maintainer ruling on #5605. + +This is the mirror of the `session.roles` retirement (#5050). That key was +declared-never-produced, so it was removed; these two are +produced-never-declared, so they are added. Same `session` block, opposite +drift, opposite fix. + +**What was broken.** `HookContextSchema` is deliberately not `.strict()` — it is +the runtime shape the engine hands a handler, and strictness there would make +every engine-side enrichment a breaking change for anyone parsing a context they +were given. The cost of that tolerance is that an undeclared key is **stripped +in silence**: + +- `HookContextSchema.parse(ctx)` — the exact call the generated reference page + documents as the way to consume a context — returned a session with the + caller's `positions` and the import's `preserveAudit` dropped on the floor. +- A handler typed the way the automation docs teach, `(ctx: HookContext)`, could + not read either key: `ctx.session?.positions` was a `TS2339`. The two + `kernel/runtime-services` pages that teach + `positions: ctx.session?.positions` compiled only because they annotate `ctx` + as `any` — copying both the code and the documented annotation did not build. + +**`positions`** (`string[]`, optional) is the ADR-0090 D3 placement vocabulary, +copied verbatim from `ExecutionContext.positions` by ObjectQL's `buildSession()`. +Its `.describe()` states the boundary the ruling asked for, because the boundary +is the whole reason this key needed a decision rather than a patch: it is +**readable context, never an authorization input**. A hook may forward it as the +sharing service's evaluation context, tailor a message, or log it. A hook must +not make the access decision itself by testing it — privilege is judged by the +security service on the execution context (capability grants `permissions`, +placements `positions`, and the derived posture). A hook re-deciding access from +this array decides, somewhere with no access to the grant model, something that +was already decided; that is structurally the mistake the `roles` tombstone +exists to prevent, one vocabulary later. + +**`preserveAudit`** (`boolean`, optional) is the #3493 historical-import flag: +server-set, opt-in, absent on normal writes, and read by the built-in audit hook +to keep a caller-supplied `updated_at`/`updated_by` instead of stamping the +import instant. It has a live consumer, so it could only ever be declared. + +Purely additive — both keys are optional, the shape stays non-strict, and no +existing context, handler or stored metadata changes. Contexts are built per +operation and never persisted, so there is nothing to migrate. diff --git a/packages/spec/src/data/hook.test.ts b/packages/spec/src/data/hook.test.ts index c35ec0a8e1..09ae194d36 100644 --- a/packages/spec/src/data/hook.test.ts +++ b/packages/spec/src/data/hook.test.ts @@ -975,3 +975,170 @@ describe('session.roles retirement (#5050, ADR-0049)', () => { expect(built.userId).toBe('user_123'); }); }); + +/** + * `HookContext.session.positions` / `.preserveAudit` declaration (#5605, + * maintainer ruling A of 2026-08-06). + * + * The MIRROR of the retirement above, and the reason both blocks live in this + * file: `roles` was declared-never-produced (delete it), these two are + * produced-never-declared (declare them). Same `session` object, opposite + * drift, opposite fix — which is why #5605 was filed apart from #5050 rather + * than folded into it. + * + * What was actually broken before the declaration, on `origin/main`: + * + * - PARSE — `HookContextSchema` is deliberately non-strict (see the + * `hook.zod.ts` header), so both keys were silently STRIPPED. Measured + * before the change: parsing a session of + * `{ userId, positions, preserveAudit }` returned `{"userId":"u1"}`. That + * is not hypothetical: the generated reference page documents + * `HookContextSchema.parse(data)` as the way to consume a context, so a + * consumer following the docs dropped the caller's positions on the floor. + * - TSC — a handler typed the way `content/docs/automation/index.mdx` teaches, + * `(ctx: HookContext)`, could not read either key: two TS2339s, on + * `ctx.session?.positions` and `ctx.session?.preserveAudit`. The two + * `runtime-services` pages that teach `positions: ctx.session?.positions` + * only look fine because they annotate `ctx` as `any` — copy the code AND + * the documented annotation and it did not compile. + * + * REVERSE VERIFICATION, direction predicted first: delete either declaration + * from `hook.zod.ts` and this block fails BOTH ways — the parse assertions go + * red (the key is stripped, so `toHaveProperty` fails), and + * `pnpm --filter @objectstack/spec typecheck` reports TS2339 on the typed + * reads below. No `@ts-expect-error` is involved here and none should be + * added: the fact under test is that documented code COMPILES, so the pin is + * a positive typed read, and its failure mode on revert is a hard type error + * rather than an unused-directive TS2578. + * + * Boundary, restated because it is the whole reason the ruling needed a + * maintainer: `positions` is readable context, NOT an authorization input. + * The `.describe()` says so and the assertion below pins that it keeps saying + * so — the next author (or the next AI) reaching for + * `session.positions.includes(...)` as an access check is exactly what the + * `roles` tombstone above was written to stop. + */ +describe('session.positions / session.preserveAudit declaration (#5605)', () => { + it('PRESERVES `positions` through a parse instead of stripping it', () => { + const context = HookContextSchema.parse({ + object: 'account', + event: 'beforeUpdate', + input: {}, + session: { userId: 'user_123', positions: ['sales_manager', 'org_admin'] }, + ql: {}, + }); + + expect(context.session).toHaveProperty('positions'); + expect(context.session?.positions).toEqual(['sales_manager', 'org_admin']); + }); + + it('PRESERVES `preserveAudit` through a parse (#3493 has a live consumer)', () => { + const context = HookContextSchema.parse({ + object: 'account', + event: 'beforeInsert', + input: {}, + session: { userId: 'user_123', preserveAudit: true }, + ql: {}, + }); + + expect(context.session).toHaveProperty('preserveAudit'); + expect(context.session?.preserveAudit).toBe(true); + }); + + it('keeps both OPTIONAL — a normal write produces neither', () => { + // `buildSession()` writes `preserveAudit` only under the historical-import + // opt-in, and `positions` is absent whenever the execution context carried + // none. Declaring them must not start requiring them. + // + // ⚠️ HONEST NOTE — this one is a COMPANION, not a pin. It asserts absence, + // and absence is also what a stripped (undeclared) key produces, so it + // stayed green under the reverse verification while its four siblings went + // red. It is kept because "declaring them did not make them required" is a + // real regression it would catch (a missing `.optional()` turns it red), + // but it is not evidence that the declaration exists — do not read it as + // such. The pins are the two preserve tests, the `buildSession()` shape, + // and the tsc read below. + const context = HookContextSchema.parse({ + object: 'account', + event: 'beforeInsert', + input: {}, + session: { userId: 'user_123' }, + ql: {}, + }); + + expect(context.session?.positions).toBeUndefined(); + expect(context.session?.preserveAudit).toBeUndefined(); + }); + + it('accepts the exact session shape `buildSession()` builds', () => { + // Field-for-field the object ObjectQL assembles (engine.ts `buildSession`) + // for a historical import by an authenticated caller. Before #5605 this + // parse quietly returned a session two keys shorter than the one the + // engine handed the handler. + const built = { + userId: 'user_123', + organizationId: 'org_456', + positions: ['sales_manager'], + accessToken: 'token_abc123', + isSystem: true, + actor: 'svc:flow:import_history', + skipTriggers: true, + skipAutomations: true, + preserveAudit: true, + }; + + const context = HookContextSchema.parse({ + object: 'account', + event: 'beforeInsert', + input: {}, + session: built, + ql: {}, + }); + + expect(context.session).toEqual(built); + }); + + it('type-checks the code the docs teach — `(ctx: HookContext)` reading both keys', () => { + // The TSC channel. Both reads were TS2339 before the declaration; this is + // `content/docs/kernel/runtime-services/sharing-service.mdx`'s snippet with + // the `any` annotation removed, which is what made the omission invisible + // there. Explicit annotations, so a widened or renamed declaration fails + // here too rather than being absorbed by inference. + const readCallerContext = (ctx: HookContext) => { + const positions: string[] | undefined = ctx.session?.positions; + const preserveAudit: boolean | undefined = ctx.session?.preserveAudit; + return { positions, preserveAudit }; + }; + + // ...and the PRODUCER side: the literal `buildSession()` returns must be + // assignable to the declared session type. + const session: NonNullable = { + userId: 'user_123', + positions: ['sales_manager'], + preserveAudit: true, + }; + + expect(readCallerContext({ + object: 'account', + event: 'beforeUpdate', + input: {}, + session, + ql: {}, + })).toEqual({ positions: ['sales_manager'], preserveAudit: true }); + }); + + it('carries the "not an authorization input" boundary in the `.describe()`', () => { + // The ruling's wording is load-bearing, not decoration: it is the only + // thing standing between this key and the next author using it as an + // access check. A `.describe()` reaches the generated reference page and + // every schema-driven surface, so pin that the boundary survives edits. + const sessionShape = HookContextSchema.shape.session.unwrap().shape; + + const positionsDoc = sessionShape.positions.description ?? ''; + expect(positionsDoc).toMatch(/security service/i); + expect(positionsDoc).toMatch(/not an authorization input/i); + + const preserveAuditDoc = sessionShape.preserveAudit.description ?? ''; + expect(preserveAuditDoc).toMatch(/not an authorization input/i); + }); +}); diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index c010ba36a6..4962681183 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -405,6 +405,66 @@ export const HookContextSchema = lazySchema(() => z.object({ isSystem: z.boolean().optional().describe('True when the call was made with an elevated system context (engine self-writes)'), skipTriggers: z.boolean().optional().describe('True when record-change automation (flow triggers) must be suppressed for this write — e.g. package seed replay. Lifecycle hooks still run.'), skipAutomations: z.boolean().optional().describe('True when metadata-bound automation hooks must be suppressed for this write — e.g. data import with "run automations" unchecked, or import undo. Implies skipTriggers; code-registered system hooks (audit, security) still run.'), + /** + * Position names held by the caller (ADR-0090 D3 vocabulary — the schema + * comment on `ExecutionContext.positions` spells it "Formerly `roles`"). + * Copied verbatim from `ExecutionContext.positions` by ObjectQL's + * `buildSession()` (`packages/objectql/src/engine.ts`). + * + * ⚠️ **Descriptive, NOT an authorization input.** A hook may READ this to + * describe the caller — forwarding it as the sharing service's evaluation + * context (`services.sharing.canEdit(..., { positions })`, the shape both + * `content/docs/kernel/runtime-services/` pages teach), tailoring a + * message, logging — and nothing more. It grants nothing on its own, no + * security middleware keys on it here, and a hook must never make the + * access decision itself by testing it + * (`session.positions.includes('sales_manager')` is the anti-pattern). + * PRIVILEGE is judged by the security service on the ExecutionContext: + * capability grants (`permissions`), placements (`positions`) and the + * derived posture (ADR-0095 D3). A hook that re-decides access from this + * array is deciding, in a place with no access to the grant model, + * something already decided — structurally the same mistake as the + * `roles` tombstone below (#5050), one vocabulary later. That is why this + * key is declared with the boundary written down rather than left to be + * inferred from its name. + * + * Declared in #5605 (maintainer ruling A): it was PRODUCED by + * `buildSession()` and TAUGHT by two kernel doc pages while the contract + * omitted it — so `HookContextSchema.parse()` silently stripped it (this + * shape is deliberately non-strict, see the header) and a handler typed + * `(ctx: HookContext)` could not read it without TS2339. Produced-never- + * declared, the mirror of `roles`' declared-never-produced. + */ + positions: z.array(z.string()).optional().describe( + 'Position names held by the caller (ADR-0090 D3; formerly `roles`), copied from ' + + 'ExecutionContext.positions. For hook READS only — e.g. forwarding to the sharing ' + + 'service as evaluation context. Authorization is decided by the security service on ' + + 'the ExecutionContext (permissions / positions / derived posture); this is NOT an ' + + 'authorization input and a hook must not gate a write by testing it.', + ), + /** + * Historical-import audit-preservation flag (#3493). Set by + * `buildSession()` only when the write context carries it, so a normal + * write leaves it absent. + * + * Its one consumer is the built-in audit hook + * (`packages/objectql/src/plugin.ts`, `applyToRecord`): when true, a + * client-supplied `updated_at` / `updated_by` is PREFERRED and kept — + * reinstating the original timeline of imported history — instead of + * being overwritten with the import instant, symmetric with how + * `created_at` / `created_by` behave on insert. It also whitelists the + * audit/timestamp family through `stripReadonlyFields()`. + * + * Server-set and opt-in; like {@link positions} it authorizes nothing — + * it selects a stamping policy for a write the security service has + * already allowed. + */ + preserveAudit: z.boolean().optional().describe( + 'True when this write is a historical import that must KEEP its caller-supplied ' + + 'updated_at/updated_by (and the readonly audit family) instead of being stamped with ' + + 'the import instant (#3493). Server-set, opt-in, absent on normal writes; read by the ' + + 'built-in audit hook. A stamping policy, not an authorization input.', + ), // `roles` REMOVED (#5050, ADR-0049 D2). It was DECLARED here, READ by two // dead exemption branches in plugin-approvals (the approval record lock and // the delegation write guard, both deleted in #4839 / PR #5049), and NEVER