Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mmabrouk
reviewed
Feb 5, 2026
Member
mmabrouk
left a comment
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
…llInView - Add "Raw" view mode to display original string values before auto-parsing - Rename "Rendered JSON" to "Field Rendering" for clarity - Preserve `originalStringValue` in PathItem for stringified JSON fields - Refactor view mode options logic to show appropriate modes based on data type - Remove text truncation from ReadOnlyTextView (now handled by editor config) - Consolidate field rendering logic and improve messages
…nd search functionality
…terals in DrillInContent
…(N) width_bucket The analytics histogram query (build_numeric_continuous_blocks) used generate_series to create bin intervals, then joined every data row against every bin using range predicates. For large datasets this caused O(N * sqrt(N)) comparisons — 332K rows with 577 bins produced 191M comparisons, exceeding the 15-second statement timeout. Replace with PostgreSQL's width_bucket(value, lo, hi, bins) which computes the bin index in O(1) per row via arithmetic. The count step becomes a simple GROUP BY. generate_series is still used for the output (to include empty bins) but only for the small bin-series-to-counts left join. Also handles the edge case where all values are identical (vmin == vmax) by using GREATEST(edge_width * 1e-9, 1e-9) as the epsilon, ensuring width_bucket always receives lo < hi. Before/after on 964K rows (30 days): - Before: >15s (statement timeout) - After: ~2.6s
- Reuse edge_width/center_width in step 9 instead of recomputing - Wrap chosen_bins in GREATEST(..., 1) to prevent division-by-zero if a user passes bins=0 via MetricSpec (pre-existing gap)
…n-widening Epsilon-widening wb_hi shifted all internal bin boundaries, causing values at exact edges (common with integer metrics like token counts) to land in the previous bin instead of the next. Fix: only add epsilon when vmin == vmax (degenerate case where width_bucket requires lo < hi), and clamp the overflow bucket (bins+1) to the last bin via LEAST. This preserves the original [start, end) half-open interval semantics.
… example Add DOCKER_NETWORK_MODE=bridge to both the env.oss.gh.example file (for new users) and explicitly to all 5 containers in docker-compose.gh.yml (api, worker-evaluations, worker-tracing, cron, services) with a default fallback — matching the pattern in docker-compose.dev.yml. This ensures existing users upgrading with their old .env file also get the fix. Without this, parse_url() never rewrites localhost to host.docker.internal inside containers, causing the services container to call itself instead of the API through traefik — breaking openapi.json auth. Fixes #3869
fix(observability): replace O(N*sqrt(N)) histogram join with O(N) width_bucket
…ker-network-mode fix: add missing DOCKER_NETWORK_MODE to docker-compose.gh.yml
…-docker-compose [chore] Add missing `with-web` profiles to docker compose files in dev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.