From 33245d4208813a901820d945778ef090c1bf742b Mon Sep 17 00:00:00 2001 From: Litant Ying Date: Fri, 4 Sep 2026 06:57:54 +0000 Subject: [PATCH 1/6] fix(runtime): type the packages-domain `protocol` service handle so undeclared request keys are compile errors (#15215) * wip(runtime): type the packages-domain protocol service handle Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * wip(runtime): add the packages-domain protocol handle typing pin Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * chore(changeset): patch note for the packages-domain protocol handle typing Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * docs(permissions): re-anchor the system-context census rows moved by the typing block Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * docs(permissions): regenerate the system-context census from the merged tree Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N --------- Co-authored-by: Claude --- .../packages-domain-protocol-handle-typed.md | 26 +++ content/docs/permissions/system-context.mdx | 4 +- .../packages-protocol-handle-typing.test.ts | 193 ++++++++++++++++++ packages/runtime/src/domains/packages.ts | 173 ++++++++++++---- 4 files changed, 359 insertions(+), 37 deletions(-) create mode 100644 .changeset/packages-domain-protocol-handle-typed.md create mode 100644 packages/runtime/src/domains/packages-protocol-handle-typing.test.ts diff --git a/.changeset/packages-domain-protocol-handle-typed.md b/.changeset/packages-domain-protocol-handle-typed.md new file mode 100644 index 0000000000..1ee601cf42 --- /dev/null +++ b/.changeset/packages-domain-protocol-handle-typed.md @@ -0,0 +1,26 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): the packages domain reaches the `protocol` service through a typed handle (#13598) + +`deps.resolveService(context, 'protocol')` answers `any` — `protocol` is +deliberately left unmapped in `ServiceSlotContracts` — so every request literal +downstream of that seam compiled against nothing. Twelve sites in +`domains/packages.ts` held that `any` (two of them on the variable declaration +rather than the call), and an undeclared or misspelt key in the ADR-0045 +publish-visibility flip's `getMetaItems` / `saveMetaItem` literals compiled +silently. Measured on the base tree: injecting `bogusUndeclaredKey: true` into +the `saveMetaItem` literal gave `tsc --noEmit` exit 0 and zero diagnostics. + +The slot is now narrowed once, at one helper, to a handle `Pick`ed from the +DECLARED contracts — `MetadataProtocol` / `PackageProtocol` from +`@objectstack/spec`, plus the producer's own exported `DeletePackageRequest` — +so the same injection is now `error TS2353`. Every member is OPTIONAL and every +`typeof protocol. === 'function'` capability probe is unchanged: a host +may occupy the slot with a partial object, and the type answers "is this key +declared?" while the probe still answers "did this host bring the verb?". + +Compile-layer signal only — no request is newly accepted or refused, no +response shape moves, and the eight verbs no contract declares keep an explicit +`any` request rather than a private restatement nothing verifies. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index edc62037c3..3b085d6acd 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -160,10 +160,10 @@ The largest single consumer — **17 of the 106 sites**. | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:401` | | 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4888`, `:6302`, `:6550`, `:6981`, `:7174` | | 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` | -| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:326`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | +| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:422`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | | 54 | Package REST route capability gate bypassed | rest | Get: package read/write over REST without `manage_metadata` / `studio.access` / `setup.access` | `package-routes.ts:102` | -| 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:145`, `:178` | +| 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:241`, `:274` | | 56 | Activation write / authoring refusals do not fire | runtime | Get: activation artifacts writable and authorable without the activation-authoring capability | `activation-gate.ts:138`, `:189` | | 57 | Automation run-state read, flow-authoring write and unrelated-screen read all pass | runtime | Get: run state, flow writes and screen reads with no grant | `domains/automation.ts:254`, `:545`, `:635` | | 58 | Audience-binding suggestion recording skipped | plugin-security | Lose: install-time suggestions are not recorded for system callers | `suggested-audience-bindings.ts:703` | diff --git a/packages/runtime/src/domains/packages-protocol-handle-typing.test.ts b/packages/runtime/src/domains/packages-protocol-handle-typing.test.ts new file mode 100644 index 0000000000..0cf071966a --- /dev/null +++ b/packages/runtime/src/domains/packages-protocol-handle-typing.test.ts @@ -0,0 +1,193 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13598 — the packages domain reaches the `protocol` service through a TYPED + * handle, and the runtime capability probes survive that typing. + * + * Two halves, because the card has two halves that pull in opposite directions + * and either one alone is a regression: + * + * 1. **Compile-time** (section 1). An undeclared key in one of this domain's + * request literals must be a COMPILE ERROR. That is the #11006 series' end + * state, and it stopped one seam short here. + * 2. **Runtime** (section 2). ⛔ A host may occupy the `protocol` slot with a + * PARTIAL object. Tightening the type and then deleting a + * `typeof … === 'function'` probe would trade the compile-time improvement + * for a runtime crash, so section 2 drives a real dispatcher whose protocol + * brings none of the verbs and pins the documented 501s. + * + * ## The defect, measured on the base tree with the same instrument + * + * `deps.resolveService(context, 'protocol')` answers `any` — `protocol` is + * deliberately unmapped in `ServiceSlotContracts`. Downstream of that seam + * nothing compiled against a contract at all. Measured at `25a59bd`, injecting + * one undeclared key (`bogusUndeclaredKey: true`) into the `saveMetaItem` + * literal of the ADR-0045 visibility flip: + * + * tsc --noEmit -p packages/runtime/tsconfig.json -> exit 0, ZERO diagnostics + * + * The same injection into the same literal after this change: + * + * ... -> exit 2 + * packages.ts(727,41): error TS2353: Object literal may only specify known + * properties, and 'bogusUndeclaredKey' does not exist in type + * '{ type: string; name: string; item: unknown; organizationId?: … }' + * + * Section 1 is that measurement made DURABLE. Each `@ts-expect-error` below is + * itself checked: if the seam ever goes back to `any` the directive stops + * matching an error and tsc reports TS2578 (unused directive) — so this file + * cannot rot into a green no-op the way an assertion-only pin could. + * + * ⚠️ These directives are NOT phantom checks: `packages/runtime`'s BUILD + * tsconfig excludes every `.test.ts` under `src`, but the sibling + * `tsconfig.test.json` + * compiles this layer and `package.json`'s `typecheck` script names it via + * `check:test-typecheck`. This file carries no entry in + * `test-typecheck-debt.json`, so any error it gains beyond the expected ones is + * red on arrival. + * + * ## Reverse verification — direction predicted BEFORE running + * + * Reverting `domains/packages.ts` to the base tree makes section 1 red as + * TS2578 x4 (every directive becomes unused, because the `any` handle accepts + * everything) — the reversal shape, not a plain "assertion failed", which is + * why the directives are the pin and not `expectTypeOf` assertions. Section 2 + * is GREEN IN BOTH DIRECTIONS by construction: the probes it exercises are + * unchanged by this card, so it is the control that says the 501s were never + * bought with a behaviour change. + */ +import { describe, expect, it } from 'vitest'; +import { HttpDispatcher } from '../http-dispatcher.js'; +import type { PackagesDomainProtocol } from './packages.js'; + +// --------------------------------------------------------------------------- +// Section 1 — compile-time pins (never executed; the checker is the assertion) +// --------------------------------------------------------------------------- + +/** + * The literals this domain actually sends, spelled exactly as the handlers + * spell them. A positive control for the four `@ts-expect-error`s below: if + * this body ever stopped compiling, those directives could be "satisfied" by a + * type that rejects everything, which pins nothing. + */ +function declaredKeysCompile(protocol: PackagesDomainProtocol) { + return [ + // ADR-0045 visibility flip — `GET` half. + protocol.getMetaItems?.({ type: 'app', packageId: 'crm', organizationId: 'org_1' }), + // ADR-0045 visibility flip — `SAVE` half. `packageId` is declared + // `nullable().optional()`, `actor` optional; both are load-bearing here. + protocol.saveMetaItem?.({ + type: 'app', + name: 'crm_console', + item: { _unpublished: false }, + packageId: 'crm', + organizationId: 'org_1', + actor: 'u_publisher', + }), + // `applyPublishedSeeds`' seed body read-back, both attempts. + protocol.getMetaItem?.({ type: 'seed', name: 'crm_seed', organizationId: 'org_1' }), + protocol.getMetaItem?.({ type: 'seed', name: 'crm_seed' }), + // The manifest-export read. + protocol.getMetaItems?.({ type: 'view', packageId: 'crm', organizationId: undefined }), + ]; +} + +/** + * ⛔ THE PIN. Each directive must match a real diagnostic; an unused one is + * TS2578 and fails `check:test-typecheck`. + */ +function undeclaredKeysAreCompileErrors(protocol: PackagesDomainProtocol) { + return [ + protocol.saveMetaItem?.({ + type: 'app', + name: 'crm_console', + item: {}, + // @ts-expect-error [#13598] `packagId` is a misspelling of the + // declared `packageId`. Through the pre-change `any` handle this + // compiled, and the write silently landed unbound to the package. + packagId: 'crm', + }), + protocol.getMetaItems?.({ + type: 'app', + // @ts-expect-error [#13598] not a member of `GetMetaItemsRequest` — + // the read has no `packageIds` plural. + packageIds: ['crm'], + }), + // A misspelt VERB, which is what the untyped handle could never catch: + // any property access on `any` is a property access on `any`. + // @ts-expect-error [#13598] `rollbackToPackageCommit` has three `m`s in + // neither of the two places this one puts them. + protocol.rollbackToPackageCommmit?.({ commitId: 'c1' }), + // ⛔ Every member is OPTIONAL and STAYS optional: a filled slot is not a + // promise that the verb is there. This directive is what would go + // unused if someone "simplified" the handle to a non-partial + // `MetadataProtocol` — which is exactly the change that deletes the + // reason the runtime probes in section 2 exist. + // @ts-expect-error [#13598] possibly `undefined` — call it behind the probe. + protocol.getMetaItems({ type: 'app' }), + ]; +} + +// --------------------------------------------------------------------------- +// Section 2 — runtime control: the capability probes SURVIVE the typing +// --------------------------------------------------------------------------- + +/** `/packages` state changes demand `manage_metadata` (#7033 / #7023). */ +const PKG_ADMIN = () => ({ + request: {}, + executionContext: { + userId: 'u_pkg_admin', + systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'], + }, +}) as any; + +/** + * A host that OCCUPIES the `protocol` slot with an object carrying none of the + * verbs — the documented reason every call site probes rather than calls. Not + * an empty slot: an empty slot would take the `!protocol` arm of each guard and + * prove nothing about the `typeof … === 'function'` half. + */ +function partialProtocolDoor() { + const kernel: any = { + getService: (name: string) => { + if (name === 'protocol') return Promise.resolve({ someUnrelatedVerb: () => undefined }); + if (name === 'objectql') { + return Promise.resolve({ + registry: { getAllPackages: () => [], getPackage: () => undefined }, + }); + } + return null; + }, + context: { getService: () => null }, + }; + return new HttpDispatcher(kernel); +} + +describe('#13598 · 1 · the compile-time pins are type-level only', () => { + it('neither pin function is invoked — tsc is the assertion', () => { + expect(typeof declaredKeysCompile).toBe('function'); + expect(typeof undeclaredKeysAreCompileErrors).toBe('function'); + }); +}); + +describe('#13598 · 2 · a PARTIAL protocol host is still answered, never crashed', () => { + const cases: Array<[string, string, string, string]> = [ + ['publish-drafts', '/crm/publish-drafts', 'POST', 'Draft publishing not supported'], + ['discard-drafts', '/crm/discard-drafts', 'POST', 'Draft discarding not supported'], + ['commits', '/crm/commits', 'GET', 'Commit history not supported'], + ['commit revert', '/crm/commits/c1/revert', 'POST', 'Commit revert not supported'], + ['rollback', '/crm/rollback', 'POST', 'Commit rollback not supported'], + ['adopt-orphans', '/crm/adopt-orphans', 'POST', 'Orphan adoption not supported'], + ['duplicate', '/crm/duplicate', 'POST', 'Package duplication not supported'], + ]; + + for (const [label, path, method, message] of cases) { + it(`${label} answers 501 from the capability probe`, async () => { + const result = await partialProtocolDoor().handlePackages( + path, method, { commitId: 'c1', targetPackageId: 'crm_copy' }, {}, PKG_ADMIN(), + ); + expect(result.response?.status).toBe(501); + expect(JSON.stringify(result.response?.body)).toContain(message); + }); + } +}); diff --git a/packages/runtime/src/domains/packages.ts b/packages/runtime/src/domains/packages.ts index e7e7bf8503..1c0349a734 100644 --- a/packages/runtime/src/domains/packages.ts +++ b/packages/runtime/src/domains/packages.ts @@ -40,10 +40,17 @@ import { OBJECT_SCHEMA_READ_ONLY_EXEMPT_CAPABILITIES } from '@objectstack/metada import { isWritablePackage } from '@objectstack/metadata-protocol'; // [#9960] The uninstall seam's DECLARED shapes, from the same producer and for // the same reason as the predicate above: this door reached `deletePackage` -// through `(protocol as any)` and routinely sent two keys — `organizationId` +// through `protocol` and routinely sent two keys — `organizationId` // and `keepData` — that the sibling REST door's own option type could not even // express. One statement of the contract, imported by both doors. import type { DeletePackageRequest, DeletePackageResponse } from '@objectstack/metadata-protocol'; +// [#13598] The DECLARED protocol contracts this domain's request literals are +// compiled against. Imported, never restated: a second hand-written +// `saveMetaItem(…)` signature here would silently drift from the one the spec +// declares and `ObjectStackProtocolImplementation` states it `implements` — +// which is the whole reason `PackagesDomainProtocol` below is `Pick`ed rather +// than written out. Same move `domains/mcp.ts` makes for its merged-read seam. +import type { MetadataProtocol, PackageProtocol } from '@objectstack/spec/api'; // [#8443] ADR-0112's disclosure rule (#8086 / #8136 / #8333), and the DECLARED // 422 that keeps the one quotable population quotable. Both imported from the // producer for the reason the line above is: this door's seed-apply fallback is @@ -58,6 +65,95 @@ import { setPackageDisabled } from '../package-state-store.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; +/** + * [#13598] The `protocol` service slot **as this domain reaches it** — one + * statement of the handle, `Pick`ed from the DECLARED contracts, replacing + * twelve independent `protocol` seams in this file. + * + * ## What was wrong with the seam + * + * `deps.resolveService(context, 'protocol')` answers `any`. That is not an + * oversight — {@link DomainHandlerDeps.resolveService} types its return from + * `ServiceSlotContracts`, and `protocol` is deliberately left unmapped there + * ("real services with no written contract, so they keep today's `any` rather + * than being given a shape here that nothing verifies"). The `any` is honest + * about the SLOT. What it also did, silently, was hand every request literal + * downstream of it an unchecked call target: the #11006 series' end state — + * "an undeclared key in a request literal is a compile error" — stopped one + * seam short here, so a misspelt or undeclared key in these literals compiled. + * + * ## Why the type is here and not on the slot + * + * Mapping `'protocol'` in `ServiceSlotContracts` would type every consumer at + * once, but it is a `packages/spec` change that would have to answer for the + * whole slot — including the seven verbs below that no contract declares at + * all — and it would state that a filled slot IS a `MetadataProtocol`, whose + * members are mostly REQUIRED. That is the shape the guards exist to deny (see + * next paragraph). So the narrowing happens at the consumer, once, exactly as + * `domains/mcp.ts` narrows the same slot to `Pick` for its merged read. + * + * ## ⛔ Every member is OPTIONAL, and the runtime guards STAY + * + * A host may occupy this slot with a partial object — that is the documented + * reason the `typeof protocol. === 'function'` probes exist, and every + * one of them survives this change unchanged in meaning. `Partial<…>` is what + * makes the type agree with them instead of contradicting them: tightening the + * type and then deleting a probe would trade a compile-time improvement for a + * runtime crash. The type answers "is this key declared?"; the probe answers + * "did THIS host bring the verb?". Two different questions, both still asked. + * + * ## Where the ledger honestly ends + * + * The first two groups name shapes someone DECLARES: the spec's + * `MetadataProtocol` / `PackageProtocol`, and — for `deletePackage` — the + * producer's own exported request type, already imported here since #9960 for + * exactly this reason. The last group has no declared request shape anywhere: + * `@objectstack/metadata-protocol` types those seven verbs inline on the + * implementation class and exports nothing for them. Writing a structural type + * for them HERE would be a private restatement that nothing verifies — the + * thing #9846 retired one file over. So their request keeps `any` and the gap + * stays visible and greppable: declaring them is producer-side work, not this + * consumer's to invent. What the entries still buy is the verb name itself — + * `protocol.rollbackToPackageCommmit` is now a compile error where the `any` + * handle took any spelling at all. + */ +export type PackagesDomainProtocol = + Partial> + & Partial> + & { + /** Declared by the producer (`@objectstack/metadata-protocol`), #9960. */ + deletePackage?(request: DeletePackageRequest): Promise; + /** ⚠️ Undeclared request shapes — see "Where the ledger honestly ends". */ + publishPackageDrafts?(request: any): Promise; + discardPackageDrafts?(request: any): Promise; + listCommits?(request: any): Promise; + revertCommit?(request: any): Promise; + rollbackToPackageCommit?(request: any): Promise; + reassignOrphanedMetadata?(request: any): Promise; + duplicatePackage?(request: any): Promise; + updatePackage?(request: any): Promise; + }; + +/** + * [#13598] Resolve the `protocol` slot as {@link PackagesDomainProtocol}. + * + * THE one narrowing point for this file. `resolveService` answers `any` for + * this name, so the widening happens here and nowhere else — every call site + * downstream holds a typed handle, and a thirteenth call site added next month + * gets the type by construction rather than by remembering to write one. + * + * ⛔ Not a guard and not a replacement for one: it neither probes for verbs nor + * rejects a partial host. `undefined` still means "no protocol service", and + * each caller still asks its own `typeof …=== 'function'` capability question. + */ +async function resolveProtocol( + deps: DomainHandlerDeps, + context: HttpProtocolContext, +): Promise { + return await deps.resolveService(context, 'protocol'); +} + export function createPackagesDomain(deps: DomainHandlerDeps): DomainRoute { return { prefix: '/packages', @@ -401,7 +497,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }; } let pkg: any; - const protocolSvc: any = await deps.resolveService(_context, 'protocol').catch(() => null); + const protocolSvc = await resolveProtocol(deps, _context).catch(() => null); if (protocolSvc && typeof protocolSvc.installPackage === 'function') { const out = await protocolSvc.installPackage({ manifest, settings: body.settings }); pkg = out?.package ?? out; @@ -466,11 +562,11 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 2 && parts[1] === 'publish-drafts' && m === 'POST') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); - const protocol = await deps.resolveService(_context, 'protocol'); - if (protocol && typeof (protocol as any).publishPackageDrafts === 'function') { + const protocol = await resolveProtocol(deps, _context); + if (protocol && typeof protocol.publishPackageDrafts === 'function') { try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const result = await (protocol as any).publishPackageDrafts({ + const result = await protocol.publishPackageDrafts({ packageId: id, ...(organizationId ? { organizationId } : {}), ...(body?.actor ? { actor: body.actor } : {}), @@ -613,10 +709,10 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin const flipOrganizationId = organizationIdForMetaWrite('app', organizationId); try { if ( - typeof (protocol as any).getMetaItems === 'function' && - typeof (protocol as any).saveMetaItem === 'function' + typeof protocol.getMetaItems === 'function' && + typeof protocol.saveMetaItem === 'function' ) { - const appsRes = await (protocol as any).getMetaItems({ + const appsRes = await protocol.getMetaItems({ type: 'app', packageId: id, ...(organizationId ? { organizationId } : {}), @@ -626,7 +722,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin : Array.isArray((appsRes as any)?.items) ? (appsRes as any).items : []; for (const app of apps) { if (app && typeof app === 'object' && app._unpublished === true && typeof app.name === 'string') { - await (protocol as any).saveMetaItem({ + await protocol.saveMetaItem({ type: 'app', name: app.name, // `false`, not a delete: ADR-0045 §3 makes @@ -792,11 +888,11 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 2 && parts[1] === 'discard-drafts' && m === 'POST') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); - const protocol = await deps.resolveService(_context, 'protocol'); - if (protocol && typeof (protocol as any).discardPackageDrafts === 'function') { + const protocol = await resolveProtocol(deps, _context); + if (protocol && typeof protocol.discardPackageDrafts === 'function') { try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const result = await (protocol as any).discardPackageDrafts({ + const result = await protocol.discardPackageDrafts({ packageId: id, ...(organizationId ? { organizationId } : {}), ...(body?.actor ? { actor: body.actor } : {}), @@ -815,11 +911,11 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 2 && parts[1] === 'commits' && m === 'GET') { const denied = requireReadCapability(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); - const protocol = await deps.resolveService(_context, 'protocol'); - if (protocol && typeof (protocol as any).listCommits === 'function') { + const protocol = await resolveProtocol(deps, _context); + if (protocol && typeof protocol.listCommits === 'function') { try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const commits = await (protocol as any).listCommits({ + const commits = await protocol.listCommits({ packageId: id, ...(organizationId ? { organizationId } : {}), }); @@ -837,11 +933,11 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 4 && parts[1] === 'commits' && parts[3] === 'revert' && m === 'POST') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const commitId = decodeURIComponent(parts[2]); - const protocol = await deps.resolveService(_context, 'protocol'); - if (protocol && typeof (protocol as any).revertCommit === 'function') { + const protocol = await resolveProtocol(deps, _context); + if (protocol && typeof protocol.revertCommit === 'function') { try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const result = await (protocol as any).revertCommit({ + const result = await protocol.revertCommit({ commitId, ...(organizationId ? { organizationId } : {}), ...(body?.actor ? { actor: body.actor } : {}), @@ -858,14 +954,14 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin // back THROUGH every commit newer than `commitId` (ADR-0067). if (parts.length === 2 && parts[1] === 'rollback' && m === 'POST') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; - const protocol = await deps.resolveService(_context, 'protocol'); - if (protocol && typeof (protocol as any).rollbackToPackageCommit === 'function') { + const protocol = await resolveProtocol(deps, _context); + if (protocol && typeof protocol.rollbackToPackageCommit === 'function') { if (!body?.commitId) { return { handled: true, response: deps.error('Body { commitId } is required', 400) }; } try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const result = await (protocol as any).rollbackToPackageCommit({ + const result = await protocol.rollbackToPackageCommit({ commitId: String(body.commitId), ...(organizationId ? { organizationId } : {}), ...(body?.actor ? { actor: body.actor } : {}), @@ -908,13 +1004,13 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 2 && parts[1] === 'adopt-orphans' && m === 'POST') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); - const protocol = await deps.resolveService(_context, 'protocol'); - if (!protocol || typeof (protocol as any).reassignOrphanedMetadata !== 'function') { + const protocol = await resolveProtocol(deps, _context); + if (!protocol || typeof protocol.reassignOrphanedMetadata !== 'function') { return { handled: true, response: deps.error('Orphan adoption not supported', 501) }; } try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const result = await (protocol as any).reassignOrphanedMetadata({ + const result = await protocol.reassignOrphanedMetadata({ targetPackageId: id, ...(organizationId ? { organizationId } : {}), ...(body?.actor ? { actor: body.actor } : {}), @@ -931,8 +1027,8 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 2 && parts[1] === 'duplicate' && m === 'POST') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); - const protocol = await deps.resolveService(_context, 'protocol'); - if (!protocol || typeof (protocol as any).duplicatePackage !== 'function') { + const protocol = await resolveProtocol(deps, _context); + if (!protocol || typeof protocol.duplicatePackage !== 'function') { return { handled: true, response: deps.error('Package duplication not supported', 501) }; } const targetPackageId = typeof body?.targetPackageId === 'string' ? body.targetPackageId.trim() : ''; @@ -941,7 +1037,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin } try { const organizationId = await deps.resolveActiveOrganizationId(_context); - const result = await (protocol as any).duplicatePackage({ + const result = await protocol.duplicatePackage({ sourcePackageId: id, targetPackageId, ...(typeof body?.targetName === 'string' ? { targetName: body.targetName } : {}), @@ -993,10 +1089,10 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin return { handled: true, response: deps.error('Body { name?, description?, version? } — nothing to update', 400) }; } - const protocol = await deps.resolveService(_context, 'protocol'); - if (protocol && typeof (protocol as any).updatePackage === 'function') { + const protocol = await resolveProtocol(deps, _context); + if (protocol && typeof protocol.updatePackage === 'function') { try { - const updated = await (protocol as any).updatePackage({ packageId: id, patch }); + const updated = await protocol.updatePackage({ packageId: id, patch }); return { handled: true, response: deps.success((updated as any)?.package ?? updated) }; } catch (e: any) { return { handled: true, response: deps.errorFromThrown(e, 500) }; @@ -1034,16 +1130,23 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin // named: `organizationId` (the key that decides an uninstall's blast radius) // and `keepData` are exactly the two the sibling REST door's option type // could not express, and nothing compared the two doors' requests. Narrowed - // HERE to the producer's declared verb, so what this door sends is checked + // to the producer's declared verb, so what this door sends is checked // against the contract the implementation states. // + // [#13598] That narrowing used to be written INLINE right here, as this + // door's own one-off `{ deletePackage?(…) }` annotation, because it was the + // only typed seam in a file of eleven untyped ones. It is now the + // `deletePackage` member of {@link PackagesDomainProtocol} — the same + // producer-declared request type, stated once for the whole file instead of + // once at the one door that happened to need it first. The rule is + // unchanged; only its address is. + // // The `typeof … === 'function'` probe STAYS and the member stays optional: // the verb is absent from the spec's `PackageProtocol` (every member of // which is optional anyway), the slot takes whatever a host registers under // the name, and registrants carrying no `deletePackage` are real in-tree. // A capability question, asked as a capability probe — not a cast. - const protocol: { deletePackage?(request: DeletePackageRequest): Promise } | undefined = - await deps.resolveService(_context, 'protocol'); + const protocol = await resolveProtocol(deps, _context); if (protocol && typeof protocol.deletePackage === 'function') { try { const organizationId = await deps.resolveActiveOrganizationId(_context); @@ -1136,7 +1239,7 @@ packageId: string, registry: any, context: HttpProtocolContext, ): Promise | null> { - const protocol = await deps.resolveService(context, 'protocol'); + const protocol = await resolveProtocol(deps, context); if (!protocol || typeof protocol.getMetaItems !== 'function') return null; const organizationId = await deps.resolveActiveOrganizationId(context); @@ -1233,7 +1336,7 @@ _context: HttpProtocolContext, // [#4127] `protocol` keeps its `any` — no written contract, so this is where // the ledger honestly ends. `metadata` and `ql` are both evidenced now, // `objectql` as of batch 3: it is the same instance the `data` slot holds. - const protocol: any = await deps.resolveService(_context, 'protocol'); + const protocol = await resolveProtocol(deps, _context); const metadata = await deps.getService(_context, CoreServiceName.enum.metadata); const ql = await deps.resolveService(_context, 'objectql'); if (!protocol || typeof protocol.getMetaItem !== 'function' || !ql || !metadata) { From b4a3b3292c0ba4aa12940352cd1ef3d7e55a1bb2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 07:03:01 +0000 Subject: [PATCH 2/6] fix(objectql): publish the record's organization on every DataEvent (#15220) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(objectql): publish the record's organization on every DataEvent (#14970) `DataEventSchema.organizationId` was declared and published by the spec half but populated by nothing, so every `data.record.*` event went out with the key absent — which the contract requires a consumer to read as "this record is behind no organization wall". `publishDataEvent` now resolves it from the row itself: the written record on `created`, the post-state on `updated`, and the by-id branch's already-read pre-image on `deleted`, so no per-event read is bought. The record's organization, never `ExecutionContext.tenantId` — that is the caller's active org, and the two diverge on exactly the system/unscoped write this key most needs to label correctly. Absence keeps one spelling: the key is omitted, never `''` (which the schema refuses outright, dropping the whole event) and never an explicit `undefined` (which survives `parse` as a present key). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ * docs(permissions): re-anchor the system-context census after the engine line shift Mechanical repair by `node scripts/check-system-context-census.mjs --fix`, the only correct writer for this table. Pure line rot: the `eventOrganizationId` helper and its threading shifted every later line in `packages/objectql/src/engine.ts`, so 14 anchors (15 citation sites — one source line is cited twice) pointed at the wrong lines. No population and no classification change: still 106 elevation read sites in 20 packages across 45 files, all anchored; 140 anchors resolve, 27 declared non-read — the same figures as before the shift. `--fix` did not refuse, and the diff is digits and nothing else (12 lines added, 12 removed, identical once digits are stripped). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --------- Co-authored-by: Claude --- .changeset/data-event-record-organization.md | 45 ++++ content/docs/permissions/system-context.mdx | 24 +- .../objectql/src/engine-data-events.test.ts | 235 ++++++++++++++++++ packages/objectql/src/engine.ts | 93 +++++++ 4 files changed, 385 insertions(+), 12 deletions(-) create mode 100644 .changeset/data-event-record-organization.md diff --git a/.changeset/data-event-record-organization.md b/.changeset/data-event-record-organization.md new file mode 100644 index 0000000000..7cbb5524fa --- /dev/null +++ b/.changeset/data-event-record-organization.md @@ -0,0 +1,45 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a published `DataEvent` now names the organization the RECORD belongs to + +`DataEventSchema.organizationId` has been declared and published since the spec +half landed, and its TSDoc states the obligation on the producer's side: *"a +producer that omits the key on an organization-stamped row publishes a +cross-tenant event, which is fixed at the publish site — never by a +consumer-side lookup."* The engine populated it on no event at all. Every +`data.record.created` / `updated` / `deleted` went out with the key absent, +which a consumer is required to read as *"this record is behind no organization +wall"* — so an organization-stamped row was published as an unwalled one, and a +tenant-scoped fan-out had nothing to discriminate on. + +`publishDataEvent` now resolves the organization from the row itself and spreads +the key in when there is one. The row is already in hand at all three call +sites — the written record on `created`, the post-state on `updated`, and the +pre-image on `deleted` (the by-id branch reads it unconditionally for its +existence gate) — so this buys **no** per-event read: the key exists precisely +to keep a per-event lookup off the fan-out path. + +Three properties are deliberate: + +- **The RECORD's organization, never the caller's.** The row's own tenant column + is the only source consulted. `ExecutionContext.tenantId` is the caller's + *active* organization; the two coincide on an ordinary tenant write and + diverge on a system or unscoped one, where substituting it would mislabel an + administrator's write into another organization as belonging to the + administrator's. +- **Absence has exactly one spelling: the key is omitted.** An object that is + not tenant-scoped, a row whose column is empty, and a value no id can be read + off all publish the key absent rather than `null`, `''` or an explicit + `undefined`. The schema refuses the empty string outright, so producing one + would have thrown at the publish site and dropped the event entirely. +- **The column is resolved the way the write path resolves it** — the + `tenancy.enabled: false` opt-out, then a declared `tenancy.tenantField`, then + the injected `organization_id` — so the event cannot name an organization for + a column the engine does not actually scope by. Note the two spellings differ: + the column is `organization_id`, the published key is `organizationId`. + +No schema, no accepted shape and no public export moves: the key was already +declared, already validated and already part of what consumers parse. Only the +implementation changed, from omitting a declared key to populating it. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 3b085d6acd..c86c955925 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -109,18 +109,18 @@ that silently does not happen. | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| -| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11290` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11473` | -| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:10025` | +| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11373` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11556` | +| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:10106` | | 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1795` | -| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10073`, `readonly-strict-errors.ts:66` | -| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5892` | -| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3736`, `:3746`, `:3773` | +| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10154`, `readonly-strict-errors.ts:66` | +| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5973` | +| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3799`, `:3809`, `:3836` | | 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` | | 26 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `identity-write-guard.ts:99` | -| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6590` | -| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12085` | -| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12014` | +| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6671` | +| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12172` | +| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12101` | ### 3. Sharing (`plugin-sharing`) @@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them. | # | Site | Package | What it does | |:--|:---|:---|:---| -| 62 | `objectql/src/engine.ts:3543` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:14523` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3606` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:14616` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | | 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report | | 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across | @@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs. |:---|:---|:---| | "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:1971` (rationale at `:1881`–`1883`, #3760), `flow.zod.ts:702` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | -| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10008`–`10025` | +| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10089`–`10106` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1580` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` | diff --git a/packages/objectql/src/engine-data-events.test.ts b/packages/objectql/src/engine-data-events.test.ts index 400fee62f0..9c5a8cf64a 100644 --- a/packages/objectql/src/engine-data-events.test.ts +++ b/packages/objectql/src/engine-data-events.test.ts @@ -398,3 +398,238 @@ describe('#4639 — predicate writes publish aggregate BulkDataEvents', () => { expect(warn).toHaveBeenCalled(); }); }); + +/** + * #14970 — the producer half of `DataEvent.organizationId`. + * + * `packages/spec/src/api/events.zod.ts` declared the member (PR #14635) and + * states the obligation on the producer: *"a producer that omits the key on an + * organization-stamped row publishes a cross-tenant event, which is fixed at + * the publish site — never by a consumer-side lookup."* The engine published + * it on no event at all, which left the landed spec term and the ready + * consumer piece (#13566's fan-out filter) both inert. + * + * ⚠️ **A green suite proves nothing here unless the pins discriminate.** The + * failure mode is "the key is absent on EVERY event", and a pin that only + * asserts *absent when there is no organization* passes happily against it. + * Two properties make these pins real: + * + * 1. **Caller organization ≠ record organization.** `execCtx.tenantId` is the + * CALLER's active org; the contract asks for the RECORD's. They coincide on + * an ordinary tenant write and diverge on a system/unscoped one, so every + * positive pin below writes a row into an organization the caller is not + * standing in — an administrator's write into another organization, the + * exact case the spec names. Substituting `execCtx.tenantId` fails them. + * 2. **The two spellings differ.** The row's COLUMN is snake_case + * (`organization_id`); the published KEY is camelCase (`organizationId`). + * Reading the wrong one publishes the key absent on every event while + * every absence pin still passes — so the positive pins assert BOTH + * spellings on the same event. + * + * And absence is asserted as OMISSION, not as `=== undefined`: the schema is + * `z.string().min(1).optional()`, so `''` is refused outright (which would + * throw inside the publish site and drop the event entirely) while a key set + * to an explicit `undefined` survives `parse` as a PRESENT key. + */ +describe('#14970 — a published DataEvent names the RECORD\'s organization', () => { + /** Tenant-scoped: the kernel-injected `organization_id` is declared. */ + const invoice = { + name: 'invoice', + label: 'Invoice', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + amount: { name: 'amount', type: 'text' as const }, + organization_id: { name: 'organization_id', type: 'text' as const }, + }, + }; + + // A SYSTEM context: `isSystem` is what lets a caller file a row under an + // organization that is not its own active one (the tenant write wall, #2946, + // rejects a foreign `organization_id` for everyone else). `tenantId` is the + // caller's org and is deliberately NOT the row's on every positive pin. + const CALLER_ORG = 'org_platform'; + const RECORD_ORG = 'org_acme'; + const sysCtx = { isSystem: true, tenantId: CALLER_ORG, userId: 'usr_admin' }; + + let engine: ObjectQL; + let published: RealtimeEventPayload[]; + let realtime: IRealtimeService; + + const payloadOf = (i = 0) => published[i].payload as Record; + const hasOrgKey = (i = 0) => + Object.prototype.hasOwnProperty.call(payloadOf(i), 'organizationId'); + + beforeEach(async () => { + published = []; + realtime = { + publish: vi.fn(async (event: RealtimeEventPayload) => { published.push(event); }), + subscribe: vi.fn(async () => 'sub-1'), + unsubscribe: vi.fn(async () => undefined), + }; + engine = new ObjectQL(); + const { driver } = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(invoice); + engine.registry.registerObject(task); + engine.setRealtimeService(realtime); + vi.spyOn((engine as any).logger, 'warn').mockImplementation(() => undefined); + }); + + it('created: names the ROW\'s organization, not the caller\'s active one', async () => { + const record = await engine.insert( + 'invoice', + { amount: '100', organization_id: RECORD_ORG }, + { context: sysCtx } as any, + ); + + expect(published).toHaveLength(1); + const event = DataEventSchema.parse(published[0].payload); + expect(event.type).toBe('data.record.created'); + expect(event.recordId).toBe(record.id); + + // The discriminating assertion: the RECORD's org, and provably not the + // caller's — `execCtx.tenantId` was a different, non-empty organization + // throughout this write. + expect(event.organizationId).toBe(RECORD_ORG); + expect(event.organizationId).not.toBe(CALLER_ORG); + + // The spelling control (see the block header): the row body carries the + // snake_case COLUMN, the event carries the camelCase KEY, and both are + // populated on this one event. Reading `row.organizationId` instead would + // leave the second one absent while the first still passed. + expect((event.after as Record).organization_id).toBe(RECORD_ORG); + }); + + it('updated: names the POST-state\'s organization, not the caller\'s', async () => { + const record = await engine.insert( + 'invoice', + { amount: '100', organization_id: RECORD_ORG }, + { context: sysCtx } as any, + ); + published.length = 0; + + await engine.update('invoice', { id: record.id, amount: '250' }, { context: sysCtx } as any); + + expect(published).toHaveLength(1); + const event = DataEventSchema.parse(published[0].payload); + expect(event.type).toBe('data.record.updated'); + expect(event.organizationId).toBe(RECORD_ORG); + expect(event.organizationId).not.toBe(CALLER_ORG); + expect((event.after as Record).organization_id).toBe(RECORD_ORG); + }); + + it('updated: a row MOVED between organizations is labelled with where it is NOW', async () => { + const record = await engine.insert( + 'invoice', + { amount: '100', organization_id: RECORD_ORG }, + { context: sysCtx } as any, + ); + published.length = 0; + + await engine.update( + 'invoice', + { id: record.id, organization_id: 'org_moved' }, + { context: sysCtx } as any, + ); + + const event = DataEventSchema.parse(published[0].payload); + // The post-state, not the pre-image — a consumer filtering on the event's + // organization must see the row where it now lives. + expect(event.organizationId).toBe('org_moved'); + expect(event.organizationId).not.toBe(RECORD_ORG); + }); + + it('deleted: names the organization off the PRE-IMAGE — the path with no `after`', async () => { + const record = await engine.insert( + 'invoice', + { amount: '100', organization_id: RECORD_ORG }, + { context: sysCtx } as any, + ); + published.length = 0; + + await engine.delete('invoice', { where: { id: record.id }, context: sysCtx } as any); + + expect(published).toHaveLength(1); + const event = DataEventSchema.parse(published[0].payload); + expect(event.type).toBe('data.record.deleted'); + expect(event.recordId).toBe(record.id); + // The delete path is the one most likely to regress silently: there is no + // post-state to read, so this value can only have come from the pre-image + // the by-id branch already holds. + expect(event.after).toBeUndefined(); + expect(event.organizationId).toBe(RECORD_ORG); + expect(event.organizationId).not.toBe(CALLER_ORG); + }); + + it('an object that is not tenant-scoped OMITS the key on all three actions', async () => { + // `task` declares no `organization_id`, so `resolveTenantFieldName` finds + // no column and nothing is published — rather than the caller's org being + // used as a stand-in, which is what makes this pin more than a tautology: + // the caller carries `tenantId: CALLER_ORG` on every one of these writes. + const record = await engine.insert('task', { title: 'no wall' }, { context: sysCtx } as any); + await engine.update('task', { id: record.id, title: 'edited' }, { context: sysCtx } as any); + await engine.delete('task', { where: { id: record.id }, context: sysCtx } as any); + + expect(published.map((e) => e.type)).toEqual([ + 'data.record.created', 'data.record.updated', 'data.record.deleted', + ]); + for (let i = 0; i < 3; i += 1) { + // OMITTED, asserted as omission: an explicitly-`undefined` key would + // survive `parse` and reach a consumer as a present key. + expect(hasOrgKey(i)).toBe(false); + expect(DataEventSchema.parse(published[i].payload).organizationId).toBeUndefined(); + } + }); + + it('a tenant-scoped object whose ROW carries no organization OMITS the key', async () => { + // Distinct from the case above: the column EXISTS, it is simply empty — + // "not behind any organization wall" for this row. The caller still has an + // active organization, and it still must not be substituted. + const record = await engine.insert( + 'invoice', + { amount: '7', organization_id: null }, + { context: sysCtx } as any, + ); + + expect(published).toHaveLength(1); + expect(hasOrgKey()).toBe(false); + expect(DataEventSchema.parse(published[0].payload).organizationId).toBeUndefined(); + expect(DataEventSchema.parse(published[0].payload).recordId).toBe(record.id); + }); + + it('an empty-string organization column OMITS the key AND still publishes the event', async () => { + // `''` is refused by `z.string().min(1)`, so handing it to the publish + // site's `parse` would throw and the event would be dropped altogether — + // a silence far worse than an absent key. The gate is in the resolver, not + // in the error handler. + await engine.insert( + 'invoice', + { amount: '9', organization_id: '' }, + { context: sysCtx } as any, + ); + + expect(published).toHaveLength(1); + expect(hasOrgKey()).toBe(false); + expect(() => DataEventSchema.parse(published[0].payload)).not.toThrow(); + }); + + it('a batch insert stamps each row with its OWN organization', async () => { + await engine.insert( + 'invoice', + [ + { amount: '1', organization_id: RECORD_ORG }, + { amount: '2', organization_id: 'org_globex' }, + { amount: '3' }, + ], + { context: sysCtx } as any, + ); + + expect(published).toHaveLength(3); + const events = published.map((e) => DataEventSchema.parse(e.payload)); + expect(events.map((e) => e.organizationId)).toEqual([RECORD_ORG, 'org_globex', undefined]); + // The org-less row omits rather than inheriting a sibling's or the + // caller's — one event per record means one organization per record. + expect(hasOrgKey(2)).toBe(false); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 9e13e1778b..860d49623b 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2189,6 +2189,69 @@ function eventUserId(execCtx?: ExecutionContext): string | undefined { return asString === '' ? undefined : asString; } +/** + * `DataEvent.organizationId` — the organization the RECORD belongs to, read + * off that row's own tenant column (#14970). + * + * The spec declares this member as an obligation on the PRODUCER, and states + * it in terms this function exists to honour literally: *"Present = exactly + * that organization, never a guess. It names the organization the RECORD + * belongs to — not the caller's active organization standing in for the + * row's, which would mislabel an administrator's write into another + * organization."* Two consequences, neither negotiable: + * + * - ⛔ **Never `execCtx.tenantId`.** That is the CALLER's active org — the + * hook-context sense {@link ObjectQL.buildHookUser} publishes, where + * `organizationId` is deliberately "the blessed developer-facing name for + * the caller's". The two coincide on an ordinary tenant write and DIVERGE + * on a system/unscoped one, which is exactly the write this key most needs + * to label correctly. The row is the only truthful source, so this reads + * the row and nothing else. Substituting the caller's org onto a + * permission-boundary column is the defect PR #14726's blocking contract + * review found on a different column. + * - ⛔ **Never a per-event read.** Every call site already holds the row — + * the written record, the post-state, or the delete's pre-image — so this + * is a threading job, not a resolution job. A lookup here would put a query + * on the fan-out path the event exists to keep O(1); triage ruled that out + * for the consumer side on 2026-08-31 and it is equally out here. + * + * The column is resolved through {@link resolveTenantFieldName} — the write + * path's own precedence (`tenancy.enabled: false` opt-out, then a declared + * `tenancy.tenantField`, then the kernel-injected `organization_id`) — so an + * object the engine does not tenant-scope resolves NOTHING rather than being + * mined for a coincidentally-named column, and a disagreement between what the + * engine scopes by and what the event names cannot arise. ⚠️ The two spellings + * differ on purpose and are easy to conflate: the COLUMN is snake_case + * (`organization_id`, machine name), the published KEY is camelCase + * (`organizationId`, the blessed developer-facing name). + * + * Returns `undefined` for every "no organization" case — object not + * tenant-scoped, row absent, column absent, `null`, `''`, or a value no id + * can be read off — and the caller then OMITS the key. Omission is the + * schema's ONE spelling for absence (`z.string().min(1).optional()`): `''` is + * refused outright, which would make `parse` throw and drop the event + * entirely, and a key set to an explicit `undefined` survives `parse` as a + * PRESENT key. Hence the conditional spread at the publish site, not an + * assignment. + */ +function eventOrganizationId(objectSchema: unknown, row: unknown): string | undefined { + const tenantField = resolveTenantFieldName(objectSchema); + if (!tenantField) return undefined; + const body = eventRecordBody(row); + if (!body) return undefined; + const value = body[tenantField]; + // The write path's own "actually supplied" predicate, so producer and + // consumer cannot disagree about what counts as an organization. + if (!carriesOrganization(value)) return undefined; + // Then the same coercion ladder `eventRecordId` uses for the other id on + // this event. Deliberately NOT a bare `String(value)`: `String(false)` is a + // perfectly valid `min(1)` string, and inventing an organization out of a + // malformed column is the "never fabricated" clause's exact failure mode. + if (typeof value === 'string') return value; + if (typeof value === 'number' || typeof value === 'bigint') return String(value); + return undefined; +} + /** * Coerce a multi-row driver result into `BulkDataEvent.matched` (#4639). * @@ -5633,6 +5696,15 @@ export class ObjectQL implements IObjectQLEngine { recordId: unknown; changes?: unknown; after?: unknown; + /** + * The row whose tenant column names this event's `organizationId` + * (#14970) — the written record on `created`, the post-state on + * `updated`, the PRE-IMAGE on `deleted` (a delete has no post-state, and + * `previous` is what every other delete-side consumer already falls back + * to). Passed explicitly rather than inferred from `after` so the delete + * path, the one with no `after`, cannot silently publish the key absent. + */ + organizationRow?: unknown; context?: ExecutionContext; }, ): Promise { @@ -5655,6 +5727,14 @@ export class ObjectQL implements IObjectQLEngine { const changes = eventRecordBody(input.changes); const after = eventRecordBody(input.after); const userId = eventUserId(input.context); + // [#14970] The RECORD's organization, off the row itself — ⛔ never + // `input.context.tenantId`, which is the CALLER's. See + // {@link eventOrganizationId}; omitted, never `''`/`undefined`, because + // absence has exactly one spelling in the schema. + const organizationId = eventOrganizationId( + this._registry.getObject(object), + input.organizationRow, + ); const event: DataEvent = DataEventSchema.parse({ id: generateEventUuid(), type: `data.record.${action}`, @@ -5663,6 +5743,7 @@ export class ObjectQL implements IObjectQLEngine { ...(changes !== undefined ? { changes } : {}), ...(after !== undefined ? { after } : {}), ...(userId !== undefined ? { userId } : {}), + ...(organizationId !== undefined ? { organizationId } : {}), timestamp, }); @@ -10286,6 +10367,8 @@ export class ObjectQL implements IObjectQLEngine { await this.publishDataEvent('created', object, { recordId: record?.id, after: record, + // [#14970] The written row names its own organization. + organizationRow: record, context: opCtx.context, }); } @@ -11616,6 +11699,10 @@ export class ObjectQL implements IObjectQLEngine { recordId: hookContext.input.id ?? resultId, changes: hookContext.input.data, after: result, + // [#14970] The POST-state's organization, not the pre-image's: + // an update that moves a row between organizations must label + // the event with where the row is NOW. + organizationRow: result, context: opCtx.context, }); } @@ -13068,6 +13155,12 @@ export class ObjectQL implements IObjectQLEngine { const resultId = (typeof result === 'object' && result && 'id' in result) ? (result as any).id : undefined; await this.publishDataEvent('deleted', object, { recordId: hookContext.input.id ?? resultId, + // [#14970] The pre-image — a delete has no post-state, and + // this branch is the by-id one, where `priorRecord` was read + // unconditionally by #7867's existence gate and proven + // non-null before `beforeDelete` ever fired. So the row is + // already in hand and NO new read is bought here. + organizationRow: priorRecord, context: opCtx.context, }); } From b741969e3c8c346df1a6eab7f4ad1d30eb0b5eb9 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 07:03:48 +0000 Subject: [PATCH 3/6] ci(reaper): arm the merged-branch reaper for scheduled deletion of claude/ branches (#15224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci(reaper): arm the merged-branch reaper for scheduled deletion of claude/ branches Flips .github/workflows/merged-branch-reaper.yml from report-only to the scheduled weekly deletion the maintainer ruled on 2026-09-04 (issue #12771, decision batch #30), reaffirming the 2026-08-31 ruling under the base-ref guard PR #15144 landed. Deletion is a SEPARATE job (`reap`), because `permissions:` is scoped per job. `sweep` keeps `contents: read` + `pull-requests: read` and remains structurally incapable of deleting a ref; `reap` holds the only `contents: write` in the file, consumes the `reapable` list `sweep` publishes as a job output, and computes no classification of its own. Fences: - `reap` never runs on `pull_request` — the self-exercising run stays a dry run — and its `if:` is an allowlist of `schedule` plus a `workflow_dispatch` on which the operator explicitly set `dry_run: false`. - the new `dry_run` workflow_dispatch input defaults to true, so the manual path is fail-closed. - the base-ref guard, `PREFIX`, `BASE_REF`, the grace window, the schedule and the `is-ancestor` prohibition are all untouched. - the whole deletion list is printed to the run log before the first delete. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012zGPuVVX3deAx9LdjK8jCk * test(reaper): pin the deletion hand-off and fence the delete job structurally The contract harness drives the `sweep` classifier and can say nothing about the job that deletes — deletion deliberately lives outside the extracted script, so what the harness judges stays a classification rather than an action. Two additions close that gap. 1. The hand-off. `sweep` now publishes `reapable_branches`, the machine-readable half of the list it prints, and `reap` consumes that and nothing else. Scenarios G1/G2/R1 pin that the list EQUALS the reapable bucket — same members, same order — over a population carrying one branch in every bucket, and mutations M13/M14 drive both directions red (held branches leaking in; the list not published at all). 2. The fence. `reapFenceFailures()` parses the shipped YAML and asserts the delete job's structure: its `if:` excludes `pull_request` and gates `workflow_dispatch` on `inputs.dry_run == false`; it declares `contents: write` and is the ONLY job in the file that does; the top-level grant stays `contents: read`; it still `needs: sweep`. New self-test battery 6 drives six mutations of the workflow text to red, each asserting its anchor was present first. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012zGPuVVX3deAx9LdjK8jCk * ci(reaper): put every excluded bucket on the run-log audit line The maintainer's ruling names the run log as the audit trail, and the notice line named three of the seven buckets — reapable, mergedElsewhere, noPr. The other four (open, closedUnmerged, grace, protectedBranch) lived only in the step summary and the uploaded artifact, so the log alone could not answer "what did it hold back, and why". Also retires two strings that stopped being true when the reaper was armed: the summary heading said "DRY RUN. Nothing was deleted." of a run that may now delete in a later job, and the notice said "Nothing was deleted" of the whole run rather than of this job. Both now speak for the `sweep` job only, which is the thing they were ever really asserting — its token grant is `contents: read` and that has not changed. No classification changed: the buckets, the guard, the grace window and the step outputs are byte-identical. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012zGPuVVX3deAx9LdjK8jCk --------- Co-authored-by: Claude --- .github/workflows/merged-branch-reaper.yml | 266 ++++++++++++++++-- .../check-merged-branch-reaper-outcome.mjs | 188 ++++++++++++- 2 files changed, 422 insertions(+), 32 deletions(-) diff --git a/.github/workflows/merged-branch-reaper.yml b/.github/workflows/merged-branch-reaper.yml index d4752c72b4..743a695535 100644 --- a/.github/workflows/merged-branch-reaper.yml +++ b/.github/workflows/merged-branch-reaper.yml @@ -1,13 +1,46 @@ -name: Merged-Branch Reaper (report-only) +name: Merged-Branch Reaper # The standing sweep for #12771: `claude/*` branches that outlived the PR they # were the head of. # -# ⛔⛔ THIS WORKFLOW DELETES NOTHING. It classifies and reports. The deleting -# mode is deliberately ABSENT, not merely switched off — see "Enabling the -# deleting mode" at the bottom of this header. The maintainer's ruling of -# 2026-08-28 requires that the first delivery produce the would-delete list for -# one human look BEFORE deletion is ever enabled. +# ⚠️⚠️ THIS WORKFLOW DELETES BRANCHES. It was ARMED on 2026-09-04 by the +# maintainer's ruling on #12771 (decision batch #30), which reaffirmed the +# ruling of 2026-08-31 once the base-ref guard of #13503 had landed (PR #15144). +# Before that it classified and reported and the deleting mode was absent; the +# history below is kept because every fence it records is still load-bearing. +# +# Exactly what deletes, and what never does: +# +# DELETES the `reapable` bucket, and nothing else: a `claude/*` branch whose +# OWN pull request reports `merged_at` AND `base.ref === 'main'`, +# past the grace window, with no open PR of its own. +# NEVER `mergedElsewhere`, `closedUnmerged`, `noPr`, `open`, `grace`, +# `protectedBranch`. All six stay REPORT-ONLY — listed in the run +# log every sweep, never touched. Widening past `reapable` is a new +# maintainer ruling, ⛔ not an edit to this file. +# NEVER on `pull_request`. The self-exercising run below stays a DRY RUN, +# which is what keeps "edit this workflow" from meaning "delete +# branches". The `reap` job's `if:` excludes it explicitly. +# NEVER on a `workflow_dispatch` unless the operator sets `dry_run: false`. +# That input defaults to TRUE, so the manual path is fail-closed: a +# manual run is for LOOKING at the list unless someone says otherwise. +# +# The deletion list is printed to the run log BEFORE the first deletion. That +# log is the audit trail the ruling asks for. +# +# ## Two jobs, because `permissions:` is scoped per JOB +# +# `sweep` classifies. It runs with `contents: read` + `pull-requests: read` and +# is structurally incapable of deleting a ref — the same token grant it was +# reviewed under, and the one every scenario of the contract harness drives. +# `reap` consumes the `reapable` list `sweep` published as a job output and is +# the ONLY job in this file holding `contents: write`. +# +# The split is the point. Deleting needs a write grant; classifying must not +# have one. A single job would have handed the write token to the ~140 lines +# that decide WHICH branches, where a defect turns into a deleted branch. As +# split, the write token reaches one job that makes no classification decision +# of its own: it re-checks the prefix, prints the list, and calls DELETE. # # ## The criterion is PR state MERGED. ⛔⛔ NEVER `is-ancestor`. # @@ -112,25 +145,44 @@ name: Merged-Branch Reaper (report-only) # NEW RULING (what proves such a branch is abandoned rather than in flight?), # not an implementation detail, and is deliberately NOT taken here. # -# ## Report-only is enforced by the TOKEN, not just by the code -# -# `permissions: contents: read` below is the whole grant. Deleting a ref needs -# `contents: write`. So even a defect in the classification cannot delete a -# branch: the token this job runs with is structurally incapable of it. That is -# the property to preserve when reviewing changes to this file. -# -# ## Enabling the deleting mode (NOT done here — the maintainer's call) -# -# Three edits, deliberately left undone so that enabling is a reviewed diff and -# not a flipped default: -# 1. raise `permissions:` to `contents: write`; -# 2. add a step calling `DELETE /repos/{owner}/{repo}/git/refs/heads/{branch}` -# over the `reapable` list this job already computes and uploads; -# 3. decide the CLOSED-unmerged policy. The default here is MERGED-ONLY. -# Reaping closed-but-unmerged branches discards work that was never -# merged — that is the maintainer's call to make, not this workflow's to -# assume. -# The grace period below should stay in place when that happens. +# ## The CLASSIFIER is still report-only by the TOKEN, and that has not changed +# +# The `sweep` job's grant is `contents: read`. Deleting a ref needs +# `contents: write`. So a defect in the classification still cannot delete a +# branch by itself: the token that job runs with remains structurally incapable +# of it, and the deletion is a separate job downstream of an explicit hand-off. +# That is the property to preserve when reviewing changes to this file — ⛔ do +# not move the delete call into the `sweep` job, and ⛔ do not raise the +# top-level `permissions:`. +# +# ## The deleting mode, as armed (2026-09-04) +# +# The three edits this header used to list as deliberately-undone, and what each +# became: +# 1. `permissions: contents: write` — done, but SCOPED TO THE `reap` JOB, not +# raised at the top level. It is the only `contents: write` in the file. +# 2. `DELETE /repos/{owner}/{repo}/git/refs/heads/{branch}` over the +# `reapable` list — done, in `reap`, consuming the list `sweep` publishes +# as a job output. ⛔ It consumes no other bucket. +# 3. the CLOSED-unmerged policy — UNCHANGED and still MERGED-ONLY. Reaping +# closed-but-unmerged branches discards work that never landed; the +# maintainer's ruling of 2026-09-04 left that exclusion exactly where it +# was. `closedUnmerged` remains report-only. +# The grace period stays in place, as that note required. +# +# ## ⛔ What arming did NOT change +# +# ⛔ No seat identity gains delete rights — option B of #12771 stays refused. +# The delete grant lives in this one job and expires with the run; no agent +# container, credential or workflow elsewhere gains it. +# ⛔ `PREFIX` is untouched: `copilot/` is on #13503's own release line and this +# workflow must not reach it. +# ⛔ `is-ancestor` remains forbidden, and the reverse check against it stays. +# ⛔ The classifier's contract harness +# (`scripts/check-merged-branch-reaper-outcome.mjs`) still drives the `sweep` +# script and still holds `reapable => merged PR based on main` over every +# scenario. Deletion deliberately lives OUTSIDE the extracted script so that +# what the harness judges is unchanged in kind: a classification, not an action. # ## Why this workflow declares no check family # @@ -172,6 +224,16 @@ on: description: 'Do not list a merged branch until its PR merged this many days ago.' required: false default: '7' + dry_run: + # ⛔ Defaults to TRUE, and the default is the point. "workflow_dispatch + # kept for a manual run" reads fail-closed: a manual run is for LOOKING + # at the list. Deleting by hand is an explicit `dry_run: false`, typed + # by whoever wants it, on the one run they want it on. The scheduled + # weekly run is the armed path the ruling authorises; this one is not. + description: 'Classify and report only. Uncheck to actually delete the reapable branches.' + type: boolean + required: false + default: true # Exercise the sweep on changes to itself, the same posture as # required-set-patrol.yml. This is what makes the FIRST delivery of this PR a # real would-delete list produced by a real runner rather than a claim about @@ -180,9 +242,12 @@ on: paths: - '.github/workflows/merged-branch-reaper.yml' -# Least privilege, and load-bearing: see "Report-only is enforced by the TOKEN". -# `contents: read` lists branches; `pull-requests: read` reads PR state. Neither -# can delete a ref. +# Least privilege, and load-bearing: see "Two jobs, because `permissions:` is +# scoped per JOB". This top-level grant is the DEFAULT and it is READ-ONLY — +# `contents: read` lists branches, `pull-requests: read` reads PR state, and +# neither can delete a ref. ⛔ Do not raise it: the one job that deletes +# overrides it locally, and that override is the only `contents: write` in this +# file precisely because this one stays read. permissions: contents: read pull-requests: read @@ -200,6 +265,16 @@ jobs: name: Merged-branch sweep (report-only) runs-on: ubuntu-latest timeout-minutes: 15 + # Stated at job level rather than inherited, so that reading THIS job never + # requires scrolling to the top of the file to learn that it cannot delete. + permissions: + contents: read + pull-requests: read + # The hand-off to `reap`. `reapable_branches` is the machine-readable half + # of the list the step above prints; `reap` consumes it and computes nothing. + outputs: + reapable_count: ${{ steps.sweep.outputs.reapable }} + reapable_branches: ${{ steps.sweep.outputs.reapable_branches }} steps: - name: Classify every claude/* branch by the state of its PR id: sweep @@ -318,10 +393,10 @@ jobs: arr.slice(0, k).map(fmt).join('\n') + (arr.length > k ? `\n_...and ${arr.length - k} more_` : ''); const lines = []; - lines.push('## Merged-branch reaper — DRY RUN. Nothing was deleted.'); + lines.push('## Merged-branch reaper — classification. THIS JOB deletes nothing.'); lines.push(''); lines.push(`Criterion: **a PR whose head ref is the branch reports \`merged_at\`**. ⛔ Never \`is-ancestor\`.`); - lines.push(`Grace period: **${graceDays} day(s)** since merge. Token grant: \`contents: read\` — this job cannot delete a ref.`); + lines.push(`Grace period: **${graceDays} day(s)** since merge. Token grant: \`contents: read\` — this job cannot delete a ref; only the \`reap\` job can, and only over the ✅ row below.`); lines.push(''); lines.push(`### Population: ${total} \`${PREFIX}\` branches (of ${allBranches.length} on the remote)`); lines.push(''); @@ -365,10 +440,23 @@ jobs: const payload = { generated_at: new Date().toISOString(), grace_days: graceDays, total_branches: allBranches.length, prefix_branches: total, buckets }; require('fs').writeFileSync(`${process.env.RUNNER_TEMP}/branch-reaper-report.json`, JSON.stringify(payload, null, 2)); - core.notice(`Dry run: ${n(buckets.reapable)} of ${total} ${PREFIX} branches would be deleted. ${n(buckets.mergedElsewhere)} merged somewhere other than \`${BASE_REF}\` and are held by the base-ref guard. ${n(buckets.noPr)} have no PR and are unreachable by this criterion. Nothing was deleted.`); + core.notice( + `Sweep: ${n(buckets.reapable)} of ${total} ${PREFIX} branches are reapable and are handed to the \`reap\` job. ` + + `Report-only, never reaped: ${n(buckets.mergedElsewhere)} merged somewhere other than \`${BASE_REF}\` and held by the base-ref guard; ` + + `${n(buckets.noPr)} have no PR and are unreachable by this criterion; ${n(buckets.open)} have an open PR; ` + + `${n(buckets.closedUnmerged)} closed unmerged; ${n(buckets.grace)} merged within the ${graceDays}-day grace window; ` + + `${n(buckets.protectedBranch)} protected. This job deleted nothing — its token grant is \`contents: read\`.`, + ); core.setOutput('reapable', String(n(buckets.reapable))); core.setOutput('no_pr', String(n(buckets.noPr))); core.setOutput('merged_elsewhere', String(n(buckets.mergedElsewhere))); + // The machine-readable half of the deletion list, and the ONLY + // thing the `reap` job consumes. It is the `reapable` bucket and + // nothing else, so what deletes is exactly what was classified + // reapable and exactly what the log above printed. The contract + // harness pins that equality (scenarios G1/G2/R1, mutations + // M13/M14) so no later edit can widen the list without a red. + core.setOutput('reapable_branches', JSON.stringify(buckets.reapable.map((r) => r.branch))); - name: Upload the full classification # `always()`: a run whose report is missing because an earlier step died @@ -379,3 +467,119 @@ jobs: name: branch-reaper-report path: ${{ runner.temp }}/branch-reaper-report.json if-no-files-found: warn + + reap: + # ⛔ NOT a required context either, and must never become one. + name: Delete the reapable branches + needs: sweep + # ⛔⛔ The fence that keeps a dry run dry. Three clauses, and the second is + # deliberately redundant with the third: + # - `success()` is WRITTEN rather than left to GitHub's implicit wrapper + # (#5343): if `sweep` died there is no classification to act on, and + # "the classifier failed" must never read the same as "nothing to do". + # - `!= 'pull_request'` on its own clause, because it is the one exclusion + # a reader must be able to find without evaluating the rest. Editing + # this workflow exercises the sweep; it must never delete a branch. + # - then an ALLOWLIST of the two armed paths: the weekly `schedule` the + # 2026-09-04 ruling authorises, and a `workflow_dispatch` on which the + # operator explicitly unset the `dry_run` default. + # An allowlist is used rather than a denylist so that a trigger added to + # this workflow later is DRY by default and has to be armed on purpose. + if: >- + success() + && github.event_name != 'pull_request' + && (github.event_name == 'schedule' + || (github.event_name == 'workflow_dispatch' && inputs.dry_run == false)) + runs-on: ubuntu-latest + timeout-minutes: 15 + # ⛔ The ONLY `contents: write` in this file, and the only job that can + # delete a ref. It expires with the run: no seat identity, container or + # credential gains delete rights from it (#12771 option B stays refused). + permissions: + contents: write + steps: + - name: Delete every branch in the reapable bucket + uses: actions/github-script@v9 + env: + REAPABLE: ${{ needs.sweep.outputs.reapable_branches }} + with: + script: | + const owner = context.repo.owner; + const repo = context.repo.repo; + // Re-checked here, not trusted. `sweep` only ever puts prefixed + // branches in the bucket, so a violation is a corrupted hand-off + // rather than a branch to delete anyway — and the cost of being + // wrong in this job is a branch that exists nowhere else. + const PREFIX = 'claude/'; + + let branches; + try { + branches = JSON.parse(process.env.REAPABLE || '[]'); + } catch (err) { + core.setFailed(`the reapable list handed over by \`sweep\` is not JSON -- ${err.message}. Nothing was deleted.`); + return; + } + if (!Array.isArray(branches)) { + core.setFailed('the reapable list handed over by `sweep` is not an array. Nothing was deleted.'); + return; + } + const stray = branches.filter((b) => typeof b !== 'string' || !b.startsWith(PREFIX)); + if (stray.length > 0) { + core.setFailed( + `the reapable list contains ${stray.length} entry/entries outside \`${PREFIX}\`: ` + + `${stray.join(', ')}. Nothing was deleted -- a hand-off this wrong is not one to act on partially.`, + ); + return; + } + if (branches.length === 0) { + core.info(`The sweep classified nothing as reapable. Nothing to delete.`); + return; + } + + // The audit trail the ruling asks for: the WHOLE list, in the run + // log, before the first deletion — so a run that dies halfway still + // leaves behind what it intended to do. + core.info(`Deleting ${branches.length} branch(es) classified \`reapable\` by the sweep job:`); + for (const branch of branches) core.info(` - ${branch}`); + + const deleted = []; + const alreadyGone = []; + const failed = []; + for (const branch of branches) { + core.info(`deleting ${branch}`); + try { + await github.rest.git.deleteRef({ owner, repo, ref: `heads/${branch}` }); + deleted.push(branch); + } catch (err) { + // A ref already gone is not a failure: `delete_branch_on_merge` + // reaps 1385 of every 1386 natively, so losing the race to the + // platform is the EXPECTED outcome, not an error. + if (err.status === 404 || err.status === 422) { + alreadyGone.push(branch); + core.info(` already gone: ${branch}`); + continue; + } + failed.push({ branch, status: err.status, message: err.message }); + core.warning(` FAILED ${branch}: ${err.status} ${err.message}`); + } + } + + const lines = ['## Merged-branch reaper — DELETION PASS', '']; + lines.push(`Deleted **${deleted.length}**, already gone **${alreadyGone.length}**, failed **${failed.length}**, of ${branches.length} branch(es) the sweep classified \`reapable\`.`); + lines.push(''); + lines.push(`⛔ Only the \`reapable\` bucket is touched. Every other bucket in the sweep's report is report-only and was not read by this job.`); + lines.push(''); + for (const b of deleted) lines.push(`- ✅ deleted \`${b}\``); + for (const b of alreadyGone) lines.push(`- • already gone \`${b}\``); + if (failed.length > 0) { + lines.push(''); + lines.push(`### ⛔ ${failed.length} deletion(s) failed`); + lines.push(''); + for (const f of failed) lines.push(`- \`${f.branch}\` — ${f.status} ${f.message}`); + } + await core.summary.addRaw(lines.join('\n')).write(); + + core.notice(`Reaped ${deleted.length} of ${branches.length} ${PREFIX} branches (${alreadyGone.length} already gone, ${failed.length} failed).`); + if (failed.length > 0) { + core.setFailed(`${failed.length} branch deletion(s) failed -- see the log for each status.`); + } diff --git a/scripts/check-merged-branch-reaper-outcome.mjs b/scripts/check-merged-branch-reaper-outcome.mjs index ab1b7bb5d1..2823c615ab 100644 --- a/scripts/check-merged-branch-reaper-outcome.mjs +++ b/scripts/check-merged-branch-reaper-outcome.mjs @@ -127,10 +127,11 @@ const SELF_TEST_BATTERY_FLOOR = 5; const UNATTRIBUTED_BATTERY = '(no battery open)'; const SELF_TEST_BATTERIES = Object.freeze({ '1. The unmutated shipped script must be green -- otherwise every red below': 1, - '2. Every mutation must be REACHED and must turn the battery red, in the': 57, + '2. Every mutation must be REACHED and must turn the battery red, in the': 67, '3. A script that does not compile is caught before any scenario runs.': 1, '4. Missing input is a failure, never a pass (#4690).': 1, '5. Wiring. A check nobody runs is the #4449 shape this repo keeps paying': 3, + '6. The DELETE job is fenced structurally -- which events reach it, and': 19, }); function repoRoot() { @@ -181,6 +182,109 @@ export function extractScript(root) { return { source, problems }; } +// ── The delete job's fence (#12771, armed 2026-09-04) ────────────────────── +// +// The scenario battery in this file drives the `sweep` script and can say +// NOTHING about the job that deletes. That job runs no classifier: it consumes +// the list `sweep` publishes and calls `git.deleteRef`. Deletion is kept out of +// the extracted script deliberately -- what this harness judges stays a +// classification rather than an action -- and the price of that is that the +// deleter's safety is entirely STRUCTURAL: which events reach it, and which +// token it holds. So it is asserted structurally, on the shipped bytes, and +// every assertion is driven to red by a mutation of those bytes (battery 6). +// +// ⛔ The clause that matters most is the `pull_request` exclusion. This +// workflow triggers on `pull_request` so that edits to it exercise the sweep +// before merging -- which means that without the exclusion, EDITING THIS FILE +// BECOMES DELETING BRANCHES, on the head of the very PR that edits it. Nothing +// in a YAML diff makes that visible; this does. + +const REAP_JOB = 'reap'; + +/** + * Structural facts about the delete job, read out of the workflow TEXT rather + * than from a path, so the self-test can mutate the text and re-read it. + * + * @param {string} text the workflow file's contents + */ +export function inspectReapJob(text) { + const doc = parseDocument(text); + if (doc.errors.length > 0) return { parsed: false, reason: `YAML parse error -- ${doc.errors[0].message}` }; + if (!isMap(doc.getIn(['jobs']))) return { parsed: false, reason: 'no `jobs:` map' }; + const scalar = (path) => { + const v = doc.getIn(path); + return v == null ? null : String(v); + }; + const jobIds = doc.getIn(['jobs']).items.map((item) => String(item.key)); + return { + parsed: true, + present: isMap(doc.getIn(['jobs', REAP_JOB])), + if: scalar(['jobs', REAP_JOB, 'if']) ?? '', + needs: scalar(['jobs', REAP_JOB, 'needs']), + contents: scalar(['jobs', REAP_JOB, 'permissions', 'contents']), + topLevelContents: scalar(['permissions', 'contents']), + contentsWriteJobs: jobIds.filter((id) => scalar(['jobs', id, 'permissions', 'contents']) === 'write'), + }; +} + +/** + * The fence stated as what is BROKEN. An empty array means it holds. + * + * @param {string} text the workflow file's contents + * @returns {string[]} + */ +export function reapFenceFailures(text) { + const r = inspectReapJob(text); + if (!r.parsed) return [`${WORKFLOW} does not parse -- ${r.reason}`]; + if (!r.present) { + return [ + `${WORKFLOW}: job \`${REAP_JOB}\` is gone. This fence describes the job that DELETES; if deletion moved, ` + + 'it moved somewhere nothing asserts, which is the state this fence exists to prevent.', + ]; + } + const f = []; + if (!r.if.includes("github.event_name != 'pull_request'")) { + f.push( + `${WORKFLOW}: job \`${REAP_JOB}\` no longer excludes \`pull_request\` in its \`if:\` ` + + `(got: ${r.if || 'no `if:` at all'}). This workflow triggers on \`pull_request\` so that edits exercise ` + + 'the sweep -- without this exclusion, editing this file deletes branches, on the head of the PR that edits it.', + ); + } + if (!r.if.includes('inputs.dry_run == false')) { + f.push( + `${WORKFLOW}: job \`${REAP_JOB}\` no longer gates \`workflow_dispatch\` on \`inputs.dry_run == false\`. ` + + 'That input defaults to TRUE; dropping the check turns every manual run into a deleting run, which is the ' + + 'opposite of the fail-closed reading the ruling asks for.', + ); + } + if (r.contents !== 'write') { + f.push( + `${WORKFLOW}: job \`${REAP_JOB}\` does not declare \`permissions: contents: write\` (got ` + + `${r.contents ?? 'nothing'}) -- it cannot delete a ref, so the reaper is armed in prose only.`, + ); + } + if (r.topLevelContents !== 'read') { + f.push( + `${WORKFLOW}: the top-level \`permissions.contents\` is \`${r.topLevelContents ?? 'unset'}\`, not \`read\`. ` + + 'The write grant must stay a local override on the one job that deletes, never the default every job inherits.', + ); + } + if (r.contentsWriteJobs.join(',') !== REAP_JOB) { + f.push( + `${WORKFLOW}: the jobs holding \`contents: write\` are [${r.contentsWriteJobs.join(', ')}], expected exactly ` + + `[${REAP_JOB}]. The classifier must keep the read-only token it was reviewed under -- a defect in ` + + 'classification can then still not delete anything by itself.', + ); + } + if (r.needs !== 'sweep') { + f.push( + `${WORKFLOW}: job \`${REAP_JOB}\` no longer declares \`needs: sweep\` (got ${r.needs ?? 'nothing'}) -- ` + + 'it would run without the classification it exists to consume.', + ); + } + return f; +} + // ── Doubles ───────────────────────────────────────────────────────────────── /** @@ -408,6 +512,10 @@ const SCENARIOS = [ t(bucket(r, 'mergedElsewhere').length === 0, 'G1 leaves the held bucket empty'), t((r.payload?.buckets?.reapable?.[0] ?? {}).base_pr === 101, 'G1 records WHICH pull request cleared the base-ref guard'), t(r.log.outputs.reapable === '1', `G1 reports one reapable branch on the step output, got ${r.log.outputs.reapable}`), + t( + r.log.outputs.reapable_branches === JSON.stringify(['claude/landed-on-main']), + `G1 publishes the machine-readable deletion list the \`reap\` job consumes, and it is exactly the reapable bucket, got ${r.log.outputs.reapable_branches}`, + ), ], }, { @@ -429,6 +537,10 @@ const SCENARIOS = [ t(r.summary().includes('claude/stacked-on-a-sibling'), 'G2 is NAMED in the report -- a held branch nobody can see is a branch excluded in silence'), t(r.summary().includes('claude/the-base'), 'G2 names the base in the report, which is what a human needs to clear it'), t(r.log.outputs.merged_elsewhere === '1', `G2 reports the held count on the step output, got ${r.log.outputs.merged_elsewhere}`), + t( + r.log.outputs.reapable_branches === '[]', + `G2 publishes an EMPTY deletion list -- a branch held by the base-ref guard must never reach the job that DELETES, and the guard is worth nothing if the hand-off leaks past it. Got ${r.log.outputs.reapable_branches}`, + ), t((r.log.notice[0] ?? '').includes('base-ref guard'), 'G2 says on the run notice that a branch was held by the guard'), ], }, @@ -603,6 +715,14 @@ const SCENARIOS = [ t(r.summary().includes('claude/r-held'), 'R1 names the held branch in the excluded section'), t(r.summaryWritten(), 'R1 writes the job summary'), t(r.log.failed.length === 0, `R1 does not fail the job over findings, got ${r.log.failed.join(' | ')}`), + t( + r.log.outputs.reapable_branches === JSON.stringify(['claude/r-reapable']), + `R1 publishes a deletion list of exactly the reapable bucket over a population with one branch in EVERY bucket, got ${r.log.outputs.reapable_branches}`, + ), + t( + JSON.parse(r.log.outputs.reapable_branches || 'null')?.join(',') === bucket(r, 'reapable').join(','), + 'R1: the published deletion list EQUALS the reapable bucket -- same members, same order. This is the contract the `reap` job rests on: it classifies nothing, so what deletes is precisely what this list says.', + ), ], }, ]; @@ -859,6 +979,20 @@ const MUTATIONS = [ to: 'if (false) {', expect: ['P4'], }, + { + id: 'M13', + what: 'the deletion list stops being the reapable bucket -- held branches leak into what `reap` deletes', + from: 'JSON.stringify(buckets.reapable.map((r) => r.branch))', + to: 'JSON.stringify(buckets.reapable.concat(buckets.mergedElsewhere).map((r) => r.branch))', + expect: ['G2', 'R1'], + }, + { + id: 'M14', + what: 'the deletion list stops being published at all, so `reap` consumes an empty hand-off and silently reaps nothing', + from: "core.setOutput('reapable_branches',", + to: "void ('reapable_branches',", + expect: ['G1', 'R1'], + }, ]; // Returned by `selfTest()` only after its verdict is printed. The dispatch @@ -933,6 +1067,58 @@ async function selfTest() { assert(body.includes(`${SELF} --self-test`), `wiring: ${LINT_WORKFLOW} runs the --self-test half too`); } + battery('6. The DELETE job is fenced structurally -- which events reach it, and'); + const wfText = readFileSync(join(root, WORKFLOW), 'utf8'); + const cleanFence = reapFenceFailures(wfText); + assert(cleanFence.length === 0, `the shipped delete job's fence holds, got: ${cleanFence.join(' | ')}`); + // Each mutation asserts its own anchor was PRESENT first: a substitution that + // matched nothing leaves the fence green and reads exactly like a pass. + const FENCE_MUTATIONS = [ + { + id: 'F1', + what: 'the `pull_request` exclusion is dropped, so editing this workflow deletes branches on the PR head', + from: "\n && github.event_name != 'pull_request'", + to: '', + }, + { + id: 'F2', + what: 'the manual path stops honouring the `dry_run` default, so every workflow_dispatch deletes', + from: ' && inputs.dry_run == false', + to: '', + }, + { + id: 'F3', + what: 'the delete job loses its write grant, so the reaper is armed in prose only', + from: ' permissions:\n contents: write', + to: ' permissions:\n contents: read', + }, + { + id: 'F4', + what: 'the write grant is raised to the top level, where the classifier inherits it too', + from: '\npermissions:\n contents: read\n', + to: '\npermissions:\n contents: write\n', + }, + { + id: 'F5', + what: 'the delete job is renamed, so this fence describes a job that no longer exists', + from: '\n reap:\n', + to: '\n reaper:\n', + }, + { + id: 'F6', + what: 'the delete job stops depending on the classification it consumes', + from: ' needs: sweep\n', + to: '', + }, + ]; + for (const m of FENCE_MUTATIONS) { + assert(wfText.includes(m.from), `${m.id}: its anchor is present in the shipped workflow (a no-op mutation proves nothing)`); + if (!wfText.includes(m.from)) continue; + const mutated = wfText.split(m.from).join(m.to); + assert(mutated !== wfText, `${m.id}: the substitution changed the workflow text`); + assert(reapFenceFailures(mutated).length > 0, `${m.id}: ${m.what} -- the fence goes RED`); + } + // ── The floor: every declared battery RAN, and ran its cases (#13489) ─── const declaredBatteries = Object.keys(SELF_TEST_BATTERIES); let floorBreached = false; From 52fc3978b8bccb34aea5a23875878d893e196c3b Mon Sep 17 00:00:00 2001 From: os-sales Date: Fri, 4 Sep 2026 07:04:43 +0000 Subject: [PATCH 4/6] fix(service-automation): compile the test layer with tsc, and repair the TS2341 x3 it hid (#15152) * wip: onboard service-automation typecheck, fix TS2341 residue * wip: onboarding gate registry entry + changeset * fix(scripts): re-measure this entry's provenance totals on the merged tree The `service-knowledge` onboarding landed on `main` between this entry's first reading and this merge, so every absolute in its provenance block (programs, pairs, packages, clean count) was a number about a tree that no longer exists. Re-taken with `--list` on the merge commit itself, all four rows plus the before/after pair, by varying only what the `typecheck` script names: no `typecheck` script absent 120 programs / 293 pairs names tsconfig.json absent 120 programs / 293 pairs names tsconfig.test PRESENT 121 programs / 302 pairs names both (the card) PRESENT 121 programs / 302 pairs before 59 of 78 packages, 120 programs, 293 pairs, 19 clean after 60 of 78 packages, 121 programs, 302 pairs, 18 clean The deltas this block actually claims (+1 package, +1 program, +9 pairs, one per dep) are unchanged; only the absolutes moved, and the block now says which merge moved them. The sibling entries' own blocks keep their own historical readings untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --------- Co-authored-by: Claude --- .../service-automation-test-tsc-program.md | 62 +++++++++++++ .../services/service-automation/package.json | 5 +- .../src/nested-region-parity.test.ts | 10 +-- .../service-automation/tsconfig.test.json | 88 +++++++++++++++++++ pnpm-lock.yaml | 3 + scripts/check-type-check-coverage.mjs | 34 ++++--- scripts/check-type-source-resolution.mjs | 54 ++++++++++++ 7 files changed, 236 insertions(+), 20 deletions(-) create mode 100644 .changeset/service-automation-test-tsc-program.md create mode 100644 packages/services/service-automation/tsconfig.test.json diff --git a/.changeset/service-automation-test-tsc-program.md b/.changeset/service-automation-test-tsc-program.md new file mode 100644 index 0000000000..8ca814c360 --- /dev/null +++ b/.changeset/service-automation-test-tsc-program.md @@ -0,0 +1,62 @@ +--- +"@objectstack/service-automation": patch +--- + +fix(service-automation): put the test layer in front of tsc, and repair the TS2341 x3 it was hiding (#15048) + +`packages/services/service-automation` had **no `typecheck` script at all** — +its scripts were `build` and `test` — so no tsc program anywhere read this +package (`turbo run typecheck` selects only packages that declare the task, so +it skipped this one silently). `tsup` transpiles with esbuild and `vitest` +runs through esbuild type-**stripping**; neither type-checks. The package's +own `tsconfig.json` does include the tests and always did, so the program that +would have read them already existed and was simply never invoked. This is +the `packages/services/**` sibling of `@objectstack/service-cluster`'s same +graduation (#14181 / PR #15032), reached by the same road in. + +What that hid was three `TS2341`s, all in +`src/nested-region-parity.test.ts` (lines 95/151/180): + +``` +error TS2341: Property 'flows' is private and only accessible within class 'AutomationEngine'. +``` + +Three tests dot-read the private `AutomationEngine#flows` map directly +instead of going through the class's own public accessor, +`await engine.getFlow(name)` — already the idiom every other test file in +this package uses. The fix replaces the three private reads with that +existing public call (making the two synchronous test bodies `async` where +they were not already); no source signature was widened, no cast was added. + +Wired by the route the `packages/plugins/**` family settled on in #14062 and +`service-cluster` carried into `packages/services/**` in #14181: a sibling +`tsconfig.test.json` that changes **module semantics only** (`esnext` / +`bundler` / `lib: ES2022`, matching how vitest actually executes these files) +with **strictness inherited and untouched**, named by a new `typecheck` +script through the shared `check:test-typecheck` gate. Measured before the +repair: 3 errors under build semantics (`tsc -p tsconfig.json`, which already +included the tests), 3 under the new config — the two readings agree, so this +package carried no config-tier pile, and all 3 were genuinely code-tier from +the start. After: 0 and 0, across a 555-file program covering all 103 of its +`src/**/*.test.ts`. + +No `test-typecheck-debt.json` is added, and its **absence is the zero**: the +gate reads a missing ledger as `{ entries: {} }`, under which any error in any +file here is red immediately. The package's `DEBT` entry in +`scripts/check-type-check-coverage.mjs` (`errors: 3`) is deleted in this PR +rather than lowered — that is the graduation the ratchet's own invariant +requires, and it is why the errors were fixed rather than ledgered. + +`scripts/check-type-source-resolution.mjs` also gains a registry entry for +this package: onboarding `tsconfig.test.json` moved the package's tsc program +set (per that gate's documented onboarding-limb terms), exposing 9 workspace +deps whose types resolve through `dist/` with no pre-existing program for them +to have been laundered through. `paths` was measured and rejected as the +alternative — it takes this package's test layer from 0 errors to 648, nearly +all billed to other packages' source. + +No runtime code changes: `src/**` (excluding the one edited test file, whose +own assertions are unchanged — only how it reaches the flow moved) is +otherwise byte-identical, so no shipped behaviour moves. The `patch` level +reflects the published `package.json` gaining `typecheck` / +`check:test-typecheck` scripts and a `tsx` devDependency. diff --git a/packages/services/service-automation/package.json b/packages/services/service-automation/package.json index 3cec97a038..cf8fe1cba0 100644 --- a/packages/services/service-automation/package.json +++ b/packages/services/service-automation/package.json @@ -20,7 +20,9 @@ }, "scripts": { "build": "tsup --config ../../../tsup.config.ts && node ../../../scripts/check-dts-emitted.mjs", - "test": "vitest run" + "test": "vitest run", + "typecheck": "tsc --noEmit && pnpm check:test-typecheck", + "check:test-typecheck": "tsx ../../../scripts/check-test-typecheck.mts --self-test && tsx ../../../scripts/check-test-typecheck.mts --package packages/services/service-automation --project tsconfig.test.json" }, "dependencies": { "@objectstack/core": "workspace:*", @@ -38,6 +40,7 @@ "@objectstack/service-job": "workspace:*", "@objectstack/service-messaging": "workspace:*", "@types/node": "^26.2.0", + "tsx": "^4.23.12", "typescript": "^6.0.3", "vitest": "^4.1.10" }, diff --git a/packages/services/service-automation/src/nested-region-parity.test.ts b/packages/services/service-automation/src/nested-region-parity.test.ts index 4236bf9b6b..645ac935b2 100644 --- a/packages/services/service-automation/src/nested-region-parity.test.ts +++ b/packages/services/service-automation/src/nested-region-parity.test.ts @@ -90,9 +90,9 @@ describe('#4347 — a loop-body predicate is canonicalized like a top-level one' it.each([ ['a bare string', CONDITION], ['an explicit CEL envelope', ENVELOPE], - ])('stores %s as the canonical envelope on BOTH edges', (_label, condition) => { + ])('stores %s as the canonical envelope on BOTH edges', async (_label, condition) => { engine.registerFlow('repro', reproFlow(condition)); - const flow = engine.flows.get('repro')!; + const flow = (await engine.getFlow('repro'))!; const topEdge = flow.edges.find(e => e.id === 'e2')!.condition; const bodyEdge = (flow.nodes.find(n => n.id === 'loop')!.config as any).body.edges[0].condition; @@ -148,13 +148,13 @@ describe('#4347 — the conversion table reaches a node inside a region', () => edges: [{ id: 'e1', source: 'start', target: 'loop', type: 'default' }], }); - expect((engine.flows.get('callout')!.nodes[1]!.config as any).body.nodes[0].type).toBe('http'); + expect(((await engine.getFlow('callout'))!.nodes[1]!.config as any).body.nodes[0].type).toBe('http'); const result = await engine.execute('callout', { params: {}, event: 'schedule' } as never); expect(result.success).toBe(true); expect(called).toEqual(['nested']); }); - it('canonicalizes a nested CRUD alias — an unconverted `filters` leaves no filter at all', () => { + it('canonicalizes a nested CRUD alias — an unconverted `filters` leaves no filter at all', async () => { const engine = new AutomationEngine(silentLogger()); registerLoopNode(engine, ctx()); engine.registerNodeExecutor({ type: 'delete_record', async execute() { return { success: true }; } } as NodeExecutor); @@ -177,7 +177,7 @@ describe('#4347 — the conversion table reaches a node inside a region', () => edges: [{ id: 'e1', source: 'start', target: 'loop', type: 'default' }], }); - expect((engine.flows.get('purge')!.nodes[1]!.config as any).body.nodes[0].config) + expect(((await engine.getFlow('purge'))!.nodes[1]!.config as any).body.nodes[0].config) .toEqual({ objectName: 'lead', filter: { status: 'stale' } }); }); }); diff --git a/packages/services/service-automation/tsconfig.test.json b/packages/services/service-automation/tsconfig.test.json new file mode 100644 index 0000000000..151ada1a66 --- /dev/null +++ b/packages/services/service-automation/tsconfig.test.json @@ -0,0 +1,88 @@ +// The TEST-layer type-check program (#15048 — the `packages/services/**` +// instance of the class #14062 settled for `packages/plugins/**` and #14181 +// carried to `service-cluster` (PR #15032), itself adopting the mechanism +// #5286 set for `packages/spec`, #5449 generalised, #12542 carried to +// `packages/rest` and #13176 to `packages/plugins/plugin-security`). +// `tsconfig.json` beside this one stays exactly as it is: it is the BUILD +// config. This sibling puts the test layer in front of tsc under the module +// semantics vitest really executes it with, and `package.json`'s `typecheck` +// script NAMES it (via `check:test-typecheck --project`), because a config no +// script invokes is exactly the phantom this whole change is about. +// +// ⚠️ WHY THIS PACKAGE COPIES `plugin-webhooks` / `service-cluster` RATHER THAN +// `plugin-auth` / `plugin-sharing` / `core`: the deciding property is what the +// BUILD config does with tests. `service-automation`'s `tsconfig.json` does +// NOT exclude `src/**/*.test.ts` and never did — its `include` is `["src"]` +// with no test exclusion — so the program that would have read them already +// existed; it was simply never invoked (no `typecheck` script at all, only +// `build` and `test`). That is the `plugin-webhooks`/`service-cluster` shape, +// not the `exclude`-and-compensate shape the other three packages carry, and +// AGENTS.md forbids ADDING such an exclusion, so their route does not +// transfer here. +// +// What differs from the build config, and what deliberately does NOT: +// - MODULE SEMANTICS ONLY, plus `lib`. The tests are written and executed as +// ESM by vitest (esbuild/vite). Matching that is FIDELITY, not laxity: it +// is the same subtraction `packages/spec`, `packages/rest`, +// `plugin-security`, `plugin-webhooks` and `service-cluster` each made. +// - ⛔ STRICTNESS IS UNTOUCHED. `strict`, `noUnusedLocals`, +// `noUnusedParameters`, `noImplicitReturns`, `noFallthroughCasesInSwitch`, +// `rootDir`, `paths` and `types` are all INHERITED from `tsconfig.json` +// (and through it the root config), and none of them is re-declared here. +// ⚠️ A child that declared its own `paths` would REPLACE the parent map +// rather than merge into it, silently sending a source-resolved specifier +// back to `dist/` — a BUILD ARTIFACT — so this file declares none. +// Nothing here may loosen a type rule; if a test does not compile, that is +// the finding. +// - `lib: ["ES2022"]`, for the same reason `packages/rest` states: the root +// config's `lib` is ES2020 and vitest runs on a Node that has es2022 +// builtins, so the gap is reported as TS2550 about the CHECK. No `DOM`: +// nothing in this layer touches a browser global. +// +// MEASURED at 2cc4610304 (origin/main), workspace closure built first +// (`pnpm --filter '@objectstack/service-automation^...' build`, then +// `tsc --noEmit --pretty false --listFiles -p tsconfig.test.json`, and the +// same command without `--listFiles`), BEFORE any fix: +// +// files in this program 555 +// own `src/**/*.test.ts` in it 103 +// errors under BUILD semantics (tsc -p tsconfig.json, which already +// included the tests) 3 +// errors under THIS config 3 +// +// The two readings AGREE, so this package carried no config-tier pile at all +// — unlike `@objectstack/core` (#14916: 98 undivided -> 4 after the split, +// nearly all TS7006 cascading from one unresolved import) — and the 3 were +// genuinely code-tier from the start: all TS2341 ("Property 'flows' is +// private…"), all in `src/nested-region-parity.test.ts` (lines 95/151/180), +// where three tests dot-read the PRIVATE `AutomationEngine#flows` map +// directly instead of going through the class's own public accessor. That +// accessor already exists and is already the idiom every other test file in +// this package uses — `await engine.getFlow(name)` (defined at +// `src/engine.ts`, returns `this.flows.get(name) ?? null`) — so the fix is +// not a workaround: it replaces three private-internals reads with the public +// surface the class was already offering, exactly as the rest of the suite +// does. AFTER: 0 and 0, across a 555-file program covering all 103 of this +// package's `src/**/*.test.ts`. +// +// There is NO `test-typecheck-debt.json` beside this config, and its ABSENCE +// is the zero: `check:test-typecheck` reads a missing ledger as +// `{ entries: {} }`, under which ANY error in ANY file here is red +// immediately, with no entry to be added to. That is strictly stronger than a +// ledger holding nothing, and it is the same call `plugin-webhooks`, +// `plugin-security` (#13176) and `service-cluster` (#14181) each recorded for +// themselves. If this package ever acquires residue that cannot be fixed in +// the PR that causes it, THAT is when a ledger and a `gen:test-typecheck-debt` +// script are owed — and adding one is maintainer-only (#5286), exactly as the +// gate says when it refuses. +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "module": "esnext", + "moduleResolution": "bundler", + "lib": ["ES2022"] + }, + "include": ["src/**/*"], + "exclude": ["node_modules", "dist"] +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a615c9086e..0c521ae930 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2336,6 +2336,9 @@ importers: '@types/node': specifier: ^26.2.0 version: 26.2.0 + tsx: + specifier: ^4.23.12 + version: 4.23.12 typescript: specifier: ^6.0.3 version: 6.0.3 diff --git a/scripts/check-type-check-coverage.mjs b/scripts/check-type-check-coverage.mjs index 7b22478012..0be82dc27e 100644 --- a/scripts/check-type-check-coverage.mjs +++ b/scripts/check-type-check-coverage.mjs @@ -700,6 +700,26 @@ const ROOT_PROGRAM_COUPLED_SCRIPT = 'scripts/check-test-typecheck.mts'; // field the real call passes (TS2339). All 4 are fixed in the test file, // matching each mock's type to the call site it stubs; `ExecutionContext` // itself was not touched (it was correct -- the test's field name was stale). +// +// `@objectstack/service-automation` GRADUATED from this ledger (#15048; entry: +// 3 raw, repaired to 0), the `packages/services/**` sibling of the +// `service-cluster` graduation above (#14181/PR #15032) -- same road in: no +// `typecheck` script at all (only `build` and `test`), and a BUILD +// `tsconfig.json` that does NOT exclude tests, so the program that would have +// read them already existed and was simply never invoked. Measured BOTH ways +// (`tsc -p tsconfig.json`, which already included the tests, vs the new +// `tsconfig.test.json`): 3 and 3 -- the two readings AGREE, so this package +// carried no config-tier pile either, and the 3 were genuinely code-tier from +// the start. All 3 were TS2341 ("Property 'flows' is private..."), all in +// `src/nested-region-parity.test.ts` (95/151/180), where three tests dot-read +// the PRIVATE `AutomationEngine#flows` map directly instead of the class's own +// public accessor -- `await engine.getFlow(name)`, already the idiom every +// other test file in this package uses. Repaired by replacing the three +// private reads with that existing public call (no widened source signature, +// no cast, no bracket-notation workaround); the tests were made `async` where +// they were not already. Repaired by the #5286 route -- a `tsconfig.test.json` +// over the test layer, named by a new `typecheck` script -- so the entry is +// deleted rather than lowered. const DEBT = { '@objectstack/cloud-connection': { errors: 13, @@ -713,20 +733,6 @@ const DEBT = { errors: 11, note: 'all code-tier (TS2554 wrong arity x10, TS2552).', }, - '@objectstack/service-automation': { - errors: 3, - note: 'code-tier 3 (TS2341 x3), all in src/nested-region-parity.test.ts at 95/151/180, where the ' - + 'tests dot-read the private `engine.flows` -- not `engine[\'flows\']`, not `as any` (the casts on ' - + 'two of those lines sit on `.config`, not on the engine, so they do not suppress it). Re-measured ' - + '3 at 53a48c93f4, DOWN from 5 at 5ab08428: the two TS2741 in engine.test.ts this note used to ' - + 'itemise alongside them have graduated -- that file now builds its pausing fixtures through a ' - + 'single defineActionDescriptor helper that declares resumeAuthority (#5561), and engine.test.ts ' - + 'still compiles in this project (`--listFiles` lists it) while reporting nothing. The residue is ' - + 'therefore one decision, not an oversight: whether tests may read private state at all. This ' - + 'entry is the specimen #5278 cites for composition drift and has now drifted BOTH ways -- 2 -> 5 ' - + 'by acquiring a second file, then 5 -> 3 by graduating the first -- so re-read what the pile is ' - + 'made of before sizing it, never just the number.', - }, '@objectstack/service-storage': { errors: 51, note: 'code-tier 8 (TS2339 x4, TS2347 x4); config-tier 26 (TS2835 x23, TS2550 x3); noise 17 ' diff --git a/scripts/check-type-source-resolution.mjs b/scripts/check-type-source-resolution.mjs index 3f3b214488..d895d3e321 100644 --- a/scripts/check-type-source-resolution.mjs +++ b/scripts/check-type-source-resolution.mjs @@ -625,6 +625,60 @@ const KNOWN_DIST_RESOLVED_TYPE_IMPORTS = { '@objectstack/lint', '@objectstack/mcp', '@objectstack/platform-objects', '@objectstack/plugin-auth', '@objectstack/spec', '@objectstack/types', ], + // #15048 re-baseline (the onboarding limb above): a NEW entry, reached ONLY + // through `tsconfig.test.json` -- a program this card ADDED. Same shape as + // the `service-cluster` re-baseline below (#14181): `service-automation` had + // NO `typecheck` script AT ALL before (its scripts were `build` and `test`), + // and its build `tsconfig.json` -- which is ALWAYS a counted program per this + // gate's own design (see `programConfigsFor`'s doc-block) -- measured clean + // on its own, so there is no pre-existing program a dep could be laundered + // through. All 9 deps here are annotated `via tsconfig.test.json` by this + // gate's own failure text. + // + // Provenance measured four ways on one checkout, by varying only what the + // `typecheck` script NAMES (`--list`, totals as printed). RE-MEASURED on the + // merge of `origin/main` @ 919beca43b, which had landed the `service-knowledge` + // onboarding below (#15049) since this card's first reading: that merge moved + // every ABSOLUTE here (+1 program, +3 pairs, +1 package before this entry + // exists) and moved none of the DELTAS, which are what this block claims. + // + // no `typecheck` script (origin/main) absent 120 programs / 293 pairs + // names `tsconfig.json` only absent 120 programs / 293 pairs + // names `tsconfig.test.json` only PRESENT 121 programs / 302 pairs + // names both (this card) PRESENT 121 programs / 302 pairs + // + // Row 2 is the load-bearing one: the BUILD program (which already includes + // every test file -- `tsconfig.json`'s `include` has never excluded them) + // carries no dist-resolved workspace type import at all, so the exposure is + // not merely first SEEN through the onboarded program, it is only REACHABLE + // through it. Numbers, before/after on the same checkout: + // + // before 59 of 78 packages, 120 programs, 293 pairs, 19 clean + // after 60 of 78 packages, 121 programs, 302 pairs, 18 clean + // + // so +1 package, +1 program, +9 pairs (one per dep below) -- this entry and + // nothing else. + // + // Why the entry and not `paths`: MEASURED, not argued. Redirecting these 9 + // deps to source takes this package's test layer from 0 errors to 648 (647 + // TS6059 `not under rootDir` + 1 TS6133), ALL 647 of the TS6059 in ANOTHER + // package's source -- zero name a file under this package's own `src/`: + // `packages/spec/src/**` 379, `packages/core/src/**` 62, + // `packages/plugins/plugin-security/src/**` 60, `packages/objectql/src/**` + // 49, `packages/services/service-messaging/src/**` 41, `packages/ + // metadata-core/src/**` 29, `packages/formula/src/**` 15, `packages/ + // services/service-job/src/**` 6, `packages/drivers/driver-sql/src/**` 6 -- + // billed to packages that cannot pay them down. Same finding as the + // `service-cluster` re-baseline below (#14181: 0 -> 435) and PR #12570's + // before it, reproduced again at a larger scale because this package pulls + // more workspace deps. The #5286 route this entry backs makes this + // package's OWN test files compile clean; `paths` would immediately re-bury + // that result under other packages' diagnostics. + '@objectstack/service-automation': [ + '@objectstack/core', '@objectstack/driver-sql', '@objectstack/formula', + '@objectstack/metadata-core', '@objectstack/objectql', '@objectstack/plugin-security', + '@objectstack/service-job', '@objectstack/service-messaging', '@objectstack/spec', + ], // #14181 re-baseline (the onboarding limb above): a NEW entry, reached ONLY // through `tsconfig.test.json` -- a program this card ADDED. This is the // limb's cleanest case rather than a borderline one: `service-cluster` had NO From 3921b51df5aa0b85e81cfb4bf38f23abbaa5e727 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 07:43:58 +0000 Subject: [PATCH 5/6] docs(platform-objects): widen sys_email.error description to cover pre-delivery rejections `sys_email.error` was declared as "Transport error message when status=failed", but since #14371 EmailService.recordRejectedMessage also writes status=failed rows for messages rejected by normalizeMessage before they reach a transport (prefixed "rejected before delivery: ..."). The declared field help was narrower than what the column actually holds. Widen the description (wording settled in triage, issue comment 5504375428) and regenerate the platform-objects i18n bundle with its own tooling (node scripts/check-i18n-bundles.mjs --write) rather than hand-editing the generated file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .../src/apps/translations/en.objects.generated.ts | 2 +- packages/platform-objects/src/audit/sys-email.object.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/platform-objects/src/apps/translations/en.objects.generated.ts b/packages/platform-objects/src/apps/translations/en.objects.generated.ts index ed722edf31..61f17e21e7 100644 --- a/packages/platform-objects/src/apps/translations/en.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/en.objects.generated.ts @@ -2476,7 +2476,7 @@ export const enObjects: NonNullable = { }, error: { label: "Error", - help: "Transport error message when status=failed" + help: "Why the message failed — a transport error, or the validation that rejected it before delivery." }, attempt_count: { label: "Attempts", diff --git a/packages/platform-objects/src/audit/sys-email.object.ts b/packages/platform-objects/src/audit/sys-email.object.ts index 073cee51bf..c154aba052 100644 --- a/packages/platform-objects/src/audit/sys-email.object.ts +++ b/packages/platform-objects/src/audit/sys-email.object.ts @@ -192,7 +192,8 @@ export const SysEmail = ObjectSchema.create({ error: Field.textarea({ label: 'Error', required: false, - description: 'Transport error message when status=failed', + description: + 'Why the message failed — a transport error, or the validation that rejected it before delivery.', group: 'State', }), From 79146dc9f9ed665b2793c807107d1735e6c36a02 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 08:00:07 +0000 Subject: [PATCH 6/6] chore(platform-objects): add changeset for sys_email.error description widening Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .../sys-email-error-description-widen.md | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .changeset/sys-email-error-description-widen.md diff --git a/.changeset/sys-email-error-description-widen.md b/.changeset/sys-email-error-description-widen.md new file mode 100644 index 0000000000..e27ce70c96 --- /dev/null +++ b/.changeset/sys-email-error-description-widen.md @@ -0,0 +1,22 @@ +--- +"@objectstack/platform-objects": patch +--- + +fix(platform-objects): `sys_email.error` field help now covers pre-delivery rejections, not only transport failures + +`sys_email.error` was declared as *"Transport error message when status=failed"*. +Since `EmailService.recordRejectedMessage` landed, the same column also carries +the reason a message was rejected by `normalizeMessage` **before** it reached a +transport (an unsendable `from`, no recipient, no subject, no body) — those rows +are written with `status: 'failed'` too, prefixed `rejected before delivery: `. + +Nothing was misleading in the *data*: the row prefixes its own reason, so an +operator reading a failed row is never sent chasing an SMTP host for a message +that never reached one. What was stale was the field's declared `description`, +which Studio surfaces as the field's help text — it named only the transport +case, narrower than what the column has held since that change landed. + +The description now reads: *"Why the message failed — a transport error, or the +validation that rejected it before delivery."* It stays true under both row +shapes and deliberately does not name the row's own `rejected before delivery:` +prefix, so it will not go stale again if that prefix's wording changes.