Skip to content

Add GET /v1/users/developer/webhook/{wtype}/health: developer webhook delivery health - #8988

Closed
ZachL111 wants to merge 10 commits into
BasedHardware:mainfrom
ZachL111:zach/dev-webhook-health
Closed

Add GET /v1/users/developer/webhook/{wtype}/health: developer webhook delivery health#8988
ZachL111 wants to merge 10 commits into
BasedHardware:mainfrom
ZachL111:zach/dev-webhook-health

Conversation

@ZachL111

@ZachL111 ZachL111 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

What

Adds GET /v1/users/developer/webhook/{wtype}/health, which returns delivery health for a user's developer webhook of a given type.

Why

Developer webhook delivery health is recorded on every attempt (record_dev_webhook_success / record_dev_webhook_failure): failure count, last success and failure timestamps, last HTTP status, last error, and an auto-disable flag once failures pass the threshold. But there was no way to read it back. The app webhook side already has get_app_webhook_health; the developer side had only writers, and the e2e test hand-reads the raw Redis hash for lack of a getter. So today a client can enable or disable a webhook but cannot see its failure count or why it was auto-disabled.

Details

  • New db helper get_dev_webhook_health(uid, wtype) reads the existing Redis health hash. Its key and type stringify mirror the writers exactly so reads and writes align, and it is fail-open (any Redis error returns None).
  • The endpoint maps the raw hash into a typed response: integer failure_count, unix-second last_success_at / last_failure_at (null when unset), last_status, last_error (null when unset), and a boolean disabled. When nothing has been recorded yet it returns zeroed / null / false with has_data: false.
  • Sits alongside the other /v1/users/developer/webhook/{wtype} routes; the /health suffix does not collide with the existing /disable and /enable, nor with the plural /webhooks/status.

Test

backend/tests/unit/test_dev_webhook_health.py: the db helper returns None when absent, decodes the hash and uses the value-based key when present, fails open on a Redis error, and stringifies a non-enum type. The endpoint response mapping (has_data false, and full field mapping including empty-string to null and the disabled flag) runs in CI, where routers.users heavy imports resolve.

Review in cubic

Failure class

Failure-Class: none

The fix: commits on this branch are within-PR iteration on its own new surface (response
modelling and contract regeneration, pyright typed-boundary cleanups, scope and manifest
registration, and test fixes). None of them repair a violated contract that matches a registered
class, and minting a new class is a separate registry PR per AGENTS.md, so this declares none.

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed: scoped backend addition, approve-only per policy (features/new endpoints are Nik's merge call).

@Git-on-my-level Git-on-my-level added security-review Touches auth, provider routing, secrets, or security-sensitive surfaces needs-maintainer-review Needs a human maintainer to sign off before merge labels Jul 4, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes maintainer review

Thanks for adding this — the change is narrowly scoped and it fills a useful gap: developer webhook health was already being recorded, but there was no authenticated endpoint to read the existing Redis state back.

What looks good:

  • The new helper uses the same dev_webhook_health:{uid}:{wtype} key shape as the success/failure writers.
  • The endpoint is authenticated with the current user's UID and only exposes that user's webhook health.
  • Response mapping is conservative: absent data returns zero/null/false defaults, and Redis read failures fail open instead of breaking the endpoint.
  • The added unit tests cover the Redis helper and endpoint field mapping, and the current backend/API checks are passing.

I am not formally approving because this touches a security/privacy-sensitive developer webhook surface and should get human maintainer review before merge. I did not find a concrete blocker in the diff.

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes maintainer re-review

Re-reviewed the latest head 005854a.

This still looks like a useful, narrowly scoped addition: it exposes the developer webhook health state that the backend is already recording, keeps the route authenticated to the current user's UID, mirrors the existing Redis key shape, and maps missing/empty health data to conservative zero/null/false defaults. The new helper and endpoint mapping tests cover the important cases, and the current CI checks are passing.

Two maintainer notes before merge:

  • This is still a security/privacy-sensitive developer webhook surface, so I’m keeping this as a positive signal rather than a formal approval under the maintainer automation policy.
  • GitHub currently reports the PR as conflicting, so it needs a rebase/update from main before it can land.

I did not find a concrete code blocker in the diff.

@ZachL111
ZachL111 force-pushed the zach/dev-webhook-health branch 2 times, most recently from 888d5d8 to 409514e Compare July 7, 2026 17:17
@ZachL111

ZachL111 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks David. Updated this branch with current main and resolved the harness-smoke conflict by keeping both watched files. I also added the small fake Firestore update() support needed by the merged canonical-memory tests. The PR is mergeable now and all current checks are green.

@ZachL111
ZachL111 force-pushed the zach/dev-webhook-health branch from 409514e to ccfaa27 Compare July 10, 2026 09:14

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes maintainer re-review

Thanks for the update. The backend logic still looks narrowly scoped and useful, but I found a concrete blocker on the current head: the public app-client OpenAPI contract is failing.

The new route is currently declared without a response model/return annotation:

@router.get('/v1/users/developer/webhook/{wtype}/health', tags=['v1'])
def get_user_webhook_health_endpoint(...):

Meanwhile the committed app-client OpenAPI/generated clients include a typed UserWebhookHealthResponse. CI’s exporter is disagreeing with the committed docs/api-reference/app-client-openapi.json (Public Developer API contract reports it is stale), which usually means the generated API docs/clients were hand-edited or generated from a different local state than the backend source.

Could you please make the backend route the source of truth by adding a proper typed response model for this endpoint (e.g. a UserWebhookHealthResponse Pydantic model and response_model=UserWebhookHealthResponse), then regenerate the OpenAPI/client artifacts from that source and push the generated output? That should make the public API contract check meaningful and keep the client types aligned with the actual FastAPI schema.

Also, the current Hygiene check is failing on the desktop UserDefaults ratchet. It may be fallout from the generated desktop client/rebase rather than the endpoint itself, but CI needs to be green before this can land.

Keeping needs-maintainer-review / security-review because this is an authenticated developer-webhook health surface. I am requesting changes for the stale/mismatched public API contract, not for product fit.

ZachL111 added 5 commits July 10, 2026 12:16
Developer webhook delivery health is recorded on every attempt (failure
count, last success/failure timestamps, last status/error, auto-disable
after sustained failures) but there was no way to read it back: the app
side has get_app_webhook_health, the developer side had only writers, and
the e2e test hand-reads the raw Redis hash. Add the getter plus a GET
endpoint so a client can see a webhook's failure count and why it was
auto-disabled. Reuses the existing Redis health hash; fail-open on error.
…he dev webhook-health route

Regenerates the app-client contract and TS client schemas so the Public Developer API contract
check passes, with a desktop changelog fragment for the regenerated desktop client file.
@ZachL111
ZachL111 force-pushed the zach/dev-webhook-health branch from ccfaa27 to ab4b7b0 Compare July 10, 2026 19:19
# Conflicts:
#	backend/route_policy_manifest.yaml
#	desktop/macos/Desktop/Sources/Generated/OmiApi.generated.swift
#	desktop/windows/src/renderer/src/lib/omiApi.generated.ts
#	web/admin/lib/services/omi-api/omiApi.generated.ts
#	web/app/src/lib/omiApi.generated.ts
#	web/personas-open-source/src/lib/omiApi.generated.ts
@ZachL111
ZachL111 force-pushed the zach/dev-webhook-health branch 2 times, most recently from 37eb92f to c251599 Compare July 11, 2026 03:41

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes maintainer re-review

Thanks for the follow-up. The issue from my previous review looks addressed on this head: the route now declares a DevWebhookHealthResponse response model, the generated OpenAPI/client artifacts match that typed backend schema, and the current CI checks (including Public Developer API contract and Hygiene) are green.

I do not see a code-level blocker in the diff. The endpoint is narrow, authenticated to the current user, reads the existing developer-webhook health hash, and preserves the existing fail-open Redis behavior. The added unit coverage exercises the Redis helper and endpoint field mapping, including empty values and the disabled flag.

I’m leaving this as a positive signal rather than a formal approval because this is still a security/privacy-sensitive developer-webhook telemetry surface and the PR carries security-review / needs-maintainer-review. A human maintainer should make the final call on whether exposing last_error / delivery-health details in the first-party app API is the desired product/security behavior before merge.

@ZachL111

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. Agreed this is the right call for a human maintainer on the product/security question. For that decision: last_error and the delivery-health fields are the developer's own webhook state, read only for the authenticated caller's UID (dev_webhook_health:{uid}:{wtype}), so there is no cross-user exposure. It is the same data the backend already records on delivery success and failure, just made readable back to its owner. If you would prefer a narrower surface, I am happy to drop or redact last_error and keep only the numeric health counters.

ZachL111 added 2 commits July 18, 2026 23:31
The five generated API clients conflicted, as they do on every endpoint PR whenever main
adds a route. The OpenAPI spec auto-merged cleanly, so the clients are regenerated from
that merged spec rather than hand-resolved, and both generators pass --check.
…dpoint

The endpoint grew routers/users.py past its frozen baseline. Raised to the measured line
count with the required one-line justification rather than splitting a health read away
from the developer-webhook handlers it shares auth and serialization with.
@Git-on-my-level
Git-on-my-level dismissed their stale review July 19, 2026 10:51

Dismissed stale Hermes request: current head declares DevWebhookHealthResponse via response_model, regenerated API artifacts match, and Public Developer API contract/Hygiene checks are green. Security-review/needs-maintainer-review still remain for human final review.

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Backend feature (re-approved on new commits) — approve-only per policy; Nik owns backend deploy.

@undivisible undivisible added human Human-authored pull request backend Backend Task (python) labels Aug 10, 2026
@undivisible undivisible added desktop macOS web javascript Pull requests that update javascript code workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior labels Aug 10, 2026
@Git-on-my-level Git-on-my-level removed macOS desktop web javascript Pull requests that update javascript code security-review Touches auth, provider routing, secrets, or security-sensitive surfaces needs-maintainer-review Needs a human maintainer to sign off before merge workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior labels Aug 12, 2026
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Hermes maintainer re-review

Re-reviewed current head ecf9c68. I don't see a remaining code-level blocker, and the existing maintainer approval on this head is consistent with the implementation I reviewed.

Specific observations:

  • .github/scripts/product_file_line_count_ratchet_baseline/backend-routers.json updates the users.py ratchet and replaces the old justification with one specific to this webhook-health route, which is appropriate for keeping the new read next to the existing developer-webhook enable/disable/status handlers.
  • backend/database/webhook_health.py adds get_dev_webhook_health() using the same dev_webhook_health:{uid}:{wtype} key shape as the writers and fails open to None on Redis errors, so this read endpoint should not break webhook management when Redis is degraded.
  • backend/route_policy_manifest.yaml records the new route as Firebase-authenticated, first-party, metrics-domain, and non-mutating; that matches the endpoint's auth.get_current_user_uid scoping in backend/routers/users.py.
  • backend/routers/users.py maps absent health data to conservative has_data: false / zero / null / false defaults, and recorded Redis fields are coerced narrowly into ints, nullable strings, and the disabled == "1" boolean.
  • backend/tests/unit/test_dev_webhook_health.py covers missing Redis data, decoded hash reads, Redis fail-open behavior, non-enum type stringification, and endpoint response mapping. I did not repeat local backend execution for this suspicious fork, but the current GitHub checks include passing Backend unit suite and Public Developer API contract.
  • docs/api-reference/app-client-openapi.json and the generated clients in desktop/macos/Desktop/Sources/Generated/OmiApi.generated.swift, desktop/windows/src/renderer/src/lib/omiApi.generated.ts, web/admin/lib/services/omi-api/omiApi.generated.ts, web/app/src/lib/omiApi.generated.ts, and web/personas-open-source/src/lib/omiApi.generated.ts all add the same typed DevWebhookHealthResponse surface and path, so the backend schema and generated consumers appear aligned.
  • The two macOS changelog files are narrow generated-client release notes for this API addition.

I also cleaned up stale automation review labels that no longer matched this head; the remaining category label is backend, which is the primary owner for this change. Leaving this as a positive maintainer signal rather than a formal bot approval because the route is auth/metrics-adjacent and already has human maintainer sign-off.

— Reviewed by Hermes Agent (glm-5.2), an AI maintainer assistant. A human maintainer should make the final merge decision.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@ZachL111

Copy link
Copy Markdown
Contributor Author

Closing: main has since grown its own database/webhook_health.py with a different design, and #10646 reworks that same surface further. This branch conflicts with both, so a dev webhook health endpoint would need a ground-up refile if it is still wanted.

@ZachL111 ZachL111 closed this Aug 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hey @ZachL111 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

  • PRODUCT.md — product north star
  • Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing.

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Backend Task (python) human Human-authored pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants