Skip to content

test(triggers): guard the provider granularity the path route depends on - #6437

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/webhook-path-delivery-rule
Aug 8, 2026
Merged

test(triggers): guard the provider granularity the path route depends on#6437
waleedlatif1 merged 2 commits into
stagingfrom
fix/webhook-path-delivery-rule

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • The public trigger route classifies delivery eligibility by provider, not by trigger id, so a provider owning both a polling and an HTTP trigger would have its HTTP deliveries rejected
  • Sim already models dual-delivery services as two providers (slack / slack_app), but nothing asserted it — every existing POLLING_PROVIDERS sync assertion still passes for a mixed provider
  • Adds that assertion, with a message pointing at the fix (split the HTTP triggers onto their own provider)

Type of Change

  • Bug fix

Testing

Verified the new assertion fails on a deliberately mixed provider and that the three existing assertions stay green through it — i.e. the suite genuinely did not cover this. Full webhook/trigger suites pass (675 tests), plus type-check, lint, check:api-validation:strict, check:boundaries, check:trigger-block-cycle.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

The public trigger route classifies delivery by provider, not by trigger
id, so a provider owning both a polling and an HTTP trigger would have its
HTTP deliveries rejected. Sim already models dual-delivery services as two
providers (slack / slack_app), but nothing asserted it: the existing
POLLING_PROVIDERS sync assertions all still pass for a mixed provider.
@vercel

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 8, 2026 8:38pm

Request Review

@cursor

cursor Bot commented Aug 8, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only change with no runtime behavior; it prevents misconfigured trigger registry entries that would break HTTP webhook delivery.

Overview
Adds a registry invariant test so a provider cannot mix polling triggers with path-delivered HTTP triggers under the same provider name.

The public trigger route treats acceptsPathWebhookDelivery at provider scope: if a provider is in POLLING_PROVIDERS, path deliveries for that whole provider are rejected (404). Existing POLLING_PROVIDERS sync tests did not catch a hypothetical mixed provider; this new case fails when both polling and non-polling triggers share a provider. Internal providers and handlers with ingressMode === 'provider' are skipped, since they do not use the generic path route.

The failure message directs authors to split providers (e.g. slack vs slack_app).

Reviewed by Cursor Bugbot for commit f939e97. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds a registry invariant ensuring providers do not combine polling triggers with triggers delivered through the public path route, while exempting providers rejected wholesale by that route.

  • Groups registered triggers by provider and delivery type.
  • Exempts internal and provider-ingress delivery providers.
  • Reports mixed providers with their polling and path trigger IDs.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/triggers/constants.test.ts Adds a focused provider-level delivery invariant and correctly completes the prior-thread fix by matching the route’s internal-provider and provider-ingress exclusions.

Reviews (2): Last reviewed commit: "test(triggers): exempt provider-gated tr..." | Re-trigger Greptile

Comment thread apps/sim/triggers/constants.test.ts Outdated
…heck

Classifying every non-polling trigger as path-delivered was too broad:
internal and app-level-ingress providers never serve the public path
route either, so mixing polling with those would have failed the
invariant despite there being no routing conflict to fix.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit f939e97. Configure here.

@waleedlatif1
waleedlatif1 merged commit 77bc8ba into staging Aug 8, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/webhook-path-delivery-rule branch August 8, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant