diff --git a/.changeset/shared-platform-row-org-resolver.md b/.changeset/shared-platform-row-org-resolver.md new file mode 100644 index 0000000000..6d451ccaa6 --- /dev/null +++ b/.changeset/shared-platform-row-org-resolver.md @@ -0,0 +1,15 @@ +--- +"@objectstack/metadata-core": minor +"@objectstack/plugin-approvals": patch +"@objectstack/service-automation": patch +"@objectstack/plugin-audit": patch +--- + +Promote `resolveRecordOrganizationField` to the shared platform-row organization resolver (the cloud#1395 Option A ruling): a platform row's organization is the SUBJECT record's organization; actor context is the fallback, never the primary. + +- `@objectstack/metadata-core` now owns the resolver (`resolveRecordOrganizationField`, `createFieldPresenceProbe`, and the new memoized `createRecordOrganizationResolver` factory) so all three sanctioned writers share one precedence. +- `@objectstack/plugin-approvals`: `openNodeRequest` stamps `sys_approval_request`, `sys_approval_action` and the `sys_approval_approver` index from the subject record's organization (acting context as fallback). Fixes the measured defect where every schedule / time-relative / api triggered approval persisted `organization_id = NULL` — locking the record it was about while being invisible in every inbox, its owner's included. +- `@objectstack/service-automation`: `sys_automation_run` rows (paused and terminal) resolve their organization from the trigger-record snapshot, with the acting tenant as fallback. Terminal rows previously never carried an organization at all. +- `@objectstack/plugin-audit`: the resolver moved out; the package re-exports it from the original paths, behavior unchanged. + +The `sys_api_key` divergence is preserved and pinned: `tenancy.organizationField` (who a row is ABOUT) still wins over the tenant wall answer, and the credential table stays unwalled. diff --git a/packages/metadata-core/src/index.ts b/packages/metadata-core/src/index.ts index 78e7a5be8d..2874f32fcb 100644 --- a/packages/metadata-core/src/index.ts +++ b/packages/metadata-core/src/index.ts @@ -81,3 +81,15 @@ export * from './item-key-discriminators.js'; // situation this package exists to resolve. `runtime` imports it from here now, // so its behaviour is unchanged and there is no second copy to drift. export * from './meta-write-org-scope.js'; + +// [#8707 / #10101] The shared platform-row organization resolver — sunk here +// from `@objectstack/plugin-audit` per the maintainer ruling recorded on +// cloud#1395 ("promoted to a shared resolver used by all three platform-row +// writers"). The three sanctioned consumers — audit stamping, the approval-row +// writer, the automation-run recorder — live in `plugin-audit`, +// `plugin-approvals` and `service-automation`, which share no other common +// home; this package's `{ @objectstack/spec, zod }`-only contract lets all +// three import ONE precedence instead of drifting a copy each. `plugin-audit` +// re-exports `createFieldPresenceProbe` from its original path, so its public +// surface is unchanged. +export * from './record-organization.js'; diff --git a/packages/metadata-core/src/record-organization.test.ts b/packages/metadata-core/src/record-organization.test.ts new file mode 100644 index 0000000000..b595c52e26 --- /dev/null +++ b/packages/metadata-core/src/record-organization.test.ts @@ -0,0 +1,171 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10101] Unit pins for the SHARED platform-row organization resolver — the + * cloud#1395 Option A ruling's artifact ("A platform row's organization is the + * SUBJECT record's organization; actor context is the fallback, never the + * primary"), promoted here from plugin-audit so audit stamping, the + * approval-row writer and the automation-run recorder share ONE precedence. + * + * The four-limb precedence is pinned per limb, and the `sys_api_key` + * divergence is pinned by name: `tenancy.organizationField` answers "which + * column says who this row is ABOUT", `tenantField`/`organization_id` answers + * "what is this object WALLED by", and the two DELIBERATELY diverge for + * credential tables (#8287). Flattening that divergence — resolving the stamp + * from the wall, or walling from the stamp — is the two-tables-disagree + * pathology this promotion exists to end. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { + createFieldPresenceProbe, + createRecordOrganizationResolver, + resolveRecordOrganizationField, +} from './record-organization.js'; + +/** Minimal engine double: `getSchema` over a name → definition map. */ +function engineOf(defs: Record) { + return { + getSchema: vi.fn((name: string) => defs[name]), + }; +} + +const hasFieldOf = (def: any) => (field: string) => + def?.fields != null && Object.prototype.hasOwnProperty.call(def.fields, field); + +describe('resolveRecordOrganizationField — the four-limb precedence', () => { + it('limb 0: a declared `tenancy.organizationField` wins over everything, the ADR-0066 opt-out included (sys_api_key)', () => { + // The shipped divergent case: an UNWALLED credential table + // (`enabled: false`) whose rows are still ABOUT one organization, under a + // column that deliberately is NOT the tenant column. + const def = { + name: 'sys_api_key', + tenancy: { enabled: false, organizationField: 'active_organization_id' }, + fields: { id: {}, name: {}, user_id: {}, active_organization_id: {}, revoked: {} }, + }; + expect(resolveRecordOrganizationField(def, hasFieldOf(def))).toBe('active_organization_id'); + }); + + it('limb 0 guard (#5315): a declared organizationField naming a MISSING column falls through, never resolves to nothing', () => { + // Missing column + disabled tenancy → limb 1 answers null (not the + // phantom name, and not organization_id either). + const def = { + name: 'sys_api_key', + tenancy: { enabled: false, organizationField: 'active_organization_id' }, + fields: { id: {}, organization_id: {} }, + }; + expect(resolveRecordOrganizationField(def, hasFieldOf(def))).toBeNull(); + }); + + it('limb 1: `tenancy.enabled === false` WITHOUT an organizationField resolves null even when an org FK exists (ADR-0066)', () => { + // The sys_sso_provider shape: platform-global, keeps an optional org FK, + // explicitly not tenant-scoped. Stamping from the FK would hide a global + // object's platform rows from the platform admin who acted. + const def = { + name: 'sys_sso_provider', + tenancy: { enabled: false }, + fields: { id: {}, organization_id: {} }, + }; + expect(resolveRecordOrganizationField(def, hasFieldOf(def))).toBeNull(); + }); + + it('limb 2: a declared `tenancy.tenantField` answers when present', () => { + const def = { + name: 'ws_doc', + tenancy: { enabled: true, tenantField: 'workspace_id' }, + fields: { id: {}, workspace_id: {}, organization_id: {} }, + }; + expect(resolveRecordOrganizationField(def, hasFieldOf(def))).toBe('workspace_id'); + }); + + it('limb 3: the canonical injected `organization_id` when nothing is declared', () => { + const def = { name: 'crm_deal', fields: { id: {}, organization_id: {} } }; + expect(resolveRecordOrganizationField(def, hasFieldOf(def))).toBe('organization_id'); + }); + + it('limb 4: no organization of its own → null (single-tenant shape)', () => { + const def = { name: 'crm_deal', fields: { id: {}, amount: {} } }; + expect(resolveRecordOrganizationField(def, hasFieldOf(def))).toBeNull(); + expect(resolveRecordOrganizationField(undefined, () => true)).toBeNull(); + expect(resolveRecordOrganizationField(null, () => true)).toBeNull(); + }); +}); + +describe('createFieldPresenceProbe', () => { + it('answers from the registered schema, map and array field shapes alike, memoized per object', () => { + const engine = engineOf({ + map_obj: { fields: { id: {}, organization_id: {} } }, + arr_obj: { fields: [{ name: 'id' }, { name: 'organization_id' }] }, + }); + const has = createFieldPresenceProbe(engine); + expect(has('map_obj', 'organization_id')).toBe(true); + expect(has('arr_obj', 'organization_id')).toBe(true); + expect(has('map_obj', 'missing')).toBe(false); + expect(has('nowhere', 'organization_id')).toBe(false); + has('map_obj', 'id'); + // one getSchema per object, not per question + expect(engine.getSchema.mock.calls.filter(([n]) => n === 'map_obj')).toHaveLength(1); + }); + + it('an engine with no getSchema reports every field absent (skip-the-stamp posture, never a throw)', () => { + const has = createFieldPresenceProbe({}); + expect(has('anything', 'organization_id')).toBe(false); + }); +}); + +describe('createRecordOrganizationResolver — the writers’ memoized face', () => { + it('organizationOf reads the resolved column off the first candidate record that carries a non-empty value', () => { + const engine = engineOf({ crm_deal: { fields: { id: {}, organization_id: {} } } }); + const r = createRecordOrganizationResolver(engine); + expect(r.organizationFieldFor('crm_deal')).toBe('organization_id'); + expect(r.organizationOf('crm_deal', { id: 'd1', organization_id: 'org_A' })).toBe('org_A'); + // precedence across candidates: first non-empty wins (live record before + // trigger snapshot, result before prior state — the callers' order) + expect( + r.organizationOf('crm_deal', { id: 'd1', organization_id: '' }, { id: 'd1', organization_id: 'org_B' }), + ).toBe('org_B'); + expect(r.organizationOf('crm_deal', undefined, null, { id: 'd1' })).toBeNull(); + }); + + it('pins the sys_api_key divergence end to end: the stamp column is active_organization_id, never the wall', () => { + const engine = engineOf({ + sys_api_key: { + tenancy: { enabled: false, organizationField: 'active_organization_id' }, + fields: { id: {}, name: {}, user_id: {}, active_organization_id: {}, revoked: {} }, + }, + }); + const r = createRecordOrganizationResolver(engine); + expect(r.organizationFieldFor('sys_api_key')).toBe('active_organization_id'); + expect( + r.organizationOf('sys_api_key', { id: 'k1', active_organization_id: 'org_key' }), + ).toBe('org_key'); + // A record carrying an `organization_id` VALUE anyway (defensive noise) + // still stamps from the DECLARED column, not the canonical spelling. + expect( + r.organizationOf('sys_api_key', { id: 'k1', organization_id: 'org_wrong', active_organization_id: 'org_key' }), + ).toBe('org_key'); + }); + + it('degrades to null — the acting-context fallback signal — on a getSchema-less double, a throwing getSchema, and an unknown object', () => { + expect(createRecordOrganizationResolver({}).organizationOf('crm_deal', { organization_id: 'org_A' })).toBeNull(); + const throwing = { getSchema: () => { throw new Error('not booted'); } }; + expect(createRecordOrganizationResolver(throwing).organizationOf('crm_deal', { organization_id: 'org_A' })).toBeNull(); + const empty = engineOf({}); + expect(createRecordOrganizationResolver(empty).organizationOf('crm_deal', { organization_id: 'org_A' })).toBeNull(); + }); + + it('memoizes the column per object (one schema read for N writes)', () => { + const engine = engineOf({ crm_deal: { fields: { id: {}, organization_id: {} } } }); + const r = createRecordOrganizationResolver(engine); + r.organizationOf('crm_deal', { organization_id: 'a' }); + r.organizationOf('crm_deal', { organization_id: 'b' }); + r.organizationFieldFor('crm_deal'); + // one call from the probe's field-set read + one from the column + // resolution — and no growth with further questions + const calls = engine.getSchema.mock.calls.filter(([n]) => n === 'crm_deal').length; + r.organizationOf('crm_deal', { organization_id: 'c' }); + expect(engine.getSchema.mock.calls.filter(([n]) => n === 'crm_deal').length).toBe(calls); + expect(calls).toBeLessThanOrEqual(2); + }); +}); diff --git a/packages/metadata-core/src/record-organization.ts b/packages/metadata-core/src/record-organization.ts new file mode 100644 index 0000000000..b95c0bcc9e --- /dev/null +++ b/packages/metadata-core/src/record-organization.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8707 / #10101] The shared platform-row organization resolver — "which + * column carries THIS object's own organization?", resolved from the object's + * REGISTERED schema, never hard-coded to one spelling. + * + * Sunk here from `@objectstack/plugin-audit` (#10101) by the same criterion as + * the engine dispatch predicates (#5619) and the metadata-plane FLS projection + * (ADR-0106): the consumers live in three packages that share no other common + * home (`plugin-audit`, `plugin-approvals`, `service-automation`), and + * `@objectstack/metadata-core` depends on `{ @objectstack/spec, zod }` only, + * so all three can import it with no new edge and no cycle. A per-writer copy + * of this resolution is precisely the disease the promotion ruling exists to + * end — two platform tables answering "whose row is this?" two ways. + * + * ## The ruling this promotion implements (maintainer, 2026-08-17, cloud#1395) + * + * > Ruled: Option A — extend the #8778 ruling: `resolveRecordOrganizationField` + * > is promoted to a shared resolver used by all three platform-row writers + * > (approvals, automation runs, audit). A platform row's organization is the + * > SUBJECT record's organization; actor context is the fallback, never the + * > primary. + * + * ⛔ The `tenancy.organizationField` key this resolver reads stays scope-pinned + * (#8778, widened by name on cloud#1395 — the annotation beside the key in + * `packages/spec/src/data/object.zod.ts` transcribes the ruling): exactly THREE + * consumers are sanctioned — audit stamping, the approval-row writer, and the + * automation-run recorder — and no others. A fourth consumer needs its own + * maintainer ruling before reading the key, exactly as #8778 required. Sharing + * the implementation here does not open the key: it closes the excuse for a + * fourth copy. + * + * A platform row is stamped from the organization the record is ABOUT (#8287's + * ruling). To do that the writer has to know which column holds it, and + * `organization_id` is not universally the answer: `sys_api_key` carries + * `active_organization_id` by deliberate design (#8287). Adding a second + * literal name beside the first would make a writer correct for exactly two + * objects and silently wrong for the third, so the question is asked of the + * schema instead. + */ + +import { isTenancyDisabled } from '@objectstack/spec/data'; +import { SystemFieldName } from '@objectstack/spec/system'; + +/** + * "Does this object's REGISTERED schema declare this field?", memoized per + * object. + * + * Extracted to module scope (#8144), and sunk here from plugin-audit's + * `audit-writers.ts` (#10101), so every consumer asks the question ONE way. The + * audit CRUD writer and the auth-event writer stamp the same two conditional + * columns on the same table, and a second hand-rolled probe would answer + * differently on the day one of them is fixed. + * + * Why the probe exists at all: the SchemaRegistry auto-injects + * `organization_id` only in multi-tenant mode (`applySystemFields({ + * multiTenant })`), so on single-tenant stacks the `sys_audit_log` / + * `sys_activity` tables have no such column. Unconditionally stamping it there + * made every audit INSERT fail with "table sys_audit_log has no column named + * organization_id" — and the error was swallowed, so audit logging was silently + * non-functional. Resolve the field set lazily from the engine schema and cache + * it; object schemas are static after registration. + * + * Best-effort in both directions: an engine with no `getSchema` (an in-memory + * test double) reports every field absent, which skips the stamp rather than + * failing the write. + */ +export function createFieldPresenceProbe( + engine: unknown, +): (objectName: string, field: string) => boolean { + const fieldSetCache = new Map | null>(); + return (objectName: string, field: string): boolean => { + let set = fieldSetCache.get(objectName); + if (set === undefined) { + set = null; + try { + const schema: any = + typeof (engine as any)?.getSchema === 'function' ? (engine as any).getSchema(objectName) : null; + const fields = schema?.fields; + if (fields && typeof fields === 'object' && !Array.isArray(fields)) { + set = new Set(Object.keys(fields)); + } else if (Array.isArray(fields)) { + set = new Set(fields.map((f: any) => f?.name).filter(Boolean)); + } + } catch { + /* ignore — best-effort; absence just means we skip the stamp */ + } + fieldSetCache.set(objectName, set); + } + return set != null && set.has(field); + }; +} + +/** + * [#8707] "Which column carries THIS object's own organization?" — resolved + * from the object's REGISTERED schema, never hard-coded to one spelling. + * + * ## Precedence — deliberately the platform's own, not a second opinion + * + * It mirrors `SqlDriver.computeTenantField` step for step, because that is the + * platform's single existing answer to "which column is this object + * tenant-scoped by", and a platform row's stamp must agree with the wall the + * row will later be read through. Re-derived here rather than imported: that + * method is `protected` on a DRIVER class, and this package takes no driver + * dependency (its contract is `@objectstack/spec` + zod only). The two shared + * inputs ARE imported — `isTenancyDisabled` (ADR-0066's single source of truth + * for the opt-out) and `SystemFieldName.ORGANIZATION_ID` — so the parts that + * could drift are one definition, and only the ordering is restated. + * + * 0. **Declared `tenancy.organizationField`, when the object really has that + * field.** The read-neutral, STAMP-ONLY declaration #8778's ruling added + * for exactly this consumer (option A; #8707's remaining half). It + * answers "which column says who this row is ABOUT" — a different + * question from "what is this object walled by", which is why it wins + * over every limb below, the ADR-0066 opt-out included: an author who + * declares it on an unwalled object (`sys_api_key`, `enabled: false` by + * necessity — the credential table must never be org-walled, #8287) is + * stating precisely that the trail should follow the record's own + * organization even though no wall does. Honoured only when the field is + * really present, same #5315 guard as limb 2. ⛔ Stamp-only cuts both + * ways: the key's consumers are pinned to the THREE platform-row writers + * the cloud#1395 ruling names (audit, approvals, automation runs) — a + * fourth consumer, or any read path, needs its own ruling first. + * 1. **`tenancy.enabled === false` → `null`.** ADR-0066 platform-global + * objects (`sys_sso_provider` is the shipped example) keep an optional org + * FK while explicitly NOT being tenant-scoped. Stamping a platform row from + * that FK would scope a global object's trail into one organization + * and hide it from the platform admin who acted — strictly LESS visible + * than today. This limb is what keeps the precedence flip from trading one + * invisibility for another; it is not an optimisation. + * 2. **Declared `tenancy.tenantField`, when the object really has that + * field.** The spec key already exists for "this object's tenant column + * genuinely is not the platform's" and the driver already honours it, so an + * object that declares one gets its platform rows stamped from the same + * column its rows are walled by. Honoured only when the field is really + * present — the same guard `computeTenantField` applies, for the same + * reason (#5315: a declared name pointing at a missing column must fall + * through, not resolve to nothing). + * 3. **The canonical injected `organization_id`, when present.** What every + * multi-tenant object gets from `applySystemFields`. + * 4. Otherwise `null` — the object has no organization of its own, and the + * caller falls back to the acting session's tenant exactly as before. + * + * ## What it deliberately does NOT do + * + * ⛔ It does not scan for "a lookup whose `reference` is `sys_organization`". + * That derivation is FALSIFIED by a shipped object: `sys_organization` itself + * declares no `organization_id` and exactly one such lookup — + * `parent_organization_id` — so the scan would stamp every organization's audit + * rows with its PARENT's id, hiding them from the very tenant they concern. + * Worse, reading `parent_organization_id` for a visibility decision is an + * ADR-0105 D6 red line that `validateOrgAxisRedLines` (@objectstack/lint) makes + * a build error for RLS policies, sharing rules and scopes; a plugin reaching + * the same conclusion through a heuristic is the same mistake with no gate on + * it. + * + * `sys_api_key.active_organization_id` is reachable through limb 0 since + * #8778 (it was the object that motivated the key). Its column is still not — + * and must never become — the object's tenant-scope column: + * `tenancy.tenantField` feeds `applyTenantScope` / `injectTenantOnInsert`, so + * declaring it there would wall the credential table on an equality that + * excludes NULL — every pre-#8287 key would vanish from its own owner's + * list, which is the defect #8287 exists to have removed. + * + * @param objectDef the registered object definition (`engine.getSchema(name)`) + * @param hasField the memoized field-presence probe for the SAME object — the + * platform asks "does the schema declare this field?" exactly one way + * ({@link createFieldPresenceProbe}), and a second hand-rolled shape check + * here would answer differently on the day one of them is fixed. + */ +export function resolveRecordOrganizationField( + objectDef: unknown, + hasField: (field: string) => boolean, +): string | null { + if (!objectDef || typeof objectDef !== 'object') return null; + const tenancy = (objectDef as { tenancy?: { organizationField?: unknown; tenantField?: unknown } }).tenancy; + // Limb 0 — the explicit stamp-only declaration (#8778) wins over everything, + // the ADR-0066 opt-out below included: see the precedence doc above. + const stampField = tenancy?.organizationField; + if (typeof stampField === 'string' && stampField.length > 0 && hasField(stampField)) return stampField; + if (isTenancyDisabled(objectDef)) return null; + const declared = tenancy?.tenantField; + if (typeof declared === 'string' && declared.length > 0 && hasField(declared)) return declared; + if (hasField(SystemFieldName.ORGANIZATION_ID)) return SystemFieldName.ORGANIZATION_ID; + return null; +} + +/** + * The memoized, engine-bound face of {@link resolveRecordOrganizationField} — + * what a platform-row WRITER actually holds. One instance per engine wraps the + * column resolution (memoized per object; object schemas are static after + * registration) and the value read, so the three sanctioned writers share the + * glue as well as the precedence: a per-writer copy of "read the resolved + * column off the record, treating empty as absent" is where the next drift + * starts. + * + * `organizationOf` reads the resolved column off each candidate record in + * order and returns the first non-empty string — the SUBJECT record's own + * organization. It answers `null` when the object has no organization of its + * own, when no candidate carries a value, or when the engine exposes no + * `getSchema` (an in-memory test double): in every one of those cases the + * caller falls back to the acting context, which is the ruled fallback — never + * the primary. + */ +export interface RecordOrganizationResolver { + /** Memoized column answer for one object; `null` = no organization of its own. */ + organizationFieldFor(objectName: string): string | null; + /** First non-empty value of the resolved column across `records`, else `null`. */ + organizationOf(objectName: string, ...records: Array): string | null; +} + +/** + * Build a {@link RecordOrganizationResolver} over an engine-like object. The + * `engine` is probed structurally for `getSchema(objectName)` — the same + * best-effort posture as {@link createFieldPresenceProbe}, and deliberately so: + * a double without `getSchema` resolves nothing, so writers keep their acting- + * context fallback instead of failing the write. + */ +export function createRecordOrganizationResolver(engine: unknown): RecordOrganizationResolver { + const hasField = createFieldPresenceProbe(engine); + const columnCache = new Map(); + const organizationFieldFor = (objectName: string): string | null => { + const hit = columnCache.get(objectName); + if (hit !== undefined) return hit; + let objectDef: unknown = null; + try { + objectDef = + typeof (engine as any)?.getSchema === 'function' ? (engine as any).getSchema(objectName) : null; + } catch { + /* ignore — best-effort; absence just means the caller falls back */ + } + const resolved = resolveRecordOrganizationField(objectDef, (field) => hasField(objectName, field)); + columnCache.set(objectName, resolved); + return resolved; + }; + const organizationOf = (objectName: string, ...records: Array): string | null => { + const column = organizationFieldFor(objectName); + if (!column) return null; + for (const record of records) { + if (!record || typeof record !== 'object') continue; + const value = (record as Record)[column]; + if (typeof value === 'string' && value.length > 0) return value; + } + return null; + }; + return { organizationFieldFor, organizationOf }; +} diff --git a/packages/plugins/plugin-approvals/src/approval-node.test.ts b/packages/plugins/plugin-approvals/src/approval-node.test.ts index 31620a12aa..01c9740e68 100644 --- a/packages/plugins/plugin-approvals/src/approval-node.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-node.test.ts @@ -15,7 +15,14 @@ const noopLogger = { * Tiny in-memory ObjectQL stand-in — supports the `where`-equality + `$in` * queries the approval service issues, enough to drive the node bridge. */ -function makeFakeEngine() { +function makeFakeEngine( + // [#10101] Optional name → object-definition map. When given, the fake + // grows the `getSchema` the shared platform-row organization resolver + // probes for; when omitted (every pre-#10101 caller) the fake has no + // schema access and `openNodeRequest` keeps the acting-context fallback — + // the documented degradation, exercised below rather than assumed. + schemas?: Record, +) { const tables = new Map(); const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!)); const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => { @@ -26,6 +33,7 @@ function makeFakeEngine() { }); return { tables, + ...(schemas ? { getSchema: (name: string) => schemas[name] } : {}), async find(object: string, opts: any = {}) { const where = opts.where ?? opts.filter ?? {}; let out = rows(object).filter(r => matches(r, where)); @@ -355,3 +363,135 @@ describe('Approval node bridge (ADR-0019)', () => { expect(await fake.find('sys_approval_request', {})).toHaveLength(0); }); }); + +// ─── Organization attribution on the request (cloud#1395, #10101) ──────────── +// +// The cloud#1395 Option A ruling, implemented by #10101: a platform row's +// organization is the SUBJECT record's organization; actor context is the +// fallback, never the primary. `openNodeRequest` resolves it through the +// SHARED platform-row resolver (`@objectstack/metadata-core`) — the same +// precedence `sys_audit_log`'s writer stamps with — and the one resolved value +// feeds the request row, `sys_approval_action`, and the `sys_approval_approver` +// index ("all three move together"). +// +// Why it matters (measured on cloud#1395, over HTTP): an approval opened with +// no acting organization LOCKED the record it was about while being invisible +// in every inbox, its owner's included — `buildRequestWhere` matches the wall +// by strict equality, so under a SYSTEM context the owner's match failed by +// the same failure as a stranger's. Every schedule / time-relative / api +// trigger produces exactly that context, by construction. +describe('openNodeRequest — organization attribution (cloud#1395, #10101)', () => { + const DEAL_SCHEMA = { crm_deal: { fields: { id: {}, amount: {}, organization_id: {} } } }; + const openInput = (over: Record = {}) => ({ + object: 'crm_deal', recordId: 'd1', runId: 'run_1', nodeId: 'approve_step', + flowName: 'deal_approval', + config: { approvers: [{ type: 'user' as const, value: 'u9' }], behavior: 'first_response' as const }, + record: { id: 'd1', amount: 100 }, + ...over, + }); + + it('stamps the SUBJECT record’s organization on the request, the action row AND the approver index — actor context present but not primary', async () => { + const fake = makeFakeEngine(DEAL_SCHEMA); + fake.tables.set('crm_deal', [{ id: 'd1', amount: 100, organization_id: 'org_subject' }]); + const svc = new ApprovalService({ engine: fake as any, logger: noopLogger }); + + await svc.openNodeRequest(openInput() as any, { userId: 'u1', tenantId: 'org_actor' } as any); + + const [req] = await fake.find('sys_approval_request', {}); + expect(req.organization_id, 'subject first — the cloud#1395 Option A ruling').toBe('org_subject'); + // Both directions pinned: the actor's organization is NOT what landed. + expect(req.organization_id).not.toBe('org_actor'); + const actions = await fake.find('sys_approval_action', {}); + expect(actions.length).toBeGreaterThan(0); + for (const a of actions) expect(a.organization_id).toBe('org_subject'); + const approvers = await fake.find('sys_approval_approver', {}); + expect(approvers.length).toBeGreaterThan(0); + for (const a of approvers) expect(a.organization_id).toBe('org_subject'); + }); + + it('a tenant-less open (the schedule / api trigger shape) resolves the subject organization instead of persisting NULL', async () => { + const fake = makeFakeEngine(DEAL_SCHEMA); + fake.tables.set('crm_deal', [{ id: 'd1', amount: 100, organization_id: 'org_subject' }]); + const svc = new ApprovalService({ engine: fake as any, logger: noopLogger }); + + // The measured defect's context: a flow run with no acting organization. + await svc.openNodeRequest(openInput() as any, { isSystem: true } as any); + + const [req] = await fake.find('sys_approval_request', {}); + expect(req.organization_id).toBe('org_subject'); + }); + + it('the live record is the first source; the trigger snapshot answers when the live read finds nothing', async () => { + const fake = makeFakeEngine(DEAL_SCHEMA); + // No crm_deal row seeded — loadLiveRecord falls back to input.record. + const svc = new ApprovalService({ engine: fake as any, logger: noopLogger }); + + await svc.openNodeRequest( + openInput({ record: { id: 'd1', amount: 100, organization_id: 'org_snapshot' } }) as any, + { userId: 'u1', tenantId: 'org_actor' } as any, + ); + const [req] = await fake.find('sys_approval_request', {}); + expect(req.organization_id).toBe('org_snapshot'); + }); + + it('the acting-context fallback still stands — pinned in BOTH directions (subject unresolvable ⇒ actor answers)', async () => { + // ① the object has no organization column of its own + const noColumn = makeFakeEngine({ crm_deal: { fields: { id: {}, amount: {} } } }); + noColumn.tables.set('crm_deal', [{ id: 'd1', amount: 100 }]); + const svc1 = new ApprovalService({ engine: noColumn as any, logger: noopLogger }); + await svc1.openNodeRequest(openInput() as any, { userId: 'u1', tenantId: 'org_actor' } as any); + expect((await noColumn.find('sys_approval_request', {}))[0].organization_id).toBe('org_actor'); + + // ② an engine double with no getSchema at all (the pre-#10101 shape) + const noSchema = makeFakeEngine(); + noSchema.tables.set('crm_deal', [{ id: 'd1', amount: 100, organization_id: 'org_subject' }]); + const svc2 = new ApprovalService({ engine: noSchema as any, logger: noopLogger }); + await svc2.openNodeRequest(openInput() as any, { userId: 'u1', tenantId: 'org_actor' } as any); + expect((await noSchema.find('sys_approval_request', {}))[0].organization_id).toBe('org_actor'); + + // ③ tenant-less AND subject-less stays NULL — fabricating an acting + // organization stays vetoed (cloud#1395 Option C) + const neither = makeFakeEngine({ crm_deal: { fields: { id: {}, amount: {} } } }); + neither.tables.set('crm_deal', [{ id: 'd1', amount: 100 }]); + const svc3 = new ApprovalService({ engine: neither as any, logger: noopLogger }); + await svc3.openNodeRequest(openInput() as any, { isSystem: true } as any); + expect((await neither.find('sys_approval_request', {}))[0].organization_id).toBeNull(); + }); + + it('⛔ pins the sys_api_key divergence: stamps the DECLARED active_organization_id, and never treats an ADR-0066 org FK as the stamp', async () => { + // The credential table (#8287/#8778): unwalled by necessity, rows still + // ABOUT one organization under `tenancy.organizationField` — limb 0 wins + // over the disabled-tenancy opt-out. + const apiKey = makeFakeEngine({ + sys_api_key: { + tenancy: { enabled: false, organizationField: 'active_organization_id' }, + fields: { id: {}, name: {}, user_id: {}, active_organization_id: {}, revoked: {} }, + }, + }); + apiKey.tables.set('sys_api_key', [{ id: 'key1', name: 'ci', active_organization_id: 'org_key' }]); + const svc = new ApprovalService({ engine: apiKey as any, logger: noopLogger }); + await svc.openNodeRequest( + openInput({ object: 'sys_api_key', recordId: 'key1', record: { id: 'key1' } }) as any, + { userId: 'u1', tenantId: 'org_actor' } as any, + ); + expect((await apiKey.find('sys_approval_request', {}))[0].organization_id).toBe('org_key'); + + // The other half of not flattening ABOUT vs WALLED-BY: a platform-global + // object (ADR-0066, `enabled: false`, NO organizationField) keeps its org + // FK out of the stamp — the acting context answers, exactly as the audit + // writer's limb 1 does. + const globalObj = makeFakeEngine({ + sys_sso_provider: { + tenancy: { enabled: false }, + fields: { id: {}, organization_id: {} }, + }, + }); + globalObj.tables.set('sys_sso_provider', [{ id: 'sso1', organization_id: 'org_fk' }]); + const svc2 = new ApprovalService({ engine: globalObj as any, logger: noopLogger }); + await svc2.openNodeRequest( + openInput({ object: 'sys_sso_provider', recordId: 'sso1', record: { id: 'sso1' } }) as any, + { userId: 'u1', tenantId: 'org_actor' } as any, + ); + expect((await globalObj.find('sys_approval_request', {}))[0].organization_id).toBe('org_actor'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 831dbfd659..4396ac0ba8 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -10,6 +10,14 @@ import { type ApprovalNodeConfig, } from '@objectstack/spec/automation'; import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formula'; +// [#10101] The SHARED platform-row organization resolver — the cloud#1395 +// ruling ("A platform row's organization is the SUBJECT record's organization; +// actor context is the fallback, never the primary"), implemented once in +// `@objectstack/metadata-core` and consumed by all three sanctioned writers +// (audit stamping, this approval-row writer, the automation-run recorder). A +// writer-local re-derivation here was rejected by name (Option B): it would be +// a third answer to a question the codebase already answered two ways. +import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core'; import { keysetWalk } from '@objectstack/types'; import { ADMIN_FULL_ACCESS, @@ -75,6 +83,15 @@ export interface ApprovalEngine { insert(object: string, data: any, options?: any): Promise; update(object: string, idOrData: any, dataOrOptions?: any, options?: any): Promise; delete(object: string, options?: any): Promise; + /** + * [#10101] Registered object definition for a name — what the shared + * platform-row organization resolver (`@objectstack/metadata-core`) reads to + * answer "which column carries this object's own organization". Optional so + * an in-memory test double without it degrades to the acting-context + * fallback rather than failing the write (the same best-effort posture the + * resolver itself takes). + */ + getSchema?(objectName: string): unknown; } export interface ApprovalClock { now(): Date } @@ -598,9 +615,18 @@ export class ApprovalService implements IApprovalService { * deployment gets on upgrade. */ private readonly recordReaderVisibleObjects: ReadonlySet; + /** + * [#10101] Memoized shared platform-row organization resolver over + * {@link ApprovalService.engine} — answers "which column carries the SUBJECT + * object's own organization, and what does this record hold there". One + * instance for the service's lifetime: object schemas are static after + * registration, and the audit writer memoizes the same way. + */ + private readonly recordOrgResolver: RecordOrganizationResolver; constructor(opts: ApprovalServiceOptions) { this.engine = opts.engine; + this.recordOrgResolver = createRecordOrganizationResolver(opts.engine); this.clock = opts.clock ?? { now: () => new Date() }; this.logger = opts.logger; this.automation = opts.automation; @@ -1956,6 +1982,29 @@ export class ApprovalService implements IApprovalService { // `organizationId` is not on the envelope — see isOverrideActor(). const ctxOrg = (context as any)?.organizationId ?? context?.tenantId ?? input.organizationId ?? null; const nowDate = this.clock.now(); + // [#10101, the cloud#1395 Option A ruling] The request's organization is + // the SUBJECT record's organization; the acting context is the fallback, + // never the primary. Resolved through the SHARED platform-row resolver + // (`@objectstack/metadata-core`) — the same precedence the audit writer + // stamps `sys_audit_log` with, so an approval row and an audit row about + // the same record land behind the same wall (`sys_api_key`'s divergent + // `active_organization_id` included, via `tenancy.organizationField`). + // + // Before this, `ctxOrg` alone stamped the row: NULL on every schedule / + // time-relative / api triggered flow (none carries an acting tenant, by + // construction), which produced the measured cloud#1395 defect — a pending + // request that LOCKS the record it is about while being invisible in every + // inbox, its owner's included (`buildRequestWhere` matches the wall by + // strict equality, so the owner fails exactly as a stranger does). + // + // The fallback is unchanged and still load-bearing: an object with no + // organization of its own (single-tenant stacks, ADR-0066 platform-global + // objects) resolves `null` here and keeps the acting context's answer. + // `requestOrg` feeds everything that means "this request's organization" — + // the request row, `sys_approval_action`, the `sys_approval_approver` + // index, AND the approver-slate expansion below: a slate resolved in a + // different organization than the wall the request lands behind would be + // approvers who cannot see the request they are asked to decide. // OOO auto-skip (#1322 M1): reroute individually-routed approvers who are // out of office. Collected hops drive the audit + notification below (M4). const substitutions: OooSubstitution[] = []; @@ -1967,9 +2016,13 @@ export class ApprovalService implements IApprovalService { // the trigger snapshot carried in `input.record`. This is the whole fix — an // earlier step may have written the field this node routes on. const liveRecord = await this.loadLiveRecord(input.object, input.recordId, input.record); + // Live state first, trigger snapshot second — the same precedence the + // approver expansion just below applies to the record itself (#3447). + const subjectOrg = this.recordOrgResolver.organizationOf(input.object, liveRecord, input.record); + const requestOrg = subjectOrg ?? ctxOrg; const resolvedFrom: Record = {}; const approvers = await this.expandApprovers( - { approvers: input.config.approvers }, liveRecord, ctxOrg, { + { approvers: input.config.approvers }, liveRecord, requestOrg, { now: nowDate.getTime(), substitutions, groups, exprCtx: { trigger: input.record ?? null, vars: input.variables ?? null }, resolvedFrom, @@ -2057,14 +2110,14 @@ export class ApprovalService implements IApprovalService { flow_run_id: input.runId, flow_node_id: input.nodeId, node_config_json: JSON.stringify(configSnapshot), - organization_id: ctxOrg, + organization_id: requestOrg, created_at: now, updated_at: now, }; await this.engine.insert('sys_approval_request', row, { context: SYSTEM_CTX }); - await this.syncApproverIndex(id, approvers, ctxOrg, now); + await this.syncApproverIndex(id, approvers, requestOrg, now); await this.engine.insert('sys_approval_action', { - id: uid('aact'), request_id: id, organization_id: ctxOrg, + id: uid('aact'), request_id: id, organization_id: requestOrg, step_name: input.nodeId, step_index: 0, action: 'submit', actor_id: input.submitterId ?? context.userId ?? null, comment: null, created_at: now, }, { context: SYSTEM_CTX }); @@ -2075,7 +2128,7 @@ export class ApprovalService implements IApprovalService { // delegate — who now owns the slot — and the skipped approver. for (const sub of substitutions) { await this.engine.insert('sys_approval_action', { - id: uid('aact'), request_id: id, organization_id: ctxOrg, + id: uid('aact'), request_id: id, organization_id: requestOrg, step_name: input.nodeId, step_index: 0, action: 'ooo_substitute', actor_id: null, comment: `${sub.from} → ${sub.to}${sub.reason ? ` — ${sub.reason}` : ''}`, diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts index a6d181bb1f..07bda77b12 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts @@ -87,46 +87,40 @@ export const SysApprovalRequest = ObjectSchema.create({ fields: { id: Field.text({ label: 'Request ID', required: true, readonly: true, group: 'System' }), - // ⚠️ MEASURED DEFECT, cloud#1395 — read this before trusting the column. + // [#10101, the cloud#1395 Option A ruling] The SUBJECT record's + // organization, with the acting context as fallback — resolved by the + // SHARED platform-row resolver (`resolveRecordOrganizationField`, + // `@objectstack/metadata-core`) in `openNodeRequest`, the row's only + // writer. The same `requestOrg` stamps `sys_approval_action` and the + // `sys_approval_approver` index, so all three move together. // - // An approval request DOES belong to an organization: it is read through the - // organization wall by the approvals inbox, and on a shared-database - // deployment a row carrying no organization is not filtered BY that wall — - // it is either invisible to everyone or visible to everyone, decided by - // whatever filter each surface happens to apply rather than by the data. + // Why subject-first: an approval request is read through the organization + // wall by the approvals inbox, and the acting context is NULL on every + // schedule / time-relative / api triggered run (none carries a tenant, by + // construction). Stamped from the actor alone — the pre-#10101 behaviour, + // measured on cloud#1395 as 27 of 27 rows org-less on a walled HotCRM SaaS + // boot — such a request LOCKED the record it was about while being + // invisible in every inbox, its owner's included. Subject-first is also + // what `sys_audit_log`'s writer already did (#8707 honouring #8287's + // ruling), so an approval row and an audit row about the same record now + // land behind the same wall instead of two. // - // The value is resolved from the ACTING CONTEXT only (`openNodeRequest`'s - // `ctxOrg`), so it is NULL whenever the flow that opened the request ran - // without one — every schedule / time-relative / api triggered run, none of - // which sets a tenant. On a walled single-database HotCRM SaaS boot this - // measured 27 of 27 rows org-less, each naming an `object_name` / - // `record_id` owned by a specific customer. - // - // ⛔ Do NOT read that as "platform tables do not carry an organization". - // `sys_audit_log` (1669 rows) was correctly attributed on the SAME boot, - // because its writer takes the organization from the RECORD the row is - // about, with the session only as fallback (plugin-audit - // `resolveRecordOrganizationField`, #8707 honouring #8287's ruling). Two - // writers read the actor; a third reads the subject. That disagreement is - // the defect. - // - // Which of the two a side-table row should follow is an open contract - // question on cloud#1395 — the audit resolver is scope-pinned to audit - // stamping by the #8778 ruling, so this writer needs its own. The same - // `ctxOrg` also stamps `sys_approval_action` and `sys_approval_approver`, - // so all three move together. + // The `sys_api_key` divergence is deliberate and preserved: its + // `tenancy.organizationField: 'active_organization_id'` (stamp-only, + // #8778) wins limb 0 of the shared resolver, while the credential table + // itself stays unwalled (`tenancy.enabled: false`) — who a row is ABOUT + // and what an object is WALLED by remain different questions. organization_id: Field.lookup('sys_organization', { label: 'Organization', required: false, group: 'System', - // ⛔ String unchanged on purpose: it is extracted into the generated i18n - // bundles (`translations/*.objects.generated.ts`, as `help`), so rewording - // it is a translation-regeneration change and not a comment. The - // correction it needs — it claims a propagation that measurably does not - // happen, and says "Tenant" where ADR-0120 §Terminology requires - // "organization" — rides the cloud#1395 write-side fix, which rewrites the - // sentence and regenerates the four locales in one pass. - description: 'Tenant that owns this approval request (propagated from submitter context)', + // Reworded with the #10101 write-side fix (was "Tenant that owns this + // approval request (propagated from submitter context)" — it claimed a + // propagation that measurably did not happen, and said "Tenant" where + // ADR-0120 §Terminology requires "organization"). Extracted into the + // generated i18n bundles as `help`; the four locales regenerate in the + // same pass. + description: 'Organization of the record this request is about (falls back to the acting context when the record has none)', }), process_name: Field.text({ diff --git a/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts index 560da1570d..3e64d45594 100644 --- a/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts @@ -25,7 +25,7 @@ export const enObjects: NonNullable = { }, organization_id: { label: "Organization", - help: "Tenant that owns this approval request (propagated from submitter context)" + help: "Organization of the record this request is about (falls back to the acting context when the record has none)" }, process_name: { label: "Source", diff --git a/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts index f7a7399a37..890c99a9dd 100644 --- a/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts @@ -25,7 +25,7 @@ export const esESObjects: NonNullable = { }, organization_id: { label: "Organización", - help: "Tenant que posee esta solicitud de aprobación (propagado desde el contexto del solicitante)." + help: "Organización del registro al que se refiere esta solicitud (recurre al contexto del actor cuando el registro no tiene ninguna)." }, process_name: { label: "Origen", diff --git a/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts index 0428500655..6a608a9b58 100644 --- a/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts @@ -25,7 +25,7 @@ export const jaJPObjects: NonNullable = { }, organization_id: { label: "組織", - help: "この承認リクエストを所有するテナント（送信者コンテキストから伝播）" + help: "このリクエストの対象レコードが属する組織（レコードに組織がない場合は操作コンテキストにフォールバック）" }, process_name: { label: "ソース", diff --git a/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts index 931731f4a6..537a51a405 100644 --- a/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts @@ -25,7 +25,7 @@ export const zhCNObjects: NonNullable = { }, organization_id: { label: "组织", - help: "拥有该审批请求的租户（从提交方上下文传播）" + help: "该请求所涉记录所属的组织（记录无组织时回退到操作上下文）" }, process_name: { label: "来源", diff --git a/packages/plugins/plugin-approvals/tsconfig.json b/packages/plugins/plugin-approvals/tsconfig.json index d42eb175a2..252ec046b4 100644 --- a/packages/plugins/plugin-approvals/tsconfig.json +++ b/packages/plugins/plugin-approvals/tsconfig.json @@ -2,9 +2,24 @@ "extends": "../../../tsconfig.json", "compilerOptions": { "outDir": "./dist", - "rootDir": "./src", + // [#10101] Widened from `./src` as a CONSEQUENCE of the `paths` rule + // below, exactly as `packages/rest` records for #9960: redirecting + // `@objectstack/metadata-core` to source puts `packages/metadata-core/ + // src/**` into this program, and `rootDir` is enforced over every + // program file even under `--noEmit` (TS6059). `../..` is the directory + // that genuinely contains every file in the program. Emit is unaffected: + // this package builds with tsup, and `typecheck` passes `--noEmit`. + "rootDir": "../..", "types": ["node"], - "lib": ["ES2021"] + "lib": ["ES2021"], + // [#10101] Resolve the shared platform-row resolver to SOURCE for + // `tsc --noEmit`, so this package's typecheck is a verdict about the + // checkout rather than about `metadata-core/dist` build state + // (`pnpm check:type-source-resolution` — same fix `packages/rest` + // records for `@objectstack/metadata-protocol`). + "paths": { + "@objectstack/metadata-core": ["../../metadata-core/src/index.ts"] + } }, "include": ["src/**/*"], "exclude": ["dist", "node_modules", "**/*.test.ts"] diff --git a/packages/plugins/plugin-approvals/vitest.config.ts b/packages/plugins/plugin-approvals/vitest.config.ts index 05eaa8de87..7bd6df505a 100644 --- a/packages/plugins/plugin-approvals/vitest.config.ts +++ b/packages/plugins/plugin-approvals/vitest.config.ts @@ -19,6 +19,13 @@ export default defineConfig({ // to `…/metadata-protocol/src/index.ts/` — `ENOTDIR`, at run // time, from a config that reads as correct. alias: [ + // [#10101] The shared platform-row resolver's home — aliased to source + // for the same #7668/#7778 reason as the metadata-protocol entry below + // (`pnpm check:test-source-alias` is the gate). + { + find: /^@objectstack\/metadata-core$/, + replacement: path.resolve(__dirname, '../../metadata-core/src/index.ts'), + }, { find: /^@objectstack\/metadata-protocol$/, replacement: path.resolve(__dirname, '../../metadata-protocol/src/index.ts'), diff --git a/packages/plugins/plugin-audit/package.json b/packages/plugins/plugin-audit/package.json index 74dad716ea..c00d2b4e7d 100644 --- a/packages/plugins/plugin-audit/package.json +++ b/packages/plugins/plugin-audit/package.json @@ -19,6 +19,7 @@ }, "dependencies": { "@objectstack/core": "workspace:*", + "@objectstack/metadata-core": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/platform-objects": "workspace:*", "@objectstack/spec": "workspace:*" diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index f7a8885d64..8ba32dcf33 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -19,12 +19,19 @@ import { SECRET_MASK, collectMaskedReadFields } from '@objectstack/objectql/core // heuristic here would be a second de-facto contract that disagrees with the // picker, the search companion and the approval inbox the day an author sets // `nameField` — the same argument the SECRET_MASK import above makes. -import { resolveDisplayField, isTenancyDisabled } from '@objectstack/spec/data'; -// [#8707] The canonical spelling of the tenant anchor, imported rather than -// re-typed. `SystemFieldName.ORGANIZATION_ID` is the reference this repo keeps -// precisely so consumers stop inventing their own (`org_id`, `tenant_id`, -// `space`) — the drift class framework#4330 / cloud#982 already paid for. -import { SystemFieldName } from '@objectstack/spec/system'; +import { resolveDisplayField } from '@objectstack/spec/data'; +// [#8707 / #10101] The platform-row organization resolver, imported rather +// than owned. It started life in THIS file (#8707, honouring #8287's ruling) +// and was promoted to `@objectstack/metadata-core` by the maintainer ruling +// recorded on cloud#1395: ONE shared resolver for all three platform-row +// writers (audit, approvals, automation runs) — a per-writer copy of the +// precedence is exactly the two-tables-disagree drift the promotion ends. +// `createFieldPresenceProbe` moved with it (same file, same criterion) and is +// re-exported below so this package's public surface is unchanged. +import { + createFieldPresenceProbe, + createRecordOrganizationResolver, +} from '@objectstack/metadata-core'; /** * Minimal structural view of `NotificationService.emit` (ADR-0030). Declared @@ -209,155 +216,15 @@ const NOISE_FIELDS = new Set([ ]); /** - * "Does this object's REGISTERED schema declare this field?", memoized per - * object. - * - * Extracted to module scope (#8144) so the CRUD writer below and the auth-event - * writer (`auth-event-audit.ts`) ask the question ONE way. Both stamp the same - * two conditional columns on the same table, and a second hand-rolled probe - * would answer differently on the day one of them is fixed. - * - * Why the probe exists at all: the SchemaRegistry auto-injects - * `organization_id` only in multi-tenant mode (`applySystemFields({ - * multiTenant })`), so on single-tenant stacks the `sys_audit_log` / - * `sys_activity` tables have no such column. Unconditionally stamping it there - * made every audit INSERT fail with "table sys_audit_log has no column named - * organization_id" — and the error was swallowed, so audit logging was silently - * non-functional. Resolve the field set lazily from the engine schema and cache - * it; object schemas are static after registration. - * - * Best-effort in both directions: an engine with no `getSchema` (an in-memory - * test double) reports every field absent, which skips the stamp rather than - * failing the write. + * [#8144 / #8707 / #10101] `createFieldPresenceProbe` and + * `resolveRecordOrganizationField` were defined HERE until #10101 promoted + * them to `@objectstack/metadata-core` (the cloud#1395 ruling: one shared + * platform-row organization resolver for audit, approvals and automation + * runs). Re-exported from this original path so this package's public surface + * — and every existing import — is unchanged; the moved modules carry the + * full precedence documentation. */ -export function createFieldPresenceProbe( - engine: unknown, -): (objectName: string, field: string) => boolean { - const fieldSetCache = new Map | null>(); - return (objectName: string, field: string): boolean => { - let set = fieldSetCache.get(objectName); - if (set === undefined) { - set = null; - try { - const schema: any = - typeof (engine as any)?.getSchema === 'function' ? (engine as any).getSchema(objectName) : null; - const fields = schema?.fields; - if (fields && typeof fields === 'object' && !Array.isArray(fields)) { - set = new Set(Object.keys(fields)); - } else if (Array.isArray(fields)) { - set = new Set(fields.map((f: any) => f?.name).filter(Boolean)); - } - } catch { - /* ignore — best-effort; absence just means we skip the stamp */ - } - fieldSetCache.set(objectName, set); - } - return set != null && set.has(field); - }; -} - -/** - * [#8707] "Which column carries THIS object's own organization?" — resolved - * from the object's REGISTERED schema, never hard-coded to one spelling. - * - * The audit row is stamped from the organization the record is ABOUT (see the - * precedence note at the `tenantId` computation below, and #8287's ruling). To - * do that the writer has to know which column holds it, and `organization_id` - * is not universally the answer: `sys_api_key` carries - * `active_organization_id` by deliberate design (#8287). Adding a second - * literal name beside the first would make this writer correct for exactly two - * objects and silently wrong for the third, so the question is asked of the - * schema instead. - * - * ## Precedence — deliberately the platform's own, not a second opinion - * - * It mirrors `SqlDriver.computeTenantField` step for step, because that is the - * platform's single existing answer to "which column is this object - * tenant-scoped by", and an audit row's stamp must agree with the wall the row - * will later be read through. Re-derived here rather than imported: that method - * is `protected` on a DRIVER class, and plugin-audit takes no driver - * dependency (its package contract is core/objectql/platform-objects/spec). - * The two shared inputs ARE imported — `isTenancyDisabled` (ADR-0066's single - * source of truth for the opt-out) and `SystemFieldName.ORGANIZATION_ID` — so - * the parts that could drift are one definition, and only the ordering is - * restated. - * - * 0. **Declared `tenancy.organizationField`, when the object really has that - * field.** The read-neutral, STAMP-ONLY declaration #8778's ruling added - * for exactly this consumer (option A; #8707's remaining half). It - * answers "which column says who this row is ABOUT" — a different - * question from "what is this object walled by", which is why it wins - * over every limb below, the ADR-0066 opt-out included: an author who - * declares it on an unwalled object (`sys_api_key`, `enabled: false` by - * necessity — the credential table must never be org-walled, #8287) is - * stating precisely that the trail should follow the record's own - * organization even though no wall does. Honoured only when the field is - * really present, same #5315 guard as limb 2. ⛔ Stamp-only cuts both - * ways: this resolver is the key's ONLY consumer by scope pin — a read - * path that starts consulting it needs its own ruling. - * 1. **`tenancy.enabled === false` → `null`.** ADR-0066 platform-global - * objects (`sys_sso_provider` is the shipped example) keep an optional org - * FK while explicitly NOT being tenant-scoped. Stamping an audit row from - * that FK would scope a global object's audit trail into one organization - * and hide it from the platform admin who acted — strictly LESS visible - * than today. This limb is what keeps the precedence flip from trading one - * invisibility for another; it is not an optimisation. - * 2. **Declared `tenancy.tenantField`, when the object really has that - * field.** The spec key already exists for "this object's tenant column - * genuinely is not the platform's" and the driver already honours it, so an - * object that declares one gets its audit rows stamped from the same column - * its rows are walled by. Honoured only when the field is really present — - * the same guard `computeTenantField` applies, for the same reason (#5315: - * a declared name pointing at a missing column must fall through, not - * resolve to nothing). - * 3. **The canonical injected `organization_id`, when present.** What every - * multi-tenant object gets from `applySystemFields`. - * 4. Otherwise `null` — the object has no organization of its own, and the - * caller falls back to the acting session's tenant exactly as before. - * - * ## What it deliberately does NOT do - * - * ⛔ It does not scan for "a lookup whose `reference` is `sys_organization`". - * That derivation is FALSIFIED by a shipped object: `sys_organization` itself - * declares no `organization_id` and exactly one such lookup — - * `parent_organization_id` — so the scan would stamp every organization's audit - * rows with its PARENT's id, hiding them from the very tenant they concern. - * Worse, reading `parent_organization_id` for a visibility decision is an - * ADR-0105 D6 red line that `validateOrgAxisRedLines` (@objectstack/lint) makes - * a build error for RLS policies, sharing rules and scopes; a plugin reaching - * the same conclusion through a heuristic is the same mistake with no gate on - * it. - * - * `sys_api_key.active_organization_id` is reachable through limb 0 since - * #8778 (it was the object that motivated the key). Its column is still not — - * and must never become — the object's tenant-scope column: - * `tenancy.tenantField` feeds `applyTenantScope` / `injectTenantOnInsert`, so - * declaring it there would wall the credential table on an equality that - * excludes NULL — every pre-#8287 key would vanish from its own owner's - * list, which is the defect #8287 exists to have removed. - * - * @param objectDef the registered object definition (`engine.getSchema(name)`) - * @param hasField the memoized field-presence probe for the SAME object — this - * file asks "does the schema declare this field?" exactly one way - * ({@link createFieldPresenceProbe}), and a second hand-rolled shape check - * here would answer differently on the day one of them is fixed. - */ -export function resolveRecordOrganizationField( - objectDef: unknown, - hasField: (field: string) => boolean, -): string | null { - if (!objectDef || typeof objectDef !== 'object') return null; - const tenancy = (objectDef as { tenancy?: { organizationField?: unknown; tenantField?: unknown } }).tenancy; - // Limb 0 — the explicit stamp-only declaration (#8778) wins over everything, - // the ADR-0066 opt-out below included: see the precedence doc above. - const stampField = tenancy?.organizationField; - if (typeof stampField === 'string' && stampField.length > 0 && hasField(stampField)) return stampField; - if (isTenancyDisabled(objectDef)) return null; - const declared = tenancy?.tenantField; - if (typeof declared === 'string' && declared.length > 0 && hasField(declared)) return declared; - if (hasField(SystemFieldName.ORGANIZATION_ID)) return SystemFieldName.ORGANIZATION_ID; - return null; -} +export { createFieldPresenceProbe, resolveRecordOrganizationField } from '@objectstack/metadata-core'; /** Action name produced from a HookContext.event string. */ function actionFor(event: string): 'create' | 'update' | 'delete' | null { @@ -987,21 +854,13 @@ export function installAuditWriters( return def; }; - // [#8707] The object's own organization COLUMN — see - // `resolveRecordOrganizationField` for the precedence and for why the answer - // comes from the schema rather than a literal. Memoized per object like the - // two caches above: object schemas are static after registration, and this - // runs on every audited write. - const orgFieldCache = new Map(); - const resolveRecordOrgField = (objectName: string): string | null => { - const hit = orgFieldCache.get(objectName); - if (hit !== undefined) return hit; - const resolved = resolveRecordOrganizationField(getObjectDef(objectName), (field) => - objectHasField(objectName, field), - ); - orgFieldCache.set(objectName, resolved); - return resolved; - }; + // [#8707 / #10101] The object's own organization COLUMN and value, through + // the SHARED platform-row resolver (`@objectstack/metadata-core`) — one + // memoized instance per installation, the same instance shape the approval + // writer and the automation-run recorder hold. See + // `resolveRecordOrganizationField` there for the precedence and for why the + // answer comes from the schema rather than a literal. + const recordOrgResolver = createRecordOrganizationResolver(engine); // Display label for an object under a given translate fn: translated label // → authored def label → API name. Shared by activity summaries and the @@ -1337,13 +1196,8 @@ export function installAuditWriters( // and B, active in B, writing an A record — the union wall permits it) and // `shared`, and on system/sudo paths that write another org's row while // carrying a session. - const orgField = resolveRecordOrgField(ctx.object); - const readRecordOrg = (rec: any): string | undefined => { - if (!orgField || !rec || typeof rec !== 'object') return undefined; - const v = (rec as Record)[orgField]; - return typeof v === 'string' && v.length > 0 ? v : undefined; - }; - const recordOrgId: string | undefined = readRecordOrg(ctx.result) ?? readRecordOrg(before); + const recordOrgId: string | undefined = + recordOrgResolver.organizationOf(ctx.object, ctx.result, before) ?? undefined; // // [#9516] The fallback arm reads `organizationId` — the ONLY name the // engine emits. `ObjectQL.buildSession` builds the hook session as a fixed diff --git a/packages/plugins/plugin-audit/tsconfig.json b/packages/plugins/plugin-audit/tsconfig.json index caff4761a5..40da004ccc 100644 --- a/packages/plugins/plugin-audit/tsconfig.json +++ b/packages/plugins/plugin-audit/tsconfig.json @@ -2,10 +2,25 @@ "extends": "../../../tsconfig.json", "compilerOptions": { "outDir": "./dist", - "rootDir": "./src", + // [#10101] Widened from `./src` as a CONSEQUENCE of the `paths` rule + // below, exactly as `packages/rest` records for #9960: redirecting + // `@objectstack/metadata-core` to source puts `packages/metadata-core/ + // src/**` into this program, and `rootDir` is enforced over every + // program file even under `--noEmit` (TS6059). `../..` is the directory + // that genuinely contains every file in the program. Emit is unaffected: + // this package builds with tsup, and `typecheck` passes `--noEmit`. + "rootDir": "../..", "types": [ "node" - ] + ], + // [#10101] Resolve the shared platform-row resolver to SOURCE for + // `tsc --noEmit`, so this package's typecheck is a verdict about the + // checkout rather than about `metadata-core/dist` build state + // (`pnpm check:type-source-resolution` — same fix `packages/rest` + // records for `@objectstack/metadata-protocol`). + "paths": { + "@objectstack/metadata-core": ["../../metadata-core/src/index.ts"] + } }, "include": [ "src/**/*" diff --git a/packages/plugins/plugin-audit/vitest.config.ts b/packages/plugins/plugin-audit/vitest.config.ts index 9aafd36b51..c0e7e4169b 100644 --- a/packages/plugins/plugin-audit/vitest.config.ts +++ b/packages/plugins/plugin-audit/vitest.config.ts @@ -27,6 +27,10 @@ export default defineConfig({ // One rule for all namespaces cannot go stale that way. alias: [ { find: /^@objectstack\/core$/, replacement: path.resolve(__dirname, '../../core/src/index.ts') }, + // [#10101] The shared platform-row resolver's home — aliased to source + // so the suite's verdict is about the checkout, not metadata-core's + // dist build state (`pnpm check:test-source-alias`). + { find: /^@objectstack\/metadata-core$/, replacement: path.resolve(__dirname, '../../metadata-core/src/index.ts') }, { find: /^@objectstack\/platform-objects\/audit$/, replacement: path.resolve(__dirname, '../../platform-objects/src/audit/index.ts'), diff --git a/packages/services/service-automation/package.json b/packages/services/service-automation/package.json index 387a648d14..cb77ca907d 100644 --- a/packages/services/service-automation/package.json +++ b/packages/services/service-automation/package.json @@ -20,6 +20,7 @@ "dependencies": { "@objectstack/core": "workspace:*", "@objectstack/formula": "workspace:*", + "@objectstack/metadata-core": "workspace:*", "@objectstack/spec": "workspace:*" }, "devDependencies": { diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 7dfb60c501..eba76b924f 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -1067,6 +1067,16 @@ export interface RunRecord { /** Failure reason for a `failed` run — what a designer needs to fix it. */ error?: string; nodeId?: string; + /** + * [#10101] The ACTING context's tenant (`AutomationContext.tenantId`), + * copied onto the record by {@link AutomationEngine.recordLog} — the ruled + * FALLBACK for the persisted `organization_id`, never the primary. The + * primary is the SUBJECT record's own organization, which the DB-backed + * store resolves from {@link RunRecord.triggerRecord} through the shared + * platform-row resolver (`@objectstack/metadata-core`, the cloud#1395 + * Option A ruling) at write time. Before #10101 nothing set this field at + * all, so every terminal history row persisted `organization_id = NULL`. + */ organizationId?: string | null; userId?: string | null; /** @@ -1089,6 +1099,19 @@ export interface RunRecord { triggerType?: string; triggerObject?: string; triggerRecordId?: string; + /** + * [#10101] The triggering record SNAPSHOT (`AutomationContext.record`), + * carried for the store's write-time subject-organization resolution — the + * same snapshot the suspended-run row resolves from, so a run's paused row + * and its terminal row agree by construction. A write-time INPUT, not a + * column: the DB-backed store reads the resolved organization column off + * it and persists only `organization_id`. Absent for triggers that carry + * no record (a plain scheduled sweep has no one subject), in which case + * the acting-context fallback ({@link RunRecord.organizationId}) stands — + * fabricating an acting organization for schedule/api triggers stays + * vetoed (cloud#1395 Option C). + */ + triggerRecord?: Record; /** * Bounded per-node step log (see {@link AutomationEngine.compactStepsForHistory}), * so "which node blew up?" survives a restart. Optional — history rows @@ -3322,7 +3345,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, output, - }); + }, context); return { success: true, @@ -3391,7 +3414,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, variables: variablesSnapshot, - }); + }, context); return { success: true, status: 'paused', @@ -3416,7 +3439,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, error: errorMessage, - }); + }, context); // Error handling strategy. // @@ -4186,7 +4209,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, output, - }); + }, context); // ── Subflow up-bubble (nested pause): this run was a subflow // child whose parent suspended awaiting it. Auto-resume the @@ -4239,7 +4262,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, variables: variablesSnapshot, - }); + }, context); return { success: true, status: 'paused', runId, durationMs, screen: err.screen }; } @@ -4256,7 +4279,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, error: errorMessage, - }); + }, context); // Subflow chain: a child failing terminally fails every // ancestor awaiting it — they can never be resumed otherwise. // The delegation path handles its own level (skipBubble). @@ -4482,7 +4505,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(run.context), steps: run.steps, error, - }); + }, run.context); } /** @@ -4560,7 +4583,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(run.context), steps: run.steps, error: reason, - }); + }, run.context); return true; } @@ -4695,7 +4718,15 @@ export class AutomationEngine implements IAutomationService { * {@link AutomationResult} hands the counts straight back without a second * fold or a `getRun` round-trip. */ - private recordLog(entry: ExecutionLogEntry): ExecutionLogEntry { + // [#10101] `context` is the run's {@link AutomationContext}, passed by + // every call site (the same value each already hands `buildRunTrigger`) so + // a TERMINAL record can carry the two organization-attribution inputs the + // durable store resolves from: the triggering record snapshot (the + // SUBJECT — primary, per the cloud#1395 Option A ruling) and the acting + // tenant (the ruled fallback). Threaded as a parameter rather than read + // off the entry because `ExecutionLogEntry` deliberately keeps the + // published `trigger` block's shape (`ExecutionLogSchema`). + private recordLog(entry: ExecutionLogEntry, context?: AutomationContext): ExecutionLogEntry { // #4354 — fold the run's outcome BEFORE anything downstream trims the // step log. History compaction keeps 200 steps; the summary must count // all 5000, or a long sweep's `acted` would shrink with its step log and @@ -4758,6 +4789,14 @@ export class AutomationEngine implements IAutomationService { durationMs: entry.durationMs, error: entry.error, userId: entry.trigger?.userId, + // [#10101] The two organization-attribution inputs, from the + // run context (see the `recordLog` doc): the acting tenant is + // the ruled FALLBACK, the trigger-record snapshot is what the + // store resolves the SUBJECT organization from — the same + // snapshot the paused row resolves from, so the two rows of + // one run agree by construction. + organizationId: context?.tenantId ?? null, + triggerRecord: context?.record, // #7533 — the rest of the trigger block, not just its userId. // The information exists at this exact point (the in-memory log // entry one line up carries it); it was simply not copied onto @@ -6599,7 +6638,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, output, - }); + }, context); // #4354 — a retried run reports its own attempt's counts, not the // failed one's: `retryExecution` returns THIS result on success. @@ -6682,7 +6721,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, variables: variablesSnapshot, - }); + }, context); return { success: true, status: 'paused', @@ -6705,7 +6744,7 @@ export class AutomationEngine implements IAutomationService { trigger: buildRunTrigger(context), steps, error: errorMessage, - }); + }, context); // [#9378] The retry loop reads only `result.success` and this // result never escapes `retryExecution` on its own, but it is the // same ran-and-failed exit as the two above and is classified the diff --git a/packages/services/service-automation/src/suspended-run-store.test.ts b/packages/services/service-automation/src/suspended-run-store.test.ts index 5e873128b9..1fddd2a086 100644 --- a/packages/services/service-automation/src/suspended-run-store.test.ts +++ b/packages/services/service-automation/src/suspended-run-store.test.ts @@ -28,7 +28,14 @@ function createTestLogger() { * exercise {@link ObjectStoreSuspendedRunStore} (and a restart through it) * without a real driver. */ -function createFakeEngine(): SuspendedRunStoreEngine & { rows: Map } { +function createFakeEngine( + // [#10101] Optional name → object-definition map. When given, the fake + // grows the `getSchema` the shared platform-row organization resolver + // probes for; when omitted (every pre-#10101 caller) the fake has no + // schema access and the store keeps the acting-context fallback — the + // documented degradation, exercised below rather than assumed. + schemas?: Record, +): SuspendedRunStoreEngine & { rows: Map } { const rows = new Map(); // Equality plus the `$lt` operator (kept for where-clause generality). const matches = (row: any, where: any) => @@ -40,6 +47,7 @@ function createFakeEngine(): SuspendedRunStoreEngine & { rows: Map }); return { rows, + ...(schemas ? { getSchema: (name: string) => schemas[name] } : {}), async find(_object, options) { const where = options?.where; const out = [...rows.values()].filter(r => matches(r, where)); @@ -548,16 +556,19 @@ describe('ObjectStoreSuspendedRunStore — trigger attribution columns (#7533)', }); }); -// ─── Organization attribution on the row (cloud#1395) ──────────────────────── +// ─── Organization attribution on the row (cloud#1395, fixed by #10101) ─────── // -// The finding this pins was measured from UNDER the wall, on a walled -// single-database HotCRM SaaS boot (cloud#1338's `verify-hotcrm-saas.mjs`, -// check `a4`): `sys_automation_run` carried `organization_id = NULL` on 31 of -// 31 rows while every one of them described a record owned by a specific -// customer — on a boot where `sys_audit_log` (1669 rows) was correctly -// attributed. That negative control is the whole reason this is a defect and -// not "platform tables do not carry an organization", so it is restated in -// every comment here and must stay restated. +// The finding this group started as was measured from UNDER the wall, on a +// walled single-database HotCRM SaaS boot (cloud#1338's +// `verify-hotcrm-saas.mjs`, check `a4`): `sys_automation_run` carried +// `organization_id = NULL` on 31 of 31 rows while every one of them described +// a record owned by a specific customer — on a boot where `sys_audit_log` +// (1669 rows) was correctly attributed. That negative control is the whole +// reason it was a defect and not "platform tables do not carry an +// organization". #10101 promoted the audit writer's subject-first resolution +// to the shared platform-row resolver (`@objectstack/metadata-core`, the +// cloud#1395 Option A ruling) and this store now stamps through it; the +// assertions below specify that behaviour, fallback directions included. // // These assertions read the persisted CELL for the same reason the #7533 group // above does: the column is what a wall filters on, and what an inbox query @@ -588,28 +599,23 @@ describe('ObjectStoreSuspendedRunStore — organization attribution (cloud#1395) expect(engine.rows.get('run_abc').organization_id).toBeNull(); }); - // ⛔ PINNED DEFECT — this is cloud#1395 IN THE SHAPE MEASURED, not a - // specification of desired behaviour. It asserts what the writer does - // today: a run whose trigger carried no acting tenant persists NO - // organization, even though `trigger_object` / `trigger_record_id` on the - // very same row name a record that belongs to one. - // - // The schedule, time-relative and api triggers set no tenant AT ALL — by - // construction, since a scheduled sweep has no single acting organization — - // so this is not an edge case, it is every run those triggers produce. - // - // ⛔ When the write side is fixed, this test must FAIL and be rewritten to - // assert the organization resolved from the trigger record. Do not "repair" - // it to keep it green; a pinned defect that quietly adapts to the fix is the - // unfalsifiable green this whole line of work exists to prevent. Its sibling - // pin is check `a4` in cloud's `verify-hotcrm-saas.mjs`, which carries the - // same instruction and must be promoted in the same change. - it('PINNED: a tenant-less trigger context persists organization_id = NULL beside a record that HAS an organization', async () => { - const engine = createFakeEngine(); + // PROMOTED (#10101) — this was the cloud#1395 PINNED DEFECT ('a tenant-less + // trigger context persists organization_id = NULL beside a record that HAS + // an organization'), rewritten per its own instruction the moment the write + // side was fixed: the assertion now specifies the ruled behaviour. Its + // sibling pins — cloud's `hotcrm-multitenant.acceptance.ts` suite and check + // `a4` in `verify-hotcrm-saas.mjs` — carry the same promote-never-repair + // instruction and follow at the next `.objectstack-sha` bump (tracked on + // cloud#1395). + it('PROMOTED (was the cloud#1395 pin): a tenant-less trigger context resolves organization_id from the SUBJECT record', async () => { + const engine = createFakeEngine({ + crm_deal: { fields: { id: {}, amount: {}, organization_id: {} } }, + }); const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); - // The shape a schedule / api trigger produces: a real triggering record, - // carrying its own `organization_id`, and no tenant on the context. + // The shape a schedule / time-relative / api trigger produces: a real + // triggering record, carrying its own `organization_id`, and no tenant + // on the context — every run those triggers produce, by construction. await store.save({ ...baseRun(), context: { object: 'crm_deal', record: { id: 'd1', organization_id: 'org_1' } } as any, @@ -617,8 +623,165 @@ describe('ObjectStoreSuspendedRunStore — organization attribution (cloud#1395) const row = engine.rows.get('run_abc'); expect(row.trigger_record_id, 'the row names the record it is about').toBe('d1'); - // …and stores no organization for it. Both halves asserted together: - // the NULL alone would be satisfiable by a row that describes nothing. - expect(row.organization_id, 'PINNED DEFECT cloud#1395 — fix this and PROMOTE the assertion').toBeNull(); + // …and now stores that record's organization. Both halves asserted + // together: the value alone would be satisfiable by a row that + // describes nothing. + expect(row.organization_id, 'the cloud#1395 Option A ruling: subject first').toBe('org_1'); + }); + + it('the SUBJECT record beats the acting tenant when both are present (actor context is the fallback, never the primary)', async () => { + const engine = createFakeEngine({ + crm_deal: { fields: { id: {}, amount: {}, organization_id: {} } }, + }); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + + // A member of A and B, active in B, whose flow touches an A record — + // the `group`-posture shape where actor-first measurably misfiles. + await store.save({ + ...baseRun(), + context: { + object: 'crm_deal', tenantId: 'org_actor', + record: { id: 'd1', organization_id: 'org_subject' }, + } as any, + }); + expect(engine.rows.get('run_abc').organization_id).toBe('org_subject'); + }); + + it('the acting-context fallback still stands: no record, no org column, or no schema access each keep tenantId', async () => { + // ① trigger carries no record (a plain scheduled sweep has no ONE + // subject — fabricating one stays vetoed, cloud#1395 Option C) + const noRecord = createFakeEngine({ crm_deal: { fields: { id: {}, organization_id: {} } } }); + await new ObjectStoreSuspendedRunStore(noRecord, createTestLogger()).save({ + ...baseRun(), context: { object: 'crm_deal', tenantId: 'org_1' } as any, + }); + expect(noRecord.rows.get('run_abc').organization_id).toBe('org_1'); + + // ② the object has no organization of its own (single-tenant shape) + const noColumn = createFakeEngine({ crm_deal: { fields: { id: {}, amount: {} } } }); + await new ObjectStoreSuspendedRunStore(noColumn, createTestLogger()).save({ + ...baseRun(), + context: { object: 'crm_deal', tenantId: 'org_1', record: { id: 'd1', amount: 100 } } as any, + }); + expect(noColumn.rows.get('run_abc').organization_id).toBe('org_1'); + + // ③ an engine double with no getSchema at all (the pre-#10101 fake + // shape) — the resolver degrades to null and the fallback answers + const noSchema = createFakeEngine(); + await new ObjectStoreSuspendedRunStore(noSchema, createTestLogger()).save({ + ...baseRun(), + context: { object: 'crm_deal', tenantId: 'org_1', record: { id: 'd1', organization_id: 'org_2' } } as any, + }); + expect(noSchema.rows.get('run_abc').organization_id).toBe('org_1'); + }); + + it('tenant-less AND subject-less stays NULL — Option C (fabricating an acting org) remains vetoed', async () => { + const engine = createFakeEngine({ crm_deal: { fields: { id: {}, organization_id: {} } } }); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + await store.save({ ...baseRun(), context: { object: 'crm_deal' } as any }); + expect(engine.rows.get('run_abc').organization_id).toBeNull(); + }); + + it('⛔ pins the sys_api_key divergence: the stamp column is the DECLARED active_organization_id, not the wall', async () => { + // The credential table: unwalled by necessity (`enabled: false`, + // #8287) while its rows are still ABOUT one organization under + // `tenancy.organizationField` (#8778). A flow triggered by an api-key + // record must file its run under the org the key authenticates into — + // limb 0 of the shared resolver, winning over the ADR-0066 opt-out. + const engine = createFakeEngine({ + sys_api_key: { + tenancy: { enabled: false, organizationField: 'active_organization_id' }, + fields: { id: {}, name: {}, user_id: {}, active_organization_id: {}, revoked: {} }, + }, + }); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + await store.save({ + ...baseRun(), + context: { + object: 'sys_api_key', + record: { id: 'key1', name: 'ci', active_organization_id: 'org_key' }, + } as any, + }); + expect(engine.rows.get('run_abc').organization_id).toBe('org_key'); + }); + + it('recordTerminal resolves the SUBJECT organization from the trigger-record snapshot, with the acting tenant as fallback', async () => { + const engine = createFakeEngine({ + crm_deal: { fields: { id: {}, amount: {}, organization_id: {} } }, + }); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + + // Subject resolvable → subject org, even beside an acting tenant. + await store.recordTerminal({ + runId: 'r1', flowName: 'f', status: 'completed', startedAt: '2026-01-01T00:00:00.000Z', + organizationId: 'org_actor', + triggerType: 'record-after-update', triggerObject: 'crm_deal', triggerRecordId: 'd1', + triggerRecord: { id: 'd1', organization_id: 'org_subject' }, + } as RunRecord); + expect(engine.rows.get('run_r1').organization_id).toBe('org_subject'); + + // No snapshot (a plain scheduled sweep) → the acting-context fallback. + await store.recordTerminal({ + runId: 'r2', flowName: 'f', status: 'completed', startedAt: '2026-01-01T00:00:00.000Z', + organizationId: 'org_actor', triggerType: 'schedule', + } as RunRecord); + expect(engine.rows.get('run_r2').organization_id).toBe('org_actor'); + + // Neither → NULL, never a fabricated value. + await store.recordTerminal({ + runId: 'r3', flowName: 'f', status: 'completed', startedAt: '2026-01-01T00:00:00.000Z', + triggerType: 'schedule', + } as RunRecord); + expect(engine.rows.get('run_r3').organization_id).toBeNull(); + }); + + it('end to end: a tenant-less engine run lands a terminal history row carrying the SUBJECT organization', async () => { + // Pins the engine → store handoff itself (`recordLog` copying the + // context's trigger-record snapshot and acting tenant onto the + // RunRecord), not just the store's resolution over a hand-built record. + const engine = createFakeEngine({ + crm_deal: { fields: { id: {}, amount: {}, organization_id: {} } }, + }); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + const e = new AutomationEngine(createTestLogger(), store); + e.registerFlow('attr_flow', { + name: 'attr_flow', label: 'Attribution Flow', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }); + const result = await e.execute('attr_flow', { + // the schedule / time-relative shape: a subject record, no tenant + object: 'crm_deal', event: 'record-after-update', + record: { id: 'd9', organization_id: 'org_1', amount: 5 }, + } as any); + expect(result.success).toBe(true); + // recordTerminal is fire-and-forget off the terminal recordLog — give + // the microtask queue one turn to land the row. + await new Promise((r) => setImmediate(r)); + const terminal = [...engine.rows.values()].find((r) => r.status === 'completed'); + expect(terminal, 'a terminal history row landed').toBeTruthy(); + expect(terminal.trigger_record_id).toBe('d9'); + expect(terminal.organization_id).toBe('org_1'); + }); + + it('a run’s paused row and its terminal row agree by construction (same inputs, same precedence)', async () => { + const engine = createFakeEngine({ + crm_deal: { fields: { id: {}, amount: {}, organization_id: {} } }, + }); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + const context = { object: 'crm_deal', record: { id: 'd1', organization_id: 'org_1' } }; + + await store.save({ ...baseRun(), context: context as any }); + await store.recordTerminal({ + runId: 'abc2', flowName: 'approval_flow', status: 'completed', + startedAt: '2026-01-01T00:00:00.000Z', + triggerType: 'record-after-update', triggerObject: context.object, + triggerRecordId: 'd1', triggerRecord: context.record, + } as RunRecord); + + expect(engine.rows.get('run_abc').organization_id).toBe('org_1'); + expect(engine.rows.get('run_abc2').organization_id).toBe('org_1'); }); }); diff --git a/packages/services/service-automation/src/suspended-run-store.ts b/packages/services/service-automation/src/suspended-run-store.ts index 24063bb89c..0bf011c33a 100644 --- a/packages/services/service-automation/src/suspended-run-store.ts +++ b/packages/services/service-automation/src/suspended-run-store.ts @@ -1,6 +1,14 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { Logger } from '@objectstack/spec/contracts'; +// [#10101] The SHARED platform-row organization resolver — the cloud#1395 +// ruling ("A platform row's organization is the SUBJECT record's organization; +// actor context is the fallback, never the primary"), implemented once in +// `@objectstack/metadata-core` and consumed by all three sanctioned writers +// (audit stamping, the approval-row writer, this automation-run recorder). A +// recorder-local re-derivation here was rejected by name (Option B): it would +// be a third answer to a question the codebase already answered two ways. +import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core'; import type { RunRecord, SuspendedRun, SuspendedRunStore } from './engine.js'; /** @@ -148,6 +156,15 @@ export interface SuspendedRunStoreEngine { insert(object: string, data: any, options?: any): Promise; update(object: string, data: any, options?: any): Promise; delete?(object: string, options?: any): Promise; + /** + * [#10101] Registered object definition for a name — what the shared + * platform-row organization resolver (`@objectstack/metadata-core`) reads to + * answer "which column carries the TRIGGERING object's own organization". + * Optional so an in-memory test double without it degrades to the + * acting-context fallback rather than failing the write (the same + * best-effort posture the resolver itself takes). + */ + getSchema?(objectName: string): unknown; } interface MinimalLogger { @@ -178,6 +195,15 @@ export interface ObjectStoreSuspendedRunStoreOptions { */ export class ObjectStoreSuspendedRunStore implements SuspendedRunStore { private readonly maxTerminalRunsPerFlow: number; + /** + * [#10101] Memoized shared platform-row organization resolver over the same + * engine the rows are written through — answers "which column carries the + * TRIGGERING object's own organization, and what does the trigger record + * hold there". One instance for the store's lifetime: object schemas are + * static after registration, and the audit and approval writers memoize the + * same way. + */ + private readonly recordOrgResolver: RecordOrganizationResolver; constructor( private readonly engine: SuspendedRunStoreEngine, @@ -186,6 +212,7 @@ export class ObjectStoreSuspendedRunStore implements SuspendedRunStore { ) { this.maxTerminalRunsPerFlow = options?.maxTerminalRunsPerFlow ?? DEFAULT_MAX_TERMINAL_RUNS_PER_FLOW; + this.recordOrgResolver = createRecordOrganizationResolver(engine); } async save(run: SuspendedRun): Promise { @@ -259,7 +286,20 @@ export class ObjectStoreSuspendedRunStore implements SuspendedRunStore { const id = HISTORY_PREFIX + record.runId; const row = { id, - organization_id: record.organizationId ?? null, + // [#10101, the cloud#1395 Option A ruling] SUBJECT first, actor second: + // the organization of the record this run is ABOUT (resolved from the + // trigger-record snapshot through the shared platform-row resolver — + // `sys_api_key`'s divergent `active_organization_id` included), falling + // back to the acting context's tenant (`record.organizationId`) when the + // trigger carries no record or the object has no organization of its + // own. A plain scheduled sweep has neither and keeps NULL — fabricating + // an acting organization stays vetoed (Option C). Same inputs and same + // precedence as `serialize()` below, so a run's paused row and its + // terminal row agree by construction. + organization_id: + this.recordOrgResolver.organizationOf(record.triggerObject ?? '', record.triggerRecord) ?? + record.organizationId ?? + null, flow_name: record.flowName, flow_version: record.flowVersion ?? null, node_id: record.nodeId ?? null, @@ -407,20 +447,29 @@ export class ObjectStoreSuspendedRunStore implements SuspendedRunStore { // safety"; a producer that wants this row attributed sets the declared // `tenantId`. // - // ⚠️ MEASURED, and NOT fixed by the line above (cloud#1395): this resolves - // to null on every trigger path that carries no acting tenant — the - // schedule, time-relative and api triggers set none at all, by - // construction, because a scheduled run has no one organization. Those runs - // persist `organization_id = NULL` while describing a record that DOES - // belong to a customer. `sys_audit_log` does not have this defect on the - // same boot because its writer resolves the organization from the RECORD it - // describes (plugin-audit `resolveRecordOrganizationField`, #8707 honouring - // #8287's ruling) and falls back to the session only when the record has - // none. ⛔ Do not read that asymmetry as "platform tables carry no org" — - // it is two writers reading the acting context where a third reads the - // subject. Which column a side-table row should take its organization from - // is the open contract question on cloud#1395. - const org = ctx.tenantId ?? null; + // [#10101, the cloud#1395 Option A ruling] SUBJECT first, actor second: + // the paused row's organization is the organization of the record this run + // is ABOUT — resolved from the trigger-record snapshot through the SHARED + // platform-row resolver (`resolveRecordOrganizationField`, + // `@objectstack/metadata-core`; `sys_api_key`'s divergent + // `active_organization_id` included, via `tenancy.organizationField`) — + // and the acting tenant above is the ruled FALLBACK, never the primary. + // + // This closes the measured half of cloud#1395: the schedule, time-relative + // and api triggers carry no acting tenant at all, by construction, so + // before this every run they produced persisted `organization_id = NULL` + // while `trigger_object` / `trigger_record_id` on the very same row named + // a record that DOES belong to a customer. It is the same subject-first + // precedence `sys_audit_log`'s writer already stamped with (#8707 + // honouring #8287's ruling) — three platform side tables, one answer now. + // + // The fallback still stands, and still matters: an object with no + // organization of its own (single-tenant stacks, ADR-0066 platform-global + // objects), a trigger with no record (a plain scheduled sweep has no ONE + // subject — fabricating an acting organization stays vetoed, Option C), + // and an engine double with no `getSchema` all resolve `null` here and + // keep the acting context's answer. + const org = this.recordOrgResolver.organizationOf(String(ctx.object ?? ''), ctx.record) ?? ctx.tenantId ?? null; // #7533 — the same three trigger columns the terminal path writes. A paused // row is a `sys_automation_run` row too, and leaving them null here would // make "which runs did this record provoke?" answer for finished runs while diff --git a/packages/services/service-automation/src/sys-automation-run.object.ts b/packages/services/service-automation/src/sys-automation-run.object.ts index 4b023c7353..238d3cbc7c 100644 --- a/packages/services/service-automation/src/sys-automation-run.object.ts +++ b/packages/services/service-automation/src/sys-automation-run.object.ts @@ -67,42 +67,40 @@ export const SysAutomationRun = ObjectSchema.create({ fields: { id: Field.text({ label: 'Run ID', required: true, readonly: true, group: 'System' }), - // ⚠️ MEASURED DEFECT, cloud#1395 — read this before trusting the column. + // [#10101, the cloud#1395 Option A ruling] The SUBJECT record's + // organization, with the acting context as fallback — resolved by the + // SHARED platform-row resolver (`resolveRecordOrganizationField`, + // `@objectstack/metadata-core`) in `ObjectStoreSuspendedRunStore`, from + // the trigger-record snapshot, on BOTH write paths (a paused row's + // `serialize()` and a terminal row's `recordTerminal()` — same inputs, + // same precedence, so the two rows of one run agree by construction). // - // The value is resolved from the ACTING CONTEXT (`AutomationContext. - // tenantId`) and from nothing else, so it is NULL for every run whose - // trigger carries no acting organization — which is all of them on the - // schedule, time-relative and api triggers, none of which sets a tenant, by - // construction: a scheduled sweep has no one acting organization. On a - // walled single-database HotCRM SaaS boot this measured 31 of 31 rows - // org-less, each one naming a `trigger_object` / `trigger_record_id` that - // DOES belong to a specific customer. + // Why subject-first: the schedule, time-relative and api triggers carry no + // acting tenant at all, by construction, so the pre-#10101 actor-only read + // measured 31 of 31 rows org-less on a walled HotCRM SaaS boot — each row + // naming a `trigger_object` / `trigger_record_id` that DOES belong to a + // specific customer. Subject-first is what `sys_audit_log`'s writer + // already did (#8707 honouring #8287's ruling); three platform side + // tables, one answer now. A trigger with no record (a plain scheduled + // sweep has no ONE subject) keeps the acting-context fallback — NULL there + // stays NULL: fabricating an acting organization stays vetoed (Option C). // - // ⛔ Do NOT read that as "platform tables do not carry an organization". - // The negative control on the same boot refutes it: `sys_audit_log` (1669 - // rows) was correctly attributed throughout, because its writer resolves the - // organization from the RECORD the row is about and falls back to the - // session only when the record has none (plugin-audit - // `resolveRecordOrganizationField`, #8707 honouring #8287's ruling). Three - // platform side tables, two answers — that disagreement is the defect, not - // the column. - // - // Which column a side-table row should take its organization from is an - // open contract question on cloud#1395: the audit writer's resolver is - // scope-pinned to audit stamping by the #8778 ruling, so a second consumer - // needs its own. Pinned meanwhile by `suspended-run-store.test.ts` - // ('PINNED: a tenant-less trigger context…') and by check `a4` in cloud's - // `verify-hotcrm-saas.mjs`; both must be PROMOTED, never repaired, when the - // write side is fixed. + // Promoted from the cloud#1395 pinned-defect state by #10101: the + // framework pin (`suspended-run-store.test.ts`, formerly 'PINNED: a + // tenant-less trigger context…') now asserts the resolved organization; + // the two cloud-side pins (`hotcrm-multitenant.acceptance.ts`, check `a4` + // in `verify-hotcrm-saas.mjs`) are tracked on cloud#1395 and follow at the + // next `.objectstack-sha` bump. organization_id: Field.lookup('sys_organization', { label: 'Organization', required: false, group: 'System', - // ⛔ String unchanged on purpose — same reason as - // `sys_approval_request.organization_id`: it is extracted into the - // generated i18n bundles, so the reword rides the write-side fix and its - // regeneration pass rather than arriving as a silent bundle drift here. - description: 'Tenant that owns this run (propagated from the trigger context)', + // Reworded with the #10101 write-side fix (was "Tenant that owns this + // run (propagated from the trigger context)" — ADR-0120 §Terminology + // requires "organization", and the value is subject-first now). + // Extracted into the generated i18n bundles as `help`; the four locales + // regenerate in the same pass. + description: 'Organization of the record that triggered this run (falls back to the acting context when the trigger has none)', }), flow_name: Field.text({ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8491e01df7..8aa146ffba 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1462,6 +1462,9 @@ importers: '@objectstack/core': specifier: workspace:* version: link:../../core + '@objectstack/metadata-core': + specifier: workspace:* + version: link:../../metadata-core '@objectstack/objectql': specifier: workspace:* version: link:../../objectql @@ -2187,6 +2190,9 @@ importers: '@objectstack/formula': specifier: workspace:* version: link:../../formula + '@objectstack/metadata-core': + specifier: workspace:* + version: link:../../metadata-core '@objectstack/spec': specifier: workspace:* version: link:../../spec @@ -2194,9 +2200,6 @@ importers: '@objectstack/driver-sql': specifier: workspace:* version: link:../../drivers/driver-sql - '@objectstack/metadata-core': - specifier: workspace:* - version: link:../../metadata-core '@objectstack/objectql': specifier: workspace:* version: link:../../objectql