[feat] Add events and webhooks#3632
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mmabrouk
left a comment
There was a problem hiding this comment.
Reviewing solely against the new backend requirements in AGENTS.md.
Must-fix (does not comply with AGENTS backend patterns)
- Layering violations (core/db depend on API + entrypoints)
- Core imports API schemas (AGENTS: core should not depend on
apis/fastapi/*):api/oss/src/core/webhooks/service.pyimportsoss.src.apis.fastapi.webhooks.schemas
- DB layer imports API schemas (AGENTS: DB should not depend on API layer):
api/oss/src/dbs/postgres/webhooks/dao.pyimportsoss.src.apis.fastapi.webhooks.schemas
- Core imports entrypoints for worker wiring via global lazy singleton (AGENTS: wire concrete deps in
api/entrypoints/*only; avoid core -> entrypoints):api/oss/src/core/webhooks/trigger.pyimportsentrypoints.worker_webhooks
Expected direction: Router -> Service -> DAO Interface -> DAO Impl -> DB.
- DBE placement doesn’t follow the new-stack structure
- Webhook DB models are added into legacy/monolithic
api/oss/src/models/db_models.py(WebhookSubscriptionDB,WebhookEventDB,WebhookDeliveryDB). - AGENTS requires new DBEs to live under
api/oss/src/dbs/postgres/<domain>/dbes.py(plus optionaldbas.py+mappings.py).
- Returning DBEs from service/router contracts
api/oss/src/core/webhooks/service.pyreturnsWebhookSubscriptionDBand lists of DBEs.- Router response models serialize DBEs via
from_attributes = True(api/oss/src/apis/fastapi/webhooks/schemas.py). - AGENTS explicitly says: do not return DBEs from router/service contracts; introduce DTOs + mapping.
- Endpoint conventions don’t follow AGENTS patterns
AGENTS conventions:
POST /queryfor filtering/search (payload support + cursor windowing)POST /{id}/archiveandPOST /{id}/unarchivefor lifecycle transitions- response envelopes with
count+ payload
Current API uses:
GET /webhooks/for list,DELETE /webhooks/{id}for “delete” (but implementation actually archives viaarchived_at)- no
/query, noWindowing
Migration seam / coupling note
- Webhook triggering is added into legacy router:
api/oss/src/routers/variants_router.pycallstrigger_webhook(...). - AGENTS says avoid net-new features in
api/oss/src/routers/*; if deploy still lives there, treat this as a temporary migration seam and keep the coupling minimal (especially avoid core importing entrypoints to make it work).
Style/consistency (lower priority)
- API contracts are in
schemas.py; AGENTS convention ismodels.pyfor request/response models. - New code doesn’t follow the keyword-only + grouped
#argument style seen in new-stack services (not mandatory everywhere, but it’s the convention documented).
What I’d expect to align with AGENTS
api/oss/src/core/webhooks/dtos.py(+ optionalinterfaces.py) and a DAO interface.- Move DBEs into
api/oss/src/dbs/postgres/webhooks/dbes.pyand addmappings.pyto isolate ORM. - Make
api/oss/src/dbs/postgres/webhooks/dao.pyaccept/return core DTOs (no imports from API schemas). - Rework router to follow
/query+ archive/unarchive patterns (or document why webhooks is an explicit exception). - Remove core -> entrypoints worker import; keep DI at composition roots (
api/entrypoints/*).
…re/webhooks # Conflicts: # api/entrypoints/routers.py # api/oss/src/routers/variants_router.py
mmabrouk
left a comment
There was a problem hiding this comment.
Thanks a lot @jp-agenta and @ashrafchowdury great work! LGTM
Devin has a couple of comments, worth looking into.
|
|
This PR implements the new Webhooks feature, including backend APIs, worker tasks, and frontend UI components.
Documentation
Please read the README.md and DESIGN.md files for detailed design and implementation documentation.