diff --git a/.changeset/adr-0078-phase3-webhook-triggers.md b/.changeset/adr-0078-phase3-webhook-triggers.md new file mode 100644 index 0000000000..cbc270d21a --- /dev/null +++ b/.changeset/adr-0078-phase3-webhook-triggers.md @@ -0,0 +1,44 @@ +--- +'@objectstack/spec': minor +'@objectstack/lint': minor +--- + +ADR-0078 Phase 3: a webhook with no `triggers` now fails at author time — and the Tier-B candidate list is corrected to what verification actually supports. + +**The rule.** `webhook/without-triggers`, error severity, in the shared `@objectstack/spec/kernel` predicate alongside the Phase 1 rules, walked by `@objectstack/lint`'s `validate-functional-completeness` over `stack.webhooks` in both collection spellings. A webhook that declares no trigger materializes into `sys_webhook`, renders in Setup looking armed, and delivers nothing. + +**Why it needed two sources, and why the first one argued against it.** The runtime skip site reads: + +``` +if (triggers.size === 0) { + // No dispatchable triggers (or a manual-only webhook with none) — + // skip auto-enqueue. + return null; +``` + +That parenthetical *blesses* the empty case as a deliberate mode — structurally identical to the `multiselect`-without-options NON-rule, where `record-validator.ts`'s `// free-form (tags without options)` is exactly why we do not flag it. On that evidence alone this candidate stays unenforced. + +The mode it names does not exist. `webhook.zod.ts`'s #3196 note records that the `api` (manual/programmatic fire) trigger was *removed* because "no manual fire path exists — the only webhook HTTP surface re-queues already-failed deliveries". There is no way to fire a webhook the auto-enqueuer dropped. Inert on every path, so: `error`. + +> **The generalization, now written into the module and pinned by a test:** a runtime comment records what its author believed, and beliefs go stale when a sibling feature is deleted. A blessing has to be corroborated by something showing the blessed mode is still *reachable* — otherwise it is a comment about a mode that no longer exists. The test asserts the finding carries both citations, so nobody demotes this rule on the strength of the comment alone. + +`triggers: []` is flagged identically to an omitted `triggers`. Unlike an action's `locations: []` — the documented headless spelling — an empty array here carries no "I meant it" signal, because turning a webhook off has its own key (`isActive`). The repo's one real webhook (`showcase_task_changed`) confirms it: shipped inactive via `isActive: false`, with a full trigger list. + +**The corrected Tier-B disposition.** Phase 3 was scoped from the 2026-06 audit's Tier-A/B catalog. Verifying each candidate before writing it — the discipline that caught four false prescriptions in #4001 — found most of the list already closed or misfiled: + +| candidate | disposition | +|---|---| +| A2 action without `locations` | **already shipped** — `validate-action-locations.ts`, which already exempts the documented `locations: []` | +| B approval empty/unresolvable approvers | **already shipped** — `validate-approval-approvers.ts` | +| B select/multiselect without options | shipped in Phase 1 | +| B write-side referential integrity | **not an authoring-lint item** — a runtime gap; no metadata omission to detect | +| B `unique:true` no-op on memory driver | **not an authoring-lint item** — a driver gap | +| B composite/repeater sub-field constraints | **not an authoring-lint item** — a runtime gap | +| B nav targets of type page/report/url/component/action | **genuine gap, different module** — the key is present but dangling, which is reference resolvability (ADR-0072), not completeness (ADR-0078) | +| B dataset with zero measures | **unverified — not shipped.** No runtime consumer in this repo; the dataset compiler lives elsewhere | +| B webhook without triggers | ✅ **this change** | +| B schedule trigger with invalid cron | **unverified — not shipped.** `normalizeSchedule` accepts any non-empty string, but the scheduler's behaviour on an invalid one was not traced | + +Two candidates are deliberately left unshipped rather than written on the audit's stated confidence, and one is left for the module that actually owns it. The audit's own lesson stands: it produces *candidates*, not confirmed bugs — the scariest one collapsed on a three-file read. + +Tracked in #4544. diff --git a/packages/lint/src/validate-functional-completeness.test.ts b/packages/lint/src/validate-functional-completeness.test.ts index f98a89c63a..de41cde094 100644 --- a/packages/lint/src/validate-functional-completeness.test.ts +++ b/packages/lint/src/validate-functional-completeness.test.ts @@ -85,6 +85,20 @@ describe('validateFunctionalCompleteness — the walk', () => { expect(findings.every((f) => f.severity === 'warning')).toBe(true); }); + it('walks webhooks in both spellings', () => { + expect(validateFunctionalCompleteness({ + webhooks: [{ name: 'notify', url: 'https://x' }], + })[0]).toMatchObject({ + rule: 'webhook/without-triggers', + severity: 'error', + where: 'webhook "notify"', + path: 'webhooks[0].triggers', + }); + expect(validateFunctionalCompleteness({ + webhooks: { notify: { url: 'https://x' } }, + })[0]).toMatchObject({ where: 'webhook "notify"', path: 'webhooks.notify.triggers' }); + }); + it('is silent on a complete stack', () => { expect(validateFunctionalCompleteness({ objects: [{ @@ -97,6 +111,7 @@ describe('validateFunctionalCompleteness — the walk', () => { ], }], views: [{ object: 'order', list: { type: 'grid' } }], + webhooks: [{ name: 'notify', url: 'https://x', triggers: ['create'] }], })).toEqual([]); }); @@ -107,6 +122,7 @@ describe('validateFunctionalCompleteness — the walk', () => { { objects: [{ name: 'o', fields: 'nope' }] }, { views: [{ list: null }] }, { views: 'nope' }, + { webhooks: 'nope' }, { webhooks: [null, 7] }, ]) { expect(() => validateFunctionalCompleteness(junk)).not.toThrow(); } diff --git a/packages/lint/src/validate-functional-completeness.ts b/packages/lint/src/validate-functional-completeness.ts index df73de63c7..e78d5b5166 100644 --- a/packages/lint/src/validate-functional-completeness.ts +++ b/packages/lint/src/validate-functional-completeness.ts @@ -25,6 +25,7 @@ import { checkFieldCompleteness, checkViewCompleteness, + checkWebhookCompleteness, type CompletenessFinding, } from '@objectstack/spec/kernel'; @@ -117,5 +118,19 @@ export function validateFunctionalCompleteness(stack: unknown): FunctionalComple } } + // ── Webhooks: stack.webhooks[] ───────────────────────────────────────── + // [ADR-0078 Phase 3] The one Tier-B candidate that survived its verification + // pass. A webhook materializes into `sys_webhook` and looks armed in Setup + // whether or not it declares a trigger, so the omission is invisible on every + // surface an author can see. + for (const hook of entriesOf(stack.webhooks)) { + push( + out, + checkWebhookCompleteness(hook.def), + `webhook "${hook.name}"`, + `webhooks${hook.key}`, + ); + } + return out; } diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 63860b443d..2114bf908f 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -1896,8 +1896,10 @@ "VersionConstraint (type)", "VersionConstraintSchema (const)", "VulnerabilitySeverity (type)", + "WEBHOOK_WITHOUT_TRIGGERS (const)", "checkFieldCompleteness (function)", "checkViewCompleteness (function)", + "checkWebhookCompleteness (function)", "classifyRequiredCapability (function)", "deriveNamespaceFromPackageId (function)", "evaluateLockForDelete (function)", diff --git a/packages/spec/src/kernel/functional-completeness.test.ts b/packages/spec/src/kernel/functional-completeness.test.ts index 4d5a640562..dc969fcaa4 100644 --- a/packages/spec/src/kernel/functional-completeness.test.ts +++ b/packages/spec/src/kernel/functional-completeness.test.ts @@ -20,12 +20,14 @@ import { describe, expect, it } from 'vitest'; import { checkFieldCompleteness, checkViewCompleteness, + checkWebhookCompleteness, FUNCTIONAL_COMPLETENESS_RULES, FIELD_SUMMARY_WITHOUT_OPERATIONS, FIELD_FORMULA_WITHOUT_EXPRESSION, FIELD_RELATIONSHIP_WITHOUT_REFERENCE, FIELD_CHOICE_WITHOUT_OPTIONS, VIEW_LAYOUT_WITHOUT_BINDING, + WEBHOOK_WITHOUT_TRIGGERS, } from './functional-completeness'; const only = (findings: ReturnType) => { @@ -135,6 +137,50 @@ describe('checkViewCompleteness — layout bindings', () => { }); }); +describe('checkWebhookCompleteness — the rule the runtime comment argued against', () => { + it('flags a webhook with no `triggers` as an ERROR', () => { + const f = only(checkWebhookCompleteness({ name: 'notify_slack', url: 'https://x' }) as never); + expect(f.rule).toBe(WEBHOOK_WITHOUT_TRIGGERS); + expect(f.severity).toBe('error'); + }); + + it('flags `triggers: []` the same — an empty array is not an off switch here', () => { + // Contrast with an action's `locations: []`, which IS the documented + // headless spelling. A webhook's off switch is `isActive: false`, so an + // empty trigger list carries no "I meant it" signal — it is the same dead + // shape written out longhand. + const f = only(checkWebhookCompleteness({ triggers: [] }) as never); + expect(f.severity).toBe('error'); + }); + + it('is silent once a trigger is declared', () => { + expect(checkWebhookCompleteness({ triggers: ['create'] })).toEqual([]); + expect(checkWebhookCompleteness({ triggers: ['create', 'update', 'delete'] })).toEqual([]); + }); + + it('carries BOTH sources, because either one alone gets this wrong', () => { + // The skip site's own comment says "or a manual-only webhook with none", + // which reads as a runtime blessing — the exact shape that makes + // `multiselect` a NON-rule. What defeats it is webhook.zod.ts's #3196 note + // that no manual fire path exists, so the blessed mode is unreachable. + // If someone later demotes or deletes this rule on the strength of that + // comment alone, this assertion is where the missing half is stated. + const [f] = checkWebhookCompleteness({}); + expect(f.message).toContain('auto-enqueuer.ts'); + expect(f.message).toContain('no manual fire path exists'); + expect(f.message).toContain('isActive'); + }); + + it('never throws on junk', () => { + for (const junk of [undefined, null, 42, 'x', [], { triggers: 'create' }, { triggers: 7 }]) { + expect(() => checkWebhookCompleteness(junk)).not.toThrow(); + } + // A non-array `triggers` is not a declared trigger list — it is the dead + // shape wearing the wrong type, so it must not slip through as "declared". + expect(checkWebhookCompleteness({ triggers: 'create' })).toHaveLength(1); + }); +}); + describe('registry hygiene', () => { it('pins the rule-id list — ids are API for suppressions and dashboards', () => { expect([...FUNCTIONAL_COMPLETENESS_RULES].sort()).toEqual([ @@ -143,6 +189,7 @@ describe('registry hygiene', () => { 'field/relationship-without-reference', 'field/summary-without-operations', 'view/layout-without-binding', + 'webhook/without-triggers', ]); }); @@ -154,8 +201,9 @@ describe('registry hygiene', () => { ...checkFieldCompleteness({ type: 'select' }), ...checkFieldCompleteness({ type: 'checkboxes' }), ...checkViewCompleteness({ type: 'kanban' }), + ...checkWebhookCompleteness({ url: 'https://x' }), ]; - expect(all).toHaveLength(6); + expect(all).toHaveLength(7); for (const f of all) { expect(f.fix.length).toBeGreaterThan(8); expect(f.message.length).toBeGreaterThan(60); diff --git a/packages/spec/src/kernel/functional-completeness.ts b/packages/spec/src/kernel/functional-completeness.ts index 3312f2677f..cc2d7abb52 100644 --- a/packages/spec/src/kernel/functional-completeness.ts +++ b/packages/spec/src/kernel/functional-completeness.ts @@ -46,6 +46,13 @@ * - `checkboxes` w/o `options` sits between the two: it shares the multi * branch's free-form validator behaviour, but a checkbox group with zero * boxes is almost certainly an omission — so it is a WARNING, not an error. + * - `webhook` w/o `triggers` → `auto-enqueuer.ts` `if (triggers.size === 0) … + * return null`. Note this one needed a SECOND source: that skip site's own + * comment blesses the empty case as "a manual-only webhook", which reads + * exactly like the `multiselect` exemption above — but `webhook.zod.ts` + * (#3196) records that no manual fire path exists, so the blessed mode is + * unreachable. See {@link checkWebhookCompleteness}. A runtime comment states + * what its author believed; a removed sibling feature can make it stale. * * Severity follows ADR-0078 decision 1: `error` when the instance is fully * inert, `warning` when it degrades to something that partially works. @@ -70,6 +77,7 @@ export const FIELD_FORMULA_WITHOUT_EXPRESSION = 'field/formula-without-expressio export const FIELD_RELATIONSHIP_WITHOUT_REFERENCE = 'field/relationship-without-reference'; export const FIELD_CHOICE_WITHOUT_OPTIONS = 'field/choice-without-options'; export const VIEW_LAYOUT_WITHOUT_BINDING = 'view/layout-without-binding'; +export const WEBHOOK_WITHOUT_TRIGGERS = 'webhook/without-triggers'; /** Every rule id this module can emit — pinned by tests so ids cannot drift. */ export const FUNCTIONAL_COMPLETENESS_RULES = [ @@ -78,6 +86,7 @@ export const FUNCTIONAL_COMPLETENESS_RULES = [ FIELD_RELATIONSHIP_WITHOUT_REFERENCE, FIELD_CHOICE_WITHOUT_OPTIONS, VIEW_LAYOUT_WITHOUT_BINDING, + WEBHOOK_WITHOUT_TRIGGERS, ] as const; type AnyRec = Record; @@ -222,3 +231,58 @@ export function checkViewCompleteness(view: unknown): CompletenessFinding[] { : "gantt: { startDateField: '', endDateField: '', titleField: '' }", }]; } + +/** + * Completeness of a single webhook definition. + * + * ## Why this one needed TWO sources, and why one of them alone was misleading + * + * The auto-enqueuer's own comment reads, at the skip site: + * + * ``` + * if (triggers.size === 0) { + * // No dispatchable triggers (or a manual-only webhook with none) — + * // skip auto-enqueue. + * return null; + * ``` + * + * Read alone, that parenthetical *blesses* the empty case as a deliberate mode + * — exactly the shape that makes `multiselect` without options a NON-rule + * above. Stopping there would have left this candidate unenforced. + * + * But the mode it names does not exist. `webhook.zod.ts`'s #3196 note records + * that `api` (manual/programmatic fire) was REMOVED as a trigger value + * precisely because "no manual fire path exists (the only webhook HTTP surface + * re-queues already-failed deliveries)". So there is no way to fire a webhook + * that the auto-enqueuer has dropped: it is inert on every path, not + * manual-only. Hence `error`, not a NON-rule. + * + * The lesson generalizes past this rule: a runtime comment describes what its + * author believed, and beliefs go stale when a sibling feature is removed. The + * blessing has to be corroborated by something that says the blessed mode is + * REACHABLE — otherwise it is a comment about a mode that no longer exists. + * + * `triggers: []` is flagged the same as an omitted `triggers`: unlike an + * action's `locations: []` (the documented headless spelling), an empty array + * here is not an "I meant it" marker — turning a webhook OFF has its own key + * (`isActive` → the row's `active`), so `[]` is simply the same dead shape + * spelled out. + */ +export function checkWebhookCompleteness(webhook: unknown): CompletenessFinding[] { + if (!isRec(webhook)) return []; + if (hasEntries(webhook.triggers)) return []; + return [{ + rule: WEBHOOK_WITHOUT_TRIGGERS, + severity: 'error', + path: 'triggers', + message: + 'A webhook with no `triggers` never fires on any path. The auto-enqueuer drops it while ' + + 'building its subscription cache (`auto-enqueuer.ts` — `if (triggers.size === 0) … return ' + + 'null`), and there is no manual fire path to reach it either: `webhook.zod.ts` (#3196) ' + + 'records that the `api` trigger was removed because "no manual fire path exists — the only ' + + 'webhook HTTP surface re-queues already-failed deliveries". The webhook materializes into ' + + '`sys_webhook`, looks armed in Setup, and delivers nothing. To disable a webhook use ' + + '`isActive: false`; an empty `triggers` is not an off switch, just a dead one.', + fix: "triggers: ['create', 'update']", + }]; +}