From b87f2706299ad1ca88e1dc5acfc7b72421ef8ec4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 02:01:25 +0000 Subject: [PATCH] fix(spec): close notify.severity to its declared info|warning|critical vocabulary (#7086) NotifyConfigSchema.severity was a bare z.string() whose .describe() read 'info | warning | critical', so the enumeration existed only in the sentence: 'urgent', 'INFO' and '' all parsed green, were forwarded raw by the notify executor, and were blind-cast by the messaging dispatcher into a union that declares those values impossible. Every other surface already declared the set closed (the describe, the Notification['severity'] type, the sys_inbox_message.severity select field), so this closes the last open one. Safe because the executor reads severity RAW -- it is one of three keys (channels, topic, severity) that never pass through interpolate() -- so a {token} template there never resolved. The module JSDoc claimed "every string-ish value except channels" is interpolated; that was stale for topic and severity and is corrected here, since the tightening's safety rests on it. Blast radius is an execute-time refusal, not a load failure: FlowNodeSchema .config is an untyped record, so stored flows still load and rehydrate. Also closes the Studio form descriptor to the same set, and extends the IO-node form/Zod ledger test to reconcile closed value vocabularies rather than key sets alone. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2 --- .../notify-severity-closed-vocabulary.md | 60 ++++++++++++++++ .../references/automation/io-node-config.mdx | 2 +- .../builtin/io-node-form-zod-ledger.test.ts | 38 ++++++++++ .../src/builtin/notify-node.ts | 10 ++- .../src/automation/io-node-config.test.ts | 70 +++++++++++++++++++ .../spec/src/automation/io-node-config.zod.ts | 32 +++++++-- 6 files changed, 205 insertions(+), 7 deletions(-) create mode 100644 .changeset/notify-severity-closed-vocabulary.md diff --git a/.changeset/notify-severity-closed-vocabulary.md b/.changeset/notify-severity-closed-vocabulary.md new file mode 100644 index 0000000000..49a0433baf --- /dev/null +++ b/.changeset/notify-severity-closed-vocabulary.md @@ -0,0 +1,60 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": minor +--- + +fix(spec): `notify.severity` closes its declared `info | warning | critical` vocabulary at the gate, not only in its describe (#7086) + + + +`NotifyConfigSchema.severity` was a bare `z.string()` whose `.describe()` read +`'info | warning | critical'` — no "e.g.", no qualifier. In this codebase that +spelling is how a genuine closed vocabulary is documented, so the enumeration +existed only in the sentence. Measured on `origin/main` before the change: + +``` +severity "info" -> ACCEPTED severity "urgent" -> ACCEPTED +severity "warning" -> ACCEPTED severity "INFO" -> ACCEPTED +severity "critical" -> ACCEPTED severity "" -> ACCEPTED +``` + +**Every other surface already declared the set closed**, which is what made the +open gate a defect rather than a design choice: the `notify` executor forwards +the value raw, the messaging dispatcher blind-casts it into the closed union +(`severity: (p.severity as Notification['severity']) ?? 'info'`), and +`sys_inbox_message.severity` is a select field offering exactly these three. So +`severity: 'urgent'` parsed green, published green, and landed in inbox rows +under a TypeScript type that says the value cannot exist — falling through every +downstream `switch` on the three names. An author (very often an AI) who wrote +`Critical` or `urgent` got no diagnostic anywhere on the path. + +The gate is now `z.enum(['info', 'warning', 'critical']).optional()`, and the +describe is a sentence about the field, because the vocabulary is carried by the +type — the generated reference renders it as an enum column instead of a bare +`string`. The refusal is self-prescribing: + +``` +Invalid option: expected one of "info"|"warning"|"critical" +``` + +**Why closing this gate takes no working authoring shape with it.** The executor +reads `severity` **raw** — it is one of the three keys (`channels`, `topic`, +`severity`) that never pass through `interpolate()` — so a `{record.x}` template +there was forwarded verbatim and never resolved. The schema's module JSDoc +claimed "every string-ish value except `channels`" is interpolated; that was +stale for `topic` and `severity`, and it is corrected here, since it is the +statement the safety of this tightening rests on. + +**Blast radius is an execute-time refusal, not a load failure.** `FlowNodeSchema.config` +is an untyped record, so a stored flow carrying `severity: 'urgent'` still loads +and rehydrates exactly as before; the `notify` step refuses when it runs, naming +the three legal values. `''` previously degraded to `info` two layers down and is +now refused at the gate. + +The `notify` descriptor's Studio form is closed in the same change +(`enum: ['info', 'warning', 'critical']`). Closing only the Zod would have left +the mirror-image drift the IO-node ledger test exists to prevent — a form +inviting a value the gate refuses at execute time — and the `screen` node's +`mode` is the in-repo precedent for enum-on-both-sides. That ledger test compared +key SETS only, which is the gap this field sat in; it now also reconciles closed +value vocabularies, so the two descriptions cannot drift apart again. diff --git a/content/docs/references/automation/io-node-config.mdx b/content/docs/references/automation/io-node-config.mdx index 5b51cc6826..78458b7b73 100644 --- a/content/docs/references/automation/io-node-config.mdx +++ b/content/docs/references/automation/io-node-config.mdx @@ -100,7 +100,7 @@ const result = HttpConfigSchema.parse(data); | **message** | `string` | optional | Notification body | | **channels** | `string \| string[]` | optional | Channels to fan out to (default: inbox) | | **topic** | `string` | optional | Event topic (default: "notify") | -| **severity** | `string` | optional | info \| warning \| critical | +| **severity** | `Enum<'info' \| 'warning' \| 'critical'>` | optional | Severity forwarded to the messaging service | | **sourceObject** | `string` | optional | Object name of the record the notification links to (writes sys_notification.source_object). Only takes effect together with sourceId — a half-specified click-through target is dropped at execute time, so the inbox never renders a dead link. | | **sourceId** | `string` | optional | Record id the notification links to (writes sys_notification.source_id). Only takes effect together with sourceObject — a half-specified click-through target is dropped at execute time, so the inbox never renders a dead link. | | **actorId** | `string` | optional | User id that caused the event (writes sys_notification.actor_id) | diff --git a/packages/services/service-automation/src/builtin/io-node-form-zod-ledger.test.ts b/packages/services/service-automation/src/builtin/io-node-form-zod-ledger.test.ts index c7b72504fe..05a8f82fa9 100644 --- a/packages/services/service-automation/src/builtin/io-node-form-zod-ledger.test.ts +++ b/packages/services/service-automation/src/builtin/io-node-form-zod-ledger.test.ts @@ -94,6 +94,44 @@ describe('IO-node form ↔ Zod reconciliation (#4045)', () => { ).toEqual([]); }); + // #7086 — the reconciliation above compares KEY SETS, which is why a key + // could agree on both sides while the two descriptions disagreed about the + // VALUES it accepts. `notify.severity` sat in exactly that gap: the form + // offered a free-text box and the Zod was a bare `z.string()`, while the + // describe on both sides spelled out a closed `info | warning | critical` + // that nothing enforced. Closing only the Zod would have produced the + // mirror-image drift — a Studio field inviting a value the gate refuses at + // execute time — so the closed set is pinned as ONE contract here. + it.each(NODES)('$nodeType: a closed vocabulary is closed on BOTH sides, with the same values', ({ nodeType, zod }) => { + const props = (engine.getActionDescriptor(nodeType)?.configSchema as + | { properties?: Record } + | undefined)?.properties ?? {}; + const shape = (zod as { shape?: Record }).shape ?? {}; + + /** The declared value set, or `undefined` for an open field. */ + const closedSet = (v: unknown): readonly string[] | undefined => { + // `.options` also exists on a ZodUnion, where it holds member SCHEMAS + // (`recipients`, `channels`) — a string-only array is what distinguishes + // a real value vocabulary from that. + const opts = (v as { options?: unknown })?.options; + return Array.isArray(opts) && opts.every((o) => typeof o === 'string') + ? (opts as readonly string[]) + : undefined; + }; + + for (const key of Object.keys(shape)) { + const node = shape[key] as { unwrap?: () => unknown }; + // Unwrap the `.optional()` wrapper before asking for the vocabulary. + const zodSet = closedSet(node) ?? closedSet(node?.unwrap?.()); + const formSet = closedSet(props[key]) ?? (Array.isArray(props[key]?.enum) ? props[key]!.enum as string[] : undefined); + + expect( + formSet, + `${nodeType}.${key}: the two descriptions disagree on whether the value set is closed`, + ).toEqual(zodSet); + } + }); + describe('connector_action: the contract is the connectorConfig sibling, not config', () => { it('publishes no configSchema (deliberately schemaless)', () => { // A published configSchema roots the schema-driven Studio form at diff --git a/packages/services/service-automation/src/builtin/notify-node.ts b/packages/services/service-automation/src/builtin/notify-node.ts index 8987fcc3c2..996a2a5fb0 100644 --- a/packages/services/service-automation/src/builtin/notify-node.ts +++ b/packages/services/service-automation/src/builtin/notify-node.ts @@ -159,7 +159,15 @@ export function registerNotifyNode(engine: AutomationEngine, ctx: PluginContext) description: 'Channels to fan out to (default: inbox)', }, topic: { type: 'string', description: 'Event topic (default: "notify")' }, - severity: { type: 'string', description: 'info | warning | critical' }, + // Closed vocabulary, declared as one so the Studio form + // offers a choice instead of a free-text box the Zod gate + // then refuses at execute time (#7086). Mirrors + // `NotifyConfigSchema.severity`; the `screen` node's `mode` + // is the in-repo precedent for enum-on-both-sides. + severity: { + type: 'string', enum: ['info', 'warning', 'critical'], + description: 'Severity forwarded to the messaging service', + }, // ── Click-through target (#2675) ───────────────────────── sourceObject: { type: 'string', diff --git a/packages/spec/src/automation/io-node-config.test.ts b/packages/spec/src/automation/io-node-config.test.ts index 9b1e02592f..46db560944 100644 --- a/packages/spec/src/automation/io-node-config.test.ts +++ b/packages/spec/src/automation/io-node-config.test.ts @@ -136,6 +136,76 @@ describe('NotifyConfigSchema — strict as of #4001 批 9', () => { expect(NotifyConfigSchema.safeParse({ recipients: 'u1', title: 't', sourceObject: 'showcase_task' }).success).toBe(true); expect(NotifyConfigSchema.safeParse({ recipients: 'u1', title: 't', sourceId: 'r1' }).success).toBe(true); }); + + // ── #7086 — severity is a CLOSED vocabulary, at the gate and not only in prose ── + // + // Until this change `severity` was a bare `z.string()` whose `.describe()` + // read `'info | warning | critical'`. The enumeration lived only in the + // sentence, so `'urgent'` parsed green here, was forwarded raw by the + // executor, and was blind-cast by the dispatcher + // (`(p.severity as Notification['severity']) ?? 'info'`) into a union that + // declares those values impossible — silently falling through every + // downstream `switch`. The three surfaces that already agreed on the closed + // set: this describe, `Notification['severity']`, and the + // `sys_inbox_message.severity` select field. + describe('severity (#7086)', () => { + /** The `severity` issues of a failed parse, or `[]` when it was accepted. */ + function severityIssues(value: unknown): ReadonlyArray<{ code: string; message: string }> { + const result = NotifyConfigSchema.safeParse({ recipients: 'u1', title: 't', severity: value }); + if (result.success) return []; + return result.error.issues.filter((i) => i.path.length === 1 && i.path[0] === 'severity'); + } + + // Green BOTH before and after this change — pre-fix everything parsed, so + // these prove nothing about the tightening. Stated plainly because the + // template presumes before-green/after-red: their real job is the opposite + // direction, that closing the gate did not OVERSHOOT and take a legal + // spelling with it. + it.each(['info', 'warning', 'critical'])('accepts the declared value %s', (value) => { + expect(NotifyConfigSchema.safeParse({ recipients: 'u1', title: 't', severity: value }).success).toBe(true); + }); + + // The pins that carry the change. Measured RED on `origin/main` before the + // fix — all three parsed green there (probe on 3e8e669c0). + // + // `code` + `path`, never a bare `success === false`: a strictObject rejects + // for several reasons, so an assertion that only asks "did it fail" would + // stay green if the refusal ever came from an unknown key instead of the + // vocabulary — the two defects this file has to keep apart. + it.each([ + ['urgent', 'an out-of-vocabulary spelling'], + ['INFO', 'a casing variant — the vocabulary is lower-case'], + ['', 'the empty string, which used to degrade to `info` two layers down'], + ])('rejects %s (%s)', (value) => { + const issues = severityIssues(value); + expect(issues.map((i) => i.code)).toEqual(['invalid_value']); + // The prescription is behaviour (this file's stated load-bearing half): + // the refusal has to tell the author what IS legal, or an AI author who + // guessed `urgent` has nothing to correct towards (ADR-0033). + for (const legal of ['info', 'warning', 'critical']) { + expect(issues[0]!.message).toContain(legal); + } + }); + + it('declares the vocabulary in the TYPE, not only in the sentence', () => { + const shape = (NotifyConfigSchema as unknown as { + shape: Record; + }).shape; + + // The gate itself carries the closed set — this is what `'urgent'` + // now collides with, and what the generated reference renders as the + // `Enum<...>` type column instead of a free-text `string`. + expect(shape.severity!.unwrap().options).toEqual(['info', 'warning', 'critical']); + + // …and the describe is now a sentence about the field rather than a + // bare value list standing in for a gate that did not exist. Non-empty + // arm first, so the negative arm below cannot pass vacuously (#6918). + const doc = shape.severity!.description ?? ''; + expect(doc.length, 'severity .describe() must not be empty').toBeGreaterThan(0); + expect(doc).toMatch(/messaging service/i); + expect(doc, 'the vocabulary belongs in the enum, not smuggled back into prose').not.toMatch(/\|/); + }); + }); }); describe('HttpConfigSchema — strict as of #4001 批 9', () => { diff --git a/packages/spec/src/automation/io-node-config.zod.ts b/packages/spec/src/automation/io-node-config.zod.ts index d55f3fad26..d37a00c463 100644 --- a/packages/spec/src/automation/io-node-config.zod.ts +++ b/packages/spec/src/automation/io-node-config.zod.ts @@ -127,9 +127,14 @@ const NOTIFY_KEY_GUIDANCE: Readonly> = { * without them). The descriptor's form deliberately publishes no `required` * array — see the comment on the `configSchema` literal — so requiredness * lives here and in the execute-time guard, not in the form. - * - Every string-ish value except `channels` passes through `interpolate()`, - * so `{record.x}` templates are legal anywhere they are; `channels` is read - * raw (channel ids are static routing, not per-record data). + * - `recipients`, `title`, `message`, `actionUrl` and `payload` pass through + * `interpolate()`, so `{record.x}` templates are legal in them. `channels`, + * `topic` and `severity` are read RAW — a `{token}` in those three is + * forwarded verbatim, never resolved (channel ids are static routing and + * `severity` is a closed vocabulary, not per-record data). Re-measured + * against `notify-node.ts` for #7086: the previous wording ("every + * string-ish value except `channels`") was stale for `topic` and `severity`, + * and it is what makes closing the `severity` gate below safe. * - `sourceObject`/`sourceId` only take effect as a PAIR — a half-specified * click-through target is dropped so the inbox never renders a dead link. * The schema keeps both optional rather than refining, because the executor @@ -156,8 +161,25 @@ export const NotifyConfigSchema = lazySchema(() => strictObject({ .describe('Channels to fan out to (default: inbox)'), /** Event topic handed to the messaging service (default: "notify"). */ topic: z.string().optional().describe('Event topic (default: "notify")'), - /** Severity forwarded to the messaging service. */ - severity: z.string().optional().describe('info | warning | critical'), + /** + * Severity forwarded to the messaging service — a CLOSED vocabulary (#7086). + * + * Was a bare `z.string()` whose `.describe()` read `'info | warning | critical'`, + * so the enumeration existed only in the sentence: `'urgent'`, `'INFO'` and `''` + * all parsed green, then rode the dispatcher's blind cast + * (`severity: (p.severity as Notification['severity']) ?? 'info'`) into + * `sys_inbox_message.severity` under a TypeScript union that says those values + * cannot exist — every downstream `switch` on the three names fell through. + * The gate is the last surface that was open: the describe, the + * `Notification['severity']` type, and the `sys_inbox_message.severity` select + * field all already declared exactly these three. + * + * Safe to close because the executor reads this key RAW — see the + * interpolation note above — so a `{token}` template here never resolved and + * a rejection removes no working authoring shape. + */ + severity: z.enum(['info', 'warning', 'critical']).optional() + .describe('Severity forwarded to the messaging service'), /** Click-through target object — only effective together with `sourceId` (#2675). */ sourceObject: z.string().optional() .describe('Object name of the record the notification links to (writes sys_notification.source_object). Only takes effect together with sourceId — a half-specified click-through target is dropped at execute time, so the inbox never renders a dead link.'),