From 474a0fd6b764445333550957d87eea49a7905351 Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Sun, 19 Jul 2026 17:01:18 -0700 Subject: [PATCH] fix(gateway): correct GitHub auto-retry claim in webhook route docs (#4289) While investigating a review on PR #4274 (which corrected the same misconception in the channel manager's dedupe-TTL comment), I found a more elaborate version of the same factual error in this route, predating that PR: the docstring claimed "GitHub retries 5xx deliveries with exponential backoff (up to ~5 attempts over ~8 hours)". No such behavior exists. Verified directly against GitHub's documentation: failed deliveries (5xx, timeout, connection error alike) are simply recorded as failed and never automatically redelivered, for both repository and GitHub App webhooks (https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries). Redelivery is always explicit: the "Redeliver" button, the REST API, or an operator's own recovery script (GitHub's own recommended pattern for the latter runs on a 6-hour cron, illustrating this is opt-in operator tooling, not a GitHub-side mechanism). The 503-vs-200 status code choice itself was already correct and is unchanged: 503 keeps a transient failure recorded as failed so it can be recovered via one of the explicit paths above, while 200 correctly marks permanent/non-retryable conditions (disabled channel, unknown event, missing service) as done so no pointless redelivery is invited. Only the rationale text describing *why* was wrong. - github_webhooks.py: reworded the route docstring and four inline comments/log messages that repeated "GitHub retries" / "so GitHub retries" framing, citing the actual documented behavior. - test_github_webhooks.py: renamed test_dispatch_failure_returns_503_so_github_retries to test_dispatch_failure_returns_503_not_200 and reworded its docstring plus two nearby docstrings/comments with the same framing. No assertions changed - same status codes and response bodies checked. - AGENTS.md: corrected the GitHub Webhooks router table row to match. All 44 tests in test_github_webhooks.py pass, plus the full test_github_dispatcher.py / test_github_channel.py / test_github_registry.py / test_channels.py suites (313 total). ruff check and ruff format --check are clean on both touched Python files. --- backend/AGENTS.md | 2 +- .../app/gateway/routers/github_webhooks.py | 59 ++++++++++++------- backend/tests/test_github_webhooks.py | 26 ++++---- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 768b1bde2..fba6fdfac 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -341,7 +341,7 @@ Localhost persistence deliberately reads the direct request `Host` and ignores ` | **Thread Runs** (`/api/threads/{id}/runs`) | `POST /` - create background run; `POST /stream` - create + SSE stream; `POST /wait` - create + block; `POST /regenerate/prepare` - prepare clean input + checkpoint metadata for regenerating the latest assistant answer; `GET /` - list runs; `GET /{rid}` - run details; `POST /{rid}/cancel` - cancel; `GET /{rid}/join` - join SSE; `GET /{rid}/messages` - paginated per-run messages `{data, has_more}`; `GET /{rid}/events` - full event stream; `GET /{rid}/workspace-changes` - workspace/output file change summary and optional diffs; `GET /../messages` - legacy thread message array; `GET /../messages/page` - backward thread-global `seq` history page with middleware/successful-regenerate filtering and page-run-scoped feedback enrichment; `GET /../token-usage` - aggregate tokens | | **Feedback** (`/api/threads/{id}/runs/{rid}/feedback`) | `PUT /` - upsert feedback; `DELETE /` - delete user feedback; `POST /` - create feedback; `GET /` - list feedback; `GET /stats` - aggregate stats; `DELETE /{fid}` - delete specific | | **Runs** (`/api/runs`) | `POST /stream` - stateless run + SSE; `POST /wait` - stateless run + block; `GET /{rid}/messages` - paginated messages by run_id `{data, has_more}` (cursor: `after_seq`/`before_seq`); `GET /{rid}/feedback` - list feedback by run_id | -| **GitHub Webhooks** (`/api/webhooks/github`) | `POST /` - receive GitHub App / repo webhook deliveries. Verifies `X-Hub-Signature-256` against `GITHUB_WEBHOOK_SECRET`; exempt from auth + CSRF because authenticity is enforced by HMAC. The route is fail-closed: mounted only when `GITHUB_WEBHOOK_SECRET` is set, or when explicit dev opt-in `DEER_FLOW_ALLOW_UNVERIFIED_GITHUB_WEBHOOKS=1` is set. Recognized events include `ping`, `issues`, `issue_comment`, `pull_request`, `pull_request_review`, and `pull_request_review_comment`; unknown events return 200 with `handled=false`. Fan-out runtime failures return 503 so GitHub retries; permanent/non-retryable conditions such as `channels.github.enabled: false`, unknown events, malformed payloads, or unavailable channel service return 200 with a skipped/handled response. | +| **GitHub Webhooks** (`/api/webhooks/github`) | `POST /` - receive GitHub App / repo webhook deliveries. Verifies `X-Hub-Signature-256` against `GITHUB_WEBHOOK_SECRET`; exempt from auth + CSRF because authenticity is enforced by HMAC. The route is fail-closed: mounted only when `GITHUB_WEBHOOK_SECRET` is set, or when explicit dev opt-in `DEER_FLOW_ALLOW_UNVERIFIED_GITHUB_WEBHOOKS=1` is set. Recognized events include `ping`, `issues`, `issue_comment`, `pull_request`, `pull_request_review`, and `pull_request_review_comment`; unknown events return 200 with `handled=false`. Fan-out runtime failures return 503, keeping the delivery recorded as failed for manual/API/scripted redelivery (GitHub does not automatically retry any failed delivery, 5xx included); permanent/non-retryable conditions such as `channels.github.enabled: false`, unknown events, malformed payloads, or unavailable channel service return 200 with a skipped/handled response. | | **GitHub Event-Driven Agents** | Custom agents can declare a `github:` block in their `config.yaml` to bind to repos and event triggers. Webhook fan-out publishes one `InboundMessage` per matching binding to the channel bus; `GitHubChannel` routes those messages through `ChannelManager`. The response `dispatch` summarizes matched/fired/skipped agents. | **Workspace change review**: `packages/harness/deerflow/workspace_changes/` diff --git a/backend/app/gateway/routers/github_webhooks.py b/backend/app/gateway/routers/github_webhooks.py index b1f80eb40..5a21bbb46 100644 --- a/backend/app/gateway/routers/github_webhooks.py +++ b/backend/app/gateway/routers/github_webhooks.py @@ -183,16 +183,23 @@ async def receive_github_webhook( - Returns ``{"ok": True, ...}`` on successful (or no-op) dispatch so GitHub marks the delivery successful and does not retry. - **Transient fan-out failures return 503**, not 200. GitHub retries 5xx - deliveries with exponential backoff (up to ~5 attempts over ~8 hours) - but does *not* retry 200 OK. A transient registry filesystem error or - bus publish failure on a 200 path would silently drop a real webhook - forever, so we return 503 instead and let GitHub redeliver — by the - time the redelivery lands the underlying outage is usually gone. The - `is_route_enabled()` startup check still handles *configuration* - errors fail-closed (route absent → 404); 503 is reserved for runtime - failures GitHub should retry. Permanent / non-retryable conditions - (unknown event, missing channel service) keep returning 200. + **Transient fan-out failures return 503**, not 200. GitHub does NOT + automatically retry a failed delivery of any kind — 5xx, timeout, or + connection error are all simply recorded as failed; see + https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries. + A 200 response marks the delivery permanently successful, so + swallowing a transient registry filesystem error or bus publish + failure into a 200 would silently drop a real webhook forever with no + way to recover it. Returning 503 instead keeps the delivery correctly + recorded as failed in GitHub's Recent Deliveries / Deliveries API, so + an operator's manual "Redeliver" click, a REST API call, or a + self-hosted recovery script (the pattern GitHub's own docs recommend) + can still recover it — by the time that redelivery lands the + underlying outage is usually gone. The `is_route_enabled()` startup + check still handles *configuration* errors fail-closed (route absent + → 404); 503 is reserved for runtime failures worth making recoverable + this way. Permanent / non-retryable conditions (unknown event, missing + channel service) keep returning 200. The route is fail-closed: :func:`is_route_enabled` should have already prevented this handler from being mounted when no secret is configured. @@ -266,11 +273,13 @@ async def receive_github_webhook( service = get_channel_service() if service is None: # Permanent state, not a transient failure: ``channels.github`` - # is not enabled in this deployment. Returning 503 would - # condemn GitHub to retry every delivery on backoff for hours - # before giving up, all the way to no-op. Stay on 200 and - # surface the reason in the response body for operators - # checking the redelivery page. + # is not enabled in this deployment. Returning 503 would mark + # this delivery "failed" and invite a manual "Redeliver" or an + # operator's recovery script to retry it — pointlessly, since + # every such retry hits this same disabled-channel state until + # the config changes. Stay on 200 and surface the reason in + # the response body for operators checking the redelivery + # page. logger.warning( "github_webhook: channel service not available — no agents fired (delivery=%s event=%s)", x_github_delivery, @@ -290,8 +299,9 @@ async def receive_github_webhook( # back to GitHub via ``gh``, contradicting the documented # off-switch. Bail before ``fanout_event`` so no inbound # ever reaches the bus consumer in ChannelManager. Stay on - # 200 (permanent state, not transient) so GitHub doesn't - # retry; surface the reason in dispatch_result for the + # 200 (permanent state, not transient) rather than mark the + # delivery failed and invite a pointless manual/scripted + # redelivery; surface the reason in dispatch_result for the # Recent Deliveries panel. logger.info( "github_webhook: channels.github.enabled=false — skipping fan-out (delivery=%s event=%s)", @@ -312,13 +322,18 @@ async def receive_github_webhook( raw_default_mention = github_channel_config.get("default_mention_login") operator_default_mention_login = raw_default_mention.strip() if isinstance(raw_default_mention, str) else None - # Let fan-out exceptions propagate as 503 so GitHub retries. + # Let fan-out exceptions propagate as 503, not 200. # ``fanout_event`` calls the registry (filesystem) and the # message bus; both can fail transiently (disk hiccup, bus # queue full, asyncio cancellation). Swallowing those into a - # 200 would permanently drop a real webhook because GitHub - # only retries on 5xx. The startup-time ``is_route_enabled`` - # check still covers fail-closed *configuration* errors. + # 200 would permanently drop a real webhook, since GitHub + # treats 200 as final success and does not automatically + # retry any failure — including 5xx (see + # https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries). + # 503 keeps the delivery recorded as failed so a manual + # "Redeliver", the REST API, or a recovery script can recover + # it. The startup-time ``is_route_enabled`` check still + # covers fail-closed *configuration* errors. try: dispatch_result = await fanout_event( service.bus, @@ -329,7 +344,7 @@ async def receive_github_webhook( ) except Exception as exc: # noqa: BLE001 — re-raised as 503 below logger.exception( - "github_webhook: fanout failed (delivery=%s event=%s) — returning 503 so GitHub retries", + "github_webhook: fanout failed (delivery=%s event=%s) — returning 503 (recoverable via manual/API redelivery)", x_github_delivery, x_github_event, ) diff --git a/backend/tests/test_github_webhooks.py b/backend/tests/test_github_webhooks.py index 98048d6b5..95dc42e42 100644 --- a/backend/tests/test_github_webhooks.py +++ b/backend/tests/test_github_webhooks.py @@ -542,17 +542,21 @@ def test_dispatch_result_included_in_response(client: TestClient, monkeypatch: p assert fake.await_count == 1 -def test_dispatch_failure_returns_503_so_github_retries(client: TestClient, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture) -> None: - """A crashing fan-out helper must return 503 so GitHub retries. +def test_dispatch_failure_returns_503_not_200(client: TestClient, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture) -> None: + """A crashing fan-out helper must return 503, not 200. The earlier behaviour swallowed every fan-out exception into a 200 OK - response (``dispatch={"error": "fanout failed"}``). GitHub only retries - 5xx — a 200 ack permanently drops the delivery. The route now lets - runtime failures propagate as 503 so a transient registry/bus error - triggers GitHub's redelivery path. The startup-time + response (``dispatch={"error": "fanout failed"}``). GitHub treats 200 + as final success and does not automatically retry any failure, + including 5xx (see + https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries) + — so a 200 ack permanently drops the delivery. The route now lets + runtime failures propagate as 503 so the delivery is correctly + recorded as failed, recoverable via a manual "Redeliver", the REST + API, or an operator's own recovery script. The startup-time ``is_route_enabled`` check still handles *configuration* failures fail-closed (route absent → 404); 503 is reserved for runtime - failures GitHub can retry past. + failures worth making recoverable this way. """ async def fake_fanout(*args, **kwargs) -> dict: @@ -611,7 +615,8 @@ def test_dispatch_failure_503_lets_github_redeliver_successfully(client: TestCli ) assert first.status_code == 503 - # GitHub redelivers — same payload, same signature. + # A redelivery — same payload, same signature (e.g. a manual + # "Redeliver" click, since GitHub does not resend this automatically). second = client.post( "/api/webhooks/github", content=body, @@ -672,8 +677,9 @@ def test_channel_disabled_skips_fanout(client: TestClient, monkeypatch: pytest.M publishing inbound onto the bus would let the ChannelManager consumer pick it up and run agents that then post back to GitHub via ``gh``, contradicting the documented off-switch. Returns 200 (permanent - state, not transient) so GitHub doesn't retry; ``dispatch.skipped`` - surfaces the reason in the Recent Deliveries panel. + state, not transient) rather than mark the delivery failed and invite + a pointless redelivery; ``dispatch.skipped`` surfaces the reason in + the Recent Deliveries panel. """ bus = MessageBus()