From 96bf83136039d33e5e690b999f2bbb1ea495b8b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 03:22:54 +0000 Subject: [PATCH] feat(lint): webhook without triggers fails at author time (ADR-0078 Phase 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A webhook that declares no `triggers` never fires on any path. It still materializes into `sys_webhook` and still renders in Setup looking armed, so the omission is invisible on every surface an author can see — the ADR-0078 shape. ## The first source argued AGAINST this rule 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. It is structurally identical to the `multiselect`-without-options NON-rule, where `record-validator.ts`'s `// free-form (tags without options)` is precisely why we do NOT flag it. On this evidence alone the 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". Nothing can fire a webhook the auto-enqueuer dropped. Inert on every path, so: error, not a NON-rule. The generalization is now in the module doc 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 needs corroboration that the blessed mode is still reachable, or it is a comment about a mode that no longer exists. The test asserts the finding carries both citations so nobody demotes the rule on 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 a webhook's off switch is `isActive`. The repo's one real webhook confirms the premise — `showcase_task_changed` ships inactive via `isActive: false` with a full trigger list, and is clean. ## Phase 3's scope was mostly already closed Verifying each Tier-B candidate before writing it — the discipline that caught four false prescriptions in #4001 — found most of the audit's list already handled or misfiled: A2 action without `locations` already shipped (validate-action-locations, which already exempts `locations: []`) approval empty approvers already shipped (validate-approval-approvers) select/multiselect options shipped in Phase 1 write-side FK integrity not an authoring-lint item (runtime gap) `unique` on memory driver not an authoring-lint item (driver gap) composite sub-field constraints not an authoring-lint item (runtime gap) nav page/report/url/component genuine gap, but reference resolvability (ADR-0072), not completeness — wrong module dataset with zero measures UNVERIFIED, not shipped: no runtime consumer in this repo schedule trigger invalid cron UNVERIFIED, not shipped: normalizeSchedule takes any non-empty string, scheduler behaviour not traced webhook without triggers this change Two candidates are left unshipped rather than written on the audit's stated confidence, and one is left to the module that owns it. Verification: 24 predicate tests (19 -> 24) and 8 walk tests (7 -> 8); the one real webhook in the repo produces no finding; full suite 132/132. Tracked in #4544. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WnqGjQFQMqd5k81LYV8SCY --- .../adr-0078-phase3-webhook-triggers.md | 44 +++++++++++++ .../validate-functional-completeness.test.ts | 16 +++++ .../src/validate-functional-completeness.ts | 15 +++++ packages/spec/api-surface.json | 2 + .../kernel/functional-completeness.test.ts | 50 ++++++++++++++- .../src/kernel/functional-completeness.ts | 64 +++++++++++++++++++ 6 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 .changeset/adr-0078-phase3-webhook-triggers.md 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']", + }]; +}