From 64cff1b836f1deebfb1fc9f7e048e6ebf35d3620 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 14:51:40 +0000 Subject: [PATCH 1/2] feat(spec,approvals): escalation.enabled defaults true and the SLA sweep enforces it (#12278) Maintainer ruling 2026-08-27 (Option C): the feature switch is whether an escalation block exists at all; within a block carrying timeoutHours, escalation is on unless explicitly turned off. - spec: ApprovalEscalationSchema.enabled .default(false) -> .default(true), declared in DEFAULT_CHANGES_BY_MAJOR (17) and registered as the approval-escalation-enabled-default-flip semantic migration entry; authorable-defaults, reference docs, spec-changes and upgrade guide regenerated by repo tooling. - runtime: runEscalations skips snapshots carrying an explicit enabled === false; snapshots created before ESCALATION_ENABLED_FLIP_CUTOFF_MS ride a read-side legacy window (they carry the old default materialized by the executor parse and are escalating today) - zero tenant writes, self-retiring as pending requests drain. - tests: explicit-false-does-not-escalate pin, legacy-window pin, omitted-key pin; spec pin for the new default materialization. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01JvjTCjJQn9zSTXEhUKgT7s --- ...pproval-escalation-enabled-default-flip.md | 48 ++++++++++++ .../docs/references/automation/approval.mdx | 4 +- docs/protocol-upgrade-guide.md | 3 + .../src/approval-service.test.ts | 76 ++++++++++++++++++- .../plugin-approvals/src/approval-service.ts | 44 ++++++++++- .../spec/authorable-defaults/automation.json | 2 +- packages/spec/scripts/lib/default-changes.ts | 31 ++++++++ packages/spec/spec-changes.json | 14 ++++ packages/spec/src/automation/approval.test.ts | 15 ++++ packages/spec/src/automation/approval.zod.ts | 8 +- ...pproval-escalation-enabled-default-flip.ts | 49 ++++++++++++ packages/spec/src/migrations/registry.ts | 45 +++++++++++ 12 files changed, 333 insertions(+), 6 deletions(-) create mode 100644 .changeset/approval-escalation-enabled-default-flip.md create mode 100644 packages/spec/src/migrations/entries/semantic/17.approval-escalation-enabled-default-flip.ts diff --git a/.changeset/approval-escalation-enabled-default-flip.md b/.changeset/approval-escalation-enabled-default-flip.md new file mode 100644 index 0000000000..8d55339245 --- /dev/null +++ b/.changeset/approval-escalation-enabled-default-flip.md @@ -0,0 +1,48 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-approvals": minor +--- + +feat(spec,plugin-approvals): `escalation.enabled` defaults to `true` and the SLA sweep finally reads it (#12278) + +**BREAKING** semantic default flip on a published authorable key, shipped as +`minor` under the repo's launch-window convention for breaking changes. +Maintainer ruling 2026-08-27 (Option C), explicitly reversing the 2026-08-26 +"spec stays as declared" ruling with fresh analysis. + +`ApprovalEscalationSchema.enabled` declared `default(false)` while the +plugin-approvals escalation sweep never read the key: any escalation block +with a positive `timeoutHours` escalated, and with `action: 'auto_approve'` +that silently approved requests their author had declared off the clock — +the ADR-0049 declared-but-unenforced shape, failing open. Worse, the +approval-node executor parses node config through the schema before +snapshotting it onto the request row, so the old default **materialized** +`enabled: false` into storage for every author who omitted the key, making +"authored off" and "defaulted off" byte-identical at the sweep site. + +One change, both halves: + +- **spec**: `enabled` now defaults to `true` (stays `z.boolean()`; no + tri-state). The feature-level switch is whether an `escalation` block + exists at all; within a block carrying `timeoutHours`, escalation is on + unless explicitly turned off — which is what the runtime, its eleven + behaviour tests, and every teaching surface have always meant. Declared in + `DEFAULT_CHANGES_BY_MAJOR` (17) and registered as the + `approval-escalation-enabled-default-flip` semantic migration entry. +- **runtime**: `runEscalations` skips a request whose snapshot carries an + explicit `escalation.enabled === false` — the declared switch is enforced. + Request snapshots created **before** the flip cutoff + (`ESCALATION_ENABLED_FLIP_CUTOFF_MS`, 2026-08-28T00:00:00Z) ride a + read-side legacy window and keep escalating exactly as they do today: their + stored `false` is overwhelmingly the old schema default materialized onto an + author who never wrote the key, every such stored row is escalating today, + and the window retires itself as pending requests drain — zero tenant rows + rewritten. + +Deployed metadata that omits `enabled` does not change behaviour (it +escalated before, it escalates after). What changes is that writing +`enabled: false` finally binds for newly opened requests — the declared +intent being honoured. To keep an SLA off, write `enabled: false`; to +escalate on timeout, an `escalation` block with `timeoutHours` is enough. + + diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index c4a2deb1df..7946a3989e 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -37,7 +37,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **enabled** | `boolean` | optional (default: `false`) | Enable SLA-based escalation for this node | +| **enabled** | `boolean` | optional (default: `true`) | SLA escalation switch. Defaults to true: an escalation block carrying timeoutHours is live unless this is explicitly false — the feature-level switch is whether the escalation block exists at all | | **timeoutHours** | `number` | ✅ | Hours before escalation triggers | | **action** | `Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>` | optional (default: `"notify"`) | Action on escalation timeout | | **escalateTo** | `string` | optional | User id or position machine name to escalate to | @@ -101,7 +101,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **enabled** | `boolean` | optional (default: `false`) | Enable SLA-based escalation for this node | +| **enabled** | `boolean` | optional (default: `true`) | SLA escalation switch. Defaults to true: an escalation block carrying timeoutHours is live unless this is explicitly false — the feature-level switch is whether the escalation block exists at all | | **timeoutHours** | `number` | ✅ | Hours before escalation triggers | | **action** | `Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>` | optional (default: `"notify"`) | Action on escalation timeout | | **escalateTo** | `string` | optional | User id or position machine name to escalate to | diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 702e15ee78..16daf731cc 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -334,6 +334,9 @@ Finally it removes the 'pdf' member of `view.exportOptions` formats (#8010, main - **`apimethod-enum-shrink`** — `data.object.enable.apiMethods (the eight legacy non-primitive values)` → the six primitives only — `get` / `list` / `create` / `update` / `delete` / `bulk`: replace each legacy value with the primitives it derives from, de-duplicate, and delete the key entirely if the result names all six - Why not automatic: The authored `enable.apiMethods` enum is now exactly the six primitives. The eight legacy values — `upsert`, `aggregate`, `history`, `search`, `restore`, `purge`, `import`, `export` — are no longer authorable, because they are DERIVED effective operations resolved by the server's single derivation table, and an enum that lets an author name both a primitive and something derived from it has two spellings for one fact. The FROM → TO is a table rather than a rename: `upsert` → `create` + `update`; `import` → `create` + `update`; `export`, `aggregate` and `search` → `list`; `history` → `get`; and `restore` / `purge` map to NOTHING — they never derived, because `enable.trash` was retired in #2377, so the value is deleted outright. That last row is why this is a semantic entry and not a mechanical conversion, and the reason is a security one: the mapping WIDENS. An allowlist naming `history` was granting read of one record's audit trail; rewritten to `get` it grants ordinary record reads, and an allowlist naming `search` becomes a grant of full `list`. A transform that applied the table silently would broaden real API permissions without anyone reading the diff, so the rewrite is delegated to the author with the widening flagged. The reporter codemod exists for exactly that shape: `node scripts/codemod/apimethods-legacy-to-primitives.mjs` scans, reports the exact replacement per site, and FLAGS the allowlists the mapping would widen so the edit stays reviewable — it reports, it does not rewrite. Stored metadata keeps parsing (permanent tolerance, narrowing only), so nothing breaks at rest; what changes is what an author may newly write. Registered by the #6350 stock reconciliation; #3543 (P2 of #3391) predates the #6148 completeness gate. ADR-0087, #3543 (backfilled #6350). - Done when: No authored `enable.apiMethods` array names a legacy value; `objectstack validate` passes. Run the reporter codemod first and read its widening flags before applying anything — ⚠️ the migration is only correct if each widened grant was INTENDED. For every object where `history` became `get` or `search` became `list`, confirm the broader operation is one the API should genuinely expose; where it is not, the answer is not a different value in this enum but a permission set that withholds the operation. Where the six primitives are all present, prefer deleting the key: that is equivalent to default-open and it tracks future primitives, whereas a hand-listed six silently stops granting anything added later. `restore` / `purge` are deleted with no replacement — if trash-like behaviour was being relied on, that capability left in #2377 and this entry is not where it returns. +- **`approval-escalation-enabled-default-flip`** — `automation.ApprovalEscalation.enabled — an OMITTED value inside an approval node's escalation block` → nothing, for the common intent (escalate on timeout): an escalation block carrying timeoutHours is live by default. To declare an SLA OFF while keeping its configuration, write enabled: false explicitly — which is now the spelling the escalation sweep actually reads + - Why not automatic: A DECLARED-DEFAULT CORRECTION plus the enforcement that makes the key real (#12278, maintainer ruling 2026-08-27) — the same category as protocol 17's `import-run-automations-declared-default-corrected`: the schema promised `enabled` defaults to `false` (SLA off) while the plugin-approvals sweep never read the key at all — any escalation block with a positive `timeoutHours` escalated, and with `action: 'auto_approve'` that silently approved requests their author had declared off the clock. The flip moves the default to `true` and, in the same change, the sweep starts honouring an explicit `enabled: false`. The feature-level switch is whether an `escalation` block exists at all; within a block carrying `timeoutHours`, escalation is on unless explicitly turned off. Deployed metadata that OMITS `enabled` does not change behaviour: it escalated before (the sweep ignored the key) and escalates after (the parse materializes `true`). Stored request snapshots written before the flip carry a MATERIALIZED `enabled: false` (the approval-node executor parses config through the old schema before snapshotting), so the sweep keeps a read-side legacy window keyed on the snapshot's `created_at`: pre-flip snapshots keep escalating exactly as they do today, and the window retires itself as those pending requests drain. What DOES change is that an explicit `enabled: false` finally binds — a flow that authored it (e.g. the console toggle switched off after a timeout was set) stops escalating on requests opened after the upgrade, which is the declared intent being honoured. + - Done when: A flow whose approval node omits `enabled` inside `escalation` still escalates on timeout (no metadata edit needed). A flow that writes `enabled: false` stops escalating for newly opened requests — verify one such request stays pending past its `timeoutHours` with no `escalate` audit row and no auto-decision. Requests opened BEFORE the upgrade keep their pre-upgrade behaviour (they escalate) regardless of the stored `enabled` bit. Clients that parse metadata through the published JSON Schema now materialize `enabled: true` where they materialized `false`; a client that needs the SLA off must write it explicitly. - **`audit-log-action-enum-retired`** — `sys_audit_log.action — the values 'export' and 'permission_change' left the select enum declared by plugin-audit (packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts). The same two values also left the shipped list-view filters on that object: 'permission_change' from the auth_events view and 'export' from the config_changes view` → nothing, for either value — both are removed rather than renamed, because neither named an event this platform records. For permission changes, read the ordinary `create` / `update` rows on the permission objects themselves: a grant or binding write is an ordinary record write and the generic audit writer already ledgers it, so a second semantically-duplicate row was never minted. For `export` there is no replacement and nothing is lost: no export feature ever wrote an audit row. A consumer filtering `sys_audit_log` on either value was reading an empty result set on every deployment, and still is — what changed is that the contract no longer promises otherwise - Why not automatic: Maintainer ruling 2026-08-12 (#7675), the retirement half of a two-half verdict: the cheap writers get built (#8144 login/logout, #8145 config_change) and the enum values with no feature behind them are retired. 原则记录:空 widget + 永远查不到东西的过滤器是可见产品缺陷;审计面宁窄勿谎. The defect was false compliance on a COMPLIANCE surface, which is the sharpest form of ADR-0049 declared-≠-enforced: an auditor reading the action enum believed the platform captured permission changes and data exports, and the shipped list views and dashboard widgets showed them a filter and a tile for exactly those events. Both were permanently empty. Measured by enumerating every `sys_audit_log` writer in the repo — there are exactly two: plugin-audit`s generic hook writer, whose `actionFor` maps afterInsert/Update/Delete to create/update/delete and nothing else, and plugin-auth`s admin user-import. Neither has ever emitted `export` or `permission_change`. This is an enum-VALUE retirement, so the bookkeeping differs from a key retirement in the two ways `hook-body-crypto-hash-removed`, `dataset-measure-array-string-agg-removed` and `action-global-nav-location-removed` already record: nothing lands in RETIRED_KEYS_BY_MAJOR (no authorable KEY changed) and the four surface ratchets are expected to be byte-identical (no def changed). It differs from all three in being a SEMANTIC entry rather than a D2 conversion, and the reason is that there is no source to rewrite: `sys_audit_log` is a platform-owned, append-only object whose every field is `readonly: true`. Nobody authors an audit row and nobody authors this enum — the values appear only in rows the runtime writes and in queries consumers send. A conversion rewrites authored metadata or a stored `sys_metadata` row; this surface is neither, so the disposition is the one `BatchOptions.validateOnly` and the notification cursor already take in this major. ⚠️ Historical ROWS are deliberately untouched. A deployment that somehow holds a row with either value keeps it, and keeps reading it back: the enum is not enforced on this object at all (`validateRecord` skips `readonly` fields, and every field here is readonly), so nothing rejects stored history and no backfill is required or wanted. Deleting audit history to satisfy a schema narrowing would be the one genuinely destructive reading of this change. ADR-0049 / ADR-0087, #8147. - Done when: No consumer filters `sys_audit_log` on `action = "export"` or `action = "permission_change"` expecting rows: both were empty everywhere before this change, so a query that returned data has not been identified and a query that returned nothing behaves identically. Concretely, check three places. (1) Saved queries, dashboards and reports over `sys_audit_log`: a filter naming either value should be deleted, not re-pointed — for permission auditing, filter the permission objects` own `create`/`update` rows by `object_name` instead. (2) Any code branching on the action string (a badge map, a label switch, an `if (row.action === ...)`): the arms for these two values are now unreachable and should go, and a `switch` with an exhaustiveness check over the enum type will now fail to compile if they stay — that compile error is the enforced channel for TypeScript consumers. (3) Custom objects or plugins inserting `sys_audit_log` rows with either value: this is the only case that needs a real decision, because the write will NOT be refused (readonly fields are not validated) — it will simply be a row whose action the object no longer declares. Pick a declared value or open an issue for the action you actually need. ⚠️ Do NOT migrate or delete existing rows: audit history is append-only and stays exactly as written. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 4e6d5c2997..c35dca9aa5 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -11,7 +11,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; -import { ApprovalService, REMIND_COOLDOWN_MS } from './approval-service.js'; +import { ApprovalService, REMIND_COOLDOWN_MS, ESCALATION_ENABLED_FLIP_CUTOFF_MS } from './approval-service.js'; import { bindApprovalLockHook, bindDelegationWriteGuard, unbindAllHooks } from './lifecycle-hooks.js'; interface FakeRow { [k: string]: any } @@ -1763,6 +1763,80 @@ describe('ApprovalService (node era)', () => { expect(out.escalated).toBe(0); }); + // ── #12278: the declared switch is enforced ───────────────────── + // The default test clock (2026-01-15) predates the flip cutoff, so these + // pins run a service whose clock sits AFTER it — both the snapshot side and + // the sweep side of the gate are exercised where the flip is in force. + + /** A service whose injected clock starts `offsetMs` past the flip cutoff. */ + function postFlipService(offsetMs: number) { + let n = 0; + return new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(ESCALATION_ENABLED_FLIP_CUTOFF_MS + offsetMs + (n++) * 1000) }, + }); + } + + it('runEscalations: explicit enabled:false does NOT escalate a post-flip snapshot (#12278)', async () => { + const late = postFlipService(2 * 3600_000); + const resumed: any[] = []; + late.attachAutomation({ async resume(runId, signal) { resumed.push({ runId, signal }); } }); + const req = await late.openNodeRequest( + openInput(['u9'], {}, { escalation: { enabled: false, timeoutHours: 1, action: 'auto_approve', notifySubmitter: false } }), CTX, + ); + // Deadline breached (due = cutoff + 1h < sweep clock at cutoff + 2h) while + // created_at stays ON the cutoff — the boundary belongs to the new regime. + const row = engine._tables['sys_approval_request'].find(r => r.id === req.id)!; + row.created_at = new Date(ESCALATION_ENABLED_FLIP_CUTOFF_MS).toISOString(); + const out = await late.runEscalations(); + // Concrete outcomes, not just counters: no decision, no audit row, no resume. + expect(out.scanned).toBe(1); + expect(out.escalated).toBe(0); + const fresh = await late.getRequest(req.id, SYS); + expect(fresh?.status).toBe('pending'); + const actions = await late.listActions(req.id, SYS); + expect(actions.map(a => a.action)).toEqual(['submit']); + expect(resumed).toHaveLength(0); + }); + + it('runEscalations: a PRE-flip snapshot with materialized enabled:false keeps escalating (#12278 legacy window)', async () => { + // Pre-flip code materialized `enabled: false` onto every author-omitted + // block via the executor parse; every such stored row is escalating today. + // The read-side window preserves exactly that behaviour for snapshots + // created before the cutoff — auto_approve still fires. + const late = postFlipService(2 * 3600_000); + const resumed: any[] = []; + late.attachAutomation({ async resume(runId, signal) { resumed.push({ runId, signal }); } }); + const req = await late.openNodeRequest( + openInput(['u9'], {}, { escalation: { enabled: false, timeoutHours: 1, action: 'auto_approve', notifySubmitter: false } }), CTX, + ); + const row = engine._tables['sys_approval_request'].find(r => r.id === req.id)!; + row.created_at = new Date(ESCALATION_ENABLED_FLIP_CUTOFF_MS - 10 * 3600_000).toISOString(); + const out = await late.runEscalations(); + expect(out.escalated).toBe(1); + const fresh = await late.getRequest(req.id, SYS); + expect(fresh?.status).toBe('approved'); + expect(resumed[0]).toMatchObject({ runId: 'run_1', signal: { branchLabel: 'approve' } }); + const actions = await late.listActions(req.id, SYS); + expect(actions.map(a => a.action)).toEqual(['submit', 'escalate', 'approve']); + }); + + it('runEscalations: an omitted enabled key still escalates post-flip (#12278)', async () => { + // Raw snapshots without the key (the eleven behaviour tests above, and any + // pre-materialization historical row) gate on `=== false` only — absence + // escalates, matching the new schema default the executor parse writes. + const late = postFlipService(2 * 3600_000); + const req = await late.openNodeRequest( + openInput(['u9'], {}, { escalation: { timeoutHours: 1, action: 'auto_reject', notifySubmitter: false } }), CTX, + ); + const row = engine._tables['sys_approval_request'].find(r => r.id === req.id)!; + row.created_at = new Date(ESCALATION_ENABLED_FLIP_CUTOFF_MS).toISOString(); + const out = await late.runEscalations(); + expect(out.escalated).toBe(1); + const fresh = await late.getRequest(req.id, SYS); + expect(fresh?.status).toBe('rejected'); + }); + // ── SLA + flow steps ──────────────────────────────────────────── it('rows expose sla_due_at when the node declares escalation.timeoutHours', async () => { diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 4396ac0ba8..dcb137efa9 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -181,6 +181,36 @@ export const ESCALATION_JOB_NAME = 'approvals-sla-escalation'; export const ESCALATION_SCAN_INTERVAL_MS = 5 * 60 * 1000; /** Reserved actor id for machine decisions made by the SLA scanner. */ export const SLA_ACTOR_ID = 'system:sla'; +/** + * Read-side legacy window for the #12278 `escalation.enabled` default flip + * (maintainer ruling 2026-08-27, Option C). + * + * Until the flip, `ApprovalEscalationSchema` declared `enabled` with + * `.default(false)` while this sweep never read the key — and the + * approval-node executor parses node config through that schema before + * `openNodeRequest` snapshots it onto the request row. Every request opened + * from a flow that OMITTED `enabled` therefore stores a MATERIALIZED + * `enabled: false` in `node_config_json`, byte-identical to an authored + * `false`, and every such stored row is escalating today (measured basis on + * the ruling: the console inspector cannot author a `timeoutHours` without + * toggling the switch on, so no stored `false + timeoutHours` row carries an + * enforceable off-intent that today's runtime honours). + * + * The sweep's `enabled === false ⇒ skip` gate below therefore applies only to + * snapshots created at/after this cutoff (written by post-flip code, where + * `false` survives the parse only when an author wrote it). Older snapshots + * keep today's behaviour — they escalate — and the window retires itself as + * those pending requests drain; no tenant row is ever rewritten. + * + * Known, deliberate bound (recorded on #12278): an install that upgrades LONG + * after this date has pending requests created between the cutoff and its + * upgrade whose materialized `false` post-dates the cutoff; those honour the + * declared off. The alternative (a per-install marker or a stored-data + * normalization) buys that edge back at the price of new machinery or tenant + * writes — both rejected as the more expensive option under the ruling's + * "pick the cheaper transition" instruction. + */ +export const ESCALATION_ENABLED_FLIP_CUTOFF_MS = Date.parse('2026-08-28T00:00:00Z'); /** Reserved actor id for requests abandoned because their run died (#3456). */ export const DEAD_RUN_ACTOR_ID = 'system:dead-run'; /** @@ -3450,7 +3480,10 @@ export class ApprovalService implements IApprovalService { * `escalation.timeoutHours` and whose deadline has passed is escalated * **at most once, ever** — the `escalate` audit row is the idempotency * marker, written before any mutation (audit-first, like reassign). One - * bad row never stops the sweep. + * bad row never stops the sweep. A block that explicitly declares + * `enabled: false` is skipped (#12278 — the declared switch is enforced; + * pre-flip snapshots ride the legacy window on + * {@link ESCALATION_ENABLED_FLIP_CUTOFF_MS}). */ async runEscalations(): Promise<{ scanned: number; escalated: number }> { let rows: any[] = []; @@ -3471,6 +3504,15 @@ export class ApprovalService implements IApprovalService { const cfg = parseJson(raw.node_config_json, undefined); const esc = cfg?.escalation; if (!esc || typeof esc.timeoutHours !== 'number' || esc.timeoutHours <= 0) continue; + // #12278: the declared switch is enforced — an explicit + // `enabled: false` skips the sweep (strict `=== false`: an absent key + // escalates, both for pre-materialization legacy rows and because the + // schema default is now `true`). Snapshots created before the flip + // cutoff carry a schema-materialized `false` that is indistinguishable + // from an authored one and are escalating today — the legacy window + // preserves them verbatim; see ESCALATION_ENABLED_FLIP_CUTOFF_MS. + if (esc.enabled === false + && Date.parse(raw.created_at ?? '') >= ESCALATION_ENABLED_FLIP_CUTOFF_MS) continue; const due = slaDueAt(raw.created_at, cfg); if (!due || Date.parse(due) > this.clock.now().getTime()) continue; diff --git a/packages/spec/authorable-defaults/automation.json b/packages/spec/authorable-defaults/automation.json index 57526ccbdc..47bd3a69c8 100644 --- a/packages/spec/authorable-defaults/automation.json +++ b/packages/spec/authorable-defaults/automation.json @@ -13,7 +13,7 @@ "automation/ActionDescriptor:supportsPause = false", "automation/ActionDescriptor:supportsRetry = true", "automation/ApprovalEscalation:action = \"notify\"", - "automation/ApprovalEscalation:enabled = false", + "automation/ApprovalEscalation:enabled = true", "automation/ApprovalEscalation:notifySubmitter = true", "automation/ApprovalNodeConfig:behavior = \"first_response\"", "automation/ApprovalNodeConfig:lockRecord = true", diff --git a/packages/spec/scripts/lib/default-changes.ts b/packages/spec/scripts/lib/default-changes.ts index 6b4c9de65b..c1d1dad025 100644 --- a/packages/spec/scripts/lib/default-changes.ts +++ b/packages/spec/scripts/lib/default-changes.ts @@ -235,5 +235,36 @@ export const DEFAULT_CHANGES_BY_MAJOR: Readonly { expect(result.notifySubmitter).toBe(true); expect(() => ApprovalEscalationSchema.parse({ enabled: true, timeoutHours: 0 })).toThrow(); }); + + // #12278 (maintainer ruling 2026-08-27): the feature switch is whether an + // escalation block exists at all; within a block carrying timeoutHours, + // escalation is ON unless explicitly turned off. Declared in + // DEFAULT_CHANGES_BY_MAJOR (17) — this pin is what keeps the fingerprint honest. + it('defaults enabled to true — a block carrying timeoutHours is live unless explicitly off (#12278)', () => { + const omitted = ApprovalEscalationSchema.parse({ timeoutHours: 24 }); + expect(omitted.enabled).toBe(true); + const explicit = ApprovalEscalationSchema.parse({ enabled: false, timeoutHours: 24 }); + expect(explicit.enabled).toBe(false); + // No escalation block at all stays ABSENT on the node config — "no block" + // and "block declared off" remain two distinguishable states post-parse. + const noBlock = ApprovalNodeConfigSchema.parse({ approvers: [{ type: 'user', value: 'u1' }] }); + expect(noBlock.escalation).toBeUndefined(); + }); }); /** diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 23e81dc8cf..5aa12ee29f 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -613,7 +613,13 @@ export const ApprovalEscalationSchema = lazySchema(() => strictObject( 'the author declared never fired the way they intended.', }, { - enabled: z.boolean().default(false).describe('Enable SLA-based escalation for this node'), + // #12278 (maintainer ruling 2026-08-27): default flipped false → true. The + // feature switch is whether an `escalation` block exists at all; within a + // block carrying `timeoutHours`, escalation is ON unless explicitly turned + // off — which is what the sweep, its behaviour tests and every teaching + // surface have always meant. Declared in DEFAULT_CHANGES_BY_MAJOR (17) and + // the `approval-escalation-enabled-default-flip` semantic migration entry. + enabled: z.boolean().default(true).describe('SLA escalation switch. Defaults to true: an escalation block carrying timeoutHours is live unless this is explicitly false — the feature-level switch is whether the escalation block exists at all'), timeoutHours: z.number().min(1).describe('Hours before escalation triggers'), action: z.enum(['reassign', 'auto_approve', 'auto_reject', 'notify']).default('notify') .describe('Action on escalation timeout'), diff --git a/packages/spec/src/migrations/entries/semantic/17.approval-escalation-enabled-default-flip.ts b/packages/spec/src/migrations/entries/semantic/17.approval-escalation-enabled-default-flip.ts new file mode 100644 index 0000000000..9c647f0521 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/17.approval-escalation-enabled-default-flip.ts @@ -0,0 +1,49 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'approval-escalation-enabled-default-flip', + // No backticks in `surface` — build-upgrade-guide.ts renders it inside a code + // span AND a table cell (see the note on `spec-type-alias-input-suffix-retired`). + surface: + 'automation.ApprovalEscalation.enabled — an OMITTED value inside an approval ' + + "node's escalation block", + replacement: + 'nothing, for the common intent (escalate on timeout): an escalation block ' + + 'carrying timeoutHours is live by default. To declare an SLA OFF while keeping ' + + 'its configuration, write enabled: false explicitly — which is now the spelling ' + + 'the escalation sweep actually reads', + reason: + 'A DECLARED-DEFAULT CORRECTION plus the enforcement that makes the key real ' + + "(#12278, maintainer ruling 2026-08-27) — the same category as protocol 17's " + + '`import-run-automations-declared-default-corrected`: the schema promised ' + + '`enabled` defaults to `false` (SLA off) while the plugin-approvals sweep never ' + + 'read the key at all — any escalation block with a positive `timeoutHours` ' + + 'escalated, and with `action: \'auto_approve\'` that silently approved requests ' + + 'their author had declared off the clock. The flip moves the default to `true` ' + + 'and, in the same change, the sweep starts honouring an explicit ' + + '`enabled: false`. The feature-level switch is whether an `escalation` block ' + + 'exists at all; within a block carrying `timeoutHours`, escalation is on unless ' + + 'explicitly turned off. Deployed metadata that OMITS `enabled` does not change ' + + 'behaviour: it escalated before (the sweep ignored the key) and escalates after ' + + '(the parse materializes `true`). Stored request snapshots written before the ' + + 'flip carry a MATERIALIZED `enabled: false` (the approval-node executor parses ' + + 'config through the old schema before snapshotting), so the sweep keeps a ' + + 'read-side legacy window keyed on the snapshot\'s `created_at`: pre-flip ' + + 'snapshots keep escalating exactly as they do today, and the window retires ' + + 'itself as those pending requests drain. What DOES change is that an explicit ' + + '`enabled: false` finally binds — a flow that authored it (e.g. the console ' + + 'toggle switched off after a timeout was set) stops escalating on requests ' + + 'opened after the upgrade, which is the declared intent being honoured.', + acceptanceCriteria: + 'A flow whose approval node omits `enabled` inside `escalation` still escalates ' + + 'on timeout (no metadata edit needed). A flow that writes `enabled: false` ' + + 'stops escalating for newly opened requests — verify one such request stays ' + + 'pending past its `timeoutHours` with no `escalate` audit row and no ' + + 'auto-decision. Requests opened BEFORE the upgrade keep their pre-upgrade ' + + 'behaviour (they escalate) regardless of the stored `enabled` bit. Clients ' + + 'that parse metadata through the published JSON Schema now materialize ' + + '`enabled: true` where they materialized `false`; a client that needs the SLA ' + + 'off must write it explicitly.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 2c2d5b775c..432b0c1571 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -1835,6 +1835,51 @@ const step17: MigrationStep = { + 'replacement — if trash-like behaviour was being relied on, that capability left in ' + '#2377 and this entry is not where it returns.', }, + { + id: 'approval-escalation-enabled-default-flip', + // No backticks in `surface` — build-upgrade-guide.ts renders it inside a code + // span AND a table cell (see the note on `spec-type-alias-input-suffix-retired`). + surface: + 'automation.ApprovalEscalation.enabled — an OMITTED value inside an approval ' + + "node's escalation block", + replacement: + 'nothing, for the common intent (escalate on timeout): an escalation block ' + + 'carrying timeoutHours is live by default. To declare an SLA OFF while keeping ' + + 'its configuration, write enabled: false explicitly — which is now the spelling ' + + 'the escalation sweep actually reads', + reason: + 'A DECLARED-DEFAULT CORRECTION plus the enforcement that makes the key real ' + + "(#12278, maintainer ruling 2026-08-27) — the same category as protocol 17's " + + '`import-run-automations-declared-default-corrected`: the schema promised ' + + '`enabled` defaults to `false` (SLA off) while the plugin-approvals sweep never ' + + 'read the key at all — any escalation block with a positive `timeoutHours` ' + + 'escalated, and with `action: \'auto_approve\'` that silently approved requests ' + + 'their author had declared off the clock. The flip moves the default to `true` ' + + 'and, in the same change, the sweep starts honouring an explicit ' + + '`enabled: false`. The feature-level switch is whether an `escalation` block ' + + 'exists at all; within a block carrying `timeoutHours`, escalation is on unless ' + + 'explicitly turned off. Deployed metadata that OMITS `enabled` does not change ' + + 'behaviour: it escalated before (the sweep ignored the key) and escalates after ' + + '(the parse materializes `true`). Stored request snapshots written before the ' + + 'flip carry a MATERIALIZED `enabled: false` (the approval-node executor parses ' + + 'config through the old schema before snapshotting), so the sweep keeps a ' + + 'read-side legacy window keyed on the snapshot\'s `created_at`: pre-flip ' + + 'snapshots keep escalating exactly as they do today, and the window retires ' + + 'itself as those pending requests drain. What DOES change is that an explicit ' + + '`enabled: false` finally binds — a flow that authored it (e.g. the console ' + + 'toggle switched off after a timeout was set) stops escalating on requests ' + + 'opened after the upgrade, which is the declared intent being honoured.', + acceptanceCriteria: + 'A flow whose approval node omits `enabled` inside `escalation` still escalates ' + + 'on timeout (no metadata edit needed). A flow that writes `enabled: false` ' + + 'stops escalating for newly opened requests — verify one such request stays ' + + 'pending past its `timeoutHours` with no `escalate` audit row and no ' + + 'auto-decision. Requests opened BEFORE the upgrade keep their pre-upgrade ' + + 'behaviour (they escalate) regardless of the stored `enabled` bit. Clients ' + + 'that parse metadata through the published JSON Schema now materialize ' + + '`enabled: true` where they materialized `false`; a client that needs the SLA ' + + 'off must write it explicitly.', + }, { id: 'audit-log-action-enum-retired', // No backticks in `surface` — build-upgrade-guide.ts renders it inside a From 71ccc576dd90c88117c9a53b0047ed7ea1d8084a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 17:33:29 +0000 Subject: [PATCH 2/2] =?UTF-8?q?test(approvals):=20narrow=20openNodeRequest?= =?UTF-8?q?=20unions=20in=20the=20#12278=20pins=20=E2=80=94=20debt=20ratch?= =?UTF-8?q?et=20back=20to=20347?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three new escalation pins accessed row properties (.id, indirectly created_at) on the raw openNodeRequest return type (ApprovalRequestRow | ApprovalNodeAutoOutcome), adding 8 raw TS2339 errors to the plugin's shrink-only TEST_DEBT layer (347 recorded, 355 measured). Fixed with the file's own sanctioned narrowing pattern ('autoApproved' in req => throw) after each open — no ledger change, no any. Raw count re-measured at exactly 347; composition returns to the ledger note to the unit (TS2339 x296). Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01JvjTCjJQn9zSTXEhUKgT7s --- packages/plugins/plugin-approvals/src/approval-service.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index c35dca9aa5..2aece88f1f 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -1784,6 +1784,7 @@ describe('ApprovalService (node era)', () => { const req = await late.openNodeRequest( openInput(['u9'], {}, { escalation: { enabled: false, timeoutHours: 1, action: 'auto_approve', notifySubmitter: false } }), CTX, ); + if ('autoApproved' in req) throw new Error('expected a pending request, not an auto-approve outcome'); // Deadline breached (due = cutoff + 1h < sweep clock at cutoff + 2h) while // created_at stays ON the cutoff — the boundary belongs to the new regime. const row = engine._tables['sys_approval_request'].find(r => r.id === req.id)!; @@ -1810,6 +1811,7 @@ describe('ApprovalService (node era)', () => { const req = await late.openNodeRequest( openInput(['u9'], {}, { escalation: { enabled: false, timeoutHours: 1, action: 'auto_approve', notifySubmitter: false } }), CTX, ); + if ('autoApproved' in req) throw new Error('expected a pending request, not an auto-approve outcome'); const row = engine._tables['sys_approval_request'].find(r => r.id === req.id)!; row.created_at = new Date(ESCALATION_ENABLED_FLIP_CUTOFF_MS - 10 * 3600_000).toISOString(); const out = await late.runEscalations(); @@ -1829,6 +1831,7 @@ describe('ApprovalService (node era)', () => { const req = await late.openNodeRequest( openInput(['u9'], {}, { escalation: { timeoutHours: 1, action: 'auto_reject', notifySubmitter: false } }), CTX, ); + if ('autoApproved' in req) throw new Error('expected a pending request, not an auto-approve outcome'); const row = engine._tables['sys_approval_request'].find(r => r.id === req.id)!; row.created_at = new Date(ESCALATION_ENABLED_FLIP_CUTOFF_MS).toISOString(); const out = await late.runEscalations();